wrapping the below for loop in if s.SecurityGroup.SecurityRules != nil { is equivalent and allows us to remove the add'l return nil response (we don't need a specific nil return for this condition because the default return before exiting the function is nil)

Should be fixed, thanks.

in this flow where are we adding any user-provided security rules? (for example if a user specifies TCP 9345)

or is that elsewhere and the purpose of this change is to filter out 22 and apiserver port if it's not included?

This flow performs only defaulting on top of the user-provided set of rules, which may not be empty. This way having a field populated does not always need to specify all allowed ports, only additional ones or overrides.

I’m currently validating how this works in tandem with ClusterClass definitions we have. The desired goal is to permit specifying registrationMethod: control-plane-endpoint and allow RKE2 to register on the allowed port. But ideally this should follow internal name resolution, so that the traffic wouldn’t go through external load balancer.

If it doesn’t work, maybe it would still require additional LB rules.

It seems reasonable to have the additional ports of the APIServer LB opened-up/have an opinion about.

Could you please elaborate on why the security rules should not be updated too ?

Sorry, maybe I’m not fully understanding. I will try to give my thoughts on this:

I was attempting to describe the flow of how the rules are processed. We only apply defaults on top of what the user provides, so the user can still specify ports like TCP 9345 explicitly in security rules. We’re not overriding or stripping anything out, just adding fallback values when needed. LB rules were needed as well, since CP endpoint is external from the cluster perspective.

Current set of changes is what was required to make cluster healthy, and added e2e test displays that. My previous comment was a set of assumptions, or “paths to explore later” if it doesn’t work.

port seems to be holding a single port here.

It was originally like so, but users can supply a list o ports with this, like 22,443

But the current implementation does not seem to be iterating over a comma-separated list of ports ?

I doublechecked it, maybe my initial assumption was wrong. It can be a number or an * there, so changed it to port as suggested.

Probably for the long term, we should change s.SecurityGroup.SecurityRules to a dictionary type for faster processing.

Why are we updating this example to a Deny SSH ?

I wanted to showcase that even though port 22 is defaulted, the rule for this can be overridden to deny it.

I think it would benefit if we show an overriding scenario the example you added that adds 9345 to the APIServer LB.

Updating the rule to Deny could throw off users in the first glance.

Was this left intentionally with replace-me value ?

This was replaced by CC patch, but it turns out that having this field set by default causes errors on double template apply, which might have affected e2e execution. Replaced with full template patch now.

What do you think of renaming AdditionalControlPlaneLBPorts to AdditionalAPIServerLBPorts ?

CAPZ has APIServerLB, NodeOutboundLB and ControlplaneOutboundLB. Since the additional ports will be used in APIServerLB, it is better, I believe, to have the additional ports renamed to AdditionalAPIServerLBPorts

But the current implementation does not seem to be iterating over a comma-separated list of ports ?

Same as above

It seems reasonable to have the additional ports of the APIServer LB opened-up/have an opinion about.

Could you please elaborate on why the security rules should not be updated too ?

Could you please add more context on setting s.AzureCluster.Spec.NetworkSpec.UpdateControlPlaneSubnet(subnet) three times in this function ?
Could we not update it once in the end of the function ?

We can, but by equivalent aggregation of ifs evaluating to true. Since the method is only setting struct default values, I assumed the cost is negligible, for the improved readability and confidence of data being persisted.

Is it possible to have a more granular check here instead of checking for not nil ?

Updated it to Ready condition, which:

is true if the Machine's deletionTimestamp is not set, Machine's BootstrapConfigReady, InfrastructureReady,
	// NodeHealthy and HealthCheckSucceeded

But the main goal of the check is to verify that CP nodes joined cluster in the expected quantity. Previous nil check fulfilled that too…

I think it would benefit if we show an overriding scenario the example you added that adds 9345 to the APIServer LB.

Updating the rule to Deny could throw off users in the first glance.