🐛 Skip InboundNatRule reconciliation if no LB is configured

[alvaroaleman]

Clusters might be externally managed in which case the apiserver
endpoint might be in a different Azure account or on a different
platform altogether. In this case, there are no inboundnatrules for the
LB to reconcile, so skip doing that if the LB name is empty.

What type of PR is this?

/kind bug

What this PR does / why we need it:

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:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

If a cluster has no APIServer LoadBalancer configured, the InboundNATRule reconciliaton for machines will be skippped

[jackfrancis]

How is it possible to create a cluster without a apiserver load balancer? In other words, how does the above condition (empty string value for AzureCluster.Spec.NetworkSpec.APIServerLB.Name) ever occur?

[alvaroaleman]

@jackfrancis as mentioned in the PR description, this can happen if the cluster is managed externally, see https://capz.sigs.k8s.io/topics/externally-managed-azure-infrastructure.html

[jackfrancis]

Thanks. The code lgtm. I am a bit confused about this line in the docs:

If the AzureCluster resource includes a “cluster.x-k8s.io/managed-by” annotation then the controller will skip any reconciliation.

If reconciliation is skipped, then why is this change needed?

[alvaroaleman]

The cluster controllers skips reconciling it, however the azuremachine-related controllers still do:

cluster-api-provider-azure/controllers/azuremachine_reconciler.go

Lines 90 to 92 in a42074c

	if err := s.inboundNatRulesSvc.Reconcile(ctx); err != nil {
	return errors.Wrap(err, "failed to create inbound NAT rule")
	}

We want to use capz to only manage worker machines, which is how we ran into this.

[jackfrancis]

cc @Jont828

[jackfrancis]

@Jont828 we were talking offline yesterday about your wanting this scenario to s.Scope.UpdatePutStatus even if it's a no-op (no LB present). Can you explain why?

[alvaroaleman]

I thought about that, but it seemed to me that setting an InboundNATRulesReadyCondition will lead to confusion, because we have to set it to false which makes it look like there is an issue but there isn't. Happy to be convinced otherwise though.

[CecileRobertMichon]

Agree that setting the condition when there is no resource is confusing which is why we have #1868 open to address that. However in the meantime we need to set all conditions that are defined in https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/azure/scope/machine.go#L550 to true otherwise the AzureMachine won't reach Ready condition as summary will include false conditions. However in this case it looks like InboundNATRulesReadyCondition is missing from the list of summary conditions (potentially a bug?).

[alvaroaleman]

I see, updated the change to do the UpdatePutStatus even when there is no LB

[alvaroaleman]

/assign @CecileRobertMichon

[CecileRobertMichon]

could you please also add a comment that explains this is to support the externally managed use case? I could see someone else reading the code later and having the exact same reaction as Jack did above

[alvaroaleman]

Added a comment, let me know what you think

[CecileRobertMichon]

/lgtm

[alvaroaleman]

/retest

[alvaroaleman]

@CecileRobertMichon who should I ping for final approval?

[CecileRobertMichon]

/assign @jackfrancis @shysank

would like a second lgtm before I approve

[shysank]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shysank

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 [shysank]

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

[alvaroaleman]

Thank you all for taking the time to review this, I appreciate it :)