From 25eee48b7a7ccff4d107e61412fc5b4953f51549 Mon Sep 17 00:00:00 2001 From: Michael Wilkerson Date: Tue, 6 Feb 2024 08:01:30 -0800 Subject: [PATCH] modified requireCanaryStableServices to reverse logic - added test cases for exceptions to requiring checks for Canary/Stable services - added test cases for verifying errors when Canary/Stable services are missing Co-authored-by: Ashwin Venkatesh Signed-off-by: Michael Wilkerson --- pkg/apis/rollouts/validation/validation.go | 18 ++- .../rollouts/validation/validation_test.go | 120 +++++++++++++++++- 2 files changed, 132 insertions(+), 6 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index f241d5b1d3..54a7336566 100644 --- a/pkg/apis/rollouts/validation/validation.go +++ b/pkg/apis/rollouts/validation/validation.go @@ -231,10 +231,24 @@ func ValidateRolloutStrategyBlueGreen(rollout *v1alpha1.Rollout, fldPath *field. // canary.canaryService to be defined func requireCanaryStableServices(rollout *v1alpha1.Rollout) bool { canary := rollout.Spec.Strategy.Canary - if canary.TrafficRouting == nil || (canary.TrafficRouting.Istio != nil && canary.TrafficRouting.Istio.DestinationRule != nil) || (canary.PingPong != nil) || (canary.TrafficRouting.Plugins["hashicorp/consul"] != nil) { + + if canary.TrafficRouting == nil { + return false + } + + switch { + case canary.TrafficRouting.ALB != nil && canary.PingPong == nil, + canary.TrafficRouting.Istio != nil && canary.TrafficRouting.Istio.DestinationRule == nil, + canary.TrafficRouting.SMI != nil, + canary.TrafficRouting.Apisix != nil, + canary.TrafficRouting.Ambassador != nil, + canary.TrafficRouting.Nginx != nil, + canary.TrafficRouting.AppMesh != nil, + canary.TrafficRouting.Traefik != nil: + return true + default: return false } - return true } func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Path) field.ErrorList { diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go index 58a4571ae7..5de38f6dad 100644 --- a/pkg/apis/rollouts/validation/validation_test.go +++ b/pkg/apis/rollouts/validation/validation_test.go @@ -129,6 +129,80 @@ func TestValidateRolloutStrategyBlueGreen(t *testing.T) { assert.Equal(t, ScaleDownLimitLargerThanRevisionLimit, allErrs[1].Detail) } +func TestValidateRolloutStrategyCanaryMissingServiceNames(t *testing.T) { + tests := []struct { + name string + trafficRouting *v1alpha1.RolloutTrafficRouting + }{ + { + name: "ALB", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + ALB: &v1alpha1.ALBTrafficRouting{RootService: "root-service"}, + }, + }, + { + name: "Istio", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + Istio: &v1alpha1.IstioTrafficRouting{}, + }, + }, + { + name: "SMI", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + SMI: &v1alpha1.SMITrafficRouting{}, + }, + }, + { + name: "Apisix", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + Apisix: &v1alpha1.ApisixTrafficRouting{}, + }, + }, + { + name: "Ambassador", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + Ambassador: &v1alpha1.AmbassadorTrafficRouting{}, + }, + }, + { + name: "Nginx", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + Nginx: &v1alpha1.NginxTrafficRouting{}, + }, + }, + { + name: "AppMesh", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + AppMesh: &v1alpha1.AppMeshTrafficRouting{}, + }, + }, + { + name: "Traefik", + trafficRouting: &v1alpha1.RolloutTrafficRouting{ + Traefik: &v1alpha1.TraefikTrafficRouting{}, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create a table of test cases + canaryStrategy := &v1alpha1.CanaryStrategy{ + Steps: []v1alpha1.CanaryStep{{ + SetWeight: pointer.Int32(10), + }}, + } + ro := &v1alpha1.Rollout{} + ro.Spec.Strategy.Canary = canaryStrategy + + // Set the traffic route we will be testing + ro.Spec.Strategy.Canary.TrafficRouting = tt.trafficRouting + allErrs := ValidateRolloutStrategyCanary(ro, field.NewPath("")) + assert.Equal(t, InvalidTrafficRoutingMessage, allErrs[0].Detail) + }) + } +} + func TestValidateRolloutStrategyCanary(t *testing.T) { canaryStrategy := &v1alpha1.CanaryStrategy{ CanaryService: "canary", @@ -165,6 +239,45 @@ func TestValidateRolloutStrategyCanary(t *testing.T) { }, } + t.Run("valid rollout", func(t *testing.T) { + validRo := ro.DeepCopy() + validRo.Spec.Strategy.Canary.Steps[0].SetWeight = pointer.Int32(10) + allErrs := ValidateRolloutStrategyCanary(validRo, field.NewPath("")) + assert.Empty(t, allErrs) + }) + + t.Run("valid plugin missing canary and stable service", func(t *testing.T) { + validRo := ro.DeepCopy() + validRo.Spec.Strategy.Canary.Steps[0].SetWeight = pointer.Int32(10) + validRo.Spec.Strategy.Canary.CanaryService = "" + validRo.Spec.Strategy.Canary.StableService = "" + validRo.Spec.Strategy.Canary.TrafficRouting.ALB = nil + validRo.Spec.Strategy.Canary.TrafficRouting.Plugins = map[string]json.RawMessage{"some-plugin": []byte(`{"key": "value"}`)} + allErrs := ValidateRolloutStrategyCanary(validRo, field.NewPath("")) + assert.Empty(t, allErrs) + }) + + t.Run("valid Istio missing canary and stable service", func(t *testing.T) { + validRo := ro.DeepCopy() + validRo.Spec.Strategy.Canary.Steps[0].SetWeight = pointer.Int32(10) + validRo.Spec.Strategy.Canary.CanaryService = "" + validRo.Spec.Strategy.Canary.StableService = "" + validRo.Spec.Strategy.Canary.TrafficRouting.Istio = &v1alpha1.IstioTrafficRouting{DestinationRule: &v1alpha1.IstioDestinationRule{Name: "destination-rule"}} + validRo.Spec.Strategy.Canary.TrafficRouting.ALB = nil + allErrs := ValidateRolloutStrategyCanary(validRo, field.NewPath("")) + assert.Empty(t, allErrs) + }) + + t.Run("valid PingPong missing canary and stable service", func(t *testing.T) { + validRo := ro.DeepCopy() + validRo.Spec.Strategy.Canary.Steps[0].SetWeight = pointer.Int32(10) + validRo.Spec.Strategy.Canary.CanaryService = "" + validRo.Spec.Strategy.Canary.StableService = "" + validRo.Spec.Strategy.Canary.PingPong = &v1alpha1.PingPongSpec{PingService: "ping", PongService: "pong"} + allErrs := ValidateRolloutStrategyCanary(validRo, field.NewPath("")) + assert.Empty(t, allErrs) + }) + t.Run("duplicate services", func(t *testing.T) { invalidRo := ro.DeepCopy() invalidRo.Spec.Strategy.Canary.CanaryService = "stable" @@ -651,7 +764,7 @@ func TestCanaryScaleDownDelaySeconds(t *testing.T) { Canary: &v1alpha1.CanaryStrategy{ StableService: "stable", CanaryService: "canary", - ScaleDownDelaySeconds: pointer.Int32Ptr(60), + ScaleDownDelaySeconds: pointer.Int32(60), }, }, Template: corev1.PodTemplateSpec{ @@ -718,7 +831,7 @@ func TestCanaryDynamicStableScale(t *testing.T) { }) t.Run("dynamicStableScale with scaleDownDelaySeconds", func(t *testing.T) { ro := ro.DeepCopy() - ro.Spec.Strategy.Canary.ScaleDownDelaySeconds = pointer.Int32Ptr(60) + ro.Spec.Strategy.Canary.ScaleDownDelaySeconds = pointer.Int32(60) ro.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ SMI: &v1alpha1.SMITrafficRouting{}, } @@ -782,7 +895,7 @@ func TestCanaryExperimentStepWithWeight(t *testing.T) { Experiment: &v1alpha1.RolloutExperimentStep{ Templates: []v1alpha1.RolloutExperimentTemplate{{ Name: "template", - Weight: pointer.Int32Ptr(20), + Weight: pointer.Int32(20), }}, }, }}, @@ -869,5 +982,4 @@ func TestCanaryExperimentStepWithWeight(t *testing.T) { assert.Equal(t, 1, len(allErrs)) assert.Equal(t, errTrafficRoutingWithExperimentSupport, allErrs[0].Detail) }) - }