Introduce config opt-in NLB provisioning with Security Groups#1158
Conversation
|
This issue is currently awaiting triage. If cloud-provider-aws contributors determine this is a relevant issue, they will accept it by applying the The 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. |
|
Hi @mtulio. 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 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. |
mtulio
left a comment
There was a problem hiding this comment.
Almost done. A few pending items in TODO, and raising questions in key points from a broad group.
|
Hey @kmala, I still have some items to review in this PR, but tests are passing locally. Would you mind stamping ok-to-test to validate if there is no local addiction from my env? Thanks |
|
/ok-to-test |
e1e47cf to
1e8e527
Compare
|
/test all |
1e8e527 to
9a0b0cc
Compare
|
i'm caught up in some bug work currently, but i will try to make time for a review this friday (29 Aug). |
|
Last changes:
|
elmiko
left a comment
There was a problem hiding this comment.
i think this is looking good, but i have not tried it in a live cluster. i would love to test drive it next week, but i can't guarantee that i will have the time.
Thanks, @elmiko . I just sent to your way the creds of a live v1.33 cluster with operands turned off, so you can save time setting up the environment :) |
|
Hello all, I just reviewed all comments to ensure everything is resolved. No pending items so far. Awaiting further review, if there are some. Thanks! |
|
@mtulio demonstrated this change for me today, in general things work as expected. we did notice some odd behavior when removing the annotation, a new lb was created, but we think the bug is not related to this code change. /lgtm |
Thanks for peer review, Mike. I just recorded the steps in the issue (already existed in controller) #1254 . cc @kmala |
|
Hey @kmala , would you mind taking a look at this PR, please? Thanks! |
|
Also gone through this today /lgtm Thanks @mtulio |
Ensure the Security Group IDs is added on NLB load balancer creation. Additionally, this is fixing the BYO SG update scenario by detecting the replaced SG on CLB and delete it when it is owned by controller. The same behavior will be implemented in the BYO SG scenario for NLB too.
Introduce hasClusterTagOwned() to validate if a resource has the kubernetes cluster tag (`kubernetes.io/cluster/clusterID`) with value `owned`, so it can quickly used when ensuring states to cloud resources managed by controller, such as SG deletions, etc.
Introduce the documentation to use the feature Service type-LoadBalancer with Security Group by opt-in through the cloud-config.
Introduce the NLB Security Group Mode configuration (NLBSecurityGroupMode) to make the controller creates the Security Group by default when provisioning Service type-LoadBalancer NLB. This configuration is opt-in and global to the cluster.
|
/lgtm |
Ensure the Security Group is managed when creating a service type-LoadBalancer NLB object, considering the global configuration to manage SGs in NLBs: NLBSecurityGroupMode=NLBSecurityGroupModeManaged
Ensure annotation matches feature NLB with Security Groups by preventing standard controller BYO SG annotations due existing controller limitations.
Ensure unit tests on EnsureLoadBalancer, including case to test NLB with security group by changing the cloud-config.
failures caused by missing unit test from recent changes which moved BYO SG annotations to dedicated validation functions, as well unit test failure caused by sdk v2 bump. Both are fixed in my last commit, additionally I moved BYO SG unit tests to the validation functions. Awaiting new CI signals. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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 |
|
Thanks @elmiko @kmala @JoelSpeed for your support on this PR. |
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR introduces managed security group support for Service type-LoadBalancer NLB through a global cloud-config option. Key changes:
NLBSecurityGroupMode: Managedcloud-config to automatically provision Security Groups for new NLBsservice.beta.kubernetes.io/aws-load-balancer-security-groupsandservice.beta.kubernetes.io/aws-load-balancer-extra-security-groupsannotations for NLBRationale for BYO SG Limitation:
BYO Security Group support for NLB is intentionally blocked in this release due to:
Which issue(s) this PR fixes:
Refs #1151
Special notes for your reviewer:
Future Work:
Managed SG is important to empower users to bypass managed SG, and enhance security control boundaries with user-managed security groups on NLBs. BYO Security Group support for NLB will be considered in a future release after:
The changes introducing the global, opt-in, cloud-config for enabling Security Group (SG) on NLB creation (similar CLB), is for users/administrators who intentionally wants to enforce SG across all new services - following AWS recommendations, and ALBC defaults. This won't change the default CCM behavior if the configuration isn't added.
Done checklist:
Related changes isolated from this PR:
Does this PR introduce a user-facing change?: