-
Notifications
You must be signed in to change notification settings - Fork 560
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow multiple security group filter matches #3526
Allow multiple security group filter matches #3526
Conversation
@dlmather: This issue is currently awaiting triage. If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Hi @dlmather. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Failing to reconcile when additional SGs could not be found is a change of behaviour, so we can do this change in the v1beta2 version. It'd be good to track this here: #2355 Since we already support list of additional SGs in other places, makes sense to add multiple additional SGs support to launchtemplates, could you file an issue and modify this PR to only cover that? |
/ok-to-test |
b31fc43
to
c916db3
Compare
Just to clarify, we would no longer fail to reconcile when SGs couldn't be found under this change. As you say though, this is definitely a change of behavior from the current setup where failing to find SGs in any listed filter drops all AdditionalSecurityGroups. |
/retest |
/test pull-cluster-api-provider-aws-e2e |
/lgtm |
cc @richardcase @sedefsavas for review/approval. |
I think this PR is good to go. |
@dlmather could you please rebase the changes. |
Based on this comment above, let's wait until we open the main branch to v1beta2 changes before rebasing to avoid double efforts. |
/milestone v1.6.0 |
@dlmather could you rebase your PR, as the main branch is now open for the API changes. |
c916db3
to
688de9f
Compare
@dlmather This looks good. please squash your commits so that I can merge. |
/milestone v2.0.0 |
@Ankitasw: You must be a member of the kubernetes-sigs/cluster-api-provider-aws-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Provider AWS Maintainers and have them propose you as an additional delegate for this responsibility. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/milestone v2.0.0 |
@dlmather could you squash your commits so that we can merge this? |
a1a3bc4
to
d05f2c1
Compare
@Ankitasw all squashed now, thanks! |
/test pull-cluster-api-provider-aws-e2e |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Ankitasw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/unhold |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Current behavior for additional security groups especially when using filters is somewhat cumbersome and mysterious. If a filter has no potential matches, it will silently blackhole all additional security groups due to returning up an error which is eventually silently dropped. Additionally, a choice was made to only return the first match from a specified filter. It would be nice to be able to instead match all security groups returned by a filter so that tags can be used to implicitly add security groups to AWSMachines in a more dynamic way. Finally, in the current setup, it is considered an error to have a filter match that returns no results... this forces the requirement that all security groups must be created before the creation of an AWSMachineTemplate using them. This creates a back-and-forth in Cluster bootstrap, since we need the VPC to exist before defining security groups, but then must circle back to retroactively add security group matchers, which in turn requires rolling all MachineDeployments that wish to use the SecurityGroups.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
Checklist: