Support NLBSecurityGroupMode for AWS Cloud Controller Manager#18211
Conversation
|
Welcome @mfbonfigli! |
|
Hi @mfbonfigli. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
|
/retest |
|
Hello @hakman, I appreciate the help in running the tests. Would you be able to review the PR (or share an approximate ETA) and let me know what you think? Thanks! |
hakman
left a comment
There was a problem hiding this comment.
Thanks @mfbonfigli, seems like a reasonable change.
I added a few comments, please take a look and let me know when ready for a new review.
There was a problem hiding this comment.
Set the new permission conditionally, as there is a flag at the cluster level for it:
| func AddCCMPermissions(p *Policy, cloudRoutes bool, nlbSecurityGroupMode bool) { |
There was a problem hiding this comment.
Thank you, good advice. I implemented this suggestion this in the last update to this PR, only slightly renaming your parameter nlbSecurityGroupMode to nlbSecurityGroupModeManaged given the parameter can take a string value and not boolean (although currently only "Managed" is supported).
In the last iteration I also re-updated the golden files and added an extra test to specifically test the permission is set when NLBSecurityGroupMode is set to Managed in the config.
| // NLBSecurityGroupMode determines if the controller manages security groups for Network Load Balancers. | ||
| // Valid value: "Managed" | ||
| // When set to "Managed", the controller creates and manages a dedicated security group for each NLB. |
There was a problem hiding this comment.
Each API file has a different description, please align.
There was a problem hiding this comment.
Thank you, good catch. I updated it in last revision by improving the comment and aligning it around the codebase.
|
Thank you @hakman . I should have implemented your comments, feel free to take a look when you have some time. |
Thanks, looks good. Please fix the failing test and squash the commits. I will take a look later today. |
|
/retest |
|
Seems the test is having issues in CI, I will take a look on Friday, please feel free to wait until then to review. |
Adds nlbSecurityGroupMode field to cluster spec and elasticloadbalancing:SetSecurityGroups IAM permission for AWS Cloud Controller Manager to support managed security groups for AWS Network Load Balancers.
Provisions the required IAM permissions needed by AWS CCM to manage NLB security groups only when the NLBSecurityGroupMode flag is set to Managed. Also improves consistency among flag descriptions in the codebase.
Updates the generated code and test files according to the last changes to IAM Policies for NLBSecurityGroupMode=Managed
Adds a test and golden file to verify the IAM permissions when the AWS Cloud Controller Manager configuration has NLBSecurityGroupMode set to Managed.
2c0f8e3 to
22ab7ab
Compare
|
/retest |
|
/test pull-kops-e2e-gce-cni-calico |
|
/retest |
|
@hakman Thanks for the help once again,I rebased on latest master and fixed the failed test, all should be good for you to take a look when you have some time. Thanks! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hakman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-kops-e2e-azure-cni-calico |
What this PR does / why we need it:
This PR adds support for the
NLBSecurityGroupModecloud configuration setting and theSetSecurityGroupsIAM permission for AWS Cloud Controller Manager.When
nlbSecurityGroupMode: Managedis set in the cluster spec, kops writesNLBSecurityGroupMode = Managedto the cloud config file, enabling CCM to create and manage dedicated security groups for each Network Load Balancer instead of provisioning NLBs without security groups.The
SetSecurityGroupsIAM permission allows CCM to attach/detach security groups from cluster-owned NLBs, bringing CCM to parity with the AWS Load Balancer Controller which already has this permission and enabling the BYO Security Groups feature for NLBs in AWS CCM.Please refer to issue #18210 for more details on why this change is needed.
Changes:
NLBSecurityGroupModetoAWSSpecandCloudConfigurationwith bidirectional conversion"Managed"value (case-sensitive) or omitted/nil.elasticloadbalancing:SetSecurityGroupsto CCM role inclusterTaggedActionblock.NLBSecurityGroupMode = Managedto/etc/kubernetes/cloud.configon master nodesWhich issue(s) this PR fixes:
Fixes #18210
Special notes for your reviewer:
Large number of changed files: many auto-generated IAM policy golden test files were updated via
hack/update-expected.shto include the newSetSecurityGroupspermission.Related upstream work: