🐛: ec2/byoip: fix EIP leak when creating machine

The instance creation flow is creating by default EIP to instances even
if the BYO IP flow is set. BYO IPv4 creates and associates the EIP to
instance after it is created, preventing paying for additional EIP
(amazon-provided) when creating the instance when the BYO IPv4 Pool is
defined to be used by the machine.

Furthermore, the fix provides additional checks to prevent duplicated EIP
in the BYO IP reconciliation loop. The extra checks include running the
EIP association many times, while the EIP is already associated, and
failures in the log when running the EIP association prematurely - when
the instance isn't ready, Eg ec2 in pending state.

Author: mtulio

controllers/awsmachine_controller.go

@@ -533,17 +533,20 @@ func (r *AWSMachineReconciler) reconcileNormal(_ context.Context, machineScope *
 	}
 
 	// BYO Public IPv4 Pool feature: allocates and associates an EIP to machine when PublicIP and
-	// cluster-wide Public IPv4 Pool configuration are set. The EIP must be associated after the instance
-	// is created and transictioned from Pending state.
-	// In the regular flow, if the instance have already a public IPv4 address (EIP) associated it will
-	// be released when a new is assigned, the createInstance() prevents that behavior by enforcing
-	// to not launch an instance with EIP, allowing ReconcileElasticIPFromPublicPool assigning
-	// a BYOIP without duplication.
+	// cluster-wide config Public IPv4 Pool configuration are set. The custom EIP is associated
+	// after the instance is created and transictioned to Running state.
+	// The CreateInstance() is enforcing to not assign public IP address when PublicIP is set with
+	// BYOIpv4 Pool, preventing a duplicated EIP creation.
 	if pool := machineScope.GetElasticIPPool(); pool != nil {
-		if err := ec2svc.ReconcileElasticIPFromPublicPool(pool, instance); err != nil {
-			machineScope.Error(err, "failed to associate elastic IP address")
+		requeue, err := ec2svc.ReconcileElasticIPFromPublicPool(pool, instance)
+		if err != nil {
+			machineScope.Error(err, "Failed to reconcile BYO Public IPv4")
 			return ctrl.Result{}, err
 		}
+		if requeue {
+			machineScope.Debug("Found instance in pending state while reconciling publicIpv4Pool, requeue", "instance", instance.ID)
+			return ctrl.Result{RequeueAfter: DefaultReconcilerRequeue}, nil
+		}
 	}
 
 	if feature.Gates.Enabled(feature.EventBridgeInstanceState) {

controllers/awsmachine_controller_unit_test.go

@@ -1023,7 +1023,7 @@ func TestAWSMachineReconciler(t *testing.T) {
 					PublicIpv4Pool:              aws.String("ipv4pool-ec2-0123456789abcdef0"),
 					PublicIpv4PoolFallBackOrder: ptr.To(infrav1.PublicIpv4PoolFallbackOrderAmazonPool),
 				}
-				ec2Svc.EXPECT().ReconcileElasticIPFromPublicPool(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+				ec2Svc.EXPECT().ReconcileElasticIPFromPublicPool(gomock.Any(), gomock.Any()).Return(false, nil).AnyTimes()
 
 				_, err := reconciler.reconcileNormal(context.Background(), ms, cs, cs, cs, cs)
 				g.Expect(err).To(BeNil())

pkg/cloud/services/ec2/eip.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"k8s.io/utils/ptr"
 
 	infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2"
 	"sigs.k8s.io/cluster-api-provider-aws/v2/pkg/record"
@@ -16,17 +17,46 @@ func getElasticIPRoleName(instanceID string) string {
 }
 
 // ReconcileElasticIPFromPublicPool reconciles the elastic IP from a custom Public IPv4 Pool.
-func (s *Service) ReconcileElasticIPFromPublicPool(pool *infrav1.ElasticIPPool, instance *infrav1.Instance) error {
-	// TODO: check if the instance is in the state allowing EIP association.
-	// Expected instance states: pending or running
-	// https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/ec2-instance-lifecycle.html
+func (s *Service) ReconcileElasticIPFromPublicPool(pool *infrav1.ElasticIPPool, instance *infrav1.Instance) (bool, error) {
+	shouldRequeue := true
+	iip := ptr.Deref(instance.PublicIP, "")
+	s.scope.Debug("Reconciling machine with custom Public IPv4 Pool", "instance-id", instance.ID, "instance-state", instance.State, "instance-public-ip", iip, "publicIpv4PoolID", pool.PublicIpv4Pool)
+
+	// Requeue when the instance is not ready to be associated.
+	if instance.State != infrav1.InstanceStateRunning {
+		s.scope.Debug("Unable to reconcile Elastic IP Pool for instance", "instance-id", instance.ID, "instance-state", instance.State)
+		return shouldRequeue, nil
+	}
+
+	// All done, must reconcile only when the instance is in running state.
+	shouldRequeue = false
+
+	// Prevent running association every reconciliation when it is already done.
+	addrs, err := s.netService.GetAddresses(getElasticIPRoleName(instance.ID))
+	if err != nil {
+		s.scope.Error(err, "error checking if addresses exists for Elastic IP Pool to machine", "eip-role", getElasticIPRoleName(instance.ID))
+		return shouldRequeue, err
+	}
+	if len(addrs.Addresses) > 0 {
+		if len(addrs.Addresses) != 1 {
+			return shouldRequeue, fmt.Errorf("unexpected number of EIPs allocated to the role. expected 1, got %d", len(addrs.Addresses))
+		}
+		addr := addrs.Addresses[0]
+		// address is already associated.
+		if addr.AssociationId != nil && addr.InstanceId != nil && *addr.InstanceId == instance.ID {
+			s.scope.Debug("Machine is already associated with an Elastic IP with custom Public IPv4 pool", "eip", addr.AllocationId, "eip-address", addr.PublicIp, "eip-associationID", addr.AssociationId, "eip-instance", addr.InstanceId)
+			return shouldRequeue, nil
+		}
+	}
+
+	// Associate EIP.
 	if err := s.getAndAssociateAddressesToInstance(pool, getElasticIPRoleName(instance.ID), instance.ID); err != nil {
-		return fmt.Errorf("failed to reconcile Elastic IP: %w", err)
+		return shouldRequeue, fmt.Errorf("failed to reconcile Elastic IP: %w", err)
 	}
-	return nil
+	return shouldRequeue, nil
 }
 
-// ReleaseElasticIP releases a specific elastic IP based on the instance role.
+// ReleaseElasticIP releases a specific Elastic IP based on the instance role.
 func (s *Service) ReleaseElasticIP(instanceID string) error {
 	return s.netService.ReleaseAddressByRole(getElasticIPRoleName(instanceID))
 }

pkg/cloud/services/ec2/instances.go

@@ -572,21 +572,13 @@ func (s *Service) runInstance(role string, i *infrav1.Instance) (*infrav1.Instan
 
 		input.NetworkInterfaces = netInterfaces
 	} else {
-		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)
-			}
+		input.NetworkInterfaces = []*ec2.InstanceNetworkInterfaceSpecification{
+			{
+				DeviceIndex:              aws.Int64(0),
+				SubnetId:                 aws.String(i.SubnetID),
+				Groups:                   aws.StringSlice(i.SecurityGroupIDs),
+				AssociatePublicIpAddress: i.PublicIPOnLaunch,
+			},
 		}
 	}
 

pkg/cloud/services/ec2/instances_test.go

@@ -672,11 +672,16 @@ func TestCreateInstance(t *testing.T) {
 				}, nil)
 				m.
 					RunInstancesWithContext(context.TODO(), &ec2.RunInstancesInput{
-						ImageId:          aws.String("abc"),
-						InstanceType:     aws.String("m5.2xlarge"),
-						KeyName:          aws.String("default"),
-						SecurityGroupIds: aws.StringSlice([]string{"2", "3"}),
-						SubnetId:         aws.String("subnet-3"),
+						ImageId:      aws.String("abc"),
+						InstanceType: aws.String("m5.2xlarge"),
+						KeyName:      aws.String("default"),
+						NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+							{
+								DeviceIndex: aws.Int64(0),
+								SubnetId:    aws.String("subnet-3"),
+								Groups:      aws.StringSlice([]string{"2", "3"}),
+							},
+						},
 						TagSpecifications: []*ec2.TagSpecification{
 							{
 								ResourceType: aws.String("instance"),
@@ -920,11 +925,16 @@ func TestCreateInstance(t *testing.T) {
 				}, nil)
 				m.
 					RunInstancesWithContext(context.TODO(), &ec2.RunInstancesInput{
-						ImageId:          aws.String("abc"),
-						InstanceType:     aws.String("m5.2xlarge"),
-						KeyName:          aws.String("default"),
-						SecurityGroupIds: aws.StringSlice([]string{"4", "3"}),
-						SubnetId:         aws.String("subnet-5"),
+						ImageId:      aws.String("abc"),
+						InstanceType: aws.String("m5.2xlarge"),
+						KeyName:      aws.String("default"),
+						NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+							{
+								DeviceIndex: aws.Int64(0),
+								SubnetId:    aws.String("subnet-5"),
+								Groups:      aws.StringSlice([]string{"4", "3"}),
+							},
+						},
 						TagSpecifications: []*ec2.TagSpecification{
 							{
 								ResourceType: aws.String("instance"),
@@ -3346,8 +3356,13 @@ func TestCreateInstance(t *testing.T) {
 						Placement: &ec2.Placement{
 							Tenancy: &tenancy,
 						},
-						SecurityGroupIds: []*string{aws.String("2"), aws.String("3")},
-						SubnetId:         aws.String("subnet-1"),
+						NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+							{
+								DeviceIndex: aws.Int64(0),
+								SubnetId:    aws.String("subnet-1"),
+								Groups:      []*string{aws.String("2"), aws.String("3")},
+							},
+						},
 						TagSpecifications: []*ec2.TagSpecification{
 							{
 								ResourceType: aws.String("instance"),
@@ -3555,8 +3570,13 @@ func TestCreateInstance(t *testing.T) {
 						Placement: &ec2.Placement{
 							GroupName: aws.String("placement-group1"),
 						},
-						SecurityGroupIds: []*string{aws.String("2"), aws.String("3")},
-						SubnetId:         aws.String("subnet-1"),
+						NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+							{
+								DeviceIndex: aws.Int64(0),
+								SubnetId:    aws.String("subnet-1"),
+								Groups:      []*string{aws.String("2"), aws.String("3")},
+							},
+						},
 						TagSpecifications: []*ec2.TagSpecification{
 							{
 								ResourceType: aws.String("instance"),
@@ -3785,8 +3805,13 @@ func TestCreateInstance(t *testing.T) {
 							Tenancy:   &tenancy,
 							GroupName: aws.String("placement-group1"),
 						},
-						SecurityGroupIds: []*string{aws.String("2"), aws.String("3")},
-						SubnetId:         aws.String("subnet-1"),
+						NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+							{
+								DeviceIndex: aws.Int64(0),
+								SubnetId:    aws.String("subnet-1"),
+								Groups:      []*string{aws.String("2"), aws.String("3")},
+							},
+						},
 						TagSpecifications: []*ec2.TagSpecification{
 							{
 								ResourceType: aws.String("instance"),
@@ -3976,8 +4001,13 @@ func TestCreateInstance(t *testing.T) {
 							GroupName:       aws.String("placement-group1"),
 							PartitionNumber: aws.Int64(2),
 						},
-						SecurityGroupIds: []*string{aws.String("2"), aws.String("3")},
-						SubnetId:         aws.String("subnet-1"),
+						NetworkInterfaces: []*ec2.InstanceNetworkInterfaceSpecification{
+							{
+								DeviceIndex: aws.Int64(0),
+								SubnetId:    aws.String("subnet-1"),
+								Groups:      aws.StringSlice([]string{"2", "3"}),
+							},
+						},
 						TagSpecifications: []*ec2.TagSpecification{
 							{
 								ResourceType: aws.String("instance"),

pkg/cloud/services/interfaces.go

@@ -82,7 +82,7 @@ type EC2Interface interface {
 	DeleteBastion() error
 	ReconcileBastion() error
 	// ReconcileElasticIPFromPublicPool reconciles the elastic IP from a custom Public IPv4 Pool.
-	ReconcileElasticIPFromPublicPool(pool *infrav1.ElasticIPPool, instance *infrav1.Instance) error
+	ReconcileElasticIPFromPublicPool(pool *infrav1.ElasticIPPool, instance *infrav1.Instance) (bool, error)
 
 	// ReleaseElasticIP reconciles the elastic IP from a custom Public IPv4 Pool.
 	ReleaseElasticIP(instanceID string) error

pkg/cloud/services/mock_services/ec2_interface_mock.go

@@ -191,12 +191,17 @@ func (s *Service) GetOrAllocateAddresses(pool *infrav1.ElasticIPPool, num int, r
 	return s.getOrAllocateAddresses(num, role, pool)
 }
 
+// GetAddresses returns the address associated to a given role.
+func (s *Service) GetAddresses(role string) (*ec2.DescribeAddressesOutput, error) {
+	return s.describeAddresses(role)
+}
+
 // ReleaseAddressByRole releases EIP addresses filtering by tag CAPA provider role.
 func (s *Service) ReleaseAddressByRole(role string) error {
-	clusterFilter := []*ec2.Filter{filter.EC2.Cluster(s.scope.Name())}
-	clusterFilter = append(clusterFilter, filter.EC2.ProviderRole(role))
-
-	return s.releaseAddressesWithFilter(clusterFilter)
+	return s.releaseAddressesWithFilter([]*ec2.Filter{
+		filter.EC2.ClusterOwned(s.scope.Name()),
+		filter.EC2.ProviderRole(role),
+	})
 }
 
 // setByoPublicIpv4 check if the config has Public IPv4 Pool defined, then