🐛: 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.

controllers/awsmachine_controller.go

@@ -541,7 +541,7 @@ func (r *AWSMachineReconciler) reconcileNormal(_ context.Context, machineScope *
 	// a BYOIP without duplication.
 	if pool := machineScope.GetElasticIPPool(); pool != nil {
 		if err := ec2svc.ReconcileElasticIPFromPublicPool(pool, instance); err != nil {
-			machineScope.Error(err, "failed to associate elastic IP address")
+			machineScope.Error(err, "failed to reconcile BYO Public IPv4")
 			return ctrl.Result{}, err
 		}
 	}

pkg/cloud/services/ec2/eip.go

@@ -17,9 +17,40 @@ 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
+	iip := ""
+	if instance.PublicIP != nil {
+		iip = aws.StringValue(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.InstanceStatePending {
+		err := fmt.Errorf("unable to reconcile Elastic IP Pool to instance %q with state: %v", instance.ID, instance.State)
+		s.scope.Error(err, "failed to set Elastic IP Pool to machine", "publicIpv4PoolID", pool.PublicIpv4Pool)
+		return err
+	}
+
+	// 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 err
+	}
+	if len(addrs.Addresses) > 1 {
+		return fmt.Errorf("unexpected number of EIPs allocated to the role. expected 1, got %d", len(addrs.Addresses))
+	}
+	for _, addr := range addrs.Addresses {
+		// address isn't associated yet.
+		if addr.AssociationId == nil {
+			break
+		}
+		if addr.InstanceId != nil && *addr.InstanceId == instance.ID {
+			s.scope.Debug("ReconcileElasticIPFromPublicPool() STOP. EIP already associated", "eip", addr.AllocationId, "eip-address", addr.PublicIp, "eip-associationID", addr.AssociationId, "eip-instance", addr.InstanceId)
+			return 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)
 	}

pkg/cloud/services/ec2/instances.go

@@ -563,21 +563,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/network/eips.go

@@ -191,6 +191,11 @@ 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())}