fix: target group verification did not work with named ports (#1485)

fix: target group verification did not work with named ports (#1485) Signed-off-by: Jesse Suen <[email protected]>
argoproj · Sep 10, 2021 · c7933eb · c7933eb
1 parent 2d3cd2a
commit c7933eb
Show file tree

Hide file tree

Showing 4 changed files with 124 additions and 24 deletions.
diff --git a/Dockerfile.dev b/Dockerfile.dev
@@ -1,6 +1,20 @@
 ####################################################################################################
 # argo-rollouts-dev
 ####################################################################################################
-FROM scratch
+FROM golang:1.16.3 as builder
+
+RUN apt-get update && apt-get install -y \
+    ca-certificates && \
+    apt-get clean && \
+    rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*
+
+FROM scratch    
+
+COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/
 COPY rollouts-controller-linux-amd64 /bin/rollouts-controller
+
+# Use numeric user, allows kubernetes to identify this user as being
+# non-root when we use a security context with runAsNonRoot: true
+USER 999
+
 ENTRYPOINT [ "/bin/rollouts-controller" ]
diff --git a/docs/features/traffic-management/alb.md b/docs/features/traffic-management/alb.md
@@ -172,6 +172,7 @@ the changes made to the Ingress object are reflected in the underlying AWS Targe
     Target Group IP verification available since Argo Rollouts v1.1
 
 The AWS LoadBalancer controller can run in one of two modes:
+
 * [Instance mode](https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/how-it-works/#instance-mode)
 * [IP mode](https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.2/how-it-works/#ip-mode)
 
@@ -277,12 +278,26 @@ spec:
 For this feature to work, the argo-rollouts deployment requires the following AWS API permissions
 under the [Elastic Load Balancing API](https://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/Welcome.html):
 
-* `DescribeTargetGroups`
-* `DescribeLoadBalancers`
-* `DescribeListeners`
-* `DescribeRules`
-* `DescribeTags`
-* `DescribeTargetHealth`
+
+```json
+{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Action": [
+                "elasticloadbalancing:DescribeTargetGroups",
+                "elasticloadbalancing:DescribeLoadBalancers",
+                "elasticloadbalancing:DescribeListeners",
+                "elasticloadbalancing:DescribeRules",
+                "elasticloadbalancing:DescribeTags",
+                "elasticloadbalancing:DescribeTargetHealth"
+            ],
+            "Resource": "*",
+            "Effect": "Allow"
+        }
+    ]
+}
+```
 
 There are various ways of granting AWS privileges to the argo-rollouts pods, which is highly
 dependent to your cluster's AWS environment, and out-of-scope of this documentation. Some solutions

diff --git a/utils/aws/aws.go b/utils/aws/aws.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"strings"
 
 	"github.com/argoproj/argo-rollouts/utils/defaults"
 	"github.com/aws/aws-sdk-go-v2/config"
@@ -295,17 +296,32 @@ func toTargetGroupBinding(obj map[string]interface{}) (*TargetGroupBinding, erro
 	return &tgb, nil
 }
 
-// getNumericPort resolves the numeric port which a AWS TargetGroup targets.
+// getNumericTargetPort resolves the numeric port which a AWS TargetGroup targets.
 // This is needed in case the TargetGroupBinding's spec.serviceRef.Port is a string and not a number
+// and/or the Service's targetPort is a string and not a number
 // Returns 0 if unable to find matching port in given service.
-func getNumericPort(tgb TargetGroupBinding, svc corev1.Service) int32 {
-	if portInt := tgb.Spec.ServiceRef.Port.IntValue(); portInt > 0 {
-		return int32(portInt)
+func getNumericTargetPort(tgb TargetGroupBinding, svc corev1.Service, endpoints corev1.Endpoints) int32 {
+	var servicePortNum int32
+	var servicePortName string
+	if portInt := tgb.Spec.ServiceRef.Port.IntVal; portInt > 0 {
+		servicePortNum = portInt
+	} else {
+		servicePortName = tgb.Spec.ServiceRef.Port.String()
 	}
-	// port is a string and not a num
 	for _, svcPort := range svc.Spec.Ports {
-		if tgb.Spec.ServiceRef.Port.StrVal == svcPort.Name {
-			return svcPort.Port
+		if (servicePortName != "" && servicePortName == svcPort.Name) || (servicePortNum > 0 && servicePortNum == svcPort.Port) {
+			if targetPortNum := svcPort.TargetPort.IntVal; targetPortNum > 0 {
+				return targetPortNum
+			}
+			// targetPort is a string and not a num. Must resort to looking at endpoints
+			targetPortName := svcPort.TargetPort.String()
+			for _, subset := range endpoints.Subsets {
+				for _, port := range subset.Ports {
+					if port.Name == targetPortName {
+						return port.Port
+					}
+				}
+			}
 		}
 	}
 	return 0
@@ -330,7 +346,7 @@ func VerifyTargetGroupBinding(ctx context.Context, logCtx *log.Entry, awsClnt Cl
 		// We only need to verify target groups using AWS CNI (spec.targetType: ip)
 		return nil, nil
 	}
-	port := getNumericPort(tgb, *svc)
+	port := getNumericTargetPort(tgb, *svc, *endpoints)
 	if port == 0 {
 		logCtx.Warn("Unable to match TargetGroupBinding spec.serviceRef.port to Service spec.ports")
 		return nil, nil
@@ -355,6 +371,9 @@ func VerifyTargetGroupBinding(ctx context.Context, logCtx *log.Entry, awsClnt Cl
 	}
 
 	logCtx.Infof("verifying %d endpoint addresses (of %d targets)", len(endpointIPs), len(targets))
+	var ignored []string
+	var verified []string
+	var unverified []string
 
 	// Iterate all registered targets in AWS TargetGroup. Mark all endpoint IPs which we see registered
 	for _, target := range targets {
@@ -365,28 +384,33 @@ func VerifyTargetGroupBinding(ctx context.Context, logCtx *log.Entry, awsClnt Cl
 		targetStr := fmt.Sprintf("%s:%d", *target.Target.Id, *target.Target.Port)
 		_, isEndpointTarget := endpointIPs[targetStr]
 		if !isEndpointTarget {
+			ignored = append(ignored, targetStr)
 			// this is a target for something not in the endpoint list (e.g. old endpoint entry). Ignore it
 			continue
 		}
 		// Verify we see the endpoint IP registered to the TargetGroup
 		// NOTE: we used to check health here, but health is not relevant for verifying service label change
 		endpointIPs[targetStr] = true
+		verified = append(verified, targetStr)
 	}
 
 	tgvr := TargetGroupVerifyResult{
 		Service:             svc.Name,
 		EndpointsTotal:      len(endpointIPs),
-		EndpointsRegistered: 0,
+		EndpointsRegistered: len(verified),
 	}
 
 	// Check if any of our desired endpoints are not yet registered
 	for epIP, seen := range endpointIPs {
 		if !seen {
-			logCtx.Infof("Service endpoint IP %s not yet registered", epIP)
-		} else {
-			tgvr.EndpointsRegistered++
+			unverified = append(unverified, epIP)
 		}
 	}
+
+	logCtx.Infof("Ignored targets: %s", strings.Join(ignored, ", "))
+	logCtx.Infof("Verified targets: %s", strings.Join(verified, ", "))
+	logCtx.Infof("Unregistered targets: %s", strings.Join(unverified, ", "))
+
 	tgvr.Verified = bool(tgvr.EndpointsRegistered == tgvr.EndpointsTotal)
 	return &tgvr, nil
 }
diff --git a/utils/aws/aws_test.go b/utils/aws/aws_test.go
@@ -57,6 +57,43 @@ func TestFindLoadBalancerByDNSName(t *testing.T) {
 	}
 }
 
+func TestGetNumericTargetPort(t *testing.T) {
+	tgb := TargetGroupBinding{
+		Spec: TargetGroupBindingSpec{
+			ServiceRef: ServiceReference{
+				Port: intstr.FromString("web"),
+			},
+		},
+	}
+	svc := corev1.Service{
+		Spec: corev1.ServiceSpec{
+			Ports: []corev1.ServicePort{
+				{
+					Name:       "web",
+					TargetPort: intstr.FromString("http"),
+				},
+			},
+		},
+	}
+	eps := corev1.Endpoints{
+		Subsets: []corev1.EndpointSubset{
+			{
+				Ports: []corev1.EndpointPort{
+					{
+						Name: "asdf",
+						Port: 1234,
+					},
+					{
+						Name: "http",
+						Port: 4567,
+					},
+				},
+			},
+		},
+	}
+	assert.Equal(t, int32(4567), getNumericTargetPort(tgb, svc, eps))
+}
+
 func TestGetTargetGroupMetadata(t *testing.T) {
 	fakeELB, c := newFakeClient()
 
@@ -254,6 +291,10 @@ func TestVerifyTargetGroupBinding(t *testing.T) {
 		Spec: TargetGroupBindingSpec{
 			TargetType:     (*TargetType)(pointer.StringPtr("ip")),
 			TargetGroupARN: "arn::1234",
+			ServiceRef: ServiceReference{
+				Name: "active",
+				Port: intstr.FromInt(80),
+			},
 		},
 	}
 	ep := corev1.Endpoints{
@@ -274,6 +315,12 @@ func TestVerifyTargetGroupBinding(t *testing.T) {
 						IP: "2.4.6.8", // not registered
 					},
 				},
+				Ports: []corev1.EndpointPort{
+					{
+						Port:     8080,
+						Protocol: "TCP",
+					},
+				},
 			},
 		},
 	}
@@ -287,7 +334,7 @@ func TestVerifyTargetGroupBinding(t *testing.T) {
 			Ports: []corev1.ServicePort{{
 				Protocol:   "TCP",
 				Port:       int32(80),
-				TargetPort: intstr.FromInt(80),
+				TargetPort: intstr.FromInt(8080),
 			}},
 		},
 	}
@@ -297,25 +344,25 @@ func TestVerifyTargetGroupBinding(t *testing.T) {
 			{
 				Target: &elbv2types.TargetDescription{
 					Id:   pointer.StringPtr("1.2.3.4"),
-					Port: pointer.Int32Ptr(80),
+					Port: pointer.Int32Ptr(8080),
 				},
 			},
 			{
 				Target: &elbv2types.TargetDescription{
 					Id:   pointer.StringPtr("5.6.7.8"),
-					Port: pointer.Int32Ptr(80),
+					Port: pointer.Int32Ptr(8080),
 				},
 			},
 			{
 				Target: &elbv2types.TargetDescription{
 					Id:   pointer.StringPtr("2.4.6.8"), // irrelevant
-					Port: pointer.Int32Ptr(81),         // wrong port
+					Port: pointer.Int32Ptr(8081),       // wrong port
 				},
 			},
 			{
 				Target: &elbv2types.TargetDescription{
 					Id:   pointer.StringPtr("9.8.7.6"), // irrelevant ip
-					Port: pointer.Int32Ptr(80),
+					Port: pointer.Int32Ptr(8080),
 				},
 			},
 		},