fix: don't create outbound LB if using NatGateway #1589

jackfrancis · 2021-08-06T19:51:44Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR fixes the nat-gateway + subnet reconciliation logic so that we stop creating unnecessary outbound LB resources if we're exclusively using NatGateway across all of our node subnets.

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 #1587

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:

don't create outbound LB if using NatGateway

api/v1alpha4/azurecluster_default.go

CecileRobertMichon · 2021-08-09T20:39:57Z

@jackfrancis looks like unit tests are failing, PTAL

api/v1alpha4/azurecluster_default.go

CecileRobertMichon · 2021-08-09T23:47:30Z

azure/services/natgateways/natgateways.go

@@ -118,12 +122,26 @@ func (s *Service) getExisting(ctx context.Context, spec azure.NatGatewaySpec) (*
 	if err != nil {
 		return nil, err
 	}
+	// We must have a non-nil PublicIPAddresses, and the underlying SubResource slice type must have a length of 1
+	// TODO do we want to eventually handle NatGateway resources w/ more than one public IP address?


eventually there might be use cases for having more than 1 IP (eg. if you need more than 64k connections) but currently, it's not supported. We should not aim to support it now but making it explicit is better than opening the door for future bugs IMO.

If there is more than one IP, is the right behavior to return an error and completely block reconciliation?

Defer to project maintainers. We could leave this here until that support is actually added (as a very obvious reminder that there are implementation details to be sorted out) or it could passively accept a bunch of IP addresses and then always just use the first one (see L130 below).

Since this error condition is new, I'll remove the novel "length must equal 1" condition and leave the comment that acknowledges that we're simply throwing away all IPs except the first one (index 0) if there are > 1.

api/v1alpha4/azurecluster_default_test.go

jackfrancis · 2021-08-10T18:44:37Z

api/v1alpha4/azurecluster_default_test.go

+			},
+		},
+		{
+			name: "NodeOutboundLB declared as input with non-respected values",


@CecileRobertMichon this is a weird case to test from a UX perspective but in fact it expresses literally what happens if you pass in a predefined LB object w/ either Name, SKU, or Type values. So if we want to statically override those values we want this UT; or, if we don't want to do that now's the time to think about what we do want to do.

The expected behavior, documented in https://capz.sigs.k8s.io/topics/node-outbound-lb.html#warning, is that these values are not configurable and modifying them should result in a validation error. It should not silently override the values. If that's what is happening right now then it's a bug. I think we should leave these values as is if already set and let the validation webhook do its job by failing if the values are not what we expect (validation should already be in place). This also applies to the control plane outbound LB.

This is a bit out of scope for this PR so feel free to open an issue if you don't want to address this as part of this PR.

I think what we're observing is that this condition (user-configurable value overrides of properties that aren't user-overridable) is artificial; as you say, validation will prevent this condition from ever reaching this point in the code flow. I vote that we just not add this artificial test case (I'll delete it).

jackfrancis · 2021-08-11T20:28:54Z

/test pull-cluster-api-provider-azure-e2e

azure/services/subnets/subnets_test.go

jackfrancis · 2021-08-17T17:37:40Z

azure/services/subnets/subnets_test.go

+			},
+		},
+		{
+			name:          "spec has empty CIDR and ID data",


@CecileRobertMichon PTAL

not sure I understand what scenario this is testing

We could also test that if CIDRBlocks and ID are empty in the existing spec, it will set them with the Azure subnet values.

ah, sorry for the confusion

what I meant there was infrav1.SubnetSpec returned has no ID/CIDR when calling s.Subnet and the Azure GET returns the values, which are used in s.SetSubnet (which means we're updating the spec to have these values persisted in AzureCluster.Spec)

CecileRobertMichon

lgtm pending comment on UT

azure/services/natgateways/natgateways.go

CecileRobertMichon

/lgtm
/assign @shysank

shysank · 2021-08-23T17:56:45Z

/lgtm
@jackfrancis I had some trouble figuring out the actual issue, It'd be great if you can add the steps to reproduce the bug. From slack conversation with @CecileRobertMichon

you can repro this by creating a cluster with nat gateway on the node subnet, the first reconcile the nat gateway gets created, no outbound LB is created. After a few minutes, the nat gateway disappears from the subnet spec in the AzureCluster and an outbound LB gets created.

jackfrancis · 2021-08-23T18:01:58Z

I think the core issue is described in the issue: #1587

In summary, a Load Balancer created solely for providing outbound SNAT is unnecessary in a cluster configuration where a NAT Gateway is declared for doing just that. Hopefully that's clear. Certainly happy to clarify if it's not already.

CecileRobertMichon · 2021-08-23T18:06:20Z

@jackfrancis can you please add details about the root cause that we observed after investigating? It's not obvious from reading the issue that the LB doesn't get created immediately and is due to the nat gateway being erased from the spec. We're also fixing another bug in this PR (IP ID comparison with IP name) which is missing from the description.

/approve
/hold
until above is addressed

k8s-ci-robot · 2021-08-23T18:06:41Z

[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

CecileRobertMichon · 2021-08-23T20:47:43Z

/hold cancel

jackfrancis · 2021-08-23T22:04:57Z

/retest

jackfrancis · 2021-08-24T00:09:51Z

/retest

CecileRobertMichon · 2021-08-24T02:17:10Z

/lgtm

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

k8s-ci-robot requested review from CecileRobertMichon and devigned August 6, 2021 19:51

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 6, 2021

jackfrancis commented Aug 6, 2021

View reviewed changes

api/v1alpha4/azurecluster_default.go Show resolved Hide resolved

jackfrancis commented Aug 9, 2021

View reviewed changes

api/v1alpha4/azurecluster_default.go Show resolved Hide resolved

CecileRobertMichon reviewed Aug 9, 2021

View reviewed changes

jackfrancis force-pushed the nat-gateway-lb-fix branch from 1f1ff9a to 8751579 Compare August 10, 2021 18:29

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 10, 2021