diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index 1327960a5f..ebf72ffc8a 100644 --- 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 index f3df814fb9..352c08c86b 100644 --- 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 index 84fc93b5dd..7c60eeeb21 100644 --- 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 index 66194afb22..339bc12357 100644 --- a/rollout/trafficrouting/alb/alb_test.go +++ b/rollout/trafficrouting/alb/alb_test.go @@ -722,15 +722,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) @@ -740,6 +755,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 @@ -1254,6 +1274,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 index 515f725ce3..5bc684fc02 100644 --- 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. diff --git a/utils/ingress/ingress_test.go b/utils/ingress/ingress_test.go index ccbd3449f1..bfa2682d7f 100644 --- a/utils/ingress/ingress_test.go +++ b/utils/ingress/ingress_test.go @@ -698,3 +698,142 @@ func TestALBHeaderBasedConditionAnnotationKey(t *testing.T) { } assert.Equal(t, "alb.ingress.kubernetes.io/conditions.route", ALBHeaderBasedConditionAnnotationKey(r, "route")) } + +func TestCheckALBTrafficRoutingHasFieldsForMultiIngressScenario(t *testing.T) { + res := CheckALBTrafficRoutingHasFieldsForMultiIngressScenario(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + }) + assert.Equal(t, res, false) + + res = CheckALBTrafficRoutingHasFieldsForMultiIngressScenario(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + Ingresses: []string{"alb-ingress", "alb-ingress-2"}, + }) + assert.Equal(t, res, true) + + res = CheckALBTrafficRoutingHasFieldsForMultiIngressScenario(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + Ingresses: []string{"alb-ingress", "alb-ingress-2"}, + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress", ServicePorts: []int32{80, 433}}, + }, + }) + assert.Equal(t, res, true) + + res = CheckALBTrafficRoutingHasFieldsForMultiIngressScenario(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress", ServicePorts: []int32{80, 433}}, + }, + }) + assert.Equal(t, res, false) +} + +func TestGetIngressesFromALBTrafficRouting(t *testing.T) { + res := GetIngressesFromALBTrafficRouting(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + }) + assert.Equal(t, res, []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress", ServicePorts: []int32{80}}, + }) + + // .ServicePort is ignored when .ServicePorts has matching ingress + res = GetIngressesFromALBTrafficRouting(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress", ServicePorts: []int32{80, 433}}, + }, + }) + assert.Equal(t, res, []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress", ServicePorts: []int32{80, 433}}, + }) + + // .Ingress is ignored when .Ingresses is specified + res = GetIngressesFromALBTrafficRouting(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + Ingresses: []string{"alb-ingress-1", "alb-ingress-2"}, + }) + assert.Equal(t, res, []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress-1", ServicePorts: []int32{80}}, + {Ingress: "alb-ingress-2", ServicePorts: []int32{80}}, + }) + + // Ports are taken from .ServicePorts if matching ingress is found + res = GetIngressesFromALBTrafficRouting(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + ServicePort: 80, + Ingresses: []string{"alb-ingress-1", "alb-ingress-2"}, + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress-2", ServicePorts: []int32{433}}, + }, + }) + assert.Equal(t, res, []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress-1", ServicePorts: []int32{80}}, + {Ingress: "alb-ingress-2", ServicePorts: []int32{433}}, + }) +} + +func TestGetAllIngressNamesFromALBTrafficRouting(t *testing.T) { + // without namespace + res := GetAllIngressNamesFromALBTrafficRouting(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + Ingresses: []string{"alb-ingress-1", "alb-ingress-2"}, + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress-3", ServicePorts: []int32{433}}, + {Ingress: "alb-ingress-4", ServicePorts: []int32{8080}}, + }, + }, "") + assert.ElementsMatch(t, res, []string{ + "alb-ingress", + "alb-ingress-1", + "alb-ingress-2", + "alb-ingress-3", + "alb-ingress-4", + }) + + // with namespace + res = GetAllIngressNamesFromALBTrafficRouting(&v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + // intentionally with duplicate + Ingresses: []string{"alb-ingress-1", "alb-ingress-2", "alb-ingress-2"}, + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress-3", ServicePorts: []int32{433}}, + {Ingress: "alb-ingress-4", ServicePorts: []int32{8080}}, + // intentionally duplicate + {Ingress: "alb-ingress-4", ServicePorts: []int32{433}}, + }, + }, "some-namespace") + assert.ElementsMatch(t, res, []string{ + "some-namespace/alb-ingress", + "some-namespace/alb-ingress-1", + "some-namespace/alb-ingress-2", + "some-namespace/alb-ingress-3", + "some-namespace/alb-ingress-4", + }) +} + +func TestCheckALBTrafficRoutingReferencesIngress(t *testing.T) { + rollout := &v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + // intentionally with duplicate + Ingresses: []string{"alb-ingress-1", "alb-ingress-2"}, + ServicePorts: []v1alpha1.ALBIngressWithPorts{ + {Ingress: "alb-ingress-3", ServicePorts: []int32{433}}, + {Ingress: "alb-ingress-4", ServicePorts: []int32{8080}}, + }, + } + assert.Equal(t, true, CheckALBTrafficRoutingReferencesIngress(rollout, "alb-ingress")) + assert.Equal(t, true, CheckALBTrafficRoutingReferencesIngress(rollout, "alb-ingress-1")) + assert.Equal(t, true, CheckALBTrafficRoutingReferencesIngress(rollout, "alb-ingress-2")) + assert.Equal(t, true, CheckALBTrafficRoutingReferencesIngress(rollout, "alb-ingress-3")) + assert.Equal(t, true, CheckALBTrafficRoutingReferencesIngress(rollout, "alb-ingress-4")) + + assert.Equal(t, false, CheckALBTrafficRoutingReferencesIngress(rollout, "alb-ingress-5")) + assert.Equal(t, false, CheckALBTrafficRoutingReferencesIngress(rollout, "")) +}