Fix bug during feed shutdown (#234)

* Fix bug during feed shutdown

- Feed instance can attach to multiple target groups belonging to the load balancer having a tag matching the value in the flag `ingress-class`. The fix is for feed instance to wait for the drain duration even if the de-register call succeeds to at least one of those target groups.

* Add instruction to include apk upgrade before package adds.

* Ignore the bind-libs vulnerability

CHANGELOG.md

@@ -1,3 +1,7 @@
+# v4.3.1
+* [BUGFIX] Fix issue with feed not waiting for the drain duration despite having at least one successful call to de-register from the list of registered target groups.
+    * Feed instance can attach to multiple target groups belonging to the load balancer having a tag matching the value in the flag `ingress-class`. The fix is for feed instance to wait for the drain duration even if the de-register call succeeds to at least one of those target groups.
+
 # v4.3.0
 * Provide support for multiple labels for namespace selection
   * Remove flag `--ingress-controller-namespace-selector`

Makefile

@@ -101,4 +101,4 @@ endif
 
 check-vulnerabilities:
 	@echo "== Checking for vulnerabilities in the docker image"
-	trivy image --exit-code=1 --severity="HIGH,CRITICAL" $(image_prefix)-ingress:latest
+	trivy image --exit-code=1 --severity="HIGH,CRITICAL" --ignorefile=trivy-ignore-file.txt $(image_prefix)-ingress:latest

docker/ingress/Dockerfile

@@ -1,5 +1,8 @@
 FROM alpine:3.13
 
+# Update list of available packages and upgrade installed packages
+RUN apk -U upgrade
+
 # Install useful diagnostic packages
 RUN apk add --no-cache \
         libcap \

nlb/nlb.go

@@ -69,7 +69,7 @@ type nlb struct {
 	expectedNumber       int
 	instanceID           string
 	privateIPAddress     string
-	elbs                 map[string]LoadBalancerDetails
+	loadBalancers        map[string]LoadBalancerDetails
 	registeredFrontends  util.SafeInt
 	initialised          initialised
 	drainDelay           time.Duration
@@ -122,7 +122,7 @@ func (e *nlb) attachToFrontEnds() error {
 
 	// Save these now so we can always know what we might have done
 	// up until this point we have only read data
-	e.elbs = clusterFrontEnds
+	e.loadBalancers = clusterFrontEnds
 	e.instanceID = instanceID
 	e.privateIPAddress = privateIP
 	registered := 0
@@ -342,54 +342,53 @@ func generateTargetDescriptionFromTargetType(targetType, instanceID, privateIP s
 
 // Stop removes this instance from all the front end NLBs
 func (e *nlb) Stop() error {
-	var failed = false
-	for _, elb := range e.elbs {
-		log.Infof("Deregistering instance %s with nlb %s", e.privateIPAddress, elb.Name)
-		err := deregisterFromLoadBalancer(e, elb)
-		if err != nil {
-			log.Warnf("unable to deregister instance %s (%s) with nlb %s: %v", e.instanceID, e.privateIPAddress, elb.Name, err)
-			failed = true
+	failedCount := 0
+	successCount := 0
+
+	for _, loadBalancer := range e.loadBalancers {
+		for _, targetGroup := range loadBalancer.TargetGroups {
+			log.Infof("De-registering instance %q from targetGroup %q of nlb %q", e.instanceID, aws.StringValue(targetGroup.TargetGroupName), loadBalancer.Name)
+			err := deregisterFromLoadBalancer(e, targetGroup)
+			if err != nil {
+				log.Warnf("de-registering instance %q from targetGroup %q of nlb %q resulted in an error: %v", e.instanceID, aws.StringValue(targetGroup.TargetGroupName), loadBalancer.Name, err)
+				failedCount++
+			} else {
+				successCount++
+			}
 		}
 	}
-	if failed {
-		return errors.New("at least one NLB failed to detach")
+
+	if successCount > 0 {
+		log.Infof("Waiting %v to finish NLB deregistration", e.drainDelay)
+		time.Sleep(e.drainDelay)
 	}
 
-	log.Infof("Waiting %v to finish NLB deregistration", e.drainDelay)
-	time.Sleep(e.drainDelay)
+	if failedCount > 0 {
+		return errors.New("at least one NLB failed to detach")
+	}
 
 	return nil
 }
 
-func deregisterFromLoadBalancer(n *nlb, lb LoadBalancerDetails) error {
-	var failedArns []string
+func deregisterFromLoadBalancer(n *nlb, tg *elbv2.TargetGroup) error {
 	instanceID := n.instanceID
 	privateIP := n.privateIPAddress
 
-	for _, tg := range lb.TargetGroups {
-		targetGroupArn := *tg.TargetGroupArn
-		targetType := *tg.TargetType
-
-		targetDescription, err := generateTargetDescriptionFromTargetType(targetType, instanceID, privateIP)
-		if err != nil {
-			log.Errorf("Could not deregister instance %s (%s) from target group %v: %v", instanceID, privateIP, targetGroupArn, err)
-			failedArns = append(failedArns, targetGroupArn)
-			continue
-		}
+	targetGroupArn := *tg.TargetGroupArn
+	targetType := *tg.TargetType
 
-		log.Infof("Deregistering instance %s (%s) from target group %s", n.instanceID, n.privateIPAddress, targetGroupArn)
-		_, err = n.awsElb.DeregisterTargets(&elbv2.DeregisterTargetsInput{
-			Targets:        targetDescription,
-			TargetGroupArn: tg.TargetGroupArn,
-		})
-		if err != nil {
-			log.Errorf("Could not deregister instance %s (%s) from target group %s: %v", n.instanceID, n.privateIPAddress, targetGroupArn, err)
-			failedArns = append(failedArns, targetGroupArn)
-		}
+	targetDescription, err := generateTargetDescriptionFromTargetType(targetType, instanceID, privateIP)
+	if err != nil {
+		return fmt.Errorf("could not deregister instance %s (%s) from target group %v: %v", instanceID, privateIP, targetGroupArn, err)
 	}
 
-	if failedArns != nil {
-		return fmt.Errorf("could not deregister target group(s) from instance %s (%s): %v", n.instanceID, n.privateIPAddress, failedArns)
+	log.Infof("De-registering instance %s (%s) from target group %s", n.instanceID, n.privateIPAddress, targetGroupArn)
+	_, err = n.awsElb.DeregisterTargets(&elbv2.DeregisterTargetsInput{
+		Targets:        targetDescription,
+		TargetGroupArn: tg.TargetGroupArn,
+	})
+	if err != nil {
+		return fmt.Errorf("could not deregister target group(s) from instance %s (%s)", n.instanceID, n.privateIPAddress)
 	}
 
 	return nil

nlb/nlb_test.go

@@ -610,6 +610,54 @@ func TestDeregistersWithAttachedELBsV2(t *testing.T) {
 		"Drain time should have caused stop to take at least 50ms.")
 }
 
+func TestStopWaitsForDrainTimeIfDeregisterCallFailsOnSomeOfTheAttachedTargetGroups(t *testing.T) {
+	// given
+	targetGroups := []*elbv2.TargetGroup{
+		{
+			TargetGroupArn: aws.String("valid-tg"),
+			TargetType:     aws.String(elbv2.TargetTypeEnumIp),
+		},
+		{
+			TargetGroupArn: aws.String("absent-tg"),
+			TargetType:     aws.String(elbv2.TargetTypeEnumInstance),
+		},
+	}
+	loadBalancers := map[string]LoadBalancerDetails{
+		"key1": {
+			Name:         "test-loadbalancer",
+			TargetGroups: targetGroups,
+		},
+	}
+	mockElbV2 := &fakeElb{}
+
+	testNlb := &nlb{
+		awsElb:           mockElbV2,
+		loadBalancers:    loadBalancers,
+		instanceID:       "some-instance",
+		privateIPAddress: "192.168.0.1",
+		drainDelay:       time.Millisecond * 500,
+	}
+
+	mockElbV2.On("DeregisterTargets", &elbv2.DeregisterTargetsInput{
+		Targets:        []*elbv2.TargetDescription{{Id: aws.String("192.168.0.1")}},
+		TargetGroupArn: aws.String("valid-tg"),
+	}).Return(&elbv2.DeregisterTargetsOutput{}, nil)
+	mockElbV2.On("DeregisterTargets", &elbv2.DeregisterTargetsInput{
+		Targets:        []*elbv2.TargetDescription{{Id: aws.String("some-instance")}},
+		TargetGroupArn: aws.String("absent-tg"),
+	}).Return(&elbv2.DeregisterTargetsOutput{}, errors.New("target group no longer present"))
+
+	//when
+	beforeStop := time.Now()
+	stopError := testNlb.Stop()
+	stopDuration := time.Now().Sub(beforeStop)
+
+	//then
+	assert.EqualError(t, stopError, "at least one NLB failed to detach")
+	assert.True(t, stopDuration.Nanoseconds() > time.Millisecond.Nanoseconds()*500,
+		"Drain time should have caused stop to take at least 500ms.")
+}
+
 func TestDeregistersWithAttachedTargetGroupsMultipleTypes(t *testing.T) {
 	// given
 	elbUpdaterV2, mockElbV2, mockMetadata := setup()

trivy-ignore-file.txt

@@ -0,0 +1,2 @@
+# The bind-libs version which has this vulnerability fixed is not yet available for alpine. Hence ignoring for now.
+CVE-2020-8625