From 0f904ebc7e700dd043b846f73ebca5c5b9dc43ed Mon Sep 17 00:00:00 2001 From: Andrii Perenesenko Date: Tue, 13 Sep 2022 12:33:44 -0700 Subject: [PATCH] feat: Allow Traffic shaping through header based routing for ALB (#2214) * Allow Traffic shaping through header based routing for ALB Signed-off-by: Andrii Perenesenko * Allow Traffic shaping through header based routing for ALB. Fix test Signed-off-by: Andrii Perenesenko * Allow Traffic shaping through header based routing for ALB. Increase coverage Signed-off-by: Andrii Perenesenko * fix after review: deprecate field Signed-off-by: Andrii Perenesenko * fix after review: sort http paths Signed-off-by: Andrii Perenesenko Signed-off-by: Andrii Perenesenko --- ingress/alb.go | 41 +-- ingress/alb_test.go | 19 +- pkg/apis/rollouts/validation/validation.go | 40 ++- .../rollouts/validation/validation_test.go | 90 +++++- rollout/trafficrouting/alb/alb.go | 213 ++++++++++++- rollout/trafficrouting/alb/alb_test.go | 107 ++++++- test/e2e/aws_test.go | 72 +++++ test/e2e/header-routing/alb-header-route.yaml | 107 +++++++ utils/defaults/defaults.go | 8 + utils/defaults/defaults_test.go | 5 + utils/ingress/ingress.go | 67 ++++- utils/ingress/ingress_test.go | 52 ++++ utils/ingress/wrapper.go | 138 +++++++++ utils/ingress/wrapper_test.go | 282 +++++++++++++++++- 14 files changed, 1171 insertions(+), 70 deletions(-) create mode 100644 test/e2e/header-routing/alb-header-route.yaml diff --git a/ingress/alb.go b/ingress/alb.go index 3fdff50932..3803f53810 100644 --- a/ingress/alb.go +++ b/ingress/alb.go @@ -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 } @@ -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 } diff --git a/ingress/alb_test.go b/ingress/alb_test.go index d3b9a8bfe2..79a1c915a1 100644 --- a/ingress/alb_test.go +++ b/ingress/alb_test.go @@ -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) @@ -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{ @@ -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) @@ -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") @@ -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")]) } @@ -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")]) } diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index de07fa9581..37bf7eaa63 100644 --- a/pkg/apis/rollouts/validation/validation.go +++ b/pkg/apis/rollouts/validation/validation.go @@ -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 @@ -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" @@ -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)...) + } } } } @@ -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)) } } } @@ -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{} diff --git a/pkg/apis/rollouts/validation/validation_test.go b/pkg/apis/rollouts/validation/validation_test.go index 8888ac29ac..9722d0d093 100644 --- a/pkg/apis/rollouts/validation/validation_test.go +++ b/pkg/apis/rollouts/validation/validation_test.go @@ -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{ @@ -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() @@ -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{ @@ -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) { diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index 62d1a465d4..103e3f5e9a 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -115,7 +115,50 @@ func (r *Reconciler) SetWeight(desiredWeight int32, additionalDestinations ...v1 return nil } -func (r *Reconciler) SetHeaderRoute(headerRouting *v1alpha1.SetHeaderRoute) error { +func (r *Reconciler) SetHeaderRoute(headerRoute *v1alpha1.SetHeaderRoute) error { + if headerRoute == nil { + return nil + } + ctx := context.TODO() + rollout := r.cfg.Rollout + ingressName := rollout.Spec.Strategy.Canary.TrafficRouting.ALB.Ingress + action := headerRoute.Name + port := rollout.Spec.Strategy.Canary.TrafficRouting.ALB.ServicePort + + ingress, err := r.cfg.IngressWrapper.GetCached(rollout.Namespace, ingressName) + if err != nil { + return err + } + + desiredAnnotations, err := getDesiredHeaderAnnotations(ingress, rollout, port, headerRoute) + if err != nil { + return err + } + desiredIngress := ingressutil.NewIngressWithSpecAndAnnotations(ingress, desiredAnnotations) + hasRule := ingressutil.HasRuleWithService(ingress, action) + if hasRule && headerRoute.Match == nil { + desiredIngress.RemovePathByServiceName(action) + } + if !hasRule && headerRoute.Match != nil { + desiredIngress.CreateAnnotationBasedPath(action) + } + desiredIngress.SortHttpPaths(rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes) + patch, modified, err := ingressutil.BuildIngressPatch(ingress.Mode(), ingress, desiredIngress, ingressutil.WithAnnotations(), ingressutil.WithSpec()) + if err != nil { + return nil + } + if !modified { + r.log.Info("no changes to the ALB Ingress for header routing") + return nil + } + r.log.WithField("patch", string(patch)).Debug("applying ALB Ingress patch") + r.cfg.Recorder.Eventf(rollout, record.EventOptions{EventReason: "PatchingALBIngress"}, "Updating Ingress `%s` to headerRoute '%d'", ingressName, headerRoute) + + _, err = r.cfg.IngressWrapper.Patch(ctx, ingress.GetNamespace(), ingress.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}) + if err != nil { + r.log.WithField("err", err.Error()).Error("error patching alb ingress") + return fmt.Errorf("error patching alb ingress `%s`: %v", ingressName, err) + } return nil } @@ -294,13 +337,125 @@ func getDesiredAnnotations(current *ingressutil.Ingress, r *v1alpha1.Rollout, po return nil, err } desired[key] = value - m, err := ingressutil.NewManagedALBActions(desired[ingressutil.ManagedActionsAnnotation]) + return modifyManagedAnnotation(desired, r.Name, true, key) +} + +func getDesiredHeaderAnnotations(current *ingressutil.Ingress, r *v1alpha1.Rollout, port int32, headerRoute *v1alpha1.SetHeaderRoute) (map[string]string, error) { + desired := current.DeepCopy().GetAnnotations() + actionKey := ingressutil.ALBHeaderBasedActionAnnotationKey(r, headerRoute.Name) + conditionKey := ingressutil.ALBHeaderBasedConditionAnnotationKey(r, headerRoute.Name) + add := headerRoute.Match != nil + if add { + actionValue, err := getTrafficForwardActionString(r, port) + if err != nil { + return nil, err + } + conditionValue, err := getTrafficForwardConditionString(headerRoute) + if err != nil { + return nil, err + } + desired[actionKey] = actionValue + desired[conditionKey] = conditionValue + } else { + delete(desired, actionKey) + delete(desired, conditionKey) + } + + return modifyManagedAnnotation(desired, r.Name, add, actionKey, conditionKey) +} + +func modifyManagedAnnotation(annotations map[string]string, rolloutName string, add bool, annotationKeys ...string) (map[string]string, error) { + m, err := ingressutil.NewManagedALBAnnotations(annotations[ingressutil.ManagedAnnotations]) if err != nil { return nil, err } - m[r.Name] = key - desired[ingressutil.ManagedActionsAnnotation] = m.String() - return desired, nil + managedAnnotation := m[rolloutName] + if managedAnnotation == nil { + managedAnnotation = ingressutil.ManagedALBAnnotation{} + } + for _, annotationKey := range annotationKeys { + if add { + if !hasValue(managedAnnotation, annotationKey) { + managedAnnotation = append(managedAnnotation, annotationKey) + } + } else { + managedAnnotation = removeValue(managedAnnotation, annotationKey) + } + } + m[rolloutName] = managedAnnotation + annotations[ingressutil.ManagedAnnotations] = m.String() + return annotations, nil +} + +func hasValue(array []string, key string) bool { + for _, item := range array { + if item == key { + return true + } + } + return false +} + +func removeValue(array []string, key string) []string { + for i, v := range array { + if v == key { + array = append(array[:i], array[i+1:]...) + } + } + return array +} + +func getTrafficForwardActionString(r *v1alpha1.Rollout, port int32) (string, error) { + _, canaryService := trafficrouting.GetStableAndCanaryServices(r) + portStr := strconv.Itoa(int(port)) + weight := int64(100) + targetGroups := make([]ingressutil.ALBTargetGroup, 0) + // create target group for canary + targetGroups = append(targetGroups, ingressutil.ALBTargetGroup{ + ServiceName: canaryService, + ServicePort: portStr, + Weight: pointer.Int64Ptr(weight), + }) + + action := ingressutil.ALBAction{ + Type: "forward", + ForwardConfig: ingressutil.ALBForwardConfig{ + TargetGroups: targetGroups, + }, + } + + var stickinessConfig = r.Spec.Strategy.Canary.TrafficRouting.ALB.StickinessConfig + if stickinessConfig != nil && stickinessConfig.Enabled { + // AWS API valid range + // https://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_TargetGroupStickinessConfig.html + if stickinessConfig.DurationSeconds < 1 || stickinessConfig.DurationSeconds > 604800 { + return "", fmt.Errorf("TargetGroupStickinessConfig's duration must be between 1 and 604800 seconds (7 days)!") + } + newStickyConfig := ingressutil.ALBTargetGroupStickinessConfig{ + Enabled: true, + DurationSeconds: stickinessConfig.DurationSeconds, + } + action.ForwardConfig.TargetGroupStickinessConfig = &newStickyConfig + } + + bytes := jsonutil.MustMarshal(action) + return string(bytes), nil +} + +func getTrafficForwardConditionString(headerRoute *v1alpha1.SetHeaderRoute) (string, error) { + var res []ingressutil.ALBCondition + for _, match := range headerRoute.Match { + condition := ingressutil.ALBCondition{ + Field: "http-header", + HttpHeaderConfig: ingressutil.HttpHeaderConfig{ + HttpHeaderName: match.HeaderName, + Values: []string{match.HeaderValue.Exact}, + }, + } + res = append(res, condition) + } + bytes := jsonutil.MustMarshal(res) + return string(bytes), nil } // UpdateHash informs a traffic routing reconciler about new canary/stable pod hashes @@ -313,5 +468,53 @@ func (r *Reconciler) SetMirrorRoute(setMirrorRoute *v1alpha1.SetMirrorRoute) err } func (r *Reconciler) RemoveManagedRoutes() error { + if len(r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes) == 0 { + return nil + } + ctx := context.TODO() + rollout := r.cfg.Rollout + ingressName := rollout.Spec.Strategy.Canary.TrafficRouting.ALB.Ingress + + ingress, err := r.cfg.IngressWrapper.GetCached(rollout.Namespace, ingressName) + if err != nil { + return err + } + + desiredAnnotations := ingress.DeepCopy().GetAnnotations() + var actionKeys []string + for _, managedRoute := range rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes { + actionKey := ingressutil.ALBHeaderBasedActionAnnotationKey(rollout, managedRoute.Name) + conditionKey := ingressutil.ALBHeaderBasedConditionAnnotationKey(rollout, managedRoute.Name) + delete(desiredAnnotations, actionKey) + delete(desiredAnnotations, conditionKey) + actionKeys = append(actionKeys, actionKey, conditionKey) + } + desiredAnnotations, err = modifyManagedAnnotation(desiredAnnotations, rollout.Name, false, actionKeys...) + if err != nil { + return err + } + + desiredIngress := ingressutil.NewIngressWithSpecAndAnnotations(ingress, desiredAnnotations) + + for _, managedRoute := range rollout.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes { + desiredIngress.RemovePathByServiceName(managedRoute.Name) + } + + patch, modified, err := ingressutil.BuildIngressPatch(ingress.Mode(), ingress, desiredIngress, ingressutil.WithAnnotations(), ingressutil.WithSpec()) + if err != nil { + return nil + } + if !modified { + r.log.Info("no changes to the ALB Ingress for header routing") + return nil + } + r.log.WithField("patch", string(patch)).Debug("applying ALB Ingress patch") + r.cfg.Recorder.Eventf(rollout, record.EventOptions{EventReason: "PatchingALBIngress"}, "Updating Ingress `%s` removing managed routes", ingressName) + + _, err = r.cfg.IngressWrapper.Patch(ctx, ingress.GetNamespace(), ingress.GetName(), types.MergePatchType, patch, metav1.PatchOptions{}) + if err != nil { + r.log.WithField("err", err.Error()).Error("error patching alb ingress") + return fmt.Errorf("error patching alb ingress `%s`: %v", ingressName, err) + } return nil } diff --git a/rollout/trafficrouting/alb/alb_test.go b/rollout/trafficrouting/alb/alb_test.go index 7e26f6f16e..3a044c537f 100644 --- a/rollout/trafficrouting/alb/alb_test.go +++ b/rollout/trafficrouting/alb/alb_test.go @@ -101,7 +101,9 @@ func albActionAnnotation(stable string) string { } func ingress(name, stableSvc, canarySvc, actionService string, port, weight int32, managedBy string, includeStickyConfig bool) *extensionsv1beta1.Ingress { - managedByValue := fmt.Sprintf("%s:%s", managedBy, albActionAnnotation(actionService)) + managedByValue := ingressutil.ManagedALBAnnotations{ + managedBy: ingressutil.ManagedALBAnnotation{albActionAnnotation(actionService)}, + } action := fmt.Sprintf(actionTemplate, canarySvc, port, weight, stableSvc, port, 100-weight) if includeStickyConfig { action = fmt.Sprintf(actionTemplateWithStickyConfig, canarySvc, port, weight, stableSvc, port, 100-weight) @@ -117,8 +119,8 @@ func ingress(name, stableSvc, canarySvc, actionService string, port, weight int3 Name: name, Namespace: metav1.NamespaceDefault, Annotations: map[string]string{ - albActionAnnotation(actionService): string(jsonutil.MustMarshal(a)), - ingressutil.ManagedActionsAnnotation: managedByValue, + albActionAnnotation(actionService): string(jsonutil.MustMarshal(a)), + ingressutil.ManagedAnnotations: managedByValue.String(), }, }, Spec: extensionsv1beta1.IngressSpec{ @@ -156,6 +158,13 @@ func TestType(t *testing.T) { assert.NoError(t, err) } +func TestAddManagedAnnotation(t *testing.T) { + annotations, _ := modifyManagedAnnotation(map[string]string{}, "argo-rollouts", true, "alb.ingress.kubernetes.io/actions.action1", "alb.ingress.kubernetes.io/conditions.action1") + assert.Equal(t, annotations[ingressutil.ManagedAnnotations], "{\"argo-rollouts\":[\"alb.ingress.kubernetes.io/actions.action1\",\"alb.ingress.kubernetes.io/conditions.action1\"]}") + _, err := modifyManagedAnnotation(map[string]string{ingressutil.ManagedAnnotations: "invalid, non-json value"}, "some-rollout", false) + assert.Error(t, err) +} + func TestIngressNotFound(t *testing.T) { ro := fakeRollout("stable-service", "canary-service", nil, "stable-ingress", 443) client := fake.NewSimpleClientset() @@ -225,7 +234,7 @@ func TestNoChanges(t *testing.T) { func TestErrorOnInvalidManagedBy(t *testing.T) { ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443) i := ingress("ingress", STABLE_SVC, CANARY_SVC, STABLE_SVC, 443, 5, ro.Name, false) - i.Annotations[ingressutil.ManagedActionsAnnotation] = "test" + i.Annotations[ingressutil.ManagedAnnotations] = "test" client := fake.NewSimpleClientset(i) k8sI := kubeinformers.NewSharedInformerFactory(client, 0) k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i) @@ -863,8 +872,11 @@ func TestVerifyWeightWithAdditionalDestinations(t *testing.T) { func TestSetHeaderRoute(t *testing.T) { ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443) - i := ingress("ingress", STABLE_SVC, CANARY_SVC, STABLE_SVC, 443, 10, ro.Name, false) - client := fake.NewSimpleClientset() + ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{ + {Name: "header-route"}, + } + i := ingress("ingress", STABLE_SVC, CANARY_SVC, "action1", 443, 10, ro.Name, false) + client := fake.NewSimpleClientset(i) k8sI := kubeinformers.NewSharedInformerFactory(client, 0) k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i) ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) @@ -880,20 +892,95 @@ func TestSetHeaderRoute(t *testing.T) { }) assert.NoError(t, err) err = r.SetHeaderRoute(&v1alpha1.SetHeaderRoute{ - Name: "set-header", + Name: "header-route", Match: []v1alpha1.HeaderRoutingMatch{{ - HeaderName: "header-name", + HeaderName: "Agent", HeaderValue: &v1alpha1.StringMatch{ - Exact: "value", + Prefix: "Chrome", }, }}, }) assert.Nil(t, err) + assert.Len(t, client.Actions(), 1) + // no managed routes, no changes expected err = r.RemoveManagedRoutes() assert.Nil(t, err) + assert.Len(t, client.Actions(), 1) +} - assert.Len(t, client.Actions(), 0) +func TestRemoveManagedRoutes(t *testing.T) { + ro := fakeRollout(STABLE_SVC, CANARY_SVC, nil, "ingress", 443) + ro.Spec.Strategy.Canary.TrafficRouting.ManagedRoutes = []v1alpha1.MangedRoutes{ + {Name: "header-route"}, + } + i := ingress("ingress", STABLE_SVC, CANARY_SVC, "action1", 443, 10, ro.Name, false) + managedByValue := ingressutil.ManagedALBAnnotations{ + ro.Name: ingressutil.ManagedALBAnnotation{ + "alb.ingress.kubernetes.io/actions.action1", + "alb.ingress.kubernetes.io/actions.header-route", + "alb.ingress.kubernetes.io/conditions.header-route", + }, + } + i.Annotations["alb.ingress.kubernetes.io/actions.header-route"] = "{}" + i.Annotations["alb.ingress.kubernetes.io/conditions.header-route"] = "{}" + i.Annotations[ingressutil.ManagedAnnotations] = managedByValue.String() + i.Spec.Rules = []extensionsv1beta1.IngressRule{ + { + IngressRuleValue: extensionsv1beta1.IngressRuleValue{ + HTTP: &extensionsv1beta1.HTTPIngressRuleValue{ + Paths: []extensionsv1beta1.HTTPIngressPath{ + { + Backend: extensionsv1beta1.IngressBackend{ + ServiceName: "action1", + ServicePort: intstr.Parse("use-annotation"), + }, + }, + }, + }, + }, + }, + { + IngressRuleValue: extensionsv1beta1.IngressRuleValue{ + HTTP: &extensionsv1beta1.HTTPIngressRuleValue{ + Paths: []extensionsv1beta1.HTTPIngressPath{ + { + Backend: extensionsv1beta1.IngressBackend{ + ServiceName: "header-route", + ServicePort: intstr.Parse("use-annotation"), + }, + }, + }, + }, + }, + }, + } + + client := fake.NewSimpleClientset(i) + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + r, err := NewReconciler(ReconcilerConfig{ + Rollout: ro, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + assert.NoError(t, err) + + err = r.SetHeaderRoute(&v1alpha1.SetHeaderRoute{ + Name: "header-route", + }) + assert.Nil(t, err) + assert.Len(t, client.Actions(), 1) + + err = r.RemoveManagedRoutes() + assert.Nil(t, err) + assert.Len(t, client.Actions(), 2) } func TestSetMirrorRoute(t *testing.T) { diff --git a/test/e2e/aws_test.go b/test/e2e/aws_test.go index 5525d4650d..a2a4434781 100644 --- a/test/e2e/aws_test.go +++ b/test/e2e/aws_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/suite" "github.com/tj/assert" + "k8s.io/utils/pointer" "github.com/argoproj/argo-rollouts/test/fixtures" ingress2 "github.com/argoproj/argo-rollouts/utils/ingress" @@ -171,3 +172,74 @@ func (s *AWSSuite) TestALBExperimentStepNoSetWeight() { Then(). Assert(assertWeights(s, "alb-rollout-canary", "alb-rollout-stable", 0, 100)) } + +func (s *AWSSuite) TestAlbHeaderRoute() { + s.Given(). + RolloutObjects("@header-routing/alb-header-route.yaml"). + When(). + ApplyManifests(). + WaitForRolloutStatus("Healthy"). + Then(). + Assert(func(t *fixtures.Then) { + assertAlbActionDoesNotExist(t, s, "header-route") + assertAlbActionServiceWeight(t, s, "action1", "canary-service", 0) + assertAlbActionServiceWeight(t, s, "action1", "stable-service", 100) + }). + When(). + UpdateSpec(). + WaitForRolloutStatus("Paused"). + Sleep(1 * time.Second). + Then(). + Assert(func(t *fixtures.Then) { + assertAlbActionDoesNotExist(t, s, "header-route") + assertAlbActionServiceWeight(t, s, "action1", "canary-service", 20) + assertAlbActionServiceWeight(t, s, "action1", "stable-service", 80) + }). + When(). + PromoteRollout(). + WaitForRolloutStatus("Paused"). + Sleep(1 * time.Second). + Then(). + Assert(func(t *fixtures.Then) { + assertAlbActionServiceWeight(t, s, "header-route", "canary-service", 100) + assertAlbActionServiceWeight(t, s, "action1", "canary-service", 20) + assertAlbActionServiceWeight(t, s, "action1", "stable-service", 80) + }). + When(). + PromoteRollout(). + WaitForRolloutStatus("Paused"). + Sleep(1 * time.Second). + Then(). + Assert(func(t *fixtures.Then) { + assertAlbActionDoesNotExist(t, s, "header-route") + }) +} + +func assertAlbActionServiceWeight(t *fixtures.Then, s *AWSSuite, actionName, serviceName string, expectedWeight int64) { + ingress := t.GetALBIngress() + key := "alb.ingress.kubernetes.io/actions." + actionName + actionStr, ok := ingress.Annotations[key] + assert.True(s.T(), ok, "Annotation for action was not found: %s", key) + + var albAction ingress2.ALBAction + err := json.Unmarshal([]byte(actionStr), &albAction) + if err != nil { + panic(err) + } + + found := false + for _, group := range albAction.ForwardConfig.TargetGroups { + if group.ServiceName == serviceName { + assert.Equal(s.T(), pointer.Int64(expectedWeight), group.Weight) + found = true + } + } + assert.True(s.T(), found, "Service %s was not found", serviceName) +} + +func assertAlbActionDoesNotExist(t *fixtures.Then, s *AWSSuite, actionName string) { + ingress := t.GetALBIngress() + key := "alb.ingress.kubernetes.io/actions." + actionName + _, ok := ingress.Annotations[key] + assert.False(s.T(), ok, "Annotation for action should not exist: %s", key) +} diff --git a/test/e2e/header-routing/alb-header-route.yaml b/test/e2e/header-routing/alb-header-route.yaml new file mode 100644 index 0000000000..71c9e7aa6f --- /dev/null +++ b/test/e2e/header-routing/alb-header-route.yaml @@ -0,0 +1,107 @@ +apiVersion: v1 +kind: Service +metadata: + name: canary-service +spec: + type: NodePort + ports: + - port: 8080 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: alb-rollout +--- +apiVersion: v1 +kind: Service +metadata: + name: stable-service +spec: + type: NodePort + ports: + - port: 8080 + targetPort: 8080 + protocol: TCP + name: http + selector: + app: alb-rollout +--- +apiVersion: networking.k8s.io/v1 +kind: Ingress +metadata: + name: alb-rollout-ingress + annotations: + alb.ingress.kubernetes.io/security-groups: 'iks-intuit-cidr-ingress-tcp-443' + alb.ingress.kubernetes.io/certificate-arn: arn:aws:acm:us-east-2:795188202216:certificate/27d920c5-a8a6-4210-9f31-bd4a2d439039 + alb.ingress.kubernetes.io/load-balancer-attributes: access_logs.s3.enabled=false + alb.ingress.kubernetes.io/ssl-policy: ELBSecurityPolicy-TLS-1-2-2017-01 + kubernetes.io/ingress.class: aws-alb + alb.ingress.kubernetes.io/load-balancer-name: rollouts-sample + alb.ingress.kubernetes.io/target-type: ip + alb.ingress.kubernetes.io/healthcheck-protocol: HTTP + alb.ingress.kubernetes.io/healthcheck-port: traffic-port + alb.ingress.kubernetes.io/healthcheck-path: /color + alb.ingress.kubernetes.io/backend-protocol: HTTP + alb.ingress.kubernetes.io/listen-ports: '[{"HTTPS": 443}]' + alb.ingress.kubernetes.io/ssl-redirect: '443' + alb.ingress.kubernetes.io/scheme: internet-facing + alb.ingress.kubernetes.io/subnets: IngressSubnetAz1, IngressSubnetAz2, IngressSubnetAz3 +spec: + rules: + - http: + paths: + - path: /* + pathType: ImplementationSpecific + backend: + service: + name: action1 + port: + name: use-annotation + +--- +apiVersion: argoproj.io/v1alpha1 +kind: Rollout +metadata: + name: rollouts-demo +spec: + replicas: 5 + selector: + matchLabels: + app: alb-rollout + template: + metadata: + labels: + app: alb-rollout + spec: + containers: + - name: alb-rollout + image: "argoproj/rollouts-demo:yellow" + ports: + - name: http + containerPort: 8080 + protocol: TCP + strategy: + canary: + scaleDownDelaySeconds: 5 + stableService: stable-service + canaryService: canary-service + trafficRouting: + managedRoutes: + - name: header-route + alb: + ingress: alb-rollout-ingress + rootService: action1 + servicePort: 8080 + steps: + - setWeight: 20 + - pause: {} + - setHeaderRoute: + name: header-route + match: + - headerName: Custom-Header + headerValue: + exact: Mozilla* + - pause: {} + - setHeaderRoute: + name: header-route + - pause: {} diff --git a/utils/defaults/defaults.go b/utils/defaults/defaults.go index c38a044c25..9b18b16db1 100644 --- a/utils/defaults/defaults.go +++ b/utils/defaults/defaults.go @@ -85,6 +85,14 @@ func init() { } } +func GetStringOrDefault(value, defaultValue string) string { + if value == "" { + return defaultValue + } else { + return value + } +} + // GetReplicasOrDefault returns the deferenced number of replicas or the default number func GetReplicasOrDefault(replicas *int32) int32 { if replicas == nil { diff --git a/utils/defaults/defaults_test.go b/utils/defaults/defaults_test.go index d099cc0343..2162f1be82 100644 --- a/utils/defaults/defaults_test.go +++ b/utils/defaults/defaults_test.go @@ -12,6 +12,11 @@ import ( "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" ) +func TestGetStringOrDefault(t *testing.T) { + assert.Equal(t, "some value", GetStringOrDefault("some value", "default value")) + assert.Equal(t, "default value", GetStringOrDefault("", "default value")) +} + func TestGetReplicasOrDefault(t *testing.T) { replicas := int32(2) assert.Equal(t, replicas, GetReplicasOrDefault(&replicas)) diff --git a/utils/ingress/ingress.go b/utils/ingress/ingress.go index 42dc61ca20..c4036dff8f 100644 --- a/utils/ingress/ingress.go +++ b/utils/ingress/ingress.go @@ -1,6 +1,7 @@ package ingress import ( + json2 "encoding/json" "errors" "fmt" "strconv" @@ -11,18 +12,25 @@ import ( "k8s.io/client-go/discovery" "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/argoproj/argo-rollouts/utils/defaults" "github.com/argoproj/argo-rollouts/utils/diff" + "github.com/argoproj/argo-rollouts/utils/json" ) const ( // CanaryIngressSuffix is the name suffix all canary ingresses created by the rollouts controller will have CanaryIngressSuffix = "-canary" // ManagedActionsAnnotation holds list of ALB actions that are managed by rollouts + // DEPRECATED in favor of ManagedAnnotations ManagedActionsAnnotation = "rollouts.argoproj.io/managed-alb-actions" + // ManagedAnnotations holds list of ALB annotations that are managed by rollouts supports multiple annotations + ManagedAnnotations = "rollouts.argoproj.io/managed-alb-annotations" //ALBIngressAnnotation is the prefix annotation that is used by the ALB Ingress controller to configure an ALB ALBIngressAnnotation = "alb.ingress.kubernetes.io" // ALBActionPrefix the prefix to specific actions within an ALB ingress. ALBActionPrefix = "/actions." + // ALBConditionPrefix the prefix to specific conditions within an ALB ingress. + ALBConditionPrefix = "/conditions." ) // ALBAction describes an ALB action that configure the behavior of an ALB. This struct is marshaled into a string @@ -32,6 +40,19 @@ type ALBAction struct { ForwardConfig ALBForwardConfig `json:"ForwardConfig"` } +// ALBCondition describes an ALB action condition that configure the behavior of an ALB. This struct is marshaled into a string +// that is added to the Ingress's annotations. +type ALBCondition struct { + Field string `json:"field"` + HttpHeaderConfig HttpHeaderConfig `json:"httpHeaderConfig"` +} + +// HttpHeaderConfig describes header config for the ALB action condition +type HttpHeaderConfig struct { + HttpHeaderName string `json:"httpHeaderName"` + Values []string `json:"values"` +} + // ALBForwardConfig describes a list of target groups that the ALB should route traffic towards type ALBForwardConfig struct { TargetGroups []ALBTargetGroup `json:"TargetGroups"` @@ -142,6 +163,26 @@ func hasLegacyIngressRuleWithService(ingress *extensionsv1beta1.Ingress, svc str // ManagedALBActions a mapping of Rollout names to the ALB action that the Rollout manages type ManagedALBActions map[string]string +type ManagedALBAnnotations map[string]ManagedALBAnnotation + +type ManagedALBAnnotation []string + +// String outputs a string of all the managed ALB annotations that is stored in the Ingress's annotations +func (m ManagedALBAnnotations) String() string { + return string(json.MustMarshal(m)) +} + +func NewManagedALBAnnotations(json string) (ManagedALBAnnotations, error) { + res := ManagedALBAnnotations{} + if json == "" { + return res, nil + } + if err := json2.Unmarshal([]byte(json), &res); err != nil { + return nil, err + } + return res, nil +} + // String outputs a string of all the managed ALB actions that is stored in the Ingress's annotations func (m ManagedALBActions) String() string { str := "" @@ -173,15 +214,23 @@ func NewManagedALBActions(annotation string) (ManagedALBActions, error) { // ALBActionAnnotationKey returns the annotation key for a specific action func ALBActionAnnotationKey(r *v1alpha1.Rollout) string { - prefix := ALBIngressAnnotation - if r.Spec.Strategy.Canary.TrafficRouting.ALB.AnnotationPrefix != "" { - prefix = r.Spec.Strategy.Canary.TrafficRouting.ALB.AnnotationPrefix - } - actionService := r.Spec.Strategy.Canary.StableService - if r.Spec.Strategy.Canary.TrafficRouting.ALB.RootService != "" { - actionService = r.Spec.Strategy.Canary.TrafficRouting.ALB.RootService - } - return fmt.Sprintf("%s%s%s", prefix, ALBActionPrefix, actionService) + actionService := defaults.GetStringOrDefault(r.Spec.Strategy.Canary.TrafficRouting.ALB.RootService, r.Spec.Strategy.Canary.StableService) + return albIngressKubernetesIoKey(r, ALBActionPrefix, actionService) +} + +// ALBHeaderBasedActionAnnotationKey returns the annotation key for a specific action +func ALBHeaderBasedActionAnnotationKey(r *v1alpha1.Rollout, action string) string { + return albIngressKubernetesIoKey(r, ALBActionPrefix, action) +} + +// ALBHeaderBasedConditionAnnotationKey returns the annotation key for a specific condition +func ALBHeaderBasedConditionAnnotationKey(r *v1alpha1.Rollout, action string) string { + return albIngressKubernetesIoKey(r, ALBConditionPrefix, action) +} + +func albIngressKubernetesIoKey(r *v1alpha1.Rollout, action, service string) string { + prefix := defaults.GetStringOrDefault(r.Spec.Strategy.Canary.TrafficRouting.ALB.AnnotationPrefix, ALBIngressAnnotation) + return fmt.Sprintf("%s%s%s", prefix, action, service) } type patchConfig struct { diff --git a/utils/ingress/ingress_test.go b/utils/ingress/ingress_test.go index eaca7e6a86..08f2a2e42f 100644 --- a/utils/ingress/ingress_test.go +++ b/utils/ingress/ingress_test.go @@ -514,3 +514,55 @@ func getExtensionsIngress() *extensionsv1beta1.Ingress { }, } } + +func TestManagedALBAnnotations(t *testing.T) { + emptyJson, _ := NewManagedALBAnnotations("") + assert.NotNil(t, emptyJson) + assert.Equal(t, 0, len(emptyJson)) + assert.Equal(t, "{}", emptyJson.String()) + + _, err := NewManagedALBAnnotations("invalid json") + assert.Error(t, err) + + json := "{\"rollouts-demo\":[\"alb.ingress.kubernetes.io/actions.action1\", \"alb.ingress.kubernetes.io/actions.header-action\", \"alb.ingress.kubernetes.io/conditions.header-action\"]}" + actual, err := NewManagedALBAnnotations(json) + assert.NoError(t, err) + + rolloutsDemoAnnotation := actual["rollouts-demo"] + assert.NotNil(t, rolloutsDemoAnnotation) + assert.Equal(t, 3, len(rolloutsDemoAnnotation)) +} + +func TestALBHeaderBasedActionAnnotationKey(t *testing.T) { + r := &v1alpha1.Rollout{ + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + ALB: &v1alpha1.ALBTrafficRouting{ + AnnotationPrefix: "alb.ingress.kubernetes.io", + }, + }, + }, + }, + }, + } + assert.Equal(t, "alb.ingress.kubernetes.io/actions.route", ALBHeaderBasedActionAnnotationKey(r, "route")) +} + +func TestALBHeaderBasedConditionAnnotationKey(t *testing.T) { + r := &v1alpha1.Rollout{ + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + ALB: &v1alpha1.ALBTrafficRouting{ + AnnotationPrefix: "alb.ingress.kubernetes.io", + }, + }, + }, + }, + }, + } + assert.Equal(t, "alb.ingress.kubernetes.io/conditions.route", ALBHeaderBasedConditionAnnotationKey(r, "route")) +} diff --git a/utils/ingress/wrapper.go b/utils/ingress/wrapper.go index a460baac70..d70b6a96ed 100644 --- a/utils/ingress/wrapper.go +++ b/utils/ingress/wrapper.go @@ -3,6 +3,7 @@ package ingress import ( "context" "errors" + "sort" "sync" corev1 "k8s.io/api/core/v1" @@ -10,11 +11,14 @@ import ( v1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" types "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/informers" extensionsv1beta1 "k8s.io/client-go/informers/extensions/v1beta1" networkingv1 "k8s.io/client-go/informers/networking/v1" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" + + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" ) // Ingress defines an Ingress resource abstraction used to allow Rollouts to @@ -68,6 +72,29 @@ func NewIngressWithAnnotations(mode IngressMode, annotations map[string]string) } } +func NewIngressWithSpecAndAnnotations(ingress *Ingress, annotations map[string]string) *Ingress { + switch ingress.mode { + case IngressModeNetworking: + i := &v1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: annotations, + }, + Spec: *ingress.ingress.Spec.DeepCopy(), + } + return NewIngress(i) + case IngressModeExtensions: + i := &v1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: annotations, + }, + Spec: *ingress.legacyIngress.Spec.DeepCopy(), + } + return NewLegacyIngress(i) + default: + return nil + } +} + func (i *Ingress) GetExtensionsIngress() (*v1beta1.Ingress, error) { if i.legacyIngress == nil { return nil, errors.New("extensions Ingress is nil in this wrapper") @@ -149,6 +176,117 @@ func (i *Ingress) SetAnnotations(annotations map[string]string) { } } +func (i *Ingress) CreateAnnotationBasedPath(actionName string) { + i.mux.Lock() + defer i.mux.Unlock() + if HasRuleWithService(i, actionName) { + return + } + switch i.mode { + case IngressModeNetworking: + t := v1.PathTypeImplementationSpecific + p := v1.HTTPIngressPath{ + Path: "/*", + PathType: &t, + Backend: v1.IngressBackend{ + Service: &v1.IngressServiceBackend{ + Name: actionName, + Port: v1.ServiceBackendPort{ + Name: "use-annotation", + }, + }, + }, + } + for _, rule := range i.ingress.Spec.Rules { + rule.HTTP.Paths = append(rule.HTTP.Paths[:1], rule.HTTP.Paths[0:]...) + rule.HTTP.Paths[0] = p + } + case IngressModeExtensions: + t := v1beta1.PathTypeImplementationSpecific + p := v1beta1.HTTPIngressPath{ + Path: "/*", + PathType: &t, + Backend: v1beta1.IngressBackend{ + ServiceName: actionName, + ServicePort: intstr.FromString("use-annotation"), + }, + } + for _, rule := range i.legacyIngress.Spec.Rules { + rule.HTTP.Paths = append(rule.HTTP.Paths[:1], rule.HTTP.Paths[0:]...) + rule.HTTP.Paths[0] = p + } + } +} + +func (i *Ingress) RemovePathByServiceName(actionName string) { + i.mux.Lock() + defer i.mux.Unlock() + switch i.mode { + case IngressModeNetworking: + for _, rule := range i.ingress.Spec.Rules { + if j := indexPathByService(rule, actionName); j != -1 { + rule.HTTP.Paths = append(rule.HTTP.Paths[:j], rule.HTTP.Paths[j+1:]...) + } + } + case IngressModeExtensions: + for _, rule := range i.legacyIngress.Spec.Rules { + if j := indexLegacyPathByService(rule, actionName); j != -1 { + rule.HTTP.Paths = append(rule.HTTP.Paths[:j], rule.HTTP.Paths[j+1:]...) + } + } + } +} + +func (i *Ingress) SortHttpPaths(routes []v1alpha1.MangedRoutes) { + var routeWeight = make(map[string]int) // map of route name for ordering + for j, route := range routes { + routeWeight[route.Name] = j + } + + i.mux.Lock() + defer i.mux.Unlock() + switch i.mode { + case IngressModeNetworking: + for _, rule := range i.ingress.Spec.Rules { + sort.SliceStable(rule.HTTP.Paths, func(i, j int) bool { + return getKeyWeight(routeWeight, rule.HTTP.Paths[i].Backend.Service.Name) < getKeyWeight(routeWeight, rule.HTTP.Paths[j].Backend.Service.Name) + }) + } + case IngressModeExtensions: + for _, rule := range i.legacyIngress.Spec.Rules { + sort.SliceStable(rule.HTTP.Paths, func(i, j int) bool { + return getKeyWeight(routeWeight, rule.HTTP.Paths[i].Backend.ServiceName) < getKeyWeight(routeWeight, rule.HTTP.Paths[j].Backend.ServiceName) + }) + } + } +} + +func getKeyWeight(weight map[string]int, key string) int { + if val, ok := weight[key]; ok { + return val + } else { + return len(weight) + } +} + +func indexPathByService(rule v1.IngressRule, name string) int { + for i, path := range rule.HTTP.Paths { + if path.Backend.Service.Name == name { + return i + } + } + return -1 +} + +func indexLegacyPathByService(rule v1beta1.IngressRule, name string) int { + for i, path := range rule.HTTP.Paths { + if path.Backend.ServiceName == name { + return i + } + } + return -1 +} + func (i *Ingress) DeepCopy() *Ingress { switch i.mode { case IngressModeNetworking: diff --git a/utils/ingress/wrapper_test.go b/utils/ingress/wrapper_test.go index 8611dcd395..9e8f511494 100644 --- a/utils/ingress/wrapper_test.go +++ b/utils/ingress/wrapper_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/argoproj/argo-rollouts/utils/ingress" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" "k8s.io/api/extensions/v1beta1" @@ -14,6 +13,10 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" kubeinformers "k8s.io/client-go/informers" k8sfake "k8s.io/client-go/kubernetes/fake" + "k8s.io/utils/pointer" + + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" + "github.com/argoproj/argo-rollouts/utils/ingress" ) func TestNewIngressWithAnnotations(t *testing.T) { @@ -68,6 +71,57 @@ func TestNewIngressWithAnnotations(t *testing.T) { }) } +func TestNewIngressWithSpecAndAnnotations(t *testing.T) { + annotations := make(map[string]string) + annotations["some.annotation.key1"] = "some.annotation.value1" + annotations["some.annotation.key2"] = "some.annotation.value2" + getAnnotations := func() map[string]string { + annotations := make(map[string]string) + annotations["some.annotation.key1"] = "some.annotation.value1" + annotations["some.annotation.key2"] = "some.annotation.value2" + return annotations + } + t.Run("will instantiate an Ingress wrapped with an annotated networkingv1.Ingress", func(t *testing.T) { + ing := networkingIngress() + + // given + t.Parallel() + + // when + i := ingress.NewIngressWithSpecAndAnnotations(ing, getAnnotations()) + + // then + assert.NotNil(t, i) + a := i.GetAnnotations() + assert.Equal(t, 2, len(a)) + a["extra-annotation-key"] = "extra-annotation-value" + i.SetAnnotations(a) + assert.Equal(t, 3, len(a)) + actualIngress, _ := i.GetNetworkingIngress() + expectedIngress, _ := ing.GetNetworkingIngress() + assert.Equal(t, expectedIngress.Spec, actualIngress.Spec) + }) + t.Run("will instantiate an Ingress wrapped with an annotated extensions/v1beta1.Ingress", func(t *testing.T) { + ing := extensionsIngress() + // given + t.Parallel() + + // when + i := ingress.NewIngressWithSpecAndAnnotations(ing, getAnnotations()) + + // then + assert.NotNil(t, i) + a := i.GetAnnotations() + assert.Equal(t, 2, len(a)) + a["extra-annotation-key"] = "extra-annotation-value" + i.SetAnnotations(a) + assert.Equal(t, 3, len(a)) + actualIngress, _ := i.GetExtensionsIngress() + expectedIngress, _ := ing.GetExtensionsIngress() + assert.Equal(t, expectedIngress.Spec, actualIngress.Spec) + }) +} + func TestGetExtensionsIngress(t *testing.T) { extensionsIngress := &v1beta1.Ingress{} t.Run("will get extensions ingress successfully", func(t *testing.T) { @@ -275,6 +329,100 @@ func TestGetObjectMeta(t *testing.T) { }) } +func TestCreateAnnotationBasedPath(t *testing.T) { + t.Run("v1 ingress, create path", func(t *testing.T) { + ing := networkingIngress() + ni, _ := ing.GetNetworkingIngress() + + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + ing.CreateAnnotationBasedPath("test-route") + assert.Equal(t, 2, len(ni.Spec.Rules[0].HTTP.Paths)) + }) + t.Run("v1 ingress, create existing path", func(t *testing.T) { + ing := networkingIngress() + ni, _ := ing.GetNetworkingIngress() + + ing.CreateAnnotationBasedPath("v1backend") + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + }) + t.Run("v1beta1 ingress, create path", func(t *testing.T) { + ing := extensionsIngress() + ni, _ := ing.GetExtensionsIngress() + + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + ing.CreateAnnotationBasedPath("test-route") + assert.Equal(t, 2, len(ni.Spec.Rules[0].HTTP.Paths)) + }) + t.Run("v1beta1 ingress, create existing path", func(t *testing.T) { + ing := extensionsIngress() + ni, _ := ing.GetExtensionsIngress() + + ing.CreateAnnotationBasedPath("v1beta1backend") + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + }) +} + +func TestRemoveAnnotationBasedPath(t *testing.T) { + t.Run("v1 ingress, remove path", func(t *testing.T) { + ing := networkingIngress() + ni, _ := ing.GetNetworkingIngress() + + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + ing.RemovePathByServiceName("v1backend") + assert.Equal(t, 0, len(ni.Spec.Rules[0].HTTP.Paths)) + }) + t.Run("v1 ingress, remove non existing path", func(t *testing.T) { + ing := networkingIngress() + ni, _ := ing.GetNetworkingIngress() + + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + ing.RemovePathByServiceName("non-exsisting-route") + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + }) + t.Run("v1beta1 ingress, remove path", func(t *testing.T) { + ing := extensionsIngress() + ni, _ := ing.GetExtensionsIngress() + + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + ing.RemovePathByServiceName("v1beta1backend") + assert.Equal(t, 0, len(ni.Spec.Rules[0].HTTP.Paths)) + }) + t.Run("v1beta1 ingress, remove non existing path", func(t *testing.T) { + ing := extensionsIngress() + ni, _ := ing.GetExtensionsIngress() + + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + ing.RemovePathByServiceName("non-exsisting-route") + assert.Equal(t, 1, len(ni.Spec.Rules[0].HTTP.Paths)) + }) +} + +func TestSortHttpPaths(t *testing.T) { + managedRoutes := []v1alpha1.MangedRoutes{{Name: "route1"}, {Name: "route2"}, {Name: "route3"}} + t.Run("v1 ingress, sort path", func(t *testing.T) { + ing := networkingIngressWithPath("action1", "route3", "route1", "route2") + ing.SortHttpPaths(managedRoutes) + ni, _ := ing.GetNetworkingIngress() + + assert.Equal(t, 4, len(ni.Spec.Rules[0].HTTP.Paths)) + assert.Equal(t, "route1", ni.Spec.Rules[0].HTTP.Paths[0].Backend.Service.Name) + assert.Equal(t, "route2", ni.Spec.Rules[0].HTTP.Paths[1].Backend.Service.Name) + assert.Equal(t, "route3", ni.Spec.Rules[0].HTTP.Paths[2].Backend.Service.Name) + assert.Equal(t, "action1", ni.Spec.Rules[0].HTTP.Paths[3].Backend.Service.Name) + }) + t.Run("v1beta1 ingress, sort path", func(t *testing.T) { + ing := extensionsIngressWithPath("action1", "route3", "route1", "route2") + ing.SortHttpPaths(managedRoutes) + ni, _ := ing.GetExtensionsIngress() + + assert.Equal(t, 4, len(ni.Spec.Rules[0].HTTP.Paths)) + assert.Equal(t, "route1", ni.Spec.Rules[0].HTTP.Paths[0].Backend.ServiceName) + assert.Equal(t, "route2", ni.Spec.Rules[0].HTTP.Paths[1].Backend.ServiceName) + assert.Equal(t, "route3", ni.Spec.Rules[0].HTTP.Paths[2].Backend.ServiceName) + assert.Equal(t, "action1", ni.Spec.Rules[0].HTTP.Paths[3].Backend.ServiceName) + }) +} + func TestDeepCopy(t *testing.T) { t.Run("will deepcopy ingress wrapped with networking.Ingress", func(t *testing.T) { // given @@ -823,3 +971,135 @@ func getExtensionsIngress() *v1beta1.Ingress { }, } } + +func networkingIngress() *ingress.Ingress { + pathType := v1.PathTypeImplementationSpecific + res := v1.Ingress{ + Spec: v1.IngressSpec{ + IngressClassName: pointer.String("v1ingress"), + Rules: []v1.IngressRule{ + { + Host: "v1host", + IngressRuleValue: v1.IngressRuleValue{ + HTTP: &v1.HTTPIngressRuleValue{ + Paths: []v1.HTTPIngressPath{ + { + Backend: v1.IngressBackend{ + Service: &v1.IngressServiceBackend{ + Name: "v1backend", + Port: v1.ServiceBackendPort{Name: "use-annotation"}, + }, + }, + Path: "/*", + PathType: &pathType, + }, + }, + }, + }, + }, + }, + }, + } + return ingress.NewIngress(&res) +} + +func networkingIngressWithPath(paths ...string) *ingress.Ingress { + var ingressPaths []v1.HTTPIngressPath + for _, path := range paths { + ingressPaths = append(ingressPaths, v1IngressPath(path)) + } + res := v1.Ingress{ + Spec: v1.IngressSpec{ + IngressClassName: pointer.String("v1ingress"), + Rules: []v1.IngressRule{ + { + Host: "v1host", + IngressRuleValue: v1.IngressRuleValue{ + HTTP: &v1.HTTPIngressRuleValue{ + Paths: ingressPaths, + }, + }, + }, + }, + }, + } + return ingress.NewIngress(&res) +} + +func v1IngressPath(serviceName string) v1.HTTPIngressPath { + pathType := v1.PathTypeImplementationSpecific + return v1.HTTPIngressPath{ + Backend: v1.IngressBackend{ + Service: &v1.IngressServiceBackend{ + Name: serviceName, + Port: v1.ServiceBackendPort{Name: "use-annotation"}, + }, + }, + Path: "/*", + PathType: &pathType, + } +} + +func extensionsIngress() *ingress.Ingress { + pathType := v1beta1.PathTypeImplementationSpecific + res := v1beta1.Ingress{ + Spec: v1beta1.IngressSpec{ + IngressClassName: pointer.String("v1beta1ingress"), + Rules: []v1beta1.IngressRule{ + { + Host: "v1beta1host", + IngressRuleValue: v1beta1.IngressRuleValue{ + HTTP: &v1beta1.HTTPIngressRuleValue{ + Paths: []v1beta1.HTTPIngressPath{ + { + Backend: v1beta1.IngressBackend{ + ServiceName: "v1beta1backend", + ServicePort: intstr.FromString("use-annotation"), + }, + Path: "/*", + PathType: &pathType, + }, + }, + }, + }, + }, + }, + }, + } + return ingress.NewLegacyIngress(&res) +} + +func extensionsIngressWithPath(paths ...string) *ingress.Ingress { + var ingressPaths []v1beta1.HTTPIngressPath + for _, path := range paths { + ingressPaths = append(ingressPaths, extensionIngressPath(path)) + } + res := v1beta1.Ingress{ + Spec: v1beta1.IngressSpec{ + IngressClassName: pointer.String("v1beta1ingress"), + Rules: []v1beta1.IngressRule{ + { + Host: "v1beta1host", + IngressRuleValue: v1beta1.IngressRuleValue{ + HTTP: &v1beta1.HTTPIngressRuleValue{ + Paths: ingressPaths, + }, + }, + }, + }, + }, + } + return ingress.NewLegacyIngress(&res) +} + +func extensionIngressPath(serviceName string) v1beta1.HTTPIngressPath { + pathType := v1beta1.PathTypeImplementationSpecific + return v1beta1.HTTPIngressPath{ + Backend: v1beta1.IngressBackend{ + ServiceName: serviceName, + ServicePort: intstr.FromString("use-annotation"), + }, + Path: "/*", + PathType: &pathType, + } +}