Skip to content

Commit

Permalink
modified requireCanaryStableServices to reverse logic
Browse files Browse the repository at this point in the history 
- 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 <[email protected]>
Signed-off-by: Michael Wilkerson <[email protected]>
  • Loading branch information
@wilkermichael @thisisnotashwin
wilkermichael and thisisnotashwin committed Feb 6, 2024
1 parent d2c7780 commit 25eee48
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 6 deletions.
18 changes: 16 additions & 2 deletions pkg/apis/rollouts/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
120 changes: 116 additions & 4 deletions pkg/apis/rollouts/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{},
}
Expand Down Expand Up @@ -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),
}},
},
}},
Expand Down Expand Up @@ -869,5 +982,4 @@ func TestCanaryExperimentStepWithWeight(t *testing.T) {
assert.Equal(t, 1, len(allErrs))
assert.Equal(t, errTrafficRoutingWithExperimentSupport, allErrs[0].Detail)
})

}

0 comments on commit 25eee48

Please sign in to comment.