From 5813e4e6eae4c39bccb085d83d87550d10ad9751 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Thu, 13 Aug 2020 14:51:24 -0700 Subject: [PATCH 1/4] Add root service to types.go --- manifests/crds/rollout-crd.yaml | 5 ++++- manifests/install.yaml | 5 ++++- manifests/namespace-install.yaml | 5 ++++- pkg/apis/rollouts/v1alpha1/openapi_generated.go | 12 +++++++++--- pkg/apis/rollouts/v1alpha1/types.go | 2 ++ pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go | 4 ++++ 6 files changed, 27 insertions(+), 6 deletions(-) diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index e3d0ef6337..7b19a9ef7b 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -397,6 +397,8 @@ spec: type: string ingress: type: string + rootService: + type: string servicePort: format: int32 type: integer @@ -2910,7 +2912,8 @@ spec: abort: type: boolean abortedAt: - type: boolean + format: date-time + type: string availableReplicas: format: int32 type: integer diff --git a/manifests/install.yaml b/manifests/install.yaml index d6e787ced9..a373bf9c5f 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -11346,6 +11346,8 @@ spec: type: string ingress: type: string + rootService: + type: string servicePort: format: int32 type: integer @@ -13859,7 +13861,8 @@ spec: abort: type: boolean abortedAt: - type: boolean + format: date-time + type: string availableReplicas: format: int32 type: integer diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index 01360d1db8..3a7350da0f 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -11346,6 +11346,8 @@ spec: type: string ingress: type: string + rootService: + type: string servicePort: format: int32 type: integer @@ -13859,7 +13861,8 @@ spec: abort: type: boolean abortedAt: - type: boolean + format: date-time + type: string availableReplicas: format: int32 type: integer diff --git a/pkg/apis/rollouts/v1alpha1/openapi_generated.go b/pkg/apis/rollouts/v1alpha1/openapi_generated.go index 4e67fb9ee8..ecfa0d5e98 100644 --- a/pkg/apis/rollouts/v1alpha1/openapi_generated.go +++ b/pkg/apis/rollouts/v1alpha1/openapi_generated.go @@ -74,8 +74,8 @@ func GetOpenAPIDefinitions(ref common.ReferenceCallback) map[string]common.OpenA "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.Rollout": schema_pkg_apis_rollouts_v1alpha1_Rollout(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutAnalysis": schema_pkg_apis_rollouts_v1alpha1_RolloutAnalysis(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutAnalysisBackground": schema_pkg_apis_rollouts_v1alpha1_RolloutAnalysisBackground(ref), - "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutAnalysisTemplate": schema_pkg_apis_rollouts_v1alpha1_RolloutAnalysisTemplate(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutAnalysisRunStatus": schema_pkg_apis_rollouts_v1alpha1_RolloutAnalysisRunStatus(ref), + "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutAnalysisTemplate": schema_pkg_apis_rollouts_v1alpha1_RolloutAnalysisTemplate(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutCondition": schema_pkg_apis_rollouts_v1alpha1_RolloutCondition(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutExperimentStep": schema_pkg_apis_rollouts_v1alpha1_RolloutExperimentStep(ref), "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1.RolloutExperimentStepAnalysisTemplateRef": schema_pkg_apis_rollouts_v1alpha1_RolloutExperimentStepAnalysisTemplateRef(ref), @@ -119,6 +119,13 @@ func schema_pkg_apis_rollouts_v1alpha1_ALBTrafficRouting(ref common.ReferenceCal Format: "int32", }, }, + "rootService": { + SchemaProps: spec.SchemaProps{ + Description: "RootService references the service in the ingress to the controller should add the action to", + Type: []string{"string"}, + Format: "", + }, + }, "annotationPrefix": { SchemaProps: spec.SchemaProps{ Description: "AnnotationPrefix has to match the configured annotation prefix on the alb ingress controller", @@ -2705,8 +2712,7 @@ func schema_pkg_apis_rollouts_v1alpha1_RolloutStatus(ref common.ReferenceCallbac "abortedAt": { SchemaProps: spec.SchemaProps{ Description: "AbortedAt indicates the controller reconciled an aborted rollout. The controller uses this to understand if the controller needs to do some specific work when a Rollout is aborted. For example, the reconcileAbort is used to indicate if the Rollout should enter an aborted state when the latest AnalysisRun is a failure, or the controller has already put the Rollout into an aborted and should create a new AnalysisRun.", - Type: []string{"boolean"}, - Format: "", + Ref: ref("k8s.io/apimachinery/pkg/apis/meta/v1.Time"), }, }, "currentPodHash": { diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index 91177958e8..c76346dab1 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -198,6 +198,8 @@ type ALBTrafficRouting struct { Ingress string `json:"ingress"` // ServicePort refers to the port that the Ingress action should route traffic to ServicePort int32 `json:"servicePort"` + // RootService references the service in the ingress to the controller should add the action to + RootService string `json:"rootService,omitempty"` // AnnotationPrefix has to match the configured annotation prefix on the alb ingress controller // +optional AnnotationPrefix string `json:"annotationPrefix,omitempty"` diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go index 1377110543..af9eced719 100644 --- a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go @@ -1457,6 +1457,10 @@ func (in *RolloutStatus) DeepCopyInto(out *RolloutStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.AbortedAt != nil { + in, out := &in.AbortedAt, &out.AbortedAt + *out = (*in).DeepCopy() + } if in.CurrentStepIndex != nil { in, out := &in.CurrentStepIndex, &out.CurrentStepIndex *out = new(int32) From 939617870a45aee6cc16e50f4f731648cb36ff16 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Thu, 13 Aug 2020 14:52:16 -0700 Subject: [PATCH 2/4] Use rootService in alb traffic routing --- rollout/trafficrouting/alb/alb.go | 10 +++++++--- rollout/trafficrouting/alb/alb_test.go | 7 ++++--- utils/ingress/ingress.go | 6 +++++- utils/ingress/ingress_test.go | 4 +++- 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/rollout/trafficrouting/alb/alb.go b/rollout/trafficrouting/alb/alb.go index 1608e7f8a7..60c4f97413 100644 --- a/rollout/trafficrouting/alb/alb.go +++ b/rollout/trafficrouting/alb/alb.go @@ -63,10 +63,14 @@ func (r *Reconciler) Reconcile(desiredWeight int32) error { if err != nil { return err } - stableService := r.cfg.Rollout.Spec.Strategy.Canary.StableService + actionService := r.cfg.Rollout.Spec.Strategy.Canary.StableService + if r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.ALB.RootService != "" { + actionService = r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.ALB.RootService + + } port := r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.ALB.ServicePort - if !ingressutil.HasRuleWithService(ingress, stableService) { - return fmt.Errorf("ingress does not use the stable service") + if !ingressutil.HasRuleWithService(ingress, actionService) { + return fmt.Errorf("ingress does not have service `%s` in rules", actionService) } desired, err := getDesiredAnnotations(ingress, rollout, port, desiredWeight) diff --git a/rollout/trafficrouting/alb/alb_test.go b/rollout/trafficrouting/alb/alb_test.go index b89e2ceffc..bedab54242 100644 --- a/rollout/trafficrouting/alb/alb_test.go +++ b/rollout/trafficrouting/alb/alb_test.go @@ -134,9 +134,10 @@ func TestIngressNotFound(t *testing.T) { assert.True(t, k8serrors.IsNotFound(err)) } -func TestStableServiceNotFoundInIngress(t *testing.T) { - ro := fakeRollout("different-stable", "canary-service", "ingress", 443) - i := ingress("ingress", "stable-service", "preview-svc", 443, 50, ro.Name) +func TestServiceNotFoundInIngress(t *testing.T) { + ro := fakeRollout("stable-stable", "canary-service", "ingress", 443) + ro.Spec.Strategy.Canary.TrafficRouting.ALB.RootService = "invalid-svc" + i := ingress("ingress", "stable-service", "canary-svc", 443, 50, ro.Name) client := fake.NewSimpleClientset() k8sI := kubeinformers.NewSharedInformerFactory(client, 0) k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i) diff --git a/utils/ingress/ingress.go b/utils/ingress/ingress.go index 6109142a83..974e11961a 100644 --- a/utils/ingress/ingress.go +++ b/utils/ingress/ingress.go @@ -140,5 +140,9 @@ func ALBActionAnnotationKey(r *v1alpha1.Rollout) string { if r.Spec.Strategy.Canary.TrafficRouting.ALB.AnnotationPrefix != "" { prefix = r.Spec.Strategy.Canary.TrafficRouting.ALB.AnnotationPrefix } - return fmt.Sprintf("%s%s%s", prefix, ALBActionPrefix, r.Spec.Strategy.Canary.StableService) + 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) } diff --git a/utils/ingress/ingress_test.go b/utils/ingress/ingress_test.go index bd50109500..4c3d24c8e9 100644 --- a/utils/ingress/ingress_test.go +++ b/utils/ingress/ingress_test.go @@ -164,7 +164,9 @@ func TestALBActionAnnotationKey(t *testing.T) { Status: v1alpha1.RolloutStatus{}, } assert.Equal(t, "test.annotation/actions.svc", ALBActionAnnotationKey(r)) + r.Spec.Strategy.Canary.TrafficRouting.ALB.RootService = "root-svc" + assert.Equal(t, "test.annotation/actions.root-svc", ALBActionAnnotationKey(r)) r.Spec.Strategy.Canary.TrafficRouting.ALB.AnnotationPrefix = "" - assert.Equal(t, "alb.ingress.kubernetes.io/actions.svc", ALBActionAnnotationKey(r)) + assert.Equal(t, "alb.ingress.kubernetes.io/actions.root-svc", ALBActionAnnotationKey(r)) } From eeb8c7f9d847a35ecde1f40c1c0183e99e85f167 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Thu, 13 Aug 2020 14:58:54 -0700 Subject: [PATCH 3/4] Add docs on rootService for ALB traffic routing --- docs/features/traffic-management/alb.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/features/traffic-management/alb.md b/docs/features/traffic-management/alb.md index 4e47abbbcf..089d977f3f 100644 --- a/docs/features/traffic-management/alb.md +++ b/docs/features/traffic-management/alb.md @@ -64,10 +64,11 @@ spec: alb: ingress: ingress # required servicePort: 443 # required + rootService: # optional annotationPrefix: custom.alb.ingress.kubernetes.io # optional ``` -The ingress field is a reference to an Ingress in the same namespace of the Rollout, and the servicePort field refers a port of a service. The Rollout requires the Ingress and servicePort to modify the ALB to route traffic to the stable and canary Services. Within the Ingress, looks for the stableService within the Ingress's rules and adds an action annotation for that the action. As the Rollout progresses through the Canary steps, the controller updates the Ingress's action annotation to reflect the desired state of the Rollout enabling traffic splitting between two different versions. +The ingress field is a reference to an Ingress in the same namespace of the Rollout, and the servicePort field refers a port of a service. The Rollout requires the Ingress and servicePort to modify the ALB to route traffic to the stable and canary Services. Within the Ingress, looks for the stableService (or the optional rootService if specified) within the Ingress's rules and adds an action annotation for that the action. As the Rollout progresses through the Canary steps, the controller updates the Ingress's action annotation to reflect the desired state of the Rollout enabling traffic splitting between two different versions. Since the ALB Ingress controller allows users to configure the annotation prefix used by the Ingress controller, Rollouts can specify the optional `annotationPrefix` field. The Ingress uses that prefix instead of the default `alb.ingress.kubernetes.io` if the field set. From 1b6d235a402a578f07a96a64352bdefca3016a91 Mon Sep 17 00:00:00 2001 From: Danny Thomson Date: Fri, 14 Aug 2020 16:39:00 -0700 Subject: [PATCH 4/4] Add validation for alb ingress about root service --- pkg/apis/rollouts/validation/validation.go | 6 ++++++ pkg/apis/rollouts/validation/validation_references.go | 11 +++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go index 5ac3a3e3ad..d5f93bf1b1 100644 --- a/pkg/apis/rollouts/validation/validation.go +++ b/pkg/apis/rollouts/validation/validation.go @@ -49,6 +49,8 @@ const ( ScaleDownLimitLargerThanRevisionLimit = "This rollout's revision history limit can not be smaller than the rollout's scale down limit" // InvalidTrafficRoutingMessage indicates that both canary and stable service must be set to use Traffic Routing InvalidTrafficRoutingMessage = "Canary service and Stable service must to be set to use Traffic Routing" + // InvalidIstioRoutesMessage indicates that rollout does not have a route specified for the istio Traffic Routing + InvalidIstioRoutesMessage = "Istio virtual service must have at least 1 route specified" ) func ValidateRollout(rollout *v1alpha1.Rollout) field.ErrorList { @@ -160,6 +162,10 @@ func ValidateRolloutStrategyCanary(rollout *v1alpha1.Rollout, fldPath *field.Pat if canary.TrafficRouting != nil && (canary.StableService == "" || canary.CanaryService == "") { allErrs = append(allErrs, field.Invalid(fldPath.Child("trafficRouting"), canary.TrafficRouting, InvalidTrafficRoutingMessage)) } + if canary.TrafficRouting != nil && canary.TrafficRouting.Istio != nil && len(canary.TrafficRouting.Istio.VirtualService.Routes) == 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("trafficRouting").Child("istio").Child("virtualService").Child("routes"), canary.TrafficRouting, InvalidIstioRoutesMessage)) + + } for i, step := range canary.Steps { stepFldPath := fldPath.Child("steps").Index(i) allErrs = append(allErrs, hasMultipleStepsType(step, stepFldPath)...) diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index e1e94053bf..56d6afac64 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -118,17 +118,24 @@ func ValidateIngress(rollout *v1alpha1.Rollout, ingress v1beta1.Ingress) field.E allErrs := field.ErrorList{} fldPath := field.NewPath("spec", "strategy", "canary", "trafficRouting") var ingressName string + var serviceName string if rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil { fldPath = fldPath.Child("nginx").Child("stableIngress") + serviceName = rollout.Spec.Strategy.Canary.StableService ingressName = rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress } else if rollout.Spec.Strategy.Canary.TrafficRouting.ALB != nil { fldPath = fldPath.Child("alb").Child("ingress") ingressName = rollout.Spec.Strategy.Canary.TrafficRouting.ALB.Ingress + serviceName = rollout.Spec.Strategy.Canary.StableService + if rollout.Spec.Strategy.Canary.TrafficRouting.ALB.RootService != "" { + serviceName = rollout.Spec.Strategy.Canary.TrafficRouting.ALB.RootService + } + } else { return allErrs } - if !ingressutil.HasRuleWithService(&ingress, rollout.Spec.Strategy.Canary.StableService) { - msg := fmt.Sprintf("ingress `%s` has no rules using service %s backend", ingress.Name, rollout.Spec.Strategy.Canary.StableService) + if !ingressutil.HasRuleWithService(&ingress, serviceName) { + msg := fmt.Sprintf("ingress `%s` has no rules using service %s backend", ingress.Name, serviceName) allErrs = append(allErrs, field.Invalid(fldPath, ingressName, msg)) } return allErrs