test(trafficrouting): add more unit-tests for ALB traffic routing to …

…improve coverage Signed-off-by: Armen Shakhbazian <armen.shakhbazian@gmail.com>
argoproj · May 6, 2024 · 7dcf17c · 7dcf17c
1 parent bba756a
commit 7dcf17c
Show file tree

Hide file tree

Showing 6 changed files with 344 additions and 4 deletions.
diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go
@@ -89,7 +89,7 @@ const (
 	InvalideStepRouteNameNotFoundInManagedRoutes = "Steps define a route that does not exist in spec.strategy.canary.trafficRouting.managedRoutes"
 
 	// ALB traffic routing errors
-	ALBServicePortsReferToUnknownIngress = "ServicePorts reference ingress which is not listed in .Ingresses field"
+	ALBServicePortsReferToUnknownIngress = "ServicePorts reference ingress which is not listed in .Ingress/.Ingresses field"
 	ALBNoServicePortsForIngress          = "Given Ingress has no associated ServicePort. Either specify default .ServicePort value or define specific ports for the Ingress under .ServicePorts"
 )
 

diff --git a/pkg/apis/rollouts/validation/validation_references_test.go b/pkg/apis/rollouts/validation/validation_references_test.go
@@ -955,7 +955,7 @@ func TestValidateRolloutAlbIngressesConfig(t *testing.T) {
 			stableIngress,
 			emptyIngresses,
 			stableIngresses,
-			failureCase2,
+			nil,
 		},
 		{
 			"Just .Ingress configured",
@@ -976,7 +976,7 @@ func TestValidateRolloutAlbIngressesConfig(t *testing.T) {
 			emptyIngress,
 			emptyIngresses,
 			stableIngresses,
-			nil,
+			failureCase1,
 		},
 	}
 

diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go
@@ -633,6 +633,124 @@ func TestValidateRolloutStrategyCanarySetHeaderRouteIstio(t *testing.T) {
 	})
 }
 
+func TestValidateRolloutTrafficRoutingALB(t *testing.T) {
+	ro := &v1alpha1.Rollout{}
+	ro.Spec.Strategy.Canary = &v1alpha1.CanaryStrategy{
+		CanaryService: "canary",
+		StableService: "stable",
+		TrafficRouting: &v1alpha1.RolloutTrafficRouting{
+			ALB: &v1alpha1.ALBTrafficRouting{
+				RootService: "action_name",
+			},
+		},
+	}
+
+	t.Run("Only .Ingress defined, .ServicePorts refers to unknown ingress", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingress: "some-ingress",
+			ServicePorts: []v1alpha1.ALBIngressWithPorts{{
+				Ingress:      "unknown-ingress",
+				ServicePorts: []int32{80},
+			}},
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Equal(t, ALBServicePortsReferToUnknownIngress, allErrs[0].Detail)
+	})
+
+	t.Run(".Ingresses defined, .ServicePorts refers to unknown ingress", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingresses: []string{"some-ingress", "some-other-ingress"},
+			ServicePorts: []v1alpha1.ALBIngressWithPorts{{
+				Ingress:      "unknown-ingress",
+				ServicePorts: []int32{80},
+			}},
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Equal(t, ALBServicePortsReferToUnknownIngress, allErrs[0].Detail)
+	})
+
+	t.Run(".Ingress defined, but neither .ServicePort nor .ServicePorts specified", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingress: "some-ingress",
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Equal(t, ALBNoServicePortsForIngress, allErrs[0].Detail)
+	})
+
+	t.Run("No service port defined for one of ingresses in .Ingresses", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingresses: []string{"some-ingress", "some-other-ingress"},
+			ServicePorts: []v1alpha1.ALBIngressWithPorts{{
+				Ingress:      "some-other-ingress",
+				ServicePorts: []int32{80},
+			}},
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Equal(t, ALBNoServicePortsForIngress, allErrs[0].Detail)
+	})
+
+	t.Run(".Ingresses and default .ServicePort specified", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingresses:   []string{"some-ingress", "some-other-ingress"},
+			ServicePort: 80,
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Empty(t, allErrs)
+	})
+
+	t.Run(".Ingresses and .ServicePorts", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingresses: []string{"some-ingress", "some-other-ingress"},
+			ServicePorts: []v1alpha1.ALBIngressWithPorts{{
+				Ingress:      "some-ingress",
+				ServicePorts: []int32{80},
+			}, {
+				Ingress:      "some-other-ingress",
+				ServicePorts: []int32{443},
+			}},
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Empty(t, allErrs)
+	})
+
+	t.Run(".Ingress defined, port specified via .ServicePorts", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingress: "some-ingress",
+			ServicePorts: []v1alpha1.ALBIngressWithPorts{{
+				Ingress:      "some-ingress",
+				ServicePorts: []int32{80},
+			}},
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Empty(t, allErrs)
+	})
+
+	t.Run(".Ingress defined, port specified via .ServicePort", func(t *testing.T) {
+		invalidRo := ro.DeepCopy()
+		invalidRo.Spec.Strategy.Canary.TrafficRouting.ALB = &v1alpha1.ALBTrafficRouting{
+			Ingress:     "some-ingress",
+			ServicePort: 80,
+		}
+
+		allErrs := ValidateRolloutStrategy(invalidRo, field.NewPath(""))
+		assert.Empty(t, allErrs)
+	})
+}
+
 func TestValidateRolloutStrategyCanarySetHeaderRoutingALB(t *testing.T) {
 	ro := &v1alpha1.Rollout{}
 	ro.Spec.Strategy.Canary = &v1alpha1.CanaryStrategy{

diff --git a/rollout/trafficrouting/alb/alb_test.go b/rollout/trafficrouting/alb/alb_test.go
@@ -721,15 +721,30 @@ func TestErrorPatchingServicePorts(t *testing.T) {
 	testErrorPatching(t, ro, []*networkingv1.Ingress{i, mi})
 }
 
+type fakeAWSClientBalancerFetchError struct {
+	Error   error
+	DNSName string
+}
+type fakeAWSTargetGroupFetchError struct {
+	Error           error
+	LoadBalancerARN string
+}
 type fakeAWSClient struct {
 	Ingresses                []string
 	targetGroups             []aws.TargetGroupMeta
+	targetGroupsErrors       []fakeAWSTargetGroupFetchError
 	loadBalancers            []*elbv2types.LoadBalancer
+	loadBalancerErrors       []fakeAWSClientBalancerFetchError
 	targetHealthDescriptions []elbv2types.TargetHealthDescription
 }
 
 func (f *fakeAWSClient) GetTargetGroupMetadata(ctx context.Context, loadBalancerARN string) ([]aws.TargetGroupMeta, error) {
 	result := []aws.TargetGroupMeta{}
+	for _, lb := range f.targetGroupsErrors {
+		if lb.LoadBalancerARN == loadBalancerARN {
+			return nil, lb.Error
+		}
+	}
 	for _, tg := range f.targetGroups {
 		if slices.Contains(tg.LoadBalancerArns, loadBalancerARN) {
 			result = append(result, tg)
@@ -739,6 +754,11 @@ func (f *fakeAWSClient) GetTargetGroupMetadata(ctx context.Context, loadBalancer
 }
 
 func (f *fakeAWSClient) FindLoadBalancerByDNSName(ctx context.Context, dnsName string) (*elbv2types.LoadBalancer, error) {
+	for _, lb := range f.loadBalancerErrors {
+		if lb.DNSName == dnsName {
+			return nil, lb.Error
+		}
+	}
 	for _, lb := range f.loadBalancers {
 		if lb.DNSName != nil && *lb.DNSName == dnsName {
 			return lb, nil
@@ -1232,6 +1252,69 @@ func TestVerifyWeightMultiIngress(t *testing.T) {
 		assert.Equal(t, status.ALBs[0], *fakeClient.getAlbStatusMultiIngress("ingress", 0, 0))
 		assert.Equal(t, status.ALBs[1], *fakeClient.getAlbStatusMultiIngress("multi-ingress", 1, 2))
 	}
+
+	// LoadBalancer found, dns-mismatch
+	{
+		var status v1alpha1.RolloutStatus
+		r, fakeClient := newFakeReconciler(&status)
+		fakeClient.loadBalancers = []*elbv2types.LoadBalancer{
+			makeLoadBalancer("lb-abc123-name", "broken-dns-verify-weight-test-abc-123.us-west-2.elb.amazonaws.com"),
+			makeLoadBalancer("lb-multi-ingress-name", "verify-weight-multi-ingress.us-west-2.elb.amazonaws.com"),
+		}
+		fakeClient.targetGroups = []aws.TargetGroupMeta{
+			makeTargetGroupForBalancer("lb-multi-ingress-name", "multi-ingress-canary-tg-abc123-name", 443, 11, "default/multi-ingress-canary-svc:443"),
+			makeTargetGroupForBalancer("lb-multi-ingress-name", "multi-ingress-stable-tg-abc123-name", 443, 89, "default/multi-ingress-stable-svc:443"),
+		}
+
+		weightVerified, err := r.VerifyWeight(10)
+		assert.NoError(t, err)
+		assert.False(t, *weightVerified)
+	}
+
+	// LoadBalancer found, but balancer fetch failed with error
+	{
+		expectedError := k8serrors.NewBadRequest("failed to fetch load balancer")
+		var status v1alpha1.RolloutStatus
+		r, fakeClient := newFakeReconciler(&status)
+		fakeClient.loadBalancerErrors = []fakeAWSClientBalancerFetchError{{
+			DNSName: "verify-weight-test-abc-123.us-west-2.elb.amazonaws.com",
+			Error:   expectedError,
+		}}
+		fakeClient.loadBalancers = []*elbv2types.LoadBalancer{
+			makeLoadBalancer("lb-multi-ingress-name", "verify-weight-multi-ingress.us-west-2.elb.amazonaws.com"),
+		}
+		fakeClient.targetGroups = []aws.TargetGroupMeta{
+			makeTargetGroupForBalancer("lb-multi-ingress-name", "multi-ingress-canary-tg-abc123-name", 443, 11, "default/multi-ingress-canary-svc:443"),
+			makeTargetGroupForBalancer("lb-multi-ingress-name", "multi-ingress-stable-tg-abc123-name", 443, 89, "default/multi-ingress-stable-svc:443"),
+		}
+
+		weightVerified, err := r.VerifyWeight(10)
+		assert.Equal(t, err, expectedError)
+		assert.False(t, *weightVerified)
+	}
+
+	// LoadBalancer found, but target group fetch failed with error
+	{
+		expectedError := k8serrors.NewBadRequest("failed to fetch target group")
+		var status v1alpha1.RolloutStatus
+		r, fakeClient := newFakeReconciler(&status)
+		fakeClient.loadBalancers = []*elbv2types.LoadBalancer{
+			makeLoadBalancer("lb-abc123-name", "verify-weight-test-abc-123.us-west-2.elb.amazonaws.com"),
+			makeLoadBalancer("lb-multi-ingress-name", "verify-weight-multi-ingress.us-west-2.elb.amazonaws.com"),
+		}
+		fakeClient.targetGroups = []aws.TargetGroupMeta{
+			makeTargetGroupForBalancer("lb-multi-ingress-name", "multi-ingress-canary-tg-abc123-name", 443, 11, "default/multi-ingress-canary-svc:443"),
+			makeTargetGroupForBalancer("lb-multi-ingress-name", "multi-ingress-stable-tg-abc123-name", 443, 89, "default/multi-ingress-stable-svc:443"),
+		}
+		fakeClient.targetGroupsErrors = []fakeAWSTargetGroupFetchError{{
+			LoadBalancerARN: *fakeClient.loadBalancers[0].LoadBalancerArn,
+			Error:           expectedError,
+		}}
+
+		weightVerified, err := r.VerifyWeight(10)
+		assert.Equal(t, err, expectedError)
+		assert.False(t, *weightVerified)
+	}
 }
 
 func TestVerifyWeightServicePorts(t *testing.T) {

diff --git a/utils/ingress/ingress.go b/utils/ingress/ingress.go
@@ -93,7 +93,7 @@ func SingleAlbIngressConfigured(rollout *v1alpha1.Rollout) bool {
 
 // CheckALBTrafficRoutingHasFieldsForMultiIngressScenario returns true if either .Ingresses or .ServicePorts are set
 func CheckALBTrafficRoutingHasFieldsForMultiIngressScenario(a *v1alpha1.ALBTrafficRouting) bool {
-	return a.Ingresses != nil || a.ServicePorts != nil
+	return a.Ingresses != nil
 }
 
 // GetIngressesFromALBTrafficRouting returns a list of Ingresses and their associated ports.