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
Support dualstack for PIP in azure_loadbalancer.go #3404
Support dualstack for PIP in azure_loadbalancer.go #3404
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
✅ Deploy Preview for kubernetes-sigs-cloud-provide-azure canceled.
|
eaaebd7
to
040df2a
Compare
040df2a
to
1d6a93d
Compare
1d6a93d
to
6fde56c
Compare
6fde56c
to
59a75fe
Compare
/test pull-cloud-provider-azure-e2e-ccm-capz |
59a75fe
to
30dc964
Compare
6d9c0ed
to
3f3a880
Compare
/retest |
3f3a880
to
a923e0c
Compare
d22371e
to
2ff52b2
Compare
2ff52b2
to
eb87587
Compare
/assign @feiskyer |
/assign @nilo19 |
} | ||
mockLBsClient := az.LoadBalancerClient.(*mockloadbalancerclient.MockInterface) | ||
mockLBsClient.EXPECT().List(gomock.Any(), az.Config.ResourceGroup).Return(c.existingLBs, nil) | ||
for _, c := range tests { |
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.
Cover dualstack scenario in this test?
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.
Thanks for the suggestion.
Yes, dualstack should be covered in this test but it needs code change in those frontend IP config related methods like getServiceLoadBalancerStatus()
. It should return 2 fipConfig
.
Considering this PR is large enough, I plan to add those changes in the next PR. wdyt?
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 mean we should cover the changes of GetLoadBalancer in this PR, which is to test ipv4 and ipv6 pips at a time.
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.
Discussed offline. Decided to cover dualstack for existsPip()
part.
f87de14
to
a8a0d12
Compare
} | ||
} | ||
|
||
if isIPv6 { | ||
if !strings.EqualFold(string(pip.PublicIPAddressVersion), string(network.IPv6)) { |
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.
It looks like a useless check.
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.
Thank you for pointing this out! I rechecked the code and it isn't useless because this methods is used to check if pip's IP version matches expected one.
However, there's still something to improve. I add a new isIPv6 bool
parameter to this method so clusterIP
is not used anymore. Also I added a UT for this method. PTAL.
* Feature code for public IP * Add UT Signed-off-by: Zhecheng Li <zhechengli@microsoft.com>
a8a0d12
to
e3bb56f
Compare
/lgtm |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Support dualstack for PIP in azure_loadbalancer.go
Which issue(s) this PR fixes:
Fixes #
related: #814
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: