Default set NATGatway for outbound traffic if cluster is not using IPv6 #3365

xiujuanx · 2023-03-27T08:33:19Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR sets the default subnet to be NAT gateway if the cluster is not using IPv6. It both sets AzureCluster and AzureClusterTemplate.

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

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:

Default use the NAT gateway for the node outbound connection if cluster is not using IPv6

k8s-ci-robot · 2023-03-27T08:33:28Z

Hi @xiujuanx. 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.

willie-yao · 2023-03-27T18:04:24Z

/ok-to-test

willie-yao · 2023-03-27T18:38:14Z

api/v1beta1/azurecluster_default.go

@@ -128,6 +130,12 @@ func (c *AzureCluster) setSubnetDefaults() {
 			if subnet.NatGateway.NatGatewayIP.Name == "" {
 				subnet.NatGateway.NatGatewayIP.Name = generateNatGatewayIPName(c.ObjectMeta.Name, subnet.Name)
 			}
+		} else {
+			if !subnet.IsIPv6Enabled() {


Should this if statement be outside of the else block? Looking through the issue details, it looks like the intended behavior is to set NAT Gateway by default if not using IPv6, regardless of whether a name has been provided or not.

Thanks @willie-yao for your review! I think put the statement be outside of the else block is ok.
For a subnet, if the NAT gateway name is provided, this means NAT gateway has beed enabled, we don't need to set it again. If the NAT gateway name doesn't be provided, then I will go to check if it's using IPv6, if not, then I will set NAT gateway by default, otherwise, I will keep the original subnet settings and do nothing.

Ah that makes sense, thanks for the detailed explanation!

instead of adding an else condition here with another nested if loop, have you considered directly modifying the conditions for which IsNatGatewayEnabled returns true? and setting the name in NAT gateway name in the block above when it's not already set. This would avoid duplicate code of setting the IP name and nested if/else statements.

@CecileRobertMichon Thanks for your review! Indeed, there has a duplicate code for setting NAT gateway IP name. But if I modify the condition of IsNatGatewayEnabled and set the name of NAT gateway name if it's not set yet in this func, then I need to call generateNatGatewayName func in type.go file. This will cause an inconsistency from current CAPZ codes in main branch. Although this does not cause any logical problems, it just looks less elegant formally. So I prefer to judge if this is a IPv6 cluster first, then go to judge if IsNatGatewayEnabled or not. This will also avoid duplication. Does this make sense?

api/v1beta1/azurecluster_default.go

api/v1beta1/types.go

api/v1beta1/types_template.go

azure/scope/cluster_test.go

api/v1beta1/azureclustertemplate_default_test.go

api/v1beta1/azurecluster_default_test.go

willie-yao · 2023-04-03T22:46:10Z

/lgtm

Apologies for the delay. Thanks for getting back to me about the tests! This looks great and thanks for the great work on this @xiujuanx

k8s-ci-robot · 2023-04-03T22:46:16Z

LGTM label has been added.

Git tree hash: 06fdafa7548ee8271626444a141eec984b95f517

xiujuanx · 2023-04-04T01:18:58Z

@willie-yao Thanks for your kind review! Does this pr can be approved now?

willie-yao · 2023-04-04T18:50:49Z

@xiujuanx Yes, pinging approvers for a review. @CecileRobertMichon @mboersma @jackfrancis

xiujuanx · 2023-04-09T12:46:23Z

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

xiujuanx · 2023-04-10T02:15:02Z

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

CecileRobertMichon

/retest

#3403 should fix the broken test

/lgtm
/approve

Thanks @xiujuanx, great work!

k8s-ci-robot · 2023-04-10T15:42:51Z

LGTM label has been added.

Git tree hash: 94d9a623e850934f5343b9da8f99f8c1f94570bf

k8s-ci-robot · 2023-04-10T15:42:54Z

[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

sonasingh46 · 2023-04-10T16:46:58Z

Late to party on this PR.
What happens in upgrade scenario when the cluster already have chosen a load balancer. Does it silently ignore it?

CecileRobertMichon · 2023-04-10T16:48:52Z

/hold

to answer @sonasingh46's question above

CecileRobertMichon · 2023-04-10T16:50:14Z

What happens in upgrade scenario when the cluster already have chosen a load balancer. Does it silently ignore it?

This is only changing the webhook default so it shouldn't affect existing clusters AFAIK

willie-yao

/lgtm

Thanks again for all the great work on this PR! 🚀

sonasingh46 · 2023-04-10T22:51:07Z

@CecileRobertMichon -- Thanks. I am just thinking about the following scenario.

Prior to this change, a user can have an existing cluster that uses load balancer because of the defaults. If NAT gateway is not specified by default then load balancer is chosen.
With this change, if NAT gateway is not specified and if it is not IPV6 then it uses NAT gateway by default now.
So as far as compatibility is concerned, nothing breaks and all works fine but I am thinking if we should have a way to migrate away from load balancer and choose NAT gateway.
Now, IIRC the migration was not possible earlier also so I think this discussion is out of scope of the PR.

sonasingh46 · 2023-04-10T22:51:17Z

/lgtm

CecileRobertMichon · 2023-04-10T23:02:23Z

@sonasingh46 I think the migration would be possible but the user would have to opt in (i.e. edit the resource and specify a NAT), have not tested this though.

agree it's out of scope for this PR

/hold cancel

CecileRobertMichon · 2023-04-10T23:02:36Z

@sonasingh46 I think the migration would be possible but the user would have to opt in (i.e. edit the resource and specify a NAT), have not tested this though.

agree it's out of scope for this PR

/hold cancel

k8s-ci-robot requested review from Jont828 and willie-yao March 27, 2023 08:33

xiujuanx force-pushed the default-set-natgateway branch from 37b3948 to 4e4db5a Compare March 27, 2023 08:35

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 Mar 27, 2023

willie-yao reviewed Mar 27, 2023

View reviewed changes

api/v1beta1/azurecluster_default.go Outdated Show resolved Hide resolved

willie-yao reviewed Mar 27, 2023

View reviewed changes

api/v1beta1/types.go Show resolved Hide resolved

willie-yao reviewed Mar 27, 2023

View reviewed changes

api/v1beta1/types_template.go Show resolved Hide resolved

xiujuanx force-pushed the default-set-natgateway branch 2 times, most recently from 6d78d72 to 950a4b1 Compare March 28, 2023 02:41

xiujuanx requested review from willie-yao and removed request for Jont828 March 28, 2023 02:42

xiujuanx force-pushed the default-set-natgateway branch from 950a4b1 to 81f8811 Compare March 28, 2023 07:34

willie-yao reviewed Mar 28, 2023

View reviewed changes

azure/scope/cluster_test.go Show resolved Hide resolved

api/v1beta1/azureclustertemplate_default_test.go Outdated Show resolved Hide resolved

api/v1beta1/azurecluster_default_test.go Show resolved Hide resolved

xiujuanx requested a review from willie-yao March 29, 2023 05:32

bavarianbidi mentioned this pull request Mar 30, 2023

Management Cluster shouldn't locked out if Workload Cluster has custom security group rules applied #3375

Closed

k8s-ci-robot assigned willie-yao Apr 3, 2023

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 9, 2023

default set NATGatway for outbound traffic if not using IPv6

e60f052

xiujuanx force-pushed the default-set-natgateway branch from 1cb71ab to e60f052 Compare April 9, 2023 08:24

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2023

xiujuanx requested a review from CecileRobertMichon April 9, 2023 08:37

CecileRobertMichon reviewed Apr 10, 2023

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon Apr 10, 2023

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

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2023

willie-yao approved these changes Apr 10, 2023

View reviewed changes

k8s-ci-robot assigned sonasingh46 Apr 10, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2023

k8s-ci-robot merged commit 946fdd3 into kubernetes-sigs:main Apr 11, 2023

CecileRobertMichon mentioned this pull request Apr 13, 2023

capi-periodic-upgrade-main-1-25-1-26 test failing due to NAT GW name length #3421

Closed

xiujuanx mentioned this pull request May 10, 2023

allow setting the NatGateway name if it's empty when update AzureCluster #3551

Merged

4 tasks

dkoshkin mentioned this pull request Jun 24, 2023

Egress on existing clusters using node load balancer breaks after v1.9.x upgrade #3664

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default set NATGatway for outbound traffic if cluster is not using IPv6 #3365

Default set NATGatway for outbound traffic if cluster is not using IPv6 #3365

xiujuanx commented Mar 27, 2023 •

edited

Loading

k8s-ci-robot commented Mar 27, 2023

willie-yao commented Mar 27, 2023

willie-yao Mar 27, 2023

xiujuanx Mar 28, 2023

willie-yao Mar 28, 2023

CecileRobertMichon Apr 4, 2023

xiujuanx Apr 6, 2023

willie-yao commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

xiujuanx commented Apr 4, 2023

willie-yao commented Apr 4, 2023

xiujuanx commented Apr 9, 2023

xiujuanx commented Apr 10, 2023

CecileRobertMichon left a comment

k8s-ci-robot commented Apr 10, 2023

k8s-ci-robot commented Apr 10, 2023

sonasingh46 commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

willie-yao left a comment

sonasingh46 commented Apr 10, 2023

sonasingh46 commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

Default set NATGatway for outbound traffic if cluster is not using IPv6 #3365

Default set NATGatway for outbound traffic if cluster is not using IPv6 #3365

Conversation

xiujuanx commented Mar 27, 2023 • edited Loading

k8s-ci-robot commented Mar 27, 2023

willie-yao commented Mar 27, 2023

willie-yao Mar 27, 2023

Choose a reason for hiding this comment

xiujuanx Mar 28, 2023

Choose a reason for hiding this comment

willie-yao Mar 28, 2023

Choose a reason for hiding this comment

CecileRobertMichon Apr 4, 2023

Choose a reason for hiding this comment

xiujuanx Apr 6, 2023

Choose a reason for hiding this comment

willie-yao commented Apr 3, 2023

k8s-ci-robot commented Apr 3, 2023

xiujuanx commented Apr 4, 2023

willie-yao commented Apr 4, 2023

xiujuanx commented Apr 9, 2023

xiujuanx commented Apr 10, 2023

CecileRobertMichon left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 10, 2023

k8s-ci-robot commented Apr 10, 2023

sonasingh46 commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

willie-yao left a comment

Choose a reason for hiding this comment

sonasingh46 commented Apr 10, 2023

sonasingh46 commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

CecileRobertMichon commented Apr 10, 2023

xiujuanx commented Mar 27, 2023 •

edited

Loading