Skip to content

Commit

Permalink
cherry-picked commit 4848fb1 from master
Browse files Browse the repository at this point in the history
  • Loading branch information
bsagiv committed Nov 17, 2022
1 parent b0b95ca commit 3a63652
Show file tree
Hide file tree
Showing 14 changed files with 1,171 additions and 70 deletions.
41 changes: 24 additions & 17 deletions ingress/alb.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import (
func (c *Controller) syncALBIngress(ingress *ingressutil.Ingress, rollouts []*v1alpha1.Rollout) error {
ctx := context.TODO()
annotations := ingress.GetAnnotations()
managedActions, err := ingressutil.NewManagedALBActions(annotations[ingressutil.ManagedActionsAnnotation])
managedActions, err := ingressutil.NewManagedALBAnnotations(annotations[ingressutil.ManagedAnnotations])
if err != nil {
return nil
}
Expand All @@ -35,31 +35,38 @@ func (c *Controller) syncALBIngress(ingress *ingressutil.Ingress, rollouts []*v1
for roName := range managedActions {
if _, ok := actionHasExistingRollout[roName]; !ok {
modified = true
actionKey := managedActions[roName]
actionKeys := managedActions[roName]
delete(managedActions, roName)
resetALBAction, err := getResetALBActionStr(ingress, actionKey)
if err != nil {
log.WithField(logutil.RolloutKey, roName).
WithField(logutil.IngressKey, ingress.GetName()).
WithField(logutil.NamespaceKey, ingress.GetNamespace()).
Error(err)
return nil
for _, actionKey := range actionKeys {
if !strings.Contains(actionKey, ingressutil.ALBActionPrefix) {
continue
}
resetALBAction, err := getResetALBActionStr(ingress, actionKey)
if err != nil {
log.WithField(logutil.RolloutKey, roName).
WithField(logutil.IngressKey, ingress.GetName()).
WithField(logutil.NamespaceKey, ingress.GetNamespace()).
Error(err)
return nil
}
annotations := newIngress.GetAnnotations()
annotations[actionKey] = resetALBAction
newIngress.SetAnnotations(annotations)
}
annotations := newIngress.GetAnnotations()
annotations[actionKey] = resetALBAction
newIngress.SetAnnotations(annotations)
}
}
if !modified {
return nil
}
newManagedStr := managedActions.String()
newAnnotations := newIngress.GetAnnotations()
newAnnotations[ingressutil.ManagedActionsAnnotation] = newManagedStr
newIngress.SetAnnotations(newAnnotations)
if newManagedStr == "" {
delete(newIngress.GetAnnotations(), ingressutil.ManagedActionsAnnotation)
if len(managedActions) == 0 {
delete(newAnnotations, ingressutil.ManagedAnnotations)
} else {
newAnnotations[ingressutil.ManagedAnnotations] = managedActions.String()
}
// delete leftovers from old implementation ManagedActionsAnnotation
delete(newAnnotations, ingressutil.ManagedActionsAnnotation)
newIngress.SetAnnotations(newAnnotations)
_, err = c.ingressWrapper.Update(ctx, ingress.GetNamespace(), newIngress)
return err
}
Expand Down
19 changes: 11 additions & 8 deletions ingress/alb_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ func albActionAnnotation(stable string) string {
func newALBIngress(name string, port int, serviceName string, rollout string, includeStickyConfig bool) *extensionsv1beta1.Ingress {
canaryService := fmt.Sprintf("%s-canary", serviceName)
albActionKey := albActionAnnotation(serviceName)
managedBy := fmt.Sprintf("%s:%s", rollout, albActionKey)
albConditionKey := fmt.Sprintf("%s%s%s", ingressutil.ALBIngressAnnotation, ingressutil.ALBConditionPrefix, serviceName)
managedBy := ingressutil.ManagedALBAnnotations{
rollout: ingressutil.ManagedALBAnnotation{albActionKey, albConditionKey},
}
action := fmt.Sprintf(actionTemplate, serviceName, port, canaryService, port)
if includeStickyConfig {
action = fmt.Sprintf(actionTemplateWithStickyConfig, serviceName, port, canaryService, port)
Expand All @@ -70,9 +73,9 @@ func newALBIngress(name string, port int, serviceName string, rollout string, in
Name: name,
Namespace: metav1.NamespaceDefault,
Annotations: map[string]string{
"kubernetes.io/ingress.class": "alb",
albActionKey: action,
ingressutil.ManagedActionsAnnotation: managedBy,
"kubernetes.io/ingress.class": "alb",
albActionKey: action,
ingressutil.ManagedAnnotations: managedBy.String(),
},
},
Spec: extensionsv1beta1.IngressSpec{
Expand Down Expand Up @@ -123,7 +126,7 @@ func rollout(name, service, ingress string) *v1alpha1.Rollout {
func TestInvalidManagedALBActions(t *testing.T) {
rollout := rollout("rollout", "stable-service", "test-ingress")
ing := newALBIngress("test-ingress", 80, "stable-service", rollout.Name, false)
ing.Annotations[ingressutil.ManagedActionsAnnotation] = "invalid-managed-by"
ing.Annotations[ingressutil.ManagedAnnotations] = "invalid-managed-by"

ctrl, kubeclient, enqueuedObjects := newFakeIngressController(t, ing, rollout)

Expand All @@ -147,7 +150,7 @@ func TestInvalidPreviousALBActionAnnotationValue(t *testing.T) {

func TestInvalidPreviousALBActionAnnotationKey(t *testing.T) {
ing := newALBIngress("test-ingress", 80, "stable-service", "also-not-existing-rollout", false)
ing.Annotations[ingressutil.ManagedActionsAnnotation] = "invalid-action-key"
ing.Annotations[ingressutil.ManagedAnnotations] = "invalid-action-key"
ctrl, kubeclient, enqueuedObjects := newFakeIngressController(t, ing, nil)

err := ctrl.syncIngress("default/test-ingress")
Expand Down Expand Up @@ -199,7 +202,7 @@ func TestALBIngressResetAction(t *testing.T) {
panic(err)
}
annotations := acc.GetAnnotations()
assert.NotContains(t, annotations, ingressutil.ManagedActionsAnnotation)
assert.NotContains(t, annotations, ingressutil.ManagedAnnotations)
expectedAction := `{"Type":"forward","ForwardConfig":{"TargetGroups":[{"ServiceName":"stable-service","ServicePort":"80","Weight":100}]}}`
assert.Equal(t, expectedAction, annotations[albActionAnnotation("stable-service")])
}
Expand All @@ -223,7 +226,7 @@ func TestALBIngressResetActionWithStickyConfig(t *testing.T) {
panic(err)
}
annotations := acc.GetAnnotations()
assert.NotContains(t, annotations, ingressutil.ManagedActionsAnnotation)
assert.NotContains(t, annotations, ingressutil.ManagedAnnotations)
expectedAction := `{"Type":"forward","ForwardConfig":{"TargetGroups":[{"ServiceName":"stable-service","ServicePort":"80","Weight":100}],"TargetGroupStickinessConfig":{"Enabled":true,"DurationSeconds":300}}}`
assert.Equal(t, expectedAction, annotations[albActionAnnotation("stable-service")])
}
40 changes: 29 additions & 11 deletions pkg/apis/rollouts/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,16 @@ const (
InvalidCanaryExperimentTemplateWeightWithoutTrafficRouting = "Experiment template weight cannot be set unless TrafficRouting is enabled"
// InvalidSetCanaryScaleTrafficPolicy indicates that TrafficRouting, required for SetCanaryScale, is missing
InvalidSetCanaryScaleTrafficPolicy = "SetCanaryScale requires TrafficRouting to be set"
// InvalidSetHeaderRoutingTrafficPolicy indicates that TrafficRouting, required for SetCanaryScale, is missing
InvalidSetHeaderRoutingTrafficPolicy = "SetHeaderRoute requires TrafficRouting, supports Istio only"
// InvalidSetHeaderRouteTrafficPolicy indicates that TrafficRouting required for SetHeaderRoute is missing
InvalidSetHeaderRouteTrafficPolicy = "SetHeaderRoute requires TrafficRouting, supports Istio and ALB"
// InvalidSetMirrorRouteTrafficPolicy indicates that TrafficRouting, required for SetCanaryScale, is missing
InvalidSetMirrorRouteTrafficPolicy = "SetMirrorRoute requires TrafficRouting, supports Istio only"
// InvalidStringMatchMultipleValuePolicy indicates that SetCanaryScale, has multiple values set
InvalidStringMatchMultipleValuePolicy = "StringMatch match value must have exactly one of the following: exact, regex, prefix"
// InvalidStringMatchMissedValuePolicy indicates that SetCanaryScale, has multiple values set
InvalidStringMatchMissedValuePolicy = "StringMatch value missed, match value must have one of the following: exact, regex, prefix"
// InvalidSetHeaderRouteALBValuePolicy indicates that SetHeaderRouting using with ALB missed the 'exact' value
InvalidSetHeaderRouteALBValuePolicy = "SetHeaderRoute match value invalid. ALB supports 'exact' value only"
// InvalidDurationMessage indicates the Duration value needs to be greater than 0
InvalidDurationMessage = "Duration needs to be greater than 0"
// InvalidMaxSurgeMaxUnavailable indicates both maxSurge and MaxUnavailable can not be set to zero
Expand Down Expand Up @@ -78,8 +80,6 @@ const (
MissedAlbRootServiceMessage = "Root service field is required for the configuration with ALB and ping-pong feature enabled"
// PingPongWithAlbOnlyMessage At this moment ping-pong feature works with the ALB traffic routing only
PingPongWithAlbOnlyMessage = "Ping-pong feature works with the ALB traffic routing only"
// InvalidStepMissingManagedRoutesField We have a step configured that requires managedRoutes to be configured which is not.
InvalidStepMissingManagedRoutesField = "Step requires spec.strategy.canary.trafficRouting.managedRoutes to be configured"
// InvalideStepRouteNameNotFoundInManagedRoutes A step has been configured that requires managedRoutes and the route name
// is missing from managedRoutes
InvalideStepRouteNameNotFoundInManagedRoutes = "Steps define a route that does not exist in spec.strategy.canary.trafficRouting.managedRoutes"
Expand Down Expand Up @@ -305,13 +305,17 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat

if step.SetHeaderRoute != nil {
trafficRouting := rollout.Spec.Strategy.Canary.TrafficRouting
if trafficRouting == nil || trafficRouting.Istio == nil {
allErrs = append(allErrs, field.Invalid(stepFldPath.Child("setHeaderRoute"), step.SetHeaderRoute, InvalidSetHeaderRoutingTrafficPolicy))
}
if step.SetHeaderRoute.Match != nil && len(step.SetHeaderRoute.Match) > 0 {
if trafficRouting == nil || (trafficRouting.Istio == nil && trafficRouting.ALB == nil) {
allErrs = append(allErrs, field.Invalid(stepFldPath.Child("setHeaderRoute"), step.SetHeaderRoute, InvalidSetHeaderRouteTrafficPolicy))
} else if step.SetHeaderRoute.Match != nil && len(step.SetHeaderRoute.Match) > 0 {
for j, match := range step.SetHeaderRoute.Match {
matchFld := stepFldPath.Child("setHeaderRoute").Child("match").Index(j)
allErrs = append(allErrs, hasMultipleMatchValues(match.HeaderValue, matchFld)...)
if trafficRouting.ALB != nil {
matchFld := stepFldPath.Child("setHeaderRoute").Child("match").Index(j)
allErrs = append(allErrs, hasALBInvalidValues(match.HeaderValue, matchFld)...)
} else {
matchFld := stepFldPath.Child("setHeaderRoute").Child("match").Index(j)
allErrs = append(allErrs, hasMultipleMatchValues(match.HeaderValue, matchFld)...)
}
}
}
}
Expand Down Expand Up @@ -340,7 +344,8 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat
if rollout.Spec.Strategy.Canary.TrafficRouting != nil {
if step.SetHeaderRoute != nil || step.SetMirrorRoute != nil {
if rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes == nil {
allErrs = append(allErrs, field.Invalid(stepFldPath, step, InvalidStepMissingManagedRoutesField))
message := fmt.Sprintf(MissingFieldMessage, "spec.strategy.canary.trafficRouting.managedRoutes")
allErrs = append(allErrs, field.Required(fldPath.Child("trafficRouting", "managedRoutes"), message))
}
}
}
Expand Down Expand Up @@ -473,6 +478,19 @@ func hasMultipleStepsType(s v1alpha1.CanaryStep, fldPath *field.Path) field.Erro
return allErrs
}

func hasALBInvalidValues(match *v1alpha1.StringMatch, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
if match == nil {
e := field.Invalid(fldPath, match, InvalidStringMatchMissedValuePolicy)
allErrs = append(allErrs, e)
return allErrs
}
if match.Exact == "" || match.Regex != "" || match.Prefix != "" {
return append(allErrs, field.Invalid(fldPath, match, InvalidSetHeaderRouteALBValuePolicy))
}
return allErrs
}

func hasMultipleMatchValues(match *v1alpha1.StringMatch, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

Expand Down
90 changes: 81 additions & 9 deletions pkg/apis/rollouts/validation/validation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -281,21 +281,15 @@ func TestValidateRolloutStrategyAntiAffinity(t *testing.T) {
assert.Equal(t, InvalidAntiAffinityWeightMessage, allErrs[0].Detail)
}

func TestValidateRolloutStrategyCanarySetHeaderRouteIstio(t *testing.T) {
func TestValidateRolloutStrategyCanarySetHeaderRoute(t *testing.T) {
ro := &v1alpha1.Rollout{}
ro.Spec.Strategy.Canary = &v1alpha1.CanaryStrategy{
CanaryService: "canary",
StableService: "stable",
TrafficRouting: &v1alpha1.RolloutTrafficRouting{
Istio: &v1alpha1.IstioTrafficRouting{
VirtualService: &v1alpha1.IstioVirtualService{Name: "virtual-service"},
},
},
}

t.Run("using SetHeaderRoute step without the traffic routing", func(t *testing.T) {
invalidRo := ro.DeepCopy()
invalidRo.Spec.Strategy.Canary.TrafficRouting = nil
invalidRo.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
SetHeaderRoute: &v1alpha1.SetHeaderRoute{
Match: []v1alpha1.HeaderRoutingMatch{
Expand All @@ -307,8 +301,21 @@ func TestValidateRolloutStrategyCanarySetHeaderRouteIstio(t *testing.T) {
},
}}
allErrs := ValidateRolloutStrategyCanary(invalidRo, field.NewPath(""))
assert.Equal(t, InvalidSetHeaderRoutingTrafficPolicy, allErrs[0].Detail)
assert.Equal(t, InvalidSetHeaderRouteTrafficPolicy, allErrs[0].Detail)
})
}

func TestValidateRolloutStrategyCanarySetHeaderRouteIstio(t *testing.T) {
ro := &v1alpha1.Rollout{}
ro.Spec.Strategy.Canary = &v1alpha1.CanaryStrategy{
CanaryService: "canary",
StableService: "stable",
TrafficRouting: &v1alpha1.RolloutTrafficRouting{
Istio: &v1alpha1.IstioTrafficRouting{
VirtualService: &v1alpha1.IstioVirtualService{Name: "virtual-service"},
},
},
}

t.Run("using SetHeaderRoute step with multiple values", func(t *testing.T) {
invalidRo := ro.DeepCopy()
Expand Down Expand Up @@ -364,6 +371,71 @@ func TestValidateRolloutStrategyCanarySetHeaderRouteIstio(t *testing.T) {
})
}

func TestValidateRolloutStrategyCanarySetHeaderRoutingALB(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("using SetHeaderRouting step with multiple values", func(t *testing.T) {
invalidRo := ro.DeepCopy()
invalidRo.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
SetHeaderRoute: &v1alpha1.SetHeaderRoute{
Match: []v1alpha1.HeaderRoutingMatch{
{
HeaderName: "agent",
HeaderValue: &v1alpha1.StringMatch{
Exact: "chrome",
Regex: "chrome(.*)",
},
},
},
},
}}
allErrs := ValidateRolloutStrategyCanary(invalidRo, field.NewPath(""))
assert.Equal(t, InvalidSetHeaderRouteALBValuePolicy, allErrs[0].Detail)
})

t.Run("using SetHeaderRouting step with missed values", func(t *testing.T) {
invalidRo := ro.DeepCopy()
invalidRo.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
SetHeaderRoute: &v1alpha1.SetHeaderRoute{
Match: []v1alpha1.HeaderRoutingMatch{
{
HeaderName: "agent",
},
},
},
}}
allErrs := ValidateRolloutStrategyCanary(invalidRo, field.NewPath(""))
assert.Equal(t, InvalidStringMatchMissedValuePolicy, allErrs[0].Detail)
})

t.Run("using SetHeaderRouting step with invalid ALB match value", func(t *testing.T) {
invalidRo := ro.DeepCopy()
invalidRo.Spec.Strategy.Canary.Steps = []v1alpha1.CanaryStep{{
SetHeaderRoute: &v1alpha1.SetHeaderRoute{
Match: []v1alpha1.HeaderRoutingMatch{
{
HeaderName: "agent",
HeaderValue: &v1alpha1.StringMatch{
Prefix: "chrome",
},
},
},
},
}}
allErrs := ValidateRolloutStrategyCanary(invalidRo, field.NewPath(""))
assert.Equal(t, InvalidSetHeaderRouteALBValuePolicy, allErrs[0].Detail)
})
}

func TestValidateRolloutStrategyCanarySetMirrorRouteIstio(t *testing.T) {
ro := &v1alpha1.Rollout{}
ro.Spec.Strategy.Canary = &v1alpha1.CanaryStrategy{
Expand Down Expand Up @@ -441,7 +513,7 @@ func TestValidateRolloutStrategyCanarySetMirrorRouteIstio(t *testing.T) {
},
}}
allErrs := ValidateRolloutStrategyCanary(invalidRo, field.NewPath(""))
assert.Equal(t, InvalidStepMissingManagedRoutesField, allErrs[0].Detail)
assert.Equal(t, fmt.Sprintf(MissingFieldMessage, "spec.strategy.canary.trafficRouting.managedRoutes"), allErrs[0].Detail)
})

t.Run("using SetMirrorRoute step without managedRoutes defined but missing route", func(t *testing.T) {
Expand Down
Loading

0 comments on commit 3a63652

Please sign in to comment.