Add deregister logic for network LB groups

pkg/actuators/machine/instances_test.go

@@ -309,7 +309,7 @@ func TestRemoveStoppedMachine(t *testing.T) {
 				Reservations: []*ec2.Reservation{
 					{
 						Instances: []*ec2.Instance{
-							stubInstance(stubAMIID, stubInstanceID),
+							stubInstance(stubAMIID, stubInstanceID, true),
 						},
 					},
 				},
@@ -321,8 +321,8 @@ func TestRemoveStoppedMachine(t *testing.T) {
 				Reservations: []*ec2.Reservation{
 					{
 						Instances: []*ec2.Instance{
-							stubInstance(stubAMIID, stubInstanceID),
-							stubInstance("ami-a9acbbd7", "i-02fcb933c5da7085d"),
+							stubInstance(stubAMIID, stubInstanceID, true),
+							stubInstance("ami-a9acbbd7", "i-02fcb933c5da7085d", true),
 						},
 					},
 				},

pkg/actuators/machine/loadbalancers.go

@@ -2,12 +2,12 @@ package machine
 
 import (
 	"fmt"
-	"strings"
 
 	errorutil "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/klog/v2"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/awserr"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/aws/aws-sdk-go/service/elb"
 	"github.com/aws/aws-sdk-go/service/elbv2"
@@ -38,43 +38,15 @@ func registerWithClassicLoadBalancers(client awsclient.Client, names []string, i
 
 func registerWithNetworkLoadBalancers(client awsclient.Client, names []string, instance *ec2.Instance) error {
 	klog.V(4).Infof("Updating network load balancer registration for %q", *instance.InstanceId)
-	lbNames := make([]*string, len(names))
-	for i, name := range names {
-		lbNames[i] = aws.String(name)
-	}
-	lbsRequest := &elbv2.DescribeLoadBalancersInput{
-		Names: lbNames,
-	}
-	lbsResponse, err := client.ELBv2DescribeLoadBalancers(lbsRequest)
+	targetGroups, err := gatherLoadBalancerTargetGroups(client, names)
 	if err != nil {
-		klog.Errorf("Failed to describe load balancers %v: %v", names, err)
 		return err
 	}
-	// Use a map for target groups to get unique target group entries across load balancers
-	targetGroups := map[string]*elbv2.TargetGroup{}
-	for _, loadBalancer := range lbsResponse.LoadBalancers {
-		klog.V(4).Infof("Retrieving target groups for load balancer %q", *loadBalancer.LoadBalancerName)
-		targetGroupsInput := &elbv2.DescribeTargetGroupsInput{
-			LoadBalancerArn: loadBalancer.LoadBalancerArn,
-		}
-		targetGroupsOutput, err := client.ELBv2DescribeTargetGroups(targetGroupsInput)
-		if err != nil {
-			klog.Errorf("Failed to retrieve load balancer target groups for %q: %v", *loadBalancer.LoadBalancerName, err)
-			return err
-		}
-		for _, targetGroup := range targetGroupsOutput.TargetGroups {
-			targetGroups[*targetGroup.TargetGroupArn] = targetGroup
-		}
-	}
-	if klog.V(4).Enabled() {
-		targetGroupArns := make([]string, 0, len(targetGroups))
-		for arn := range targetGroups {
-			targetGroupArns = append(targetGroupArns, fmt.Sprintf("%q", arn))
-		}
-		klog.Infof("Registering instance %q with target groups: %v", *instance.InstanceId, strings.Join(targetGroupArns, ","))
-	}
+
 	errs := []error{}
 	for _, targetGroup := range targetGroups {
+		klog.V(4).Infof("Unregistering instance %q registered by ip from target group: %v", *instance.InstanceId, *targetGroup.TargetGroupArn)
+
 		var target *elbv2.TargetDescription
 		switch *targetGroup.TargetType {
 		case elbv2.TargetTypeEnumInstance:
@@ -101,3 +73,84 @@ func registerWithNetworkLoadBalancers(client awsclient.Client, names []string, i
 	}
 	return nil
 }
+
+// deregisterNetworkLoadBalancers serves manual instance removal from Network LoadBalancer TargetGroup list
+// for the instances attached by IP. Unlike instance reference, IP attachment should be cleaned manually.
+func deregisterNetworkLoadBalancers(client awsclient.Client, names []string, instance *ec2.Instance) error {
+	if instance.PrivateIpAddress == nil {
+		klog.V(4).Infof("Instance %q does not have private ip, skipping...", *instance.InstanceId)
+		return nil
+	}
+
+	klog.V(4).Infof("Removing network load balancer registration for %q", *instance.InstanceId)
+	targetGroupsOutput, err := gatherLoadBalancerTargetGroups(client, names)
+	if err != nil {
+		return err
+	}
+
+	filteredGroupsByIP := []*elbv2.TargetGroup{}
+	for _, targetGroup := range targetGroupsOutput {
+		if *targetGroup.TargetType == elbv2.TargetTypeEnumIp {
+			filteredGroupsByIP = append(filteredGroupsByIP, targetGroup)
+		}
+	}
+
+	errs := []error{}
+	for _, targetGroup := range filteredGroupsByIP {
+		klog.V(4).Infof("Unregistering instance %q registered by ip from target group: %v", *instance.InstanceId, *targetGroup.TargetGroupArn)
+
+		deregisterTargetsInput := &elbv2.DeregisterTargetsInput{
+			TargetGroupArn: targetGroup.TargetGroupArn,
+			Targets: []*elbv2.TargetDescription{{
+				Id: instance.PrivateIpAddress,
+			}},
+		}
+		_, err := client.ELBv2DeregisterTargets(deregisterTargetsInput)
+		if err != nil {
+			if aerr, ok := err.(awserr.Error); ok {
+				switch aerr.Code() {
+				case elbv2.ErrCodeInvalidTargetException, elbv2.ErrCodeTargetGroupNotFoundException:
+					// Ignoring error when LB target group was already removed
+					continue
+				}
+			}
+			klog.Errorf("Failed to unregister instance %q from target group %q: %v", *instance.InstanceId, *targetGroup.TargetGroupArn, err)
+			errs = append(errs, fmt.Errorf("%s: %v", *targetGroup.TargetGroupArn, err))
+		}
+	}
+	if len(errs) > 0 {
+		return errorutil.NewAggregate(errs)
+	}
+	return nil
+}
+
+func gatherLoadBalancerTargetGroups(client awsclient.Client, names []string) ([]*elbv2.TargetGroup, error) {
+	lbNames := make([]*string, len(names))
+	for i, name := range names {
+		lbNames[i] = aws.String(name)
+	}
+	lbsRequest := &elbv2.DescribeLoadBalancersInput{
+		Names: lbNames,
+	}
+	lbsResponse, err := client.ELBv2DescribeLoadBalancers(lbsRequest)
+	if err != nil {
+		klog.Errorf("Failed to describe load balancers %v: %v", names, err)
+		return nil, err
+	}
+	// Use a map for target groups to get unique target group entries across load balancers
+	targetGroups := []*elbv2.TargetGroup{}
+	for _, loadBalancer := range lbsResponse.LoadBalancers {
+		klog.V(4).Infof("Retrieving target groups for load balancer %s", *loadBalancer.LoadBalancerName)
+		targetGroupsInput := &elbv2.DescribeTargetGroupsInput{
+			LoadBalancerArn: loadBalancer.LoadBalancerArn,
+		}
+		targetGroupsOutput, err := client.ELBv2DescribeTargetGroups(targetGroupsInput)
+		if err != nil {
+			klog.Errorf("Failed to retrieve load balancer target groups for %q: %v", *loadBalancer.LoadBalancerName, err)
+			return nil, err
+		}
+		targetGroups = append(targetGroups, targetGroupsOutput.TargetGroups...)
+	}
+
+	return targetGroups, nil
+}

pkg/actuators/machine/loadbalancers_test.go

@@ -4,6 +4,9 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/aws/aws-sdk-go/aws/awserr"
+	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/elbv2"
 	"github.com/golang/mock/gomock"
 	mockaws "sigs.k8s.io/cluster-api-provider-aws/pkg/client/mock"
 )
@@ -33,7 +36,7 @@ func TestRegisterWithNetworkLoadBalancers(t *testing.T) {
 		},
 	}
 
-	instance := stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c")
+	instance := stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", true)
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
@@ -46,3 +49,81 @@ func TestRegisterWithNetworkLoadBalancers(t *testing.T) {
 		})
 	}
 }
+
+func TestDeregisterNetworkLoadBalancers(t *testing.T) {
+	cases := []struct {
+		name                           string
+		instance                       *ec2.Instance
+		lbErr                          error
+		describeLoadBalancersCallTimes int
+		targetGroupErr                 error
+		describeTargetGroupsCallTimes  int
+		unregisterTargetErr            error
+		deregisterCallTimes            int
+		expectErr                      error
+	}{
+		{
+			name:     "No action if ip is unset",
+			instance: stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", false),
+		},
+		{
+			name:                           "No error",
+			instance:                       stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", true),
+			describeLoadBalancersCallTimes: 1,
+			describeTargetGroupsCallTimes:  1,
+			deregisterCallTimes:            1,
+		},
+		{
+			name:                           "With describe lb error",
+			instance:                       stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", true),
+			lbErr:                          fmt.Errorf("error"),
+			describeLoadBalancersCallTimes: 1,
+			describeTargetGroupsCallTimes:  0,
+			deregisterCallTimes:            0,
+			expectErr:                      fmt.Errorf("error"),
+		},
+		{
+			name:                           "With target group error",
+			instance:                       stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", true),
+			targetGroupErr:                 fmt.Errorf("error"),
+			describeLoadBalancersCallTimes: 1,
+			describeTargetGroupsCallTimes:  1,
+			deregisterCallTimes:            0,
+			expectErr:                      fmt.Errorf("error"),
+		},
+		{
+			name:                           "With target already unregistered error",
+			instance:                       stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", true),
+			unregisterTargetErr:            awserr.New(elbv2.ErrCodeTargetGroupNotFoundException, "error", nil),
+			describeLoadBalancersCallTimes: 1,
+			describeTargetGroupsCallTimes:  1,
+			deregisterCallTimes:            1,
+			expectErr:                      nil,
+		},
+		{
+			name:                           "With register target unknown error",
+			instance:                       stubInstance("ami-a9acbbd6", "i-02fcb933c5da7085c", true),
+			unregisterTargetErr:            fmt.Errorf("error"),
+			describeLoadBalancersCallTimes: 1,
+			describeTargetGroupsCallTimes:  1,
+			deregisterCallTimes:            1,
+			expectErr:                      fmt.Errorf("arn2: error"),
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			mockCtrl := gomock.NewController(t)
+			mockAWSClient := mockaws.NewMockClient(mockCtrl)
+			mockAWSClient.EXPECT().ELBv2DescribeLoadBalancers(gomock.Any()).Return(stubDescribeLoadBalancersOutput(), tc.lbErr).Times(tc.describeLoadBalancersCallTimes)
+			mockAWSClient.EXPECT().ELBv2DescribeTargetGroups(gomock.Any()).Return(stubDescribeTargetGroupsOutput(), tc.targetGroupErr).Times(tc.describeTargetGroupsCallTimes)
+			mockAWSClient.EXPECT().ELBv2DeregisterTargets(gomock.Any()).Return(nil, tc.unregisterTargetErr).Times(tc.deregisterCallTimes)
+			err := deregisterNetworkLoadBalancers(mockAWSClient, []string{"name1", "name2"}, tc.instance)
+			mockCtrl.Finish()
+
+			if fmt.Sprintf("%s", err) != fmt.Sprintf("%s", tc.expectErr) {
+				t.Errorf("Unexpeted error output: expected '%s', got '%s'", tc.expectErr, err)
+			}
+		})
+	}
+}

pkg/actuators/machine/reconciler.go

@@ -129,6 +129,15 @@ func (r *Reconciler) delete() error {
 		return fmt.Errorf("failed to delete instaces: %w", err)
 	}
 
+	if err = r.removeFromLoadBalancers(existingInstances); err != nil {
+		metrics.RegisterFailedInstanceDelete(&metrics.MachineLabels{
+			Name:      r.machine.Name,
+			Namespace: r.machine.Namespace,
+			Reason:    err.Error(),
+		})
+		return fmt.Errorf("failed to updated update load balancers: %w", err)
+	}
+
 	if len(terminatingInstances) == 1 {
 		if terminatingInstances[0] != nil && terminatingInstances[0].CurrentState != nil && terminatingInstances[0].CurrentState.Name != nil {
 			r.machine.Annotations[machinecontroller.MachineInstanceStateAnnotationName] = aws.StringValue(terminatingInstances[0].CurrentState.Name)
@@ -312,6 +321,35 @@ func (r *Reconciler) updateLoadBalancers(instance *ec2.Instance) error {
 	return nil
 }
 
+// updateLoadBalancers adds a given machine instance to the load balancers specified in its provider config
+func (r *Reconciler) removeFromLoadBalancers(instances []*ec2.Instance) error {
+	if len(r.providerSpec.LoadBalancers) == 0 {
+		klog.V(4).Infof("%s: Instances have no load balancers configured. Skipping", r.machine.Name)
+		return nil
+	}
+	networkLoadBalancerNames := []string{}
+	for _, loadBalancerRef := range r.providerSpec.LoadBalancers {
+		if loadBalancerRef.Type == awsproviderv1.NetworkLoadBalancerType {
+			networkLoadBalancerNames = append(networkLoadBalancerNames, loadBalancerRef.Name)
+		}
+	}
+
+	errs := []error{}
+	if len(networkLoadBalancerNames) > 0 {
+		for _, instance := range instances {
+			err := deregisterNetworkLoadBalancers(r.awsClient, networkLoadBalancerNames, instance)
+			if err != nil {
+				klog.Errorf("%s: Failed to register network load balancers: %v", r.machine.Name, err)
+				errs = append(errs, err)
+			}
+		}
+	}
+	if len(errs) > 0 {
+		return errorutil.NewAggregate(errs)
+	}
+	return nil
+}
+
 // setProviderID adds providerID in the machine spec
 func (r *Reconciler) setProviderID(instance *ec2.Instance) error {
 	existingProviderID := r.machine.Spec.ProviderID

pkg/actuators/machine/stubs.go

@@ -164,7 +164,11 @@ func GenerateAwsCredentialsSecretFromEnv(secretName, namespace string) *corev1.S
 	}
 }
 
-func stubInstance(imageID, instanceID string) *ec2.Instance {
+func stubInstance(imageID, instanceID string, setIP bool) *ec2.Instance {
+	var ipAddr *string
+	if setIP {
+		ipAddr = aws.String("1.1.1.1")
+	}
 	return &ec2.Instance{
 		ImageId:    aws.String(imageID),
 		InstanceId: aws.String(instanceID),
@@ -175,8 +179,8 @@ func stubInstance(imageID, instanceID string) *ec2.Instance {
 		LaunchTime:       aws.Time(time.Now()),
 		PublicDnsName:    aws.String("publicDNS"),
 		PrivateDnsName:   aws.String("privateDNS"),
-		PublicIpAddress:  aws.String("1.1.1.1"),
-		PrivateIpAddress: aws.String("1.1.1.1"),
+		PublicIpAddress:  ipAddr,
+		PrivateIpAddress: ipAddr,
 		Tags: []*ec2.Tag{
 			{
 				Key:   aws.String("key"),

pkg/client/client.go

@@ -85,6 +85,7 @@ type Client interface {
 	ELBv2DescribeLoadBalancers(*elbv2.DescribeLoadBalancersInput) (*elbv2.DescribeLoadBalancersOutput, error)
 	ELBv2DescribeTargetGroups(*elbv2.DescribeTargetGroupsInput) (*elbv2.DescribeTargetGroupsOutput, error)
 	ELBv2RegisterTargets(*elbv2.RegisterTargetsInput) (*elbv2.RegisterTargetsOutput, error)
+	ELBv2DeregisterTargets(*elbv2.DeregisterTargetsInput) (*elbv2.DeregisterTargetsOutput, error)
 }
 
 type awsClient struct {
@@ -153,6 +154,10 @@ func (c *awsClient) ELBv2RegisterTargets(input *elbv2.RegisterTargetsInput) (*el
 	return c.elbv2Client.RegisterTargets(input)
 }
 
+func (c *awsClient) ELBv2DeregisterTargets(input *elbv2.DeregisterTargetsInput) (*elbv2.DeregisterTargetsOutput, error) {
+	return c.elbv2Client.DeregisterTargets(input)
+}
+
 // NewClient creates our client wrapper object for the actual AWS clients we use.
 // For authentication the underlying clients will use either the cluster AWS credentials
 // secret if defined (i.e. in the root cluster),

pkg/client/fake/fake.go

@@ -125,6 +125,11 @@ func (c *awsClient) ELBv2RegisterTargets(*elbv2.RegisterTargetsInput) (*elbv2.Re
 	return &elbv2.RegisterTargetsOutput{}, nil
 }
 
+func (c *awsClient) ELBv2DeregisterTargets(*elbv2.DeregisterTargetsInput) (*elbv2.DeregisterTargetsOutput, error) {
+	// Feel free to extend the returned values
+	return &elbv2.DeregisterTargetsOutput{}, nil
+}
+
 // NewClient creates our client wrapper object for the actual AWS clients we use.
 // For authentication the underlying clients will use either the cluster AWS credentials
 // secret if defined (i.e. in the root cluster),