Add IPv6 address of metadata service to sg rules #136
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
sebageek
reviewed
Mar 26, 2024
| @@ -233,7 +233,7 @@ | |||
| "id": "HTTP", | |||
| "display_name": "HTTP", | |||
| "source_groups": ["ANY"], | |||
| "destination_groups": ["169.254.169.254"], | |||
| "destination_groups": ["169.254.169.254", "fe80::a9fe:a9fe"], | |||
There was a problem hiding this comment.
If you like constants we have neutron_lib.constants.METADATA_V6_IP. There's also one for v4.
| @@ -635,13 +635,27 @@ def _setup_default_infrastructure_rules(self): | |||
| path = API.POLICY.format(policy["id"]) | |||
| res = self.client.get(path=path) | |||
| if res.ok: | |||
| continue | |||
| if self._check_for_infrastructure_changes(): | |||
There was a problem hiding this comment.
What is this method? I haven't found it in the source. Do you want to call _check_infrastructure_rules_for_updates() here?
| elif res.status_code == 404: | ||
| LOG.info("Infrastructure Policy %s not found, creating...", policy["display_name"]) | ||
| self.client.put(path=path, data=policy).raise_for_status() | ||
| else: | ||
| res.raise_for_status() | ||
|
|
||
| def _check_infrastructure_rules_for_updates(self, policies_from_cfg, security_policies): | ||
| for rule_from_cfg in policies_from_cfg['rules']: | ||
| for rule_policy in sg_policy["rules"]: |
There was a problem hiding this comment.
What is sg_policy? is that security_policies but with a different name?
| for rule_from_cfg in policies_from_cfg['rules']: | ||
| for rule_policy in sg_policy["rules"]: | ||
| if rule_policy['id'] == rule_from_cfg['id']: | ||
| shared_keys = set(rule.keys()).intersection(set(rule_policy.keys())) |
There was a problem hiding this comment.
[optional] I like using operators for set operations, but it depends of what you like yourself. For intersection you have the option of using &, so set(rule) & set(rule_policy) (the .keys() is implicit for dict-to-list-conversion but that's also a choice on how verbose you like to be, makes it more ovious that you're dealing with a dict).
| if rule_policy['id'] == rule_from_cfg['id']: | ||
| shared_keys = set(rule.keys()).intersection(set(rule_policy.keys())) | ||
| diff = set(o for o in shared_keys if rule[o] != rule_policy[o]) | ||
| LOG.info("Infrastructure rule %s has changed in %s - new rule %s" % (rule_from_cfg, diff, rule_from_cfg)) |
There was a problem hiding this comment.
I'd pass the format attributes as arguments to LOG.info() instead of doing the formatting beforehand.
| LOG.info("Infrastructure rule %s has changed in %s - new rule %s" % (rule_from_cfg, diff, rule_from_cfg)) | ||
| return True | ||
| else: | ||
| LOG.info("New infrastructure rule found %s" % rule_from_cfg) |
There was a problem hiding this comment.
Same here for the Log args.
joroaf
reviewed
Mar 26, 2024
| @@ -635,13 +635,27 @@ def _setup_default_infrastructure_rules(self): | |||
| path = API.POLICY.format(policy["id"]) | |||
| res = self.client.get(path=path) | |||
| if res.ok: | |||
| continue | |||
| if self._check_for_infrastructure_changes(): | |||
| self.client.put(path=path, data=policy).raise_for_status() | |||
There was a problem hiding this comment.
Should we update the default infra policy every time with the desired rules? What if the operator added his own rules? Is this the desired behaviour?
sven-rosenzweig
force-pushed
the
change_infrastructure_rules
branch
5 times, most recently
from
0726ebd
to
2b25bbd
Compare
sven-rosenzweig
force-pushed
the
change_infrastructure_rules
branch
3 times, most recently
from
41e29c7
to
4bf2266
Compare
Creating the default security policies (DHCP, ICMP and Metadata allow), happens on driver start (in case they have not been realized so far). Changing those was not supported. Before starting the driver, it is now checked if the realized rules differs from the rules defined in NSX-T constants. If a change is detected, the rules defined in the NSX-T constants will be realized as defined.
So far only DHCPv6 and ICMPv6 traffic is refelcted in the default infrastructure rules of NSX-T. The IPv6 address of the metadata service requires clearing as well.
sven-rosenzweig
force-pushed
the
change_infrastructure_rules
branch
from
4bf2266
to
e0afa85
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So far only DHCPv6 and ICMPv6 traffic is refelcted in the default infrastructure rules of NSX-T. The IPv6 address of the metadata service requires clearing as well.
In order to reflect those changes in NSX-T, it requires a check if the rule configuration has changed. So far only creation of missing policies is supported.