Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #4508 (2023-09-18 14:12:31)

Save Nat Gateways IPs to status on every reconciliation
**What type of PR is this?**

/kind bug

**What this PR does / why we need it**:

When we first create the nat gateways, they go to the `Pending` state. At that point, they don't have a PublicIP just yet. So if we only save the nat gateway public IP to the `status` field right after creating the nat gateway, we won't have the public IP available, meaning we won't be able to save it to the status.

Now the logic is changed so we always save the NAT Gateways IPs to the status field.

**Special notes for your reviewer**:

I've tested this version in our management clusters, adding our custom ingress rules and it all worked fine.

There was a missmatch in how the ingress rules were modeled in CAPA and the SDK, making the controller to keep reconciliating the same AWSCluster because the `SecurityGroups` saved in the field `.status.networkStatus.securityGroups` never matched the security groups returned by the AWS SDK. Now they match and no extra reconciliations are needed.

**Checklist**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR in which case these can be deleted -->

- [ ] squashed commits
- [ ] includes documentation
- [ ] adds unit tests
- [ ] adds or updates e2e tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
Fix how NAT gateways IPs are saved in the status field
```


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [justinsb](https://github.com/justinsb) for approval. For more information see [the Kubernetes Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["justinsb"]} -->
k8s-ci-robot: Hi @fiunchinho. Thanks for your PR.

I'm waiting for a [kubernetes-sigs](https://github.com/orgs/kubernetes-sigs/people) 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](https://git.k8s.io/community/community-membership.md#member) 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](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

Ankitasw: /test pull-cluster-api-provider-aws-e2e
Ankitasw: @fiunchinho did you test locally to check the status field for NAT gateway IP?
fiunchinho: Yes, it's saving the IPs on the status field. But the controller is logging errors, because the ingress rules are being wrongly modified by a bug that it's fixed here https://github.com/kubernetes-sigs/cluster-api-provider-aws/pull/4359
k8s-ci-robot: @fiunchinho: 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-aws-test | 212e9340120d98d3c4aad279feb25e3cef902535 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/4508/pull-cluster-api-provider-aws-test/1704811530089926656) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=4508). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Afiunchinho). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @fiunchinho: 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-aws-test | 0413cf589e1d173b979545d350d9773d91875a69 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api-provider-aws/4508/pull-cluster-api-provider-aws-test/1704881326357745664) | true | `/test pull-cluster-api-provider-aws-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=cluster-api-provider-aws&pr=4508). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Afiunchinho). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/cloud/services/network/natgateways.go b/pkg/cloud/services/network/natgateways.go
index 2134e4d5e..194816cb7 100644
--- a/pkg/cloud/services/network/natgateways.go
+++ b/pkg/cloud/services/network/natgateways.go
@@ -69,6 +69,7 @@ func (s *Service) reconcileNatGateways() error {
 		return err
 	}
 
+	natGatewaysIPs := []string{}
 	subnetIDs := []string{}
 
 	for _, sn := range s.scope.Subnets().FilterPublic() {
@@ -77,6 +78,9 @@ func (s *Service) reconcileNatGateways() error {
 		}
 
 		if ngw, ok := existing[sn.ID]; ok {
+			if len(ngw.NatGatewayAddresses) > 0 && ngw.NatGatewayAddresses[0].PublicIp != nil {
+				natGatewaysIPs = append(natGatewaysIPs, *ngw.NatGatewayAddresses[0].PublicIp)
+			}
 			// Make sure tags are up to date.
 			if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
 				buildParams := s.getNatGatewayTagParams(*ngw.NatGatewayId)
@@ -96,6 +100,8 @@ func (s *Service) reconcileNatGateways() error {
 		subnetIDs = append(subnetIDs, sn.ID)
 	}
 
+	s.scope.SetNatGatewaysIPs(natGatewaysIPs)
+
 	// Batch the creation of NAT gateways
 	if len(subnetIDs) > 0 {
 		// set NatGatewayCreationStarted if the condition has never been set before
@@ -106,18 +112,12 @@ func (s *Service) reconcileNatGateways() error {
 			}
 		}
 		ngws, err := s.createNatGateways(subnetIDs)
-		var natGatewaysIPs []string
 
 		for _, ng := range ngws {
 			subnet := s.scope.Subnets().FindByID(*ng.SubnetId)
 			subnet.NatGatewayID = ng.NatGatewayId
-			if len(ng.NatGatewayAddresses) > 0 && ng.NatGatewayAddresses[0].PublicIp != nil {
-				natGatewaysIPs = append(natGatewaysIPs, *ng.NatGatewayAddresses[0].PublicIp)
-			}
 		}
 
-		s.scope.SetNatGatewaysIPs(natGatewaysIPs)
-
 		if err != nil {
 			return err
 		}
diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go
index b5b6fd57e..ddbf67aee 100644
--- a/pkg/cloud/services/securitygroup/securitygroups.go
+++ b/pkg/cloud/services/securitygroup/securitygroups.go
@@ -728,68 +728,63 @@ func ingressRuleToSDKType(scope scope.SGScope, i *infrav1.IngressRule) (res *ec2
 }
 
 func ingressRulesFromSDKType(v *ec2.IpPermission) (res infrav1.IngressRules) {
+	for _, ec2range := range v.IpRanges {
+		rule := ingressRuleFromSDKProtocol(v)
+		if ec2range.Description != nil && *ec2range.Description != "" {
+			rule.Description = *ec2range.Description
+		}
+
+		rule.CidrBlocks = []string{*ec2range.CidrIp}
+		res = append(res, rule)
+	}
+
+	for _, ec2range := range v.Ipv6Ranges {
+		rule := ingressRuleFromSDKProtocol(v)
+		if ec2range.Description != nil && *ec2range.Description != "" {
+			rule.Description = *ec2range.Description
+		}
+
+		rule.IPv6CidrBlocks = []string{*ec2range.CidrIpv6}
+		res = append(res, rule)
+	}
+
+	for _, pair := range v.UserIdGroupPairs {
+		rule := ingressRuleFromSDKProtocol(v)
+		if pair.GroupId == nil {
+			continue
+		}
+
+		if pair.Description != nil && *pair.Description != "" {
+			rule.Description = *pair.Description
+		}
+
+		rule.SourceSecurityGroupIDs = []string{*pair.GroupId}
+		res = append(res, rule)
+	}
+
+	return res
+}
+
+func ingressRuleFromSDKProtocol(v *ec2.IpPermission) infrav1.IngressRule {
 	// Ports are only well-defined for TCP and UDP protocols, but EC2 overloads the port range
 	// in the case of ICMP(v6) traffic to indicate which codes are allowed. For all other protocols,
 	// including the custom "-1" All Traffic protocol, FromPort and ToPort are omitted from the response.
 	// See: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_IpPermission.html
-	var ir infrav1.IngressRule
 	switch *v.IpProtocol {
 	case IPProtocolTCP,
 		IPProtocolUDP,
 		IPProtocolICMP,
 		IPProtocolICMPv6:
-		ir = infrav1.IngressRule{
+		return infrav1.IngressRule{
 			Protocol: infrav1.SecurityGroupProtocol(*v.IpProtocol),
 			FromPort: *v.FromPort,
 			ToPort:   *v.ToPort,
 		}
 	default:
-		ir = infrav1.IngressRule{
+		return infrav1.IngressRule{
 			Protocol: infrav1.SecurityGroupProtocol(*v.IpProtocol),
 		}
 	}
-
-	if len(v.IpRanges) > 0 {
-		r1 := ir
-		for _, ec2range := range v.IpRanges {
-			if ec2range.Description != nil && *ec2range.Description != "" {
-				r1.Description = *ec2range.Description
-			}
-
-			r1.CidrBlocks = append(r1.CidrBlocks, *ec2range.CidrIp)
-		}
-		res = append(res, r1)
-	}
-
-	if len(v.Ipv6Ranges) > 0 {
-		r1 := ir
-		for _, ec2range := range v.Ipv6Ranges {
-			if ec2range.Description != nil && *ec2range.Description != "" {
-				r1.Description = *ec2range.Description
-			}
-
-			r1.IPv6CidrBlocks = append(r1.IPv6CidrBlocks, *ec2range.CidrIpv6)
-		}
-		res = append(res, r1)
-	}
-
-	if len(v.UserIdGroupPairs) > 0 {
-		r2 := ir
-		for _, pair := range v.UserIdGroupPairs {
-			if pair.GroupId == nil {
-				continue
-			}
-
-			if pair.Description != nil && *pair.Description != "" {
-				r2.Description = *pair.Description
-			}
-
-			r2.SourceSecurityGroupIDs = append(r2.SourceSecurityGroupIDs, *pair.GroupId)
-		}
-		res = append(res, r2)
-	}
-
-	return res
 }
 
 // getIngressRulesToAllowKubeletToAccessTheControlPlaneLB returns ingress rules required in the control plane LB.
@@ -800,7 +795,11 @@ func (s *Service) getIngressRulesToAllowKubeletToAccessTheControlPlaneLB() infra
 		return s.getIngressRuleToAllowVPCCidrInTheAPIServer()
 	}
 
+	natGatewaysCidrs := []string{}
 	natGatewaysIPs := s.scope.GetNatGatewaysIPs()
+	for _, ip := range natGatewaysIPs {
+		natGatewaysCidrs = append(natGatewaysCidrs, fmt.Sprintf("%s/32", ip))
+	}
 	if len(natGatewaysIPs) > 0 {
 		return infrav1.IngressRules{
 			{
@@ -808,7 +807,7 @@ func (s *Service) getIngressRulesToAllowKubeletToAccessTheControlPlaneLB() infra
 				Protocol:    infrav1.SecurityGroupProtocolTCP,
 				FromPort:    int64(s.scope.APIServerPort()),
 				ToPort:      int64(s.scope.APIServerPort()),
-				CidrBlocks:  natGatewaysIPs,
+				CidrBlocks:  natGatewaysCidrs,
 			},
 		}
 	}


Test patch:
diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go
index 5d4555288..2e194a9a3 100644
--- a/pkg/cloud/services/securitygroup/securitygroups_test.go
+++ b/pkg/cloud/services/securitygroup/securitygroups_test.go
@@ -871,7 +871,7 @@ func TestControlPlaneLoadBalancerIngressRules(t *testing.T) {
 					Protocol:    infrav1.SecurityGroupProtocolTCP,
 					FromPort:    6443,
 					ToPort:      6443,
-					CidrBlocks:  []string{"1.2.3.4"},
+					CidrBlocks:  []string{"1.2.3.4/32"},
 				},
 				infrav1.IngressRule{
 					Description: "Kubernetes API",
@@ -915,7 +915,7 @@ func TestControlPlaneLoadBalancerIngressRules(t *testing.T) {
 					Protocol:    infrav1.SecurityGroupProtocolTCP,
 					FromPort:    6443,
 					ToPort:      6443,
-					CidrBlocks:  []string{"1.2.3.4"},
+					CidrBlocks:  []string{"1.2.3.4/32"},
 				},
 				infrav1.IngressRule{
 					Description: "My custom ingress rule",
@@ -1250,6 +1250,40 @@ func TestIngressRulesFromSDKType(t *testing.T) {
 		input    *ec2.IpPermission
 		expected infrav1.IngressRules
 	}{
+		{
+			name: "two ingress rules",
+			input: &ec2.IpPermission{
+				IpProtocol: aws.String("tcp"),
+				FromPort:   aws.Int64(6443),
+				ToPort:     aws.Int64(6443),
+				IpRanges: []*ec2.IpRange{
+					{
+						CidrIp:      aws.String("0.0.0.0/0"),
+						Description: aws.String("Kubernetes API"),
+					},
+					{
+						CidrIp:      aws.String("192.168.1.1/32"),
+						Description: aws.String("My VPN"),
+					},
+				},
+			},
+			expected: infrav1.IngressRules{
+				{
+					Description: "Kubernetes API",
+					Protocol:    "tcp",
+					FromPort:    6443,
+					ToPort:      6443,
+					CidrBlocks:  []string{"0.0.0.0/0"},
+				},
+				{
+					Description: "My VPN",
+					Protocol:    "tcp",
+					FromPort:    6443,
+					ToPort:      6443,
+					CidrBlocks:  []string{"192.168.1.1/32"},
+				},
+			},
+		},
 		{
 			name: "Two group pairs",
 			input: &ec2.IpPermission{
@@ -1275,7 +1309,14 @@ func TestIngressRulesFromSDKType(t *testing.T) {
 					Protocol:               "tcp",
 					FromPort:               10250,
 					ToPort:                 10250,
-					SourceSecurityGroupIDs: []string{"sg-source-1", "sg-source-2"},
+					SourceSecurityGroupIDs: []string{"sg-source-1"},
+				},
+				{
+					Description:            "Kubelet API",
+					Protocol:               "tcp",
+					FromPort:               10250,
+					ToPort:                 10250,
+					SourceSecurityGroupIDs: []string{"sg-source-2"},
 				},
 			},
 		},