[release-2.4] 🐛 ec2: instances: fix assigning public IP (#4908)

* 🐛ec2: instances: fix check for public subnets

It's not enough to check MapPublicIPOnLaunch since public subnets can
have that off.

* 🐛ec2: instances: fix assigning public IP

In the scenario where the user brings their own VPC, if no subnet ID is
set in the machine spec and PublicIP is true, CAPA will choose one from
the available public subnets. However, if the subnet doesn't have
MapPublicIPOnLaunch == true, the instance will not be assigned a public
IP. As a result, the instance will have no internet access, contrary to
the user's expectation.

This change guarantees an instance will be assigned a public IP even if
the subnet doesn't do it on instance launch. Instead, we set the option
in the instance's network interface.

* 🌱ec2: instances: add unit tests for MapPublicIpOnLaunch=false

The tests check that a NetworkInterface is defined with
`AssociatePublicIpAddress` in the `RunInstances` input.

---------

Co-authored-by: Rafael Fonseca <[email protected]>

Author: k8s-infra-cherrypick-robot

api/v1beta1/awscluster_conversion.go

@@ -57,6 +57,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {
 		dst.Status.Bastion.InstanceMetadataOptions = restored.Status.Bastion.InstanceMetadataOptions
 		dst.Status.Bastion.PlacementGroupName = restored.Status.Bastion.PlacementGroupName
 		dst.Status.Bastion.PrivateDNSName = restored.Status.Bastion.PrivateDNSName
+		dst.Status.Bastion.PublicIPOnLaunch = restored.Status.Bastion.PublicIPOnLaunch
 	}
 	dst.Spec.Partition = restored.Spec.Partition
 

api/v1beta1/zz_generated.conversion.go

@@ -237,6 +237,10 @@ type Instance struct {
 	// PrivateDNSName is the options for the instance hostname.
 	// +optional
 	PrivateDNSName *PrivateDNSName `json:"privateDnsName,omitempty"`
+
+	// PublicIPOnLaunch is the option to associate a public IP on instance launch
+	// +optional
+	PublicIPOnLaunch *bool `json:"publicIPOnLaunch,omitempty"`
 }
 
 // InstanceMetadataState describes the state of InstanceMetadataOptions.HttpEndpoint and InstanceMetadataOptions.InstanceMetadataTags

api/v1beta2/types.go

@@ -1132,6 +1132,10 @@ spec:
                   privateIp:
                     description: The private IPv4 address assigned to the instance.
                     type: string
+                  publicIPOnLaunch:
+                    description: PublicIPOnLaunch is the option to associate a public
+                      IP on instance launch
+                    type: boolean
                   publicIp:
                     description: The public IPv4 address assigned to the instance,
                       if applicable.
@@ -2984,6 +2988,10 @@ spec:
                   privateIp:
                     description: The private IPv4 address assigned to the instance.
                     type: string
+                  publicIPOnLaunch:
+                    description: PublicIPOnLaunch is the option to associate a public
+                      IP on instance launch
+                    type: boolean
                   publicIp:
                     description: The public IPv4 address assigned to the instance,
                       if applicable.

api/v1beta2/zz_generated.deepcopy.go

@@ -1907,6 +1907,10 @@ spec:
                   privateIp:
                     description: The private IPv4 address assigned to the instance.
                     type: string
+                  publicIPOnLaunch:
+                    description: PublicIPOnLaunch is the option to associate a public
+                      IP on instance launch
+                    type: boolean
                   publicIp:
                     description: The public IPv4 address assigned to the instance,
                       if applicable.

config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml

@@ -181,6 +181,21 @@ func (s *Service) CreateInstance(scope *scope.MachineScope, userData []byte, use
 	}
 	input.SubnetID = subnetID
 
+	if ptr.Deref(scope.AWSMachine.Spec.PublicIP, false) {
+		subnets, err := s.getFilteredSubnets(&ec2.Filter{
+			Name:   aws.String("subnet-id"),
+			Values: aws.StringSlice([]string{subnetID}),
+		})
+		if err != nil {
+			return nil, fmt.Errorf("could not query if subnet has MapPublicIpOnLaunch set: %w", err)
+		}
+		if len(subnets) == 0 {
+			return nil, fmt.Errorf("expected to find subnet %q", subnetID)
+		}
+		// If the subnet does not assign public IPs, set that option in the instance's network interface
+		input.PublicIPOnLaunch = ptr.To(!aws.BoolValue(subnets[0].MapPublicIpOnLaunch))
+	}
+
 	if !scope.IsControlPlaneExternallyManaged() && !scope.IsExternallyManaged() && !scope.IsEKSManaged() && s.scope.Network().APIServerELB.DNSName == "" {
 		record.Eventf(s.scope.InfraCluster(), "FailedCreateInstance", "Failed to run controlplane, APIServer ELB not available")
 		return nil, awserrors.NewFailedDependency("failed to run controlplane, APIServer ELB not available")
@@ -334,7 +349,7 @@ func (s *Service) findSubnet(scope *scope.MachineScope) (string, error) {
 					*subnet.SubnetId, *subnet.AvailabilityZone, *failureDomain)
 				continue
 			}
-			if scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP && !*subnet.MapPublicIpOnLaunch {
+			if scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP && !s.scope.Subnets().FindByID(*subnet.SubnetId).IsPublic {
 				errMessage += fmt.Sprintf(" subnet %q is a private subnet.", *subnet.SubnetId)
 				continue
 			}
@@ -538,13 +553,25 @@ func (s *Service) runInstance(role string, i *infrav1.Instance) (*infrav1.Instan
 				DeviceIndex:        aws.Int64(int64(index)),
 			})
 		}
+		netInterfaces[0].AssociatePublicIpAddress = i.PublicIPOnLaunch
 
 		input.NetworkInterfaces = netInterfaces
 	} else {
-		input.SubnetId = aws.String(i.SubnetID)
+		if ptr.Deref(i.PublicIPOnLaunch, false) {
+			input.NetworkInterfaces = []*ec2.InstanceNetworkInterfaceSpecification{
+				{
+					DeviceIndex:              aws.Int64(0),
+					SubnetId:                 aws.String(i.SubnetID),
+					Groups:                   aws.StringSlice(i.SecurityGroupIDs),
+					AssociatePublicIpAddress: i.PublicIPOnLaunch,
+				},
+			}
+		} else {
+			input.SubnetId = aws.String(i.SubnetID)
 
-		if len(i.SecurityGroupIDs) > 0 {
-			input.SecurityGroupIds = aws.StringSlice(i.SecurityGroupIDs)
+			if len(i.SecurityGroupIDs) > 0 {
+				input.SecurityGroupIds = aws.StringSlice(i.SecurityGroupIDs)
+			}
 		}
 	}