Integrate some of my original code with the newly integrated NSX-T generator #342
Conversation
# Fix the wrong use of notes instead of description, notes are supposed to be used for rule lock/unlock # Add comment keyword instead of notes (comments are translated now to description in NSX-T API) # Check description length against its maximum # Add check for maximum number of rules # Add support for term expiration # Add a more granular management of ip_protocol and not blindly set it to IPV4_IPV6 # Fix bug convert 0.0.0.0/0 to ANY # Add support for source_excluded # Add support for verbose keyword # Add check for mixed rule (v4 and v6) with v4 only source and no non-ANY v4 destination and v6 only source and no non-ANY v6 destination # Add check for maximum number of source and destination IPs # Fix issue when str of Term return nothing # Add support for numerical protocol # Fix bug when there's a single port, don't convert it into a range
jcatrouillet
changed the title
|
Small heads up: I had to do an assortment of other fixes, so I did internal code changes way before seeing your pull request. Once it is submitted, I’ll try to do a rebase myself that you can squash into your pull request, since this is the second time the contribution is being preempted. Apologies for this! |
|
Ack, please let me know when you're done. |
There was a problem hiding this comment.
I've added some notes for whoever does the followup here (whether myself or @jcatrouillet). These are not so much comment and feedback, of the type I would usually do in code review -- more notes on what was changed here, because so much of it was already done in previous commits.
The main thing to add would be the max length checks. Adding support for verbose requires some more consideration not to break existing terms/policies (including our existing internal integration).
Thank you for your contribution.
| if (len(self.nsxt_policies) > 1): | ||
| raise NsxtUnsupportedManyPoliciesError('Only one policy can be rendered') | ||
| raise NsxtUnsupportedManyPolicies('Only one policy can be rendered') |
There was a problem hiding this comment.
The exception is named NsxtUnsupportedManyPoliciesError
| #Catch the case if term is not valid | ||
| try: | ||
| rules = rules + [json.loads(str(term))] | ||
| except: |
There was a problem hiding this comment.
It's not a great practice to catch exceptions overly widely, especially without even logging the issue. I'd say that failing loudly is better to allow for a bad term to be caught rather than to be skipped (given this is intended for securing an environment).
| if term.name in term_names: | ||
| raise NsxtDuplicateTermError('There are multiple terms named: %s' % | ||
| term.name) | ||
| term_names.add(term.name) | ||
|
|
||
| term.name = self.FixTermLength(term.name) | ||
|
|
||
| new_terms.append(Term(term, filter_type, applied_to, 4)) | ||
| # Handle term expiration |
There was a problem hiding this comment.
Support for this was added in 6e7f105.
|
|
||
| cnt_terms += 1 | ||
|
|
||
| if cnt_terms > self._RULECOUNT_WARN_THRESHOLD: |
| "destination_address", 4 | ||
| ) + self.term.GetAddressOfVersion("destination_address", 6) | ||
| if daddrs[0].with_prefixlen in ("0.0.0.0/0", "::/0"): | ||
| daddrs = "ANY" |
| @@ -131,38 +157,66 @@ def get(self): | |||
| new_service = service.copy() | |||
| new_service['icmp_type'] = icmp_type | |||
| services.append(new_service) | |||
| #Is the return supposed to be in the for loop? Potential bug to be tested with multiple ICMP types | |||
There was a problem hiding this comment.
Correct, this was a bug. Indentation fixed in 6e7f105
| @@ -13,13 +13,31 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # | |||
| # | |||
There was a problem hiding this comment.
Changelog shouldn't be in the code itself.
| # 'profiles': ['ANY'], | ||
| # 'scope': ['ANY'], | ||
| 'logged': bool(self.term.logging), | ||
| 'description': description, |
There was a problem hiding this comment.
Per read security policy API doc which says the API returns a SecurityPolicy containing the Rule array, there's both notes and description.
Maybe the same1 string belongs in both? Not sure where these appear in the NSX-T UI.
The comments added on top of the file suggest we should indeed switch to description.
Footnotes
-
Not quite the same because max length of
descriptionis 1024 and max length ofnotesis 2048. ↩
| class Term(aclgenerator.Term): | ||
| """Creates a single ACL Term for NSX-T.""" | ||
|
|
||
| def __init__(self, term, filter_type, applied_to=None, af=4): | ||
| _MAX_TERM_NAME_LENGTH = 255 | ||
| _MAX_TERM_COMMENT_LENGTH = 1024 |
There was a problem hiding this comment.
If we keep using the notes field, then this is 20481.
Footnotes
| @@ -13,13 +13,31 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
| # | |||
| # | |||
| # VERSION 1.01 | |||
| # Fix the wrong use of notes instead of description, notes are supposed to be used for rule lock/unlock | |||
There was a problem hiding this comment.
This is an important explanation, this would be a clear reason to put the notes into description (possibly in addition to notes).
|
When approaching this, I might create a completely new commit/PR and appropriately use the |
Fix the wrong use of notes instead of description, notes are supposed to be used for rule lock/unlock
Add comment keyword instead of notes (comments are translated now to description in NSX-T API)
Check description length against its maximum
Add check for maximum number of rules
Add support for term expiration
Add a more granular management of ip_protocol and not blindly set it to IPV4_IPV6
Fix bug convert 0.0.0.0/0 to ANY
Add support for source_excluded
Add support for verbose keyword
Add check for mixed rule (v4 and v6) with v4 only source and no non-ANY v4 destination and v6 only source and no non-ANY v6 destination
Add check for maximum number of source and destination IPs
Fix issue when str of Term return nothing
Add support for numerical protocol
Fix bug when there's a single port, don't convert it into a range