-
Notifications
You must be signed in to change notification settings - Fork 302
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
use subnets of ensured NLBs when update worker node SG rules #763
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 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. |
pkg/providers/v1/aws.go
Outdated
for _, az := range v2LoadBalancer.AvailabilityZones { | ||
existingSubnetIDs = append(existingSubnetIDs, *az.SubnetId) | ||
} | ||
if len(existingSubnetIDs) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
under which case will this condition be true?
shouldn't lbs always have subnets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about if it's a new NLB. but checking through the ensureLoadBalancerv2(), if it's a new NLB it will use the discoveredSubnetIDs. So yeah there should always be subnets. I'll update the PR
Updated: since we got the subnets from loadBalancer.AvailabilityZones
, and for newly created LBs, the AZs are none, since the ensureLoadBalancerV2() does not pass AZ info as part to createRequest(): https://github.com/kubernetes/cloud-provider-aws/blob/master/pkg/providers/v1/aws_loadbalancer.go#L157. So I add a check here. If it's for newly created LBs, and in any case the output LB does not have AZ info, we use discovered subnets.
5fc7dde
to
242c8c9
Compare
242c8c9
to
3ba520a
Compare
/retest |
3ba520a
to
c7a5d4e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
c7a5d4e
to
f21fcc4
Compare
rebase main to fix the govlun failure |
errors in e2e tests:
/test pull-cloud-provider-aws-e2e-kubetest2-quick |
@dims do you know what's up with the OIDC thing? Tracking: https://kubernetes.slack.com/archives/CCK68P2Q2/p1702414384604979 |
/retest |
/test pull-cloud-provider-aws-check |
6f75cb4
to
d9e335e
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cartermckinnon, M00nF1sh 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The controller gets subnets by the order of subnet ids everytime when it ensures NLB, and modifies the worker node SG rules according to subnet CIDRs. This may cause issue under some use case for existing NLBs. For example, if there's an existing NLB-a on subnet-3 and subnet-4, later the user adds new subnets, say subnet-1 and subnet-2, the controller will get subnet-1 and subnet-2 when ensure NLB-a, remove the SG rule on subnet-3 and subnet-4 and add new SG rule on subnet-1 and subnet-2 for the NLB-a, while the NLB-a is still on subnet-3 and subnet-4.
This PR fixes this issue by checking if there are existing subnets on the NLB, if so, we use the cidrs on the existing subnets for SG rules. otherwise, we use the cidrs of discovered subnets.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I added some debug lines locally and ran the unit test
TestNLBNodeRegistration
, noticing that for the newly created LB, the loadbalancer.AvailabilityZones are empty, suspecting it may because for the createRequest, we don't pass any AZ info to it.Does this PR introduce a user-facing change?: