cluster-api-provider-aws
cluster-api-provider-aws copied to clipboard
kubernetes-sigs
Allow multiple security group filter matches
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:
- [X] squashed commits
- [ ] includes documentation
- [X] adds unit tests
- [ ] adds or updates e2e tests
@dlmather: This issue is currently awaiting triage.
If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.
The triage/accepted label can be added by org members by writing /triage accepted in a comment.
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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
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: https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/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?
Failing to reconcile when additional SGs could not be found is a change of behaviour
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.
Based on this comment above, let's wait until we open the main branch to v1beta2 changes before rebasing to avoid double efforts.
@dlmather could you rebase your PR, as the main branch is now open for the API changes.
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sedefsavas for approval by writing /assign @sedefsavas in a comment. For more information see:The Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@dlmather This looks good. please squash your commits so that I can merge.
@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:
/milestone v2.0.0
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.
/test pull-cluster-api-provider-aws-e2e /test pull-cluster-api-provider-aws-e2e-eks
[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
- ~~OWNERS~~ [Ankitasw]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment