Fix custom backendPool not being used #3676

RadekManak · 2023-06-29T08:48:24Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
In some places the default backendPool name was being used instead of using the specified value. This caused machine creation to fail when attempting to use loadBalancer with custom backendPool name.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

cherry-pick candidate

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Fix default `backendPool` name being used when custom name is set.

k8s-ci-robot · 2023-06-29T08:48:32Z

Welcome @RadekManak!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api-provider-azure has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-06-29T08:48:33Z

Hi @RadekManak. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

nawazkh

I didn't review the changes on *_mock.go, putting it to later since I had some implementation questions.

nawazkh · 2023-06-30T03:03:22Z

azure/scope/cluster.go

-func (s *ClusterScope) APIServerLBPoolName(loadBalancerName string) string {
-	return azure.GenerateBackendAddressPoolName(loadBalancerName)
+func (s *ClusterScope) APIServerLBPoolName() string {
+	return s.APIServerLB().BackendPool.Name


Is it possible that s.APIServerLB() to be nil? If so, can we run into nil pointer dereference ?

Also, do you think it is valid to use azure.GenerateBackendAddressPoolName(loadBalancerName) as a backup name if s.APIServerLB().BackendPool.Name is nil?

Is it possible that s.APIServerLB() to be nil? If so, can we run into nil pointer dereference ?

It cannot be nil. A function above is creating the pointer from a struct field.
// APIServerLB returns the cluster API Server load balancer.
func (s *ClusterScope) APIServerLB() *infrav1.LoadBalancerSpec {
return &s.AzureCluster.Spec.NetworkSpec.APIServerLB
}

azure.GenerateBackendAddressPoolName(loadBalancerName) is a valid default name for the backendPool, but it is already being set by the defaulting webhook if it is empty. I don't believe that adding the defaulting here as well has any benefit.

nawazkh · 2023-06-30T03:11:57Z

azure/scope/cluster.go

-// OutboundLBName returns the name of the outbound LB.
-func (s *ClusterScope) OutboundLBName(role string) string {
+// OutboundLB returns the outbound LB.
+func (s *ClusterScope) OutboundLB(role string) *infrav1.LoadBalancerSpec {


Are we using OutboundLB() anywhere else outside this package? Any specific reason on exporting it out?

If it's just the lb.Name we are interested in, can we just return the name instead of the whole spec?

You're right should be private. I have decomposed the load balancer selection logic into a function because it is needed in both OutboundLBName and OutboundPoolName.

azure/scope/machine.go

In some places the default backendPool name was being used instead of the specified value.

CecileRobertMichon

/ok-to-test

k8s-ci-robot · 2023-07-05T22:42:28Z

@RadekManak: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	`cb94940`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

CecileRobertMichon

/lgtm
/assign @nawazkh
/milestone v1.10

k8s-ci-robot · 2023-07-06T21:04:26Z

LGTM label has been added.

Git tree hash: ad701eb14f834c9085128f6eb6d25b6b2d905fc5

codecov · 2023-07-06T21:24:05Z

Codecov Report

Patch coverage: 95.23% and project coverage change: +0.24 🎉

Comparison is base (7059146) 53.74% compared to head (cb94940) 53.98%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3676      +/-   ##
==========================================
+ Coverage   53.74%   53.98%   +0.24%     
==========================================
  Files         185      185              
  Lines       18595    18699     +104     
==========================================
+ Hits         9994    10095     +101     
- Misses       8059     8061       +2     
- Partials      542      543       +1

Impacted Files	Coverage Δ
azure/scope/machinepool.go	`32.31% <0.00%> (ø)`
azure/scope/cluster.go	`55.74% <100.00%> (+0.17%)`	⬆️
azure/scope/machine.go	`50.71% <100.00%> (-0.41%)`	⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

nawazkh · 2023-07-07T18:34:28Z

pull-cluster-api-provider-azure-apidiff test seems to be failing since we are changing the interface in here. Does it classify as a breaking change? Do we need to take care of any more stuff? Maybe call out in the Release Notes?

nawazkh · 2023-07-07T18:35:28Z

Looks good to me.
Just waiting on below.

pull-cluster-api-provider-azure-apidiff test seems to be failing since we are changing the interface in here. Does it classify as a breaking change? Do we need to take care of any more stuff? Maybe call out in the Release Notes?

/lgtm

CecileRobertMichon · 2023-07-07T18:59:36Z

pull-cluster-api-provider-azure-apidiff test seems to be failing since we are changing the interface in here. Does it classify as a breaking change? Do we need to take care of any more stuff? Maybe call out in the Release Notes?

thanks for calling that out. In this case we should be fine since it's only touching the scope functions which we don't make guarantees on (breaking changes in the user-facing API is what we want to watch out for). That being said, we should really move code that isn't meant to be consumed as a library to an internal/ directory to avoid these false positives.

CecileRobertMichon · 2023-07-07T19:00:26Z

/approve
/cherry-pick release-1.9

Let's cherry-pick this one since it fixes a bug ^

k8s-infra-cherrypick-robot · 2023-07-07T19:00:27Z

@CecileRobertMichon: once the present PR merges, I will cherry-pick it on top of release-1.9 in a new PR and assign it to you.

In response to this:

/approve
/cherry-pick release-1.9

Let's cherry-pick this one since it fixes a bug ^

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.

k8s-ci-robot · 2023-07-07T19:00:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

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:

~~OWNERS~~ [CecileRobertMichon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-infra-cherrypick-robot · 2023-07-07T20:15:49Z

@CecileRobertMichon: new pull request created: #3690

In response to this:

/approve
/cherry-pick release-1.9

Let's cherry-pick this one since it fixes a bug ^

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.

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 29, 2023

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 29, 2023

k8s-ci-robot requested review from CecileRobertMichon and willie-yao June 29, 2023 08:48

nawazkh reviewed Jun 30, 2023

View reviewed changes

Fix custom backendPool not being used

cb94940

In some places the default backendPool name was being used instead of the specified value.

RadekManak force-pushed the fix-custom-backendpool-name branch from f947836 to cb94940 Compare June 30, 2023 09:03

CecileRobertMichon reviewed Jul 5, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 5, 2023

CecileRobertMichon reviewed Jul 6, 2023

View reviewed changes

k8s-ci-robot assigned nawazkh Jul 6, 2023

k8s-ci-robot added this to the v1.10 milestone Jul 6, 2023

k8s-ci-robot assigned CecileRobertMichon Jul 6, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 6, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2023

k8s-ci-robot merged commit 3f3c8c8 into kubernetes-sigs:main Jul 7, 2023
4 checks passed

k8s-infra-cherrypick-robot mentioned this pull request Jul 7, 2023

[release-1.9] Fix custom backendPool not being used #3690

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix custom backendPool not being used #3676

Fix custom backendPool not being used #3676

RadekManak commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

nawazkh left a comment

nawazkh Jun 30, 2023

nawazkh Jun 30, 2023

RadekManak Jun 30, 2023 •

edited

Loading

nawazkh Jun 30, 2023

nawazkh Jun 30, 2023

RadekManak Jun 30, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 5, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Jul 6, 2023

codecov bot commented Jul 6, 2023 •

edited

Loading

nawazkh commented Jul 7, 2023

nawazkh commented Jul 7, 2023

CecileRobertMichon commented Jul 7, 2023 •

edited

Loading

CecileRobertMichon commented Jul 7, 2023

k8s-infra-cherrypick-robot commented Jul 7, 2023

k8s-ci-robot commented Jul 7, 2023

k8s-infra-cherrypick-robot commented Jul 7, 2023

Fix custom backendPool not being used #3676

Fix custom backendPool not being used #3676

Conversation

RadekManak commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

k8s-ci-robot commented Jun 29, 2023

nawazkh left a comment

Choose a reason for hiding this comment

nawazkh Jun 30, 2023

Choose a reason for hiding this comment

nawazkh Jun 30, 2023

Choose a reason for hiding this comment

RadekManak Jun 30, 2023 • edited Loading

Choose a reason for hiding this comment

nawazkh Jun 30, 2023

Choose a reason for hiding this comment

nawazkh Jun 30, 2023

Choose a reason for hiding this comment

RadekManak Jun 30, 2023

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 5, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 6, 2023

codecov bot commented Jul 6, 2023 • edited Loading

Codecov Report

nawazkh commented Jul 7, 2023

nawazkh commented Jul 7, 2023

CecileRobertMichon commented Jul 7, 2023 • edited Loading

CecileRobertMichon commented Jul 7, 2023

k8s-infra-cherrypick-robot commented Jul 7, 2023

k8s-ci-robot commented Jul 7, 2023

k8s-infra-cherrypick-robot commented Jul 7, 2023

RadekManak Jun 30, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading

CecileRobertMichon commented Jul 7, 2023 •

edited

Loading