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
[IPv6] Choose correct primary IP config #3715
[IPv6] Choose correct primary IP config #3715
Conversation
✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.
|
0fe4c3c
to
f121ba8
Compare
f121ba8
to
3007954
Compare
442a72a
to
589f009
Compare
589f009
to
6d1c2b3
Compare
0b77b12
to
d8167f6
Compare
/retest |
pkg/provider/azure_vmss.go
Outdated
isIPv6 := isBackendPoolIPv6(backendPoolID) | ||
|
||
if !isIPv6 { | ||
// There should be one and only once primary IP config. |
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.
nit: there can only be at most one
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.
Updated.
d8167f6
to
5a53ffb
Compare
pkg/provider/azure_vmss.go
Outdated
} | ||
} | ||
|
||
return nil, fmt.Errorf("failed to find a IPconfiguration(IPv6=%v) for the scale set VM %q", IPv6, nodeName) | ||
return nil, fmt.Errorf("failed to find a primary IP configuration (IPv6=%v) for the VMSS VM or VMSS %q", isIPv6, resource) |
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.
s/%v/%t
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.
Updated.
pkg/provider/azure_vmss.go
Outdated
if primaryIPConfig.LoadBalancerBackendAddressPools != nil { | ||
loadBalancerBackendAddressPools = *primaryIPConfig.LoadBalancerBackendAddressPools | ||
} | ||
handleBackendPool := func(backendPoolID string) bool { |
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.
Can we extract it to a standalone function and reuse it across all the references?
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.
Good idea. I updated 3 places but don't touch the one (ensureBackendPoolDeletedFromVmssUniform) which is different (with a vmssNamesMap). Also, I added a UT for this new function.
5a53ffb
to
8f10b38
Compare
Regardless of IPv6 only or dualstack clusters, IPv4 IP config is always primary. So for IPv6 backend address pool, IP config's IP version needs consideration. Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
8f10b38
to
99ef413
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
thanks for the fix!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, lzhecheng 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 |
/cherrypick release-1.26 |
/cherrypick release-1.25 |
/cherrypick release-1.24 |
/cherrypick release-1.23 |
@lzhecheng: #3715 failed to apply on top of branch "release-1.26":
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. |
@lzhecheng: #3715 failed to apply on top of branch "release-1.25":
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. |
@lzhecheng: #3715 failed to apply on top of branch "release-1.24":
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. |
@lzhecheng: #3715 failed to apply on top of branch "release-1.23":
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. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Choose correct primary IP config in ensureBackendPoolDeletedFromNode(). Regardless of IPv6 only or dualstack clusters, IPv4 IP config is always primary. So for IPv6 backend address pool, IP config's IP version needs consideration.
Which issue(s) this PR fixes:
Fixes #
Related: #3707
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: