Reduce frequency of calls to register targets with load balancers

pkg/actuators/machine/actuator_test.go

@@ -176,17 +176,19 @@ func TestMachineEvents(t *testing.T) {
 			if tc.awsError {
 				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(nil, errors.New("AWS error")).AnyTimes()
 			} else {
-				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), nil).AnyTimes()
+				mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning, "192.168.0.10"), nil).AnyTimes()
 			}
 
-			mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
+			mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c", "192.168.0.10"), nil).AnyTimes()
 			mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil)
 			mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
 			mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil).AnyTimes()
 			mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
 			mockAWSClient.EXPECT().ELBv2DescribeLoadBalancers(gomock.Any()).Return(stubDescribeLoadBalancersOutput(), nil).AnyTimes()
 			mockAWSClient.EXPECT().ELBv2DescribeTargetGroups(gomock.Any()).Return(stubDescribeTargetGroupsOutput(), nil).AnyTimes()
 			mockAWSClient.EXPECT().ELBv2RegisterTargets(gomock.Any()).Return(nil, nil).AnyTimes()
+			mockAWSClient.EXPECT().ELBv2DescribeTargetHealth(gomock.Any()).Return(stubDescribeTargetHealthOutput(), nil).AnyTimes()
+			mockAWSClient.EXPECT().ELBv2DeregisterTargets(gomock.Any()).Return(nil, nil).AnyTimes()
 			mockAWSClient.EXPECT().DescribeVpcs(gomock.Any()).Return(StubDescribeVPCs()).AnyTimes()
 			mockAWSClient.EXPECT().DescribeDHCPOptions(gomock.Any()).Return(StubDescribeDHCPOptions()).AnyTimes()
 			mockAWSClient.EXPECT().CreateTags(gomock.Any()).Return(&ec2.CreateTagsOutput{}, nil).AnyTimes()

pkg/actuators/machine/instances_test.go

@@ -495,7 +495,7 @@ func TestLaunchInstance(t *testing.T) {
 					},
 				},
 			},
-			instancesOutput: stubReservation(stubAMIID, stubInstanceID),
+			instancesOutput: stubReservation(stubAMIID, stubInstanceID, "192.168.0.10"),
 			succeeds:        true,
 			runInstancesInput: &ec2.RunInstancesInput{
 				IamInstanceProfile: &ec2.IamInstanceProfileSpecification{
@@ -585,7 +585,7 @@ func TestLaunchInstance(t *testing.T) {
 					},
 				},
 			},
-			instancesOutput: stubReservation(stubAMIID, stubInstanceID),
+			instancesOutput: stubReservation(stubAMIID, stubInstanceID, "192.168.0.10"),
 			succeeds:        true,
 			runInstancesInput: &ec2.RunInstancesInput{
 				IamInstanceProfile: &ec2.IamInstanceProfileSpecification{
@@ -655,7 +655,7 @@ func TestLaunchInstance(t *testing.T) {
 					},
 				},
 			},
-			instancesOutput: stubReservation(stubAMIID, stubInstanceID),
+			instancesOutput: stubReservation(stubAMIID, stubInstanceID, "192.168.0.10"),
 			succeeds:        true,
 			runInstancesInput: &ec2.RunInstancesInput{
 				IamInstanceProfile: &ec2.IamInstanceProfileSpecification{
@@ -768,7 +768,7 @@ func TestLaunchInstance(t *testing.T) {
 					},
 				},
 			},
-			instancesOutput: stubReservation(stubAMIID, stubInstanceID),
+			instancesOutput: stubReservation(stubAMIID, stubInstanceID, "192.168.0.10"),
 			succeeds:        true,
 			runInstancesInput: &ec2.RunInstancesInput{
 				IamInstanceProfile: &ec2.IamInstanceProfileSpecification{

pkg/actuators/machine/loadbalancers.go

@@ -45,25 +45,38 @@ func registerWithNetworkLoadBalancers(client awsclient.Client, names []string, i
 
 	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:
 			target = &elbv2.TargetDescription{
 				Id: instance.InstanceId,
 			}
+			klog.V(4).Infof("Registering instance %q by instance ID to target group: %v", *instance.InstanceId, *targetGroup.TargetGroupArn)
 		case elbv2.TargetTypeEnumIp:
 			target = &elbv2.TargetDescription{
 				Id: instance.PrivateIpAddress,
 			}
+			klog.V(4).Infof("Registering instance %q by IP to target group: %v", *instance.InstanceId, *targetGroup.TargetGroupArn)
+		}
+
+		registeredTargets, err := gatherLoadBalancerTargetGroupRegisteredTargets(client, targetGroup.TargetGroupArn)
+		if err != nil {
+			klog.Errorf("Failed to gather registered targets for target group %q: %v", *targetGroup.TargetGroupArn, err)
+			errs = append(errs, fmt.Errorf("%s: %v", *targetGroup.TargetGroupArn, err))
 		}
+		if registeredTargets != nil {
+			if _, ok := registeredTargets[*target.Id]; ok {
+				klog.V(4).Infof("Skipping registration for instance %q to target group %q: Instance already registered", *instance.InstanceId, *targetGroup.TargetGroupArn)
+				continue
+			}
+		}
+
 		registerTargetsInput := &elbv2.RegisterTargetsInput{
 			TargetGroupArn: targetGroup.TargetGroupArn,
 			Targets:        []*elbv2.TargetDescription{target},
 		}
-		_, err := client.ELBv2RegisterTargets(registerTargetsInput)
-		if err != nil {
+		if _, err := client.ELBv2RegisterTargets(registerTargetsInput); err != nil {
 			klog.Errorf("Failed to register instance %q with target group %q: %v", *instance.InstanceId, *targetGroup.TargetGroupArn, err)
 			errs = append(errs, fmt.Errorf("%s: %v", *targetGroup.TargetGroupArn, err))
 		}
@@ -154,3 +167,24 @@ func gatherLoadBalancerTargetGroups(client awsclient.Client, names []string) ([]
 
 	return targetGroups, nil
 }
+
+// gatherLoadBalancerTargetGroupRegisteredTargets looks for all targets that are registered to a particular targetGroup.
+// Within the AWS API, the only way to find the targets that are registered is to look at the target health for the group.
+// The target health response contains all of the targets and importantly, their IDs which we need later to compare with
+// the target ID we are wanting to register.
+func gatherLoadBalancerTargetGroupRegisteredTargets(client awsclient.Client, targetGroupArn *string) (map[string]struct{}, error) {
+	targetHealthRequest := &elbv2.DescribeTargetHealthInput{
+		TargetGroupArn: targetGroupArn,
+	}
+	targetHealthResponse, err := client.ELBv2DescribeTargetHealth(targetHealthRequest)
+	if err != nil {
+		klog.Errorf("Failed to describe target health: %v", err)
+		return nil, err
+	}
+
+	targetIDs := make(map[string]struct{})
+	for _, targetHealth := range targetHealthResponse.TargetHealthDescriptions {
+		targetIDs[*targetHealth.Target.Id] = struct{}{}
+	}
+	return targetIDs, nil
+}

pkg/actuators/machine/loadbalancers_test.go

@@ -45,6 +45,7 @@ func TestRegisterWithNetworkLoadBalancers(t *testing.T) {
 			mockAWSClient.EXPECT().ELBv2DescribeLoadBalancers(gomock.Any()).Return(stubDescribeLoadBalancersOutput(), tc.lbErr)
 			mockAWSClient.EXPECT().ELBv2DescribeTargetGroups(gomock.Any()).Return(stubDescribeTargetGroupsOutput(), tc.targetGroupErr).AnyTimes()
 			mockAWSClient.EXPECT().ELBv2RegisterTargets(gomock.Any()).Return(nil, tc.registerTargetErr).AnyTimes()
+			mockAWSClient.EXPECT().ELBv2DescribeTargetHealth(gomock.Any()).Return(&elbv2.DescribeTargetHealthOutput{}, nil).AnyTimes()
 			registerWithNetworkLoadBalancers(mockAWSClient, []string{"name1", "name2"}, instance)
 		})
 	}

pkg/actuators/machine/reconciler_test.go

@@ -198,12 +198,13 @@ func TestCreate(t *testing.T) {
 	mockAWSClient.EXPECT().DescribeSecurityGroups(gomock.Any()).Return(nil, fmt.Errorf("describeSecurityGroups error")).AnyTimes()
 	mockAWSClient.EXPECT().DescribeAvailabilityZones(gomock.Any()).Return(nil, fmt.Errorf("describeAvailabilityZones error")).AnyTimes()
 	mockAWSClient.EXPECT().DescribeImages(gomock.Any()).Return(nil, fmt.Errorf("describeImages error")).AnyTimes()
-	mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning), nil).AnyTimes()
+	mockAWSClient.EXPECT().DescribeInstances(gomock.Any()).Return(stubDescribeInstancesOutput("ami-a9acbbd6", "i-02fcb933c5da7085c", ec2.InstanceStateNameRunning, "192.168.0.10"), nil).AnyTimes()
 	mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(&ec2.TerminateInstancesOutput{}, nil).AnyTimes()
-	mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c"), nil).AnyTimes()
+	mockAWSClient.EXPECT().RunInstances(gomock.Any()).Return(stubReservation("ami-a9acbbd6", "i-02fcb933c5da7085c", "192.168.0.10"), nil).AnyTimes()
 	mockAWSClient.EXPECT().RegisterInstancesWithLoadBalancer(gomock.Any()).Return(nil, nil).AnyTimes()
 	mockAWSClient.EXPECT().ELBv2DescribeLoadBalancers(gomock.Any()).Return(stubDescribeLoadBalancersOutput(), nil)
 	mockAWSClient.EXPECT().ELBv2DescribeTargetGroups(gomock.Any()).Return(stubDescribeTargetGroupsOutput(), nil).AnyTimes()
+	mockAWSClient.EXPECT().ELBv2DescribeTargetHealth(gomock.Any()).Return(stubDescribeTargetHealthOutput(), nil).AnyTimes()
 	mockAWSClient.EXPECT().ELBv2RegisterTargets(gomock.Any()).Return(nil, nil).AnyTimes()
 	mockAWSClient.EXPECT().DescribeVpcs(gomock.Any()).Return(StubDescribeVPCs()).AnyTimes()
 	mockAWSClient.EXPECT().DescribeDHCPOptions(gomock.Any()).Return(StubDescribeDHCPOptions()).AnyTimes()
@@ -569,7 +570,7 @@ func TestGetMachineInstances(t *testing.T) {
 				}
 
 				mockAWSClient.EXPECT().DescribeInstances(request).Return(
-					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning),
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning, "192.168.0.10"),
 					nil,
 				).Times(1)
 
@@ -590,7 +591,7 @@ func TestGetMachineInstances(t *testing.T) {
 				}
 
 				mockAWSClient.EXPECT().DescribeInstances(request).Return(
-					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning),
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameRunning, "192.168.0.10"),
 					nil,
 				).Times(1)
 
@@ -609,7 +610,7 @@ func TestGetMachineInstances(t *testing.T) {
 				first := mockAWSClient.EXPECT().DescribeInstances(&ec2.DescribeInstancesInput{
 					InstanceIds: aws.StringSlice([]string{instanceID}),
 				}).Return(
-					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated),
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated, "192.168.0.10"),
 					nil,
 				).Times(1)
 
@@ -623,7 +624,7 @@ func TestGetMachineInstances(t *testing.T) {
 						clusterFilter(clusterID),
 					},
 				}).Return(
-					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated),
+					stubDescribeInstancesOutput(imageID, instanceID, ec2.InstanceStateNameTerminated, "192.168.0.10"),
 					nil,
 				).Times(1).After(first)
 

pkg/actuators/machine/stubs.go

@@ -258,7 +258,11 @@ func stubDescribeTargetGroupsOutput() *elbv2.DescribeTargetGroupsOutput {
 	}
 }
 
-func stubReservation(imageID, instanceID string) *ec2.Reservation {
+func stubDescribeTargetHealthOutput() *elbv2.DescribeTargetHealthOutput {
+	return &elbv2.DescribeTargetHealthOutput{}
+}
+
+func stubReservation(imageID, instanceID string, privateIP string) *ec2.Reservation {
 	az := defaultAvailabilityZone
 	return &ec2.Reservation{
 		Instances: []*ec2.Instance{
@@ -273,12 +277,13 @@ func stubReservation(imageID, instanceID string) *ec2.Reservation {
 				Placement: &ec2.Placement{
 					AvailabilityZone: &az,
 				},
+				PrivateIpAddress: aws.String(privateIP),
 			},
 		},
 	}
 }
 
-func stubDescribeInstancesOutput(imageID, instanceID string, state string) *ec2.DescribeInstancesOutput {
+func stubDescribeInstancesOutput(imageID, instanceID string, state string, privateIP string) *ec2.DescribeInstancesOutput {
 	return &ec2.DescribeInstancesOutput{
 		Reservations: []*ec2.Reservation{
 			{
@@ -290,7 +295,8 @@ func stubDescribeInstancesOutput(imageID, instanceID string, state string) *ec2.
 							Name: aws.String(state),
 							Code: aws.Int64(16),
 						},
-						LaunchTime: aws.Time(time.Now()),
+						LaunchTime:       aws.Time(time.Now()),
+						PrivateIpAddress: aws.String(privateIP),
 					},
 				},
 			},

pkg/client/client.go

@@ -84,6 +84,7 @@ type Client interface {
 	RegisterInstancesWithLoadBalancer(*elb.RegisterInstancesWithLoadBalancerInput) (*elb.RegisterInstancesWithLoadBalancerOutput, error)
 	ELBv2DescribeLoadBalancers(*elbv2.DescribeLoadBalancersInput) (*elbv2.DescribeLoadBalancersOutput, error)
 	ELBv2DescribeTargetGroups(*elbv2.DescribeTargetGroupsInput) (*elbv2.DescribeTargetGroupsOutput, error)
+	ELBv2DescribeTargetHealth(*elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error)
 	ELBv2RegisterTargets(*elbv2.RegisterTargetsInput) (*elbv2.RegisterTargetsOutput, error)
 	ELBv2DeregisterTargets(*elbv2.DeregisterTargetsInput) (*elbv2.DeregisterTargetsOutput, error)
 }
@@ -150,6 +151,10 @@ func (c *awsClient) ELBv2DescribeTargetGroups(input *elbv2.DescribeTargetGroupsI
 	return c.elbv2Client.DescribeTargetGroups(input)
 }
 
+func (c *awsClient) ELBv2DescribeTargetHealth(input *elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error) {
+	return c.elbv2Client.DescribeTargetHealth(input)
+}
+
 func (c *awsClient) ELBv2RegisterTargets(input *elbv2.RegisterTargetsInput) (*elbv2.RegisterTargetsOutput, error) {
 	return c.elbv2Client.RegisterTargets(input)
 }

pkg/client/fake/fake.go

@@ -120,6 +120,10 @@ func (c *awsClient) ELBv2DescribeTargetGroups(*elbv2.DescribeTargetGroupsInput)
 	return &elbv2.DescribeTargetGroupsOutput{}, nil
 }
 
+func (c *awsClient) ELBv2DescribeTargetHealth(*elbv2.DescribeTargetHealthInput) (*elbv2.DescribeTargetHealthOutput, error) {
+	return &elbv2.DescribeTargetHealthOutput{}, nil
+}
+
 func (c *awsClient) ELBv2RegisterTargets(*elbv2.RegisterTargetsInput) (*elbv2.RegisterTargetsOutput, error) {
 	// Feel free to extend the returned values
 	return &elbv2.RegisterTargetsOutput{}, nil