diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index ff2d356420..5dca32ff94 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -104,8 +104,8 @@ jobs: # This symlink is necessary to ensure that `git diff` detects changes - name: Create symlink in GOPATH run: | - mkdir -p ~/go/src/github.com/argoproj - ln -s $(pwd) ~/go/src/github.com/argoproj/argo-rollouts + mkdir -p ~/go/src/github.com/rallyhealth + ln -s $(pwd) ~/go/src/github.com/rallyhealth/argo-rollouts - uses: actions/cache@v2 with: path: /home/runner/.cache/go-build diff --git a/analysis/analysis.go b/analysis/analysis.go index 5bae4a8892..53d5358647 100644 --- a/analysis/analysis.go +++ b/analysis/analysis.go @@ -316,7 +316,7 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa for _, task := range tasks { wg.Add(1) - go func(t metricTask) error { + go func(t metricTask) { defer wg.Done() //redact secret values from logs logger := logutil.WithRedactor(*logutil.WithAnalysisRun(run).WithField("metric", t.metric.Name), secrets) @@ -326,9 +326,12 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa resultsLock.Unlock() provider, err := c.newProvider(*logger, t.metric) + //Fix for https://github.com/argoproj/argo-rollouts/issues/2024 this error is not bubbled to runMeasurements function + //it just stops the go routine to prevent nil pointer usage. Just keeping this simple due to it being a patch for a bug. + //We probably want to handle errors in this goroutine in a different way in master but for now just prevent crashing. if err != nil { log.Errorf("Error in getting provider :%v", err) - return err + return } if metricResult == nil { metricResult = &v1alpha1.MetricResult{ @@ -408,7 +411,7 @@ func (c *Controller) runMeasurements(run *v1alpha1.AnalysisRun, tasks []metricTa resultsLock.Lock() analysisutil.SetResult(run, *metricResult) resultsLock.Unlock() - return nil + }(task) } wg.Wait() diff --git a/ingress/ingress_test.go b/ingress/ingress_test.go index 3db4f1bcca..88125b3206 100644 --- a/ingress/ingress_test.go +++ b/ingress/ingress_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" extensionsv1beta1 "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/intstr" kubeinformers "k8s.io/client-go/informers" k8sfake "k8s.io/client-go/kubernetes/fake" @@ -85,7 +86,15 @@ func newNginxIngressWithAnnotation(name string, port int, serviceName string) *e } } +func newFakeIngressControllerMultiIngress(t *testing.T, ing []*extensionsv1beta1.Ingress, rollout *v1alpha1.Rollout) (*Controller, *k8sfake.Clientset, map[string]int) { + return underlyingControllerBuilder(t, ing, rollout) +} + func newFakeIngressController(t *testing.T, ing *extensionsv1beta1.Ingress, rollout *v1alpha1.Rollout) (*Controller, *k8sfake.Clientset, map[string]int) { + return underlyingControllerBuilder(t, []*extensionsv1beta1.Ingress{ing}, rollout) +} + +func underlyingControllerBuilder(t *testing.T, ing []*extensionsv1beta1.Ingress, rollout *v1alpha1.Rollout) (*Controller, *k8sfake.Clientset, map[string]int) { t.Helper() client := fake.NewSimpleClientset() if rollout != nil { @@ -93,7 +102,13 @@ func newFakeIngressController(t *testing.T, ing *extensionsv1beta1.Ingress, roll } kubeclient := k8sfake.NewSimpleClientset() if ing != nil { - kubeclient = k8sfake.NewSimpleClientset(ing) + var x []runtime.Object + for _, i := range ing { + if i != nil { + x = append(x, i) + } + } + kubeclient = k8sfake.NewSimpleClientset(x...) } i := informers.NewSharedInformerFactory(client, 0) k8sI := kubeinformers.NewSharedInformerFactory(kubeclient, 0) @@ -138,7 +153,11 @@ func newFakeIngressController(t *testing.T, ing *extensionsv1beta1.Ingress, roll } if ing != nil { - k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(ing) + for _, i := range ing { + if i != nil { + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(i) + } + } } if rollout != nil { i.Argoproj().V1alpha1().Rollouts().Informer().GetIndexer().Add(rollout) @@ -164,6 +183,20 @@ func TestSyncIngressNotReferencedByRollout(t *testing.T) { assert.Len(t, actions, 0) } +func TestSyncIngressNotReferencedByRolloutMultiIngress(t *testing.T) { + ings := []*extensionsv1beta1.Ingress{ + newNginxIngress("test-stable-ingress", 80, "stable-service"), + newNginxIngress("test-stable-ingress-additional", 80, "stable-service"), + } + + ctrl, kubeclient, _ := newFakeIngressControllerMultiIngress(t, ings, nil) + + err := ctrl.syncIngress("default/test-stable-ingress") + assert.NoError(t, err) + actions := kubeclient.Actions() + assert.Len(t, actions, 0) +} + func TestSyncIngressReferencedByRollout(t *testing.T) { ing := newNginxIngress("test-stable-ingress", 80, "stable-service") @@ -227,3 +260,78 @@ func TestSkipIngressWithNoClass(t *testing.T) { assert.Len(t, actions, 0) assert.Len(t, enqueuedObjects, 0) } + +func TestSkipIngressWithNoClassMultiIngress(t *testing.T) { + ings := []*extensionsv1beta1.Ingress{ + newNginxIngressWithAnnotation("test-stable-ingress", 80, "stable-service"), + newNginxIngressWithAnnotation("test-stable-ingress-additional", 80, "stable-service"), + } + for _, i := range ings { + i.Annotations = nil + } + + rollout := &v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rollout", + Namespace: metav1.NamespaceDefault, + }, + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + StableService: "stable-service", + CanaryService: "canary-service", + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + Nginx: &v1alpha1.NginxTrafficRouting{ + StableIngress: "test-stable-ingress", + AdditionalStableIngresses: []string{"test-stable-ingress-additional"}, + }, + }, + }, + }, + }, + } + + ctrl, kubeclient, enqueuedObjects := newFakeIngressControllerMultiIngress(t, ings, rollout) + + err := ctrl.syncIngress("default/test-stable-ingress") + assert.NoError(t, err) + actions := kubeclient.Actions() + assert.Len(t, actions, 0) + assert.Len(t, enqueuedObjects, 0) +} + +func TestSyncIngressReferencedByRolloutMultiIngress(t *testing.T) { + ings := []*extensionsv1beta1.Ingress{ + newNginxIngress("test-stable-ingress", 80, "stable-service"), + newNginxIngress("test-stable-ingress-additional", 80, "stable-service"), + } + + rollout := &v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "rollout", + Namespace: metav1.NamespaceDefault, + }, + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + StableService: "stable-service", + CanaryService: "canary-service", + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + Nginx: &v1alpha1.NginxTrafficRouting{ + StableIngress: "test-stable-ingress", + AdditionalStableIngresses: []string{"test-stable-ingress-additional"}, + }, + }, + }, + }, + }, + } + + ctrl, kubeclient, enqueuedObjects := newFakeIngressControllerMultiIngress(t, ings, rollout) + + err := ctrl.syncIngress("default/test-stable-ingress") + assert.NoError(t, err) + actions := kubeclient.Actions() + assert.Len(t, actions, 0) + assert.Equal(t, 1, enqueuedObjects["default/rollout"]) +} diff --git a/manifests/crds/rollout-crd.yaml b/manifests/crds/rollout-crd.yaml index 67a9b8d7aa..1518a5afcb 100644 --- a/manifests/crds/rollout-crd.yaml +++ b/manifests/crds/rollout-crd.yaml @@ -816,6 +816,10 @@ spec: additionalProperties: type: string type: object + additionalStableIngresses: + items: + type: string + type: array annotationPrefix: type: string stableIngress: diff --git a/manifests/install.yaml b/manifests/install.yaml index d32be4f257..22ea1f53a3 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -11817,6 +11817,10 @@ spec: additionalProperties: type: string type: object + additionalStableIngresses: + items: + type: string + type: array annotationPrefix: type: string stableIngress: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index e6370dd98a..c53eeb0c67 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -11817,6 +11817,10 @@ spec: additionalProperties: type: string type: object + additionalStableIngresses: + items: + type: string + type: array annotationPrefix: type: string stableIngress: diff --git a/pkg/apiclient/rollout/rollout.swagger.json b/pkg/apiclient/rollout/rollout.swagger.json index 44e663763e..ac5b5a404b 100644 --- a/pkg/apiclient/rollout/rollout.swagger.json +++ b/pkg/apiclient/rollout/rollout.swagger.json @@ -1039,6 +1039,13 @@ "type": "string" }, "title": "+optional" + }, + "additionalStableIngresses": { + "type": "array", + "items": { + "type": "string" + }, + "title": "AdditionalStableIngresses refers to the names of `Ingress` resources in the same namespace as the `Rollout` in a multi ingress scenario\n+optional" } }, "title": "NginxTrafficRouting configuration for Nginx ingress controller to control traffic routing" diff --git a/pkg/apis/api-rules/violation_exceptions.list b/pkg/apis/api-rules/violation_exceptions.list index 479a0f52c1..4393612dcd 100644 --- a/pkg/apis/api-rules/violation_exceptions.list +++ b/pkg/apis/api-rules/violation_exceptions.list @@ -25,6 +25,7 @@ API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,IstioVirtualService,TLSRoutes API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,KayentaMetric,Scopes API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,MetricResult,Measurements +API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,NginxTrafficRouting,AdditionalStableIngresses API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutAnalysis,Args API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutAnalysis,DryRun API rule violation: list_type_missing,github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1,RolloutAnalysis,MeasurementRetention diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go index de417edb16..05ac261d5c 100644 --- a/pkg/apis/rollouts/v1alpha1/types.go +++ b/pkg/apis/rollouts/v1alpha1/types.go @@ -407,6 +407,9 @@ type NginxTrafficRouting struct { StableIngress string `json:"stableIngress" protobuf:"bytes,2,opt,name=stableIngress"` // +optional AdditionalIngressAnnotations map[string]string `json:"additionalIngressAnnotations,omitempty" protobuf:"bytes,3,rep,name=additionalIngressAnnotations"` + // AdditionalStableIngresses refers to the names of `Ingress` resources in the same namespace as the `Rollout` in a multi ingress scenario + // +optional + AdditionalStableIngresses []string `json:"additionalStableIngresses,omitempty" protobuf:"bytes,4,rep,name=additionalStableIngresses"` } // IstioTrafficRouting configuration for Istio service mesh to enable fine grain configuration diff --git a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go index 809e6e5c51..e16f370a91 100644 --- a/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/rollouts/v1alpha1/zz_generated.deepcopy.go @@ -1678,6 +1678,11 @@ func (in *NginxTrafficRouting) DeepCopyInto(out *NginxTrafficRouting) { (*out)[key] = val } } + if in.AdditionalStableIngresses != nil { + in, out := &in.AdditionalStableIngresses, &out.AdditionalStableIngresses + *out = make([]string, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/apis/rollouts/validation/validation_references.go b/pkg/apis/rollouts/validation/validation_references.go index a4d5561f4a..55128f993b 100644 --- a/pkg/apis/rollouts/validation/validation_references.go +++ b/pkg/apis/rollouts/validation/validation_references.go @@ -218,23 +218,39 @@ func setArgValuePlaceHolder(Args []v1alpha1.Argument) { func ValidateIngress(rollout *v1alpha1.Rollout, ingress *ingressutil.Ingress) field.ErrorList { allErrs := field.ErrorList{} fldPath := field.NewPath("spec", "strategy", "canary", "trafficRouting") + canary := rollout.Spec.Strategy.Canary var ingressName string var serviceName string - if rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil { + if canary.TrafficRouting.Nginx != nil { + // If there are additional stable ingresses + if len(canary.TrafficRouting.Nginx.AdditionalStableIngresses) > 0 { + // validate each ingress as valid + fldPath = fldPath.Child("nginx").Child("additionalStableIngresses") + serviceName = canary.StableService + for _, ing := range canary.TrafficRouting.Nginx.AdditionalStableIngresses { + ingressName = ing + allErrs = reportErrors(ingress, serviceName, ingressName, fldPath, allErrs) + } + } 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 { + serviceName = canary.StableService + ingressName = canary.TrafficRouting.Nginx.StableIngress + + allErrs = reportErrors(ingress, serviceName, ingressName, fldPath, allErrs) + } else if 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 + ingressName = canary.TrafficRouting.ALB.Ingress + serviceName = canary.StableService + if canary.TrafficRouting.ALB.RootService != "" { + serviceName = canary.TrafficRouting.ALB.RootService } - - } else { - return allErrs + allErrs = reportErrors(ingress, serviceName, ingressName, fldPath, allErrs) } + + return allErrs +} + +func reportErrors(ingress *ingressutil.Ingress, serviceName, ingressName string, fldPath *field.Path, allErrs field.ErrorList) field.ErrorList { if !ingressutil.HasRuleWithService(ingress, serviceName) { msg := fmt.Sprintf("ingress `%s` has no rules using service %s backend", ingress.GetName(), serviceName) allErrs = append(allErrs, field.Invalid(fldPath, ingressName, msg)) diff --git a/rollout/controller.go b/rollout/controller.go index e314adf4af..79c7f9facd 100644 --- a/rollout/controller.go +++ b/rollout/controller.go @@ -798,6 +798,19 @@ func (c *rolloutContext) getReferencedIngresses() (*[]ingressutil.Ingress, error } ingresses = append(ingresses, *ingress) } else if canary.TrafficRouting.Nginx != nil { + // If the rollout resource manages more than 1 ingress + if len(canary.TrafficRouting.Nginx.AdditionalStableIngresses) > 0 { + for _, ing := range canary.TrafficRouting.Nginx.AdditionalStableIngresses { + ingress, err := c.ingressWrapper.GetCached(c.rollout.Namespace, ing) + if k8serrors.IsNotFound(err) { + return nil, field.Invalid(fldPath.Child("nginx", "AdditionalStableIngresses"), canary.TrafficRouting.Nginx.StableIngress, err.Error()) + } + if err != nil { + return nil, err + } + ingresses = append(ingresses, *ingress) + } + } ingress, err := c.ingressWrapper.GetCached(c.rollout.Namespace, canary.TrafficRouting.Nginx.StableIngress) if k8serrors.IsNotFound(err) { return nil, field.Invalid(fldPath.Child("nginx", "stableIngress"), canary.TrafficRouting.Nginx.StableIngress, err.Error()) diff --git a/rollout/controller_test.go b/rollout/controller_test.go index 0e97a405e9..ec7ac4e355 100644 --- a/rollout/controller_test.go +++ b/rollout/controller_test.go @@ -1712,6 +1712,8 @@ func TestGetReferencedIngressesNginx(t *testing.T) { defer f.Close() t.Run("get referenced Nginx ingress - fail", func(t *testing.T) { + // clear fixture + f.ingressLister = []*ingressutil.Ingress{} c, _, _ := f.newController(noResyncPeriodFunc) roCtx, err := c.newRolloutContext(r) assert.NoError(t, err) @@ -1721,6 +1723,8 @@ func TestGetReferencedIngressesNginx(t *testing.T) { }) t.Run("get referenced Nginx ingress - success", func(t *testing.T) { + // clear fixture + f.ingressLister = []*ingressutil.Ingress{} ingress := &extensionsv1beta1.Ingress{ ObjectMeta: metav1.ObjectMeta{ Name: "nginx-ingress-name", @@ -1735,6 +1739,93 @@ func TestGetReferencedIngressesNginx(t *testing.T) { assert.NoError(t, err) }) } +func TestGetReferencedIngressesNginxMultiIngress(t *testing.T) { + f := newFixture(t) + defer f.Close() + r := newCanaryRollout("rollout", 1, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1)) + r.Spec.Strategy.Canary.TrafficRouting = &v1alpha1.RolloutTrafficRouting{ + Nginx: &v1alpha1.NginxTrafficRouting{ + StableIngress: "nginx-ingress-name", + AdditionalStableIngresses: []string{"nginx-ingress-additional"}, + }, + } + r.Namespace = metav1.NamespaceDefault + defer f.Close() + + t.Run("get referenced Nginx ingress - fail on secondary when both missing", func(t *testing.T) { + // clear fixture + f.ingressLister = []*ingressutil.Ingress{} + c, _, _ := f.newController(noResyncPeriodFunc) + roCtx, err := c.newRolloutContext(r) + assert.NoError(t, err) + _, err = roCtx.getReferencedIngresses() + expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "nginx", "AdditionalStableIngresses"), "nginx-ingress-name", "ingress.extensions \"nginx-ingress-additional\" not found") + assert.Equal(t, expectedErr.Error(), err.Error()) + }) + + t.Run("get referenced Nginx ingress - fail on primary when additional present", func(t *testing.T) { + // clear fixture + f.ingressLister = []*ingressutil.Ingress{} + ingressAdditional := &extensionsv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-ingress-additional", + Namespace: metav1.NamespaceDefault, + }, + } + f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ingressAdditional)) + c, _, _ := f.newController(noResyncPeriodFunc) + roCtx, err := c.newRolloutContext(r) + assert.NoError(t, err) + _, err = roCtx.getReferencedIngresses() + expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "nginx", "stableIngress"), "nginx-ingress-name", "ingress.extensions \"nginx-ingress-name\" not found") + assert.Equal(t, expectedErr.Error(), err.Error()) + }) + + t.Run("get referenced Nginx ingress - fail on secondary when only secondary missing", func(t *testing.T) { + // clear fixture + f.ingressLister = []*ingressutil.Ingress{} + ingress := &extensionsv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-ingress-name", + Namespace: metav1.NamespaceDefault, + }, + } + f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ingress)) + c, _, _ := f.newController(noResyncPeriodFunc) + roCtx, err := c.newRolloutContext(r) + assert.NoError(t, err) + _, err = roCtx.getReferencedIngresses() + if err == nil { + fmt.Println("ERROR IS NIL") + } + expectedErr := field.Invalid(field.NewPath("spec", "strategy", "canary", "trafficRouting", "nginx", "AdditionalStableIngresses"), "nginx-ingress-name", "ingress.extensions \"nginx-ingress-additional\" not found") + assert.Equal(t, expectedErr.Error(), err.Error()) + }) + + t.Run("get referenced Nginx ingress - success", func(t *testing.T) { + // clear fixture + f.ingressLister = []*ingressutil.Ingress{} + ingress := &extensionsv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-ingress-name", + Namespace: metav1.NamespaceDefault, + }, + } + ingressAdditional := &extensionsv1beta1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nginx-ingress-additional", + Namespace: metav1.NamespaceDefault, + }, + } + f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ingress)) + f.ingressLister = append(f.ingressLister, ingressutil.NewLegacyIngress(ingressAdditional)) + c, _, _ := f.newController(noResyncPeriodFunc) + roCtx, err := c.newRolloutContext(r) + assert.NoError(t, err) + _, err = roCtx.getReferencedIngresses() + assert.NoError(t, err) + }) +} func TestGetReferencedAppMeshResources(t *testing.T) { r := newCanaryRollout("rollout", 1, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1)) diff --git a/rollout/trafficrouting/nginx/nginx.go b/rollout/trafficrouting/nginx/nginx.go index 8bb6353ef3..52a687a496 100644 --- a/rollout/trafficrouting/nginx/nginx.go +++ b/rollout/trafficrouting/nginx/nginx.go @@ -222,88 +222,106 @@ func (r *Reconciler) canaryIngress(stableIngress *ingressutil.Ingress, name stri // SetWeight modifies Nginx Ingress resources to reach desired state func (r *Reconciler) SetWeight(desiredWeight int32, additionalDestinations ...v1alpha1.WeightDestination) error { - ctx := context.TODO() - stableIngressName := r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress - canaryIngressName := ingressutil.GetCanaryIngressName(r.cfg.Rollout) - - // Check if stable ingress exists (from lister, which has a cache), error if it does not - stableIngress, err := r.cfg.IngressWrapper.GetCached(r.cfg.Rollout.Namespace, stableIngressName) - if err != nil { - r.log.WithField(logutil.IngressKey, stableIngressName).WithField("err", err.Error()).Error("error retrieving stableIngress") - return fmt.Errorf("error retrieving stableIngress `%s` from cache: %v", stableIngressName, err) - } - // Check if canary ingress exists (from lister which has a cache), determines whether we later call Create() or Update() - canaryIngress, err := r.cfg.IngressWrapper.GetCached(r.cfg.Rollout.Namespace, canaryIngressName) - - canaryIngressExists := true - if err != nil { - if !k8serrors.IsNotFound(err) { - // An error other than "not found" occurred - r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error retrieving canary ingress") - return fmt.Errorf("error retrieving canary ingress `%s` from cache: %v", canaryIngressName, err) + // Set weight for additional ingresses if present + if ingresses := r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses; ingresses != nil { + // Fail out if there is an issue setting weight on additional ingresesses. + // Fundamental assumption is that each additional Ingress is equal in importance + // as primary Ingress resource. + if err := r.SetWeightPerIngress(desiredWeight, ingresses); err != nil { + return err } - r.log.WithField(logutil.IngressKey, canaryIngressName).Infof("canary ingress not found") - canaryIngressExists = false - } - // Construct the desired canary Ingress resource - desiredCanaryIngress, err := r.canaryIngress(stableIngress, canaryIngressName, desiredWeight) - if err != nil { - r.log.WithField(logutil.IngressKey, canaryIngressName).Error(err.Error()) - return err } - if !canaryIngressExists { - r.cfg.Recorder.Eventf(r.cfg.Rollout, record.EventOptions{EventReason: "CreatingCanaryIngress"}, "Creating canary ingress `%s` with weight `%d`", canaryIngressName, desiredWeight) - _, err = r.cfg.IngressWrapper.Create(ctx, r.cfg.Rollout.Namespace, desiredCanaryIngress, metav1.CreateOptions{}) - if err == nil { - return nil + return r.SetWeightPerIngress(desiredWeight, []string{r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress}) +} + +// SetWeightMultiIngress modifies each Nginx Ingress resource to reach desired state in the scenario of a rollout +// having multiple Ngnix Ingress resources. +func (r *Reconciler) SetWeightPerIngress(desiredWeight int32, ingresses []string) error { + for _, ingress := range ingresses { + ctx := context.TODO() + stableIngressName := ingress + canaryIngressName := ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), stableIngressName) + + // Check if stable ingress exists (from lister, which has a cache), error if it does not + stableIngress, err := r.cfg.IngressWrapper.GetCached(r.cfg.Rollout.Namespace, stableIngressName) + if err != nil { + r.log.WithField(logutil.IngressKey, stableIngressName).WithField("err", err.Error()).Error("error retrieving stableIngress") + return fmt.Errorf("error retrieving stableIngress `%s` from cache: %v", stableIngressName, err) } - if !k8serrors.IsAlreadyExists(err) { - r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error creating canary ingress") - return fmt.Errorf("error creating canary ingress `%s`: %v", canaryIngressName, err) + // Check if canary ingress exists (from lister which has a cache), determines whether we later call Create() or Update() + canaryIngress, err := r.cfg.IngressWrapper.GetCached(r.cfg.Rollout.Namespace, canaryIngressName) + + canaryIngressExists := true + if err != nil { + if !k8serrors.IsNotFound(err) { + // An error other than "not found" occurred + r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error retrieving canary ingress") + return fmt.Errorf("error retrieving canary ingress `%s` from cache: %v", canaryIngressName, err) + } + r.log.WithField(logutil.IngressKey, canaryIngressName).Infof("canary ingress not found") + canaryIngressExists = false } - // Canary ingress was created by a different reconcile call before this one could complete (race) - // This means we just read it from the API now (instead of cache) and continue with the normal - // flow we take when the canary already existed. - canaryIngress, err = r.cfg.IngressWrapper.Get(ctx, r.cfg.Rollout.Namespace, canaryIngressName, metav1.GetOptions{}) + + // Construct the desired canary Ingress resource + desiredCanaryIngress, err := r.canaryIngress(stableIngress, canaryIngressName, desiredWeight) if err != nil { r.log.WithField(logutil.IngressKey, canaryIngressName).Error(err.Error()) - return fmt.Errorf("error retrieving canary ingress `%s` from api: %v", canaryIngressName, err) + return err + } + + if !canaryIngressExists { + r.cfg.Recorder.Eventf(r.cfg.Rollout, record.EventOptions{EventReason: "CreatingCanaryIngress"}, "Creating canary ingress `%s` with weight `%d`", canaryIngressName, desiredWeight) + _, err = r.cfg.IngressWrapper.Create(ctx, r.cfg.Rollout.Namespace, desiredCanaryIngress, metav1.CreateOptions{}) + if err == nil { + return nil + } + if !k8serrors.IsAlreadyExists(err) { + r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error creating canary ingress") + return fmt.Errorf("error creating canary ingress `%s`: %v", canaryIngressName, err) + } + // Canary ingress was created by a different reconcile call before this one could complete (race) + // This means we just read it from the API now (instead of cache) and continue with the normal + // flow we take when the canary already existed. + canaryIngress, err = r.cfg.IngressWrapper.Get(ctx, r.cfg.Rollout.Namespace, canaryIngressName, metav1.GetOptions{}) + if err != nil { + r.log.WithField(logutil.IngressKey, canaryIngressName).Error(err.Error()) + return fmt.Errorf("error retrieving canary ingress `%s` from api: %v", canaryIngressName, err) + } } - } - // Canary Ingress already exists, apply a patch if needed + // Canary Ingress already exists, apply a patch if needed - // Only modify canaryIngress if it is controlled by this Rollout - if !metav1.IsControlledBy(canaryIngress.GetObjectMeta(), r.cfg.Rollout) { - r.log.WithField(logutil.IngressKey, canaryIngressName).Error("canary ingress controlled by different object") - return fmt.Errorf("canary ingress `%s` controlled by different object", canaryIngressName) - } + // Only modify canaryIngress if it is controlled by this Rollout + if !metav1.IsControlledBy(canaryIngress.GetObjectMeta(), r.cfg.Rollout) { + r.log.WithField(logutil.IngressKey, canaryIngressName).Error("canary ingress controlled by different object") + return fmt.Errorf("canary ingress `%s` controlled by different object", canaryIngressName) + } - // Make patches - patch, modified, err := ingressutil.BuildIngressPatch(canaryIngress.Mode(), canaryIngress, - desiredCanaryIngress, ingressutil.WithAnnotations(), ingressutil.WithLabels(), ingressutil.WithSpec()) + // Make patches + patch, modified, err := ingressutil.BuildIngressPatch(canaryIngress.Mode(), canaryIngress, + desiredCanaryIngress, ingressutil.WithAnnotations(), ingressutil.WithLabels(), ingressutil.WithSpec()) - if err != nil { - r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error constructing canary ingress patch") - return fmt.Errorf("error constructing canary ingress patch for `%s`: %v", canaryIngressName, err) - } - if !modified { - r.log.WithField(logutil.IngressKey, canaryIngressName).Info("No changes to canary ingress - skipping patch") - return nil - } + if err != nil { + r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error constructing canary ingress patch") + return fmt.Errorf("error constructing canary ingress patch for `%s`: %v", canaryIngressName, err) + } + if !modified { + r.log.WithField(logutil.IngressKey, canaryIngressName).Info("No changes to canary ingress - skipping patch") + return nil + } - r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("patch", string(patch)).Debug("applying canary Ingress patch") - r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("desiredWeight", desiredWeight).Info("updating canary Ingress") - r.cfg.Recorder.Eventf(r.cfg.Rollout, record.EventOptions{EventReason: "PatchingCanaryIngress"}, "Updating Ingress `%s` to desiredWeight '%d'", canaryIngressName, desiredWeight) + r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("patch", string(patch)).Debug("applying canary Ingress patch") + r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("desiredWeight", desiredWeight).Info("updating canary Ingress") + r.cfg.Recorder.Eventf(r.cfg.Rollout, record.EventOptions{EventReason: "PatchingCanaryIngress"}, "Updating Ingress `%s` to desiredWeight '%d'", canaryIngressName, desiredWeight) - _, err = r.cfg.IngressWrapper.Patch(ctx, r.cfg.Rollout.Namespace, canaryIngressName, types.MergePatchType, patch, metav1.PatchOptions{}) - if err != nil { - r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error patching canary ingress") - return fmt.Errorf("error patching canary ingress `%s`: %v", canaryIngressName, err) + _, err = r.cfg.IngressWrapper.Patch(ctx, r.cfg.Rollout.Namespace, canaryIngressName, types.MergePatchType, patch, metav1.PatchOptions{}) + if err != nil { + r.log.WithField(logutil.IngressKey, canaryIngressName).WithField("err", err.Error()).Error("error patching canary ingress") + return fmt.Errorf("error patching canary ingress `%s`: %v", canaryIngressName, err) + } } - return nil } diff --git a/rollout/trafficrouting/nginx/nginx_test.go b/rollout/trafficrouting/nginx/nginx_test.go index a8d3517d8d..7aca48c9d5 100644 --- a/rollout/trafficrouting/nginx/nginx_test.go +++ b/rollout/trafficrouting/nginx/nginx_test.go @@ -137,6 +137,12 @@ func fakeRollout(stableSvc, canarySvc, stableIng string) *v1alpha1.Rollout { } } +func fakeRolloutWithMultiIngress(stableSvc, canarySvc, stableIng, addStableIng string) *v1alpha1.Rollout { + rollout := fakeRollout(stableSvc, canarySvc, stableIng) + rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses = []string{addStableIng} + return rollout +} + func checkBackendService(t *testing.T, ing *ingressutil.Ingress, serviceName string) { t.Helper() switch ing.Mode() { @@ -188,7 +194,7 @@ func TestCanaryIngressCreate(t *testing.T) { stableIngress.Spec.IngressClassName = pointer.StringPtr("nginx-ext") i := ingressutil.NewLegacyIngress(stableIngress) - desiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout), 10) + desiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 10) assert.Nil(t, err, "No error returned when calling canaryIngress") checkBackendService(t, desiredCanaryIngress, "canary-service") @@ -203,6 +209,45 @@ func TestCanaryIngressCreate(t *testing.T) { assert.Equal(t, "nginx-ext", *desired.Spec.IngressClassName) } +func TestCanaryIngressCreateMultiIngress(t *testing.T) { + r := Reconciler{ + cfg: ReconcilerConfig{ + Rollout: fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress"), + }, + } + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + stableIngress.Spec.IngressClassName = pointer.StringPtr("nginx-ext") + i := ingressutil.NewLegacyIngress(stableIngress) + + desiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 10) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, desiredCanaryIngress, "canary-service") + desired, err := desiredCanaryIngress.GetExtensionsIngress() + if err != nil { + t.Error(err) + t.FailNow() + } + assert.Equal(t, "true", desired.Annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "10", desired.Annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.NotNil(t, desired.Spec.IngressClassName) + assert.Equal(t, "nginx-ext", *desired.Spec.IngressClassName) + + additionalDesiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses[0]), 10) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, additionalDesiredCanaryIngress, "canary-service") + desired, err = additionalDesiredCanaryIngress.GetExtensionsIngress() + if err != nil { + t.Error(err) + t.FailNow() + } + assert.Equal(t, "true", desired.Annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "10", desired.Annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.NotNil(t, desired.Spec.IngressClassName) + assert.Equal(t, "nginx-ext", *desired.Spec.IngressClassName) +} + func TestCanaryIngressPatchWeight(t *testing.T) { r := Reconciler{ cfg: ReconcilerConfig{ @@ -218,7 +263,7 @@ func TestCanaryIngressPatchWeight(t *testing.T) { stableIngress := ingressutil.NewLegacyIngress(stable) canaryIngress := ingressutil.NewLegacyIngress(canary) - desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout), 15) + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) assert.Nil(t, err, "No error returned when calling canaryIngress") checkBackendService(t, desiredCanaryIngress, "canary-service") @@ -230,6 +275,53 @@ func TestCanaryIngressPatchWeight(t *testing.T) { assert.Equal(t, "{\"metadata\":{\"annotations\":{\"nginx.ingress.kubernetes.io/canary-weight\":\"15\"}}}", string(patch), "compareCanaryIngresses returns expected patch") } +func TestCanaryIngressPatchWeightMultiIngress(t *testing.T) { + r := Reconciler{ + cfg: ReconcilerConfig{ + Rollout: fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress"), + }, + } + stable := extensionsIngress("stable-ingress", 80, "stable-service") + canary := extensionsIngress("canary-ingress", 80, "canary-service") + canary.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "10", + }) + stableIngress := ingressutil.NewLegacyIngress(stable) + canaryIngress := ingressutil.NewLegacyIngress(canary) + + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, desiredCanaryIngress, "canary-service") + + patch, modified, err := ingressutil.BuildIngressPatch(canaryIngress.Mode(), canaryIngress, desiredCanaryIngress, + ingressutil.WithAnnotations(), ingressutil.WithLabels(), ingressutil.WithSpec()) + assert.Nil(t, err, "compareCanaryIngresses returns no error") + assert.True(t, modified, "compareCanaryIngresses returns modified=true") + assert.Equal(t, "{\"metadata\":{\"annotations\":{\"nginx.ingress.kubernetes.io/canary-weight\":\"15\"}}}", string(patch), "compareCanaryIngresses returns expected patch") + + addStable := extensionsIngress("additional-stable-ingress", 80, "stable-service") + addCanary := extensionsIngress("additional-canary-ingress", 80, "canary-service") + addCanary.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "10", + }) + addStableIngress := ingressutil.NewLegacyIngress(addStable) + addCanaryIngress := ingressutil.NewLegacyIngress(addCanary) + + addDesiredCanaryIngress, err := r.canaryIngress(addStableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses[0]), 15) + assert.Nil(t, err, "No error returned when calling addCanaryIngress") + + checkBackendService(t, addDesiredCanaryIngress, "canary-service") + + patch, modified, err = ingressutil.BuildIngressPatch(addCanaryIngress.Mode(), addCanaryIngress, addDesiredCanaryIngress, + ingressutil.WithAnnotations(), ingressutil.WithLabels(), ingressutil.WithSpec()) + assert.Nil(t, err, "compareCanaryIngresses returns no error") + assert.True(t, modified, "compareCanaryIngresses returns modified=true") + assert.Equal(t, "{\"metadata\":{\"annotations\":{\"nginx.ingress.kubernetes.io/canary-weight\":\"15\"}}}", string(patch), "compareCanaryIngresses returns expected patch") +} + func TestCanaryIngressUpdatedRoute(t *testing.T) { r := Reconciler{ cfg: ReconcilerConfig{ @@ -246,7 +338,36 @@ func TestCanaryIngressUpdatedRoute(t *testing.T) { stableIngress := ingressutil.NewLegacyIngress(stable) canaryIngress := ingressutil.NewLegacyIngress(canary) - desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout), 15) + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, desiredCanaryIngress, "canary-service") + + patch, modified, err := ingressutil.BuildIngressPatch(canaryIngress.Mode(), canaryIngress, desiredCanaryIngress, + ingressutil.WithAnnotations(), ingressutil.WithLabels(), ingressutil.WithSpec()) + assert.Nil(t, err, "compareCanaryIngresses returns no error") + assert.True(t, modified, "compareCanaryIngresses returns modified=true") + assert.Equal(t, "{\"spec\":{\"rules\":[{\"host\":\"fakehost.example.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"canary-service\",\"servicePort\":80},\"path\":\"/bar\"}]}}]}}", string(patch), "compareCanaryIngresses returns expected patch") +} + +func TestCanaryIngressUpdatedRouteMultiIngress(t *testing.T) { + r := Reconciler{ + cfg: ReconcilerConfig{ + Rollout: fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress"), + }, + } + + stable := extensionsIngress("stable-ingress", 80, "stable-service") + stable.Spec.Rules[0].HTTP.Paths[0].Path = "/bar" + canary := extensionsIngress("canary-ingress", 80, "canary-service") + canary.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + stableIngress := ingressutil.NewLegacyIngress(stable) + canaryIngress := ingressutil.NewLegacyIngress(canary) + + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) assert.Nil(t, err, "No error returned when calling canaryIngress") checkBackendService(t, desiredCanaryIngress, "canary-service") @@ -256,6 +377,27 @@ func TestCanaryIngressUpdatedRoute(t *testing.T) { assert.Nil(t, err, "compareCanaryIngresses returns no error") assert.True(t, modified, "compareCanaryIngresses returns modified=true") assert.Equal(t, "{\"spec\":{\"rules\":[{\"host\":\"fakehost.example.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"canary-service\",\"servicePort\":80},\"path\":\"/bar\"}]}}]}}", string(patch), "compareCanaryIngresses returns expected patch") + + addStable := extensionsIngress("stable-ingress", 80, "stable-service") + addStable.Spec.Rules[0].HTTP.Paths[0].Path = "/bar" + addCanary := extensionsIngress("canary-ingress", 80, "canary-service") + addCanary.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + addStableIngress := ingressutil.NewLegacyIngress(addStable) + addCanaryIngress := ingressutil.NewLegacyIngress(addCanary) + + addDesiredCanaryIngress, err := r.canaryIngress(addStableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses[0]), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, addDesiredCanaryIngress, "canary-service") + + patch, modified, err = ingressutil.BuildIngressPatch(addCanaryIngress.Mode(), addCanaryIngress, addDesiredCanaryIngress, + ingressutil.WithAnnotations(), ingressutil.WithLabels(), ingressutil.WithSpec()) + assert.Nil(t, err, "compareCanaryIngresses returns no error") + assert.True(t, modified, "compareCanaryIngresses returns modified=true") + assert.Equal(t, "{\"spec\":{\"rules\":[{\"host\":\"fakehost.example.com\",\"http\":{\"paths\":[{\"backend\":{\"serviceName\":\"canary-service\",\"servicePort\":80},\"path\":\"/bar\"}]}}]}}", string(patch), "compareCanaryIngresses returns expected patch") } func TestCanaryIngressRetainIngressClass(t *testing.T) { @@ -270,7 +412,7 @@ func TestCanaryIngressRetainIngressClass(t *testing.T) { }) stableIngress := ingressutil.NewLegacyIngress(stable) - desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout), 15) + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) assert.Nil(t, err, "No error returned when calling canaryIngress") checkBackendService(t, desiredCanaryIngress, "canary-service") @@ -281,6 +423,45 @@ func TestCanaryIngressRetainIngressClass(t *testing.T) { assert.Equal(t, "nginx-foo", annotations["kubernetes.io/ingress.class"], "ingress-class annotation retained") } +func TestCanaryIngressRetainIngressClassMultiIngress(t *testing.T) { + r := Reconciler{ + cfg: ReconcilerConfig{ + Rollout: fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress"), + }, + } + stable := extensionsIngress("stable-ingress", 80, "stable-service") + stable.SetAnnotations(map[string]string{ + "kubernetes.io/ingress.class": "nginx-foo", + }) + stableIngress := ingressutil.NewLegacyIngress(stable) + + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, desiredCanaryIngress, "canary-service") + + annotations := desiredCanaryIngress.GetAnnotations() + assert.Equal(t, "true", annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "15", annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.Equal(t, "nginx-foo", annotations["kubernetes.io/ingress.class"], "ingress-class annotation retained") + + addStable := extensionsIngress("stable-ingress", 80, "stable-service") + addStable.SetAnnotations(map[string]string{ + "kubernetes.io/ingress.class": "nginx-foo", + }) + addStableIngress := ingressutil.NewLegacyIngress(addStable) + + addDesiredCanaryIngress, err := r.canaryIngress(addStableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses[0]), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, addDesiredCanaryIngress, "canary-service") + + annotations = addDesiredCanaryIngress.GetAnnotations() + assert.Equal(t, "true", annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "15", annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.Equal(t, "nginx-foo", annotations["kubernetes.io/ingress.class"], "ingress-class annotation retained") +} + func TestCanaryIngressAdditionalAnnotations(t *testing.T) { r := Reconciler{ cfg: ReconcilerConfig{ @@ -294,7 +475,33 @@ func TestCanaryIngressAdditionalAnnotations(t *testing.T) { stable := extensionsIngress("stable-ingress", 80, "stable-service") stableIngress := ingressutil.NewLegacyIngress(stable) - desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout), 15) + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, desiredCanaryIngress, "canary-service") + + annotations := desiredCanaryIngress.GetAnnotations() + assert.Equal(t, "true", annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "15", annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.Equal(t, "X-Foo", annotations["nginx.ingress.kubernetes.io/canary-by-header"], "canary-by-header annotation set") + assert.Equal(t, "DoCanary", annotations["nginx.ingress.kubernetes.io/canary-by-header-value"], "canary-by-header-value annotation set") +} + +func TestCanaryIngressAdditionalAnnotationsMultiIngress(t *testing.T) { + r := Reconciler{ + cfg: ReconcilerConfig{ + Rollout: fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress"), + }, + } + r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalIngressAnnotations = map[string]string{ + "canary-by-header": "X-Foo", + "canary-by-header-value": "DoCanary", + } + + stable := extensionsIngress("stable-ingress", 80, "stable-service") + stableIngress := ingressutil.NewLegacyIngress(stable) + + desiredCanaryIngress, err := r.canaryIngress(stableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 15) assert.Nil(t, err, "No error returned when calling canaryIngress") checkBackendService(t, desiredCanaryIngress, "canary-service") @@ -304,6 +511,20 @@ func TestCanaryIngressAdditionalAnnotations(t *testing.T) { assert.Equal(t, "15", annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") assert.Equal(t, "X-Foo", annotations["nginx.ingress.kubernetes.io/canary-by-header"], "canary-by-header annotation set") assert.Equal(t, "DoCanary", annotations["nginx.ingress.kubernetes.io/canary-by-header-value"], "canary-by-header-value annotation set") + + addStable := extensionsIngress("additional-stable-ingress", 80, "stable-service") + addStableIngress := ingressutil.NewLegacyIngress(addStable) + + addDesiredCanaryIngress, err := r.canaryIngress(addStableIngress, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses[0]), 15) + assert.Nil(t, err, "No error returned when calling canaryIngress") + + checkBackendService(t, addDesiredCanaryIngress, "canary-service") + + annotations = addDesiredCanaryIngress.GetAnnotations() + assert.Equal(t, "true", annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "15", annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.Equal(t, "X-Foo", annotations["nginx.ingress.kubernetes.io/canary-by-header"], "canary-by-header annotation set") + assert.Equal(t, "DoCanary", annotations["nginx.ingress.kubernetes.io/canary-by-header-value"], "canary-by-header-value annotation set") } func TestReconciler_canaryIngress(t *testing.T) { @@ -320,7 +541,38 @@ func TestReconciler_canaryIngress(t *testing.T) { i := ingressutil.NewIngress(stableIngress) // when - desiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout), 10) + desiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 10) + + // then + assert.Nil(t, err, "No error returned when calling canaryIngress") + checkBackendService(t, desiredCanaryIngress, "canary-service") + desired, err := desiredCanaryIngress.GetNetworkingIngress() + if err != nil { + t.Fatal(err) + } + assert.Equal(t, "true", desired.Annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "10", desired.Annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.NotNil(t, desired.Spec.IngressClassName) + assert.Equal(t, "nginx-ext", *desired.Spec.IngressClassName) + }) +} + +func TestReconciler_canaryIngressWithMultiIngress(t *testing.T) { + t.Run("will build desired networking ingress successfully", func(t *testing.T) { + // given + t.Parallel() + r := Reconciler{ + cfg: ReconcilerConfig{ + Rollout: fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress"), + }, + } + + stableIngress := networkingIngress("stable-ingress", 80, "stable-service") + stableIngress.Spec.IngressClassName = pointer.StringPtr("nginx-ext") + i := ingressutil.NewIngress(stableIngress) + + // when + desiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), 10) // then assert.Nil(t, err, "No error returned when calling canaryIngress") @@ -333,6 +585,25 @@ func TestReconciler_canaryIngress(t *testing.T) { assert.Equal(t, "10", desired.Annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") assert.NotNil(t, desired.Spec.IngressClassName) assert.Equal(t, "nginx-ext", *desired.Spec.IngressClassName) + + addStableIngress := networkingIngress("additional-stable-ingress", 80, "stable-service") + addStableIngress.Spec.IngressClassName = pointer.StringPtr("nginx-ext") + i = ingressutil.NewIngress(addStableIngress) + + // when + addDesiredCanaryIngress, err := r.canaryIngress(i, ingressutil.GetCanaryIngressName(r.cfg.Rollout.GetName(), r.cfg.Rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses[0]), 10) + + // then + assert.Nil(t, err, "No error returned when calling canaryIngress") + checkBackendService(t, desiredCanaryIngress, "canary-service") + desired, err = addDesiredCanaryIngress.GetNetworkingIngress() + if err != nil { + t.Fatal(err) + } + assert.Equal(t, "true", desired.Annotations["nginx.ingress.kubernetes.io/canary"], "canary annotation set to true") + assert.Equal(t, "10", desired.Annotations["nginx.ingress.kubernetes.io/canary-weight"], "canary-weight annotation set to expected value") + assert.NotNil(t, desired.Spec.IngressClassName) + assert.Equal(t, "nginx-ext", *desired.Spec.IngressClassName) }) } @@ -398,6 +669,41 @@ func TestReconcileStableIngressFound(t *testing.T) { } } +func TestReconcileStableIngressFoundMultiIngress(t *testing.T) { + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + addStableIngress := extensionsIngress("additional-stable-ingress", 80, "stable-service") + + client := fake.NewSimpleClientset() + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + err = r.SetWeight(10) + assert.Nil(t, err, "Reconcile returns no error") + actions := client.Actions() + assert.Len(t, actions, 2) + if !t.Failed() { + // Avoid "index out of range" errors + assert.Equal(t, "create", actions[0].GetVerb(), "action: create canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[0].GetResource(), "action: create canary ingress") + + assert.Equal(t, "create", actions[1].GetVerb(), "action: create canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[1].GetResource(), "action: create canary ingress") + } +} + func TestReconcileStableIngressFoundWrongBackend(t *testing.T) { rollout := fakeRollout("stable-service", "canary-service", "stable-ingress") stableIngress := extensionsIngress("stable-ingress", 80, "other-service") @@ -422,6 +728,34 @@ func TestReconcileStableIngressFoundWrongBackend(t *testing.T) { assert.Contains(t, err.Error(), "has no rules using service", "correct error is returned") } +func TestReconcileStableIngressFoundWrongBackendMultiIngress(t *testing.T) { + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + // this one will work + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + // This is the one that should error out + addStableIngress := extensionsIngress("additional-stable-ingress", 80, "other-service") + + client := fake.NewSimpleClientset() + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + err = r.SetWeight(10) + assert.NotNil(t, err, "Reconcile returns error") + assert.Contains(t, err.Error(), "has no rules using service", "correct error is returned") +} + func TestReconcileStableAndCanaryIngressFoundNoOwner(t *testing.T) { rollout := fakeRollout("stable-service", "canary-service", "stable-ingress") stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") @@ -511,6 +845,54 @@ func TestReconcileStableAndCanaryIngressFoundPatch(t *testing.T) { } } +func TestReconcileStableAndCanaryIngressFoundPatchMultiIngress(t *testing.T) { + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + addStableIngress := extensionsIngress("additional-stable-ingress", 80, "stable-service") + canaryIngress := extensionsIngress("rollout-stable-ingress-canary", 80, "canary-service") + canaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + addCanaryIngress := extensionsIngress("rollout-additional-stable-ingress-canary", 80, "canary-service") + addCanaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + setIngressOwnerRef(canaryIngress, rollout) + setIngressOwnerRef(addCanaryIngress, rollout) + client := fake.NewSimpleClientset(canaryIngress, addCanaryIngress) + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(canaryIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addCanaryIngress) + + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + err = r.SetWeight(10) + assert.Nil(t, err, "Reconcile returns no error") + actions := client.Actions() + assert.Len(t, actions, 2) + if !t.Failed() { + // Avoid "index out of range" errors + assert.Equal(t, "patch", actions[0].GetVerb(), "action: patch canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[0].GetResource(), "action: patch canary ingress") + assert.Equal(t, "patch", actions[1].GetVerb(), "action: patch canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[1].GetResource(), "action: patch canary ingress") + } +} + func TestReconcileWillInvokeNetworkingIngress(t *testing.T) { // given rollout := fakeRollout("stable-service", "canary-service", "stable-ingress") @@ -551,6 +933,57 @@ func TestReconcileWillInvokeNetworkingIngress(t *testing.T) { } } +func TestReconcileWillInvokeNetworkingIngressMultiIngress(t *testing.T) { + // given + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + stableIngress := networkingIngress("stable-ingress", 80, "stable-service") + addStableIngress := networkingIngress("additional-stable-ingress", 80, "stable-service") + canaryIngress := networkingIngress("rollout-stable-ingress-canary", 80, "canary-service") + canaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + addCanaryIngress := networkingIngress("rollout-additional-stable-ingress-canary", 80, "canary-service") + addCanaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + canaryIngress.SetOwnerReferences([]metav1.OwnerReference{*metav1.NewControllerRef(rollout, schema.GroupVersionKind{Group: "argoproj.io", Version: "v1alpha1", Kind: "Rollout"})}) + addCanaryIngress.SetOwnerReferences([]metav1.OwnerReference{*metav1.NewControllerRef(rollout, schema.GroupVersionKind{Group: "argoproj.io", Version: "v1alpha1", Kind: "Rollout"})}) + client := fake.NewSimpleClientset(stableIngress, canaryIngress, addStableIngress, addCanaryIngress) + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + k8sI.Networking().V1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Networking().V1().Ingresses().Informer().GetIndexer().Add(canaryIngress) + k8sI.Networking().V1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + k8sI.Networking().V1().Ingresses().Informer().GetIndexer().Add(addCanaryIngress) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeNetworking, client, k8sI) + if err != nil { + t.Fatal(err) + } + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + // when + err = r.SetWeight(10) + + // then + assert.Nil(t, err, "Reconcile returns no error") + actions := client.Actions() + assert.Len(t, actions, 2) + if !t.Failed() { + // Avoid "index out of range" errors + assert.Equal(t, "patch", actions[0].GetVerb(), "action: patch canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "networking.k8s.io", Version: "v1", Resource: "ingresses"}, actions[0].GetResource(), "action: patch canary ingress") + assert.Equal(t, "patch", actions[1].GetVerb(), "action: patch canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "networking.k8s.io", Version: "v1", Resource: "ingresses"}, actions[1].GetResource(), "action: patch canary ingress") + } +} + func TestReconcileStableAndCanaryIngressFoundNoChange(t *testing.T) { rollout := fakeRollout("stable-service", "canary-service", "stable-ingress") stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") @@ -582,6 +1015,46 @@ func TestReconcileStableAndCanaryIngressFoundNoChange(t *testing.T) { assert.Len(t, actions, 0) } +func TestReconcileStableAndCanaryIngressFoundNoChangeMultiIngress(t *testing.T) { + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + addStableIngress := extensionsIngress("additional-stable-ingress", 80, "stable-service") + canaryIngress := extensionsIngress("rollout-stable-ingress-canary", 80, "canary-service") + addCanaryIngress := extensionsIngress("rollout-additional-stable-ingress-canary", 80, "canary-service") + setIngressOwnerRef(canaryIngress, rollout) + setIngressOwnerRef(addCanaryIngress, rollout) + canaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "10", + }) + addCanaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "10", + }) + client := fake.NewSimpleClientset() + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(canaryIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addCanaryIngress) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + err = r.SetWeight(10) + assert.Nil(t, err, "Reconcile returns no error") + actions := client.Actions() + assert.Len(t, actions, 0) +} + func TestReconcileCanaryCreateError(t *testing.T) { rollout := fakeRollout("stable-service", "canary-service", "stable-ingress") stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") @@ -622,6 +1095,48 @@ func TestReconcileCanaryCreateError(t *testing.T) { } } +func TestReconcileCanaryCreateErrorMultiIngress(t *testing.T) { + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + addStableIngress := extensionsIngress("additional-stable-ingress", 80, "stable-service") + + client := fake.NewSimpleClientset() + client.ReactionChain = nil + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + + // stableIngress exists + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + // Return with AlreadyExists error to create for canary + r.cfg.Client.(*fake.Clientset).Fake.AddReactor("create", "ingresses", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, errors.New("fake error") + }) + + err = r.SetWeight(10) + assert.NotNil(t, err, "Reconcile returns error") + assert.Equal(t, "error creating canary ingress `rollout-additional-stable-ingress-canary`: fake error", err.Error()) + actions := client.Actions() + assert.Len(t, actions, 1) + if !t.Failed() { + // Avoid "index out of range" errors + assert.Equal(t, "create", actions[0].GetVerb(), "action: create canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[0].GetResource(), "action: create canary ingress") + } +} + func TestReconcileCanaryCreateErrorAlreadyExistsPatch(t *testing.T) { rollout := fakeRollout("stable-service", "canary-service", "stable-ingress") stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") @@ -717,3 +1232,76 @@ func TestSetMirrorRoute(t *testing.T) { err = r.RemoveManagedRoutes() assert.Nil(t, err) } + +func TestReconcileCanaryCreateErrorAlreadyExistsPatchMultiIngress(t *testing.T) { + rollout := fakeRolloutWithMultiIngress("stable-service", "canary-service", "stable-ingress", "additional-stable-ingress") + stableIngress := extensionsIngress("stable-ingress", 80, "stable-service") + addStableIngress := extensionsIngress("additional-stable-ingress", 80, "stable-service") + canaryIngress := extensionsIngress("rollout-stable-ingress-canary", 80, "canary-service") + addCanaryIngress := extensionsIngress("rollout-additional-stable-ingress-canary", 80, "canary-service") + canaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + addCanaryIngress.SetAnnotations(map[string]string{ + "nginx.ingress.kubernetes.io/canary": "true", + "nginx.ingress.kubernetes.io/canary-weight": "15", + }) + setIngressOwnerRef(canaryIngress, rollout) + setIngressOwnerRef(addCanaryIngress, rollout) + + client := fake.NewSimpleClientset() + client.ReactionChain = nil + k8sI := kubeinformers.NewSharedInformerFactory(client, 0) + + // stableIngress exists + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(stableIngress) + k8sI.Extensions().V1beta1().Ingresses().Informer().GetIndexer().Add(addStableIngress) + ingressWrapper, err := ingressutil.NewIngressWrapper(ingressutil.IngressModeExtensions, client, k8sI) + if err != nil { + t.Fatal(err) + } + + r := NewReconciler(ReconcilerConfig{ + Rollout: rollout, + Client: client, + Recorder: record.NewFakeEventRecorder(), + ControllerKind: schema.GroupVersionKind{Group: "foo", Version: "v1", Kind: "Bar"}, + IngressWrapper: ingressWrapper, + }) + + // Return with AlreadyExists error to create for canary + r.cfg.Client.(*fake.Clientset).Fake.AddReactor("create", "ingresses", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, k8serrors.NewAlreadyExists(schema.GroupResource{ + Group: "extensions", + Resource: "ingresses", + }, "rollout-stable-ingress-canary") + }) + + // Respond with canaryIngress on GET + r.cfg.Client.(*fake.Clientset).Fake.AddReactor("get", "ingresses", func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) { + return true, canaryIngress, nil + }) + + err = r.SetWeight(10) + assert.Nil(t, err, "Reconcile returns no error") + actions := client.Actions() + assert.Len(t, actions, 6) + if !t.Failed() { + // Avoid "index out of range" errors + // primary ingress + assert.Equal(t, "create", actions[0].GetVerb(), "action: create canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[0].GetResource(), "action: create canary ingress") + assert.Equal(t, "get", actions[1].GetVerb(), "action: get canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[1].GetResource(), "action: get canary ingress") + assert.Equal(t, "patch", actions[2].GetVerb(), "action: patch canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[2].GetResource(), "action: patch canary ingress") + // additional ingress + assert.Equal(t, "create", actions[3].GetVerb(), "action: create canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[0].GetResource(), "action: create canary ingress") + assert.Equal(t, "get", actions[4].GetVerb(), "action: get canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[1].GetResource(), "action: get canary ingress") + assert.Equal(t, "patch", actions[5].GetVerb(), "action: patch canary ingress") + assert.Equal(t, schema.GroupVersionResource{Group: "extensions", Version: "v1beta1", Resource: "ingresses"}, actions[2].GetResource(), "action: patch canary ingress") + } +} diff --git a/ui/src/models/rollout/generated/api.ts b/ui/src/models/rollout/generated/api.ts index 1ff2433d07..6416533dda 100644 --- a/ui/src/models/rollout/generated/api.ts +++ b/ui/src/models/rollout/generated/api.ts @@ -810,6 +810,12 @@ export interface GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1NginxTraffi * @memberof GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1NginxTrafficRouting */ additionalIngressAnnotations?: { [key: string]: string; }; + /** + * + * @type {Array} + * @memberof GithubComArgoprojArgoRolloutsPkgApisRolloutsV1alpha1NginxTrafficRouting + */ + additionalStableIngresses?: Array; } /** * diff --git a/utils/ingress/ingress.go b/utils/ingress/ingress.go index 42dc61ca20..d5bcb99d4d 100644 --- a/utils/ingress/ingress.go +++ b/utils/ingress/ingress.go @@ -62,13 +62,31 @@ func GetRolloutIngressKeys(rollout *v1alpha1.Rollout) []string { rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil && rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress != "" { + stableIngress := rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress // Also start watcher for `-canary` ingress which is created by the trafficmanagement controller ingresses = append( ingresses, - fmt.Sprintf("%s/%s", rollout.Namespace, rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress), - fmt.Sprintf("%s/%s", rollout.Namespace, GetCanaryIngressName(rollout)), + fmt.Sprintf("%s/%s", rollout.Namespace, stableIngress), + fmt.Sprintf("%s/%s", rollout.Namespace, GetCanaryIngressName(rollout.GetName(), stableIngress)), ) } + + // Scenario where one rollout is managing multiple Ngnix ingresses. + if rollout.Spec.Strategy.Canary != nil && + rollout.Spec.Strategy.Canary.TrafficRouting != nil && + rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil && + len(rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses) > 0 { + + for _, stableIngress := range rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses { + // Also start watcher for `-canary` ingress which is created by the trafficmanagement controller + ingresses = append( + ingresses, + fmt.Sprintf("%s/%s", rollout.Namespace, stableIngress), + fmt.Sprintf("%s/%s", rollout.Namespace, GetCanaryIngressName(rollout.GetName(), stableIngress)), + ) + } + } + if rollout.Spec.Strategy.Canary != nil && rollout.Spec.Strategy.Canary.TrafficRouting != nil && rollout.Spec.Strategy.Canary.TrafficRouting.ALB != nil && @@ -83,14 +101,10 @@ func GetRolloutIngressKeys(rollout *v1alpha1.Rollout) []string { } // GetCanaryIngressName constructs the name to use for the canary ingress resource from a given Rollout -func GetCanaryIngressName(rollout *v1alpha1.Rollout) string { +func GetCanaryIngressName(rolloutName, stableIngressName string) string { // names limited to 253 characters - if rollout.Spec.Strategy.Canary != nil && - rollout.Spec.Strategy.Canary.TrafficRouting != nil && - rollout.Spec.Strategy.Canary.TrafficRouting.Nginx != nil && - rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress != "" { - - prefix := fmt.Sprintf("%s-%s", rollout.GetName(), rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress) + if stableIngressName != "" { + prefix := fmt.Sprintf("%s-%s", rolloutName, stableIngressName) if len(prefix) > 253-len(CanaryIngressSuffix) { // trim prefix prefix = prefix[0 : 253-len(CanaryIngressSuffix)] diff --git a/utils/ingress/ingress_test.go b/utils/ingress/ingress_test.go index eaca7e6a86..a11fd5abc2 100644 --- a/utils/ingress/ingress_test.go +++ b/utils/ingress/ingress_test.go @@ -61,6 +61,33 @@ func TestGetRolloutIngressKeysForCanaryWithTrafficRouting(t *testing.T) { assert.ElementsMatch(t, keys, []string{"default/stable-ingress", "default/myrollout-stable-ingress-canary", "default/alb-ingress"}) } +func TestGetRolloutIngressKeysForCanaryWithTrafficRoutingMultiIngress(t *testing.T) { + keys := GetRolloutIngressKeys(&v1alpha1.Rollout{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myrollout", + Namespace: "default", + }, + Spec: v1alpha1.RolloutSpec{ + Strategy: v1alpha1.RolloutStrategy{ + Canary: &v1alpha1.CanaryStrategy{ + CanaryService: "canary-service", + StableService: "stable-service", + TrafficRouting: &v1alpha1.RolloutTrafficRouting{ + Nginx: &v1alpha1.NginxTrafficRouting{ + StableIngress: "stable-ingress", + AdditionalStableIngresses: []string{"stable-ingress-additional"}, + }, + ALB: &v1alpha1.ALBTrafficRouting{ + Ingress: "alb-ingress", + }, + }, + }, + }, + }, + }) + assert.ElementsMatch(t, keys, []string{"default/stable-ingress", "default/myrollout-stable-ingress-canary", "default/stable-ingress-additional", "default/myrollout-stable-ingress-additional-canary", "default/alb-ingress"}) +} + func TestGetCanaryIngressName(t *testing.T) { rollout := &v1alpha1.Rollout{ ObjectMeta: metav1.ObjectMeta{ @@ -74,7 +101,8 @@ func TestGetCanaryIngressName(t *testing.T) { StableService: "stable-service", TrafficRouting: &v1alpha1.RolloutTrafficRouting{ Nginx: &v1alpha1.NginxTrafficRouting{ - StableIngress: "stable-ingress", + StableIngress: "stable-ingress", + AdditionalStableIngresses: []string{"stable-ingress-additional"}, }, }, }, @@ -82,20 +110,34 @@ func TestGetCanaryIngressName(t *testing.T) { }, } - t.Run("NoTrim", func(t *testing.T) { + t.Run("StableIngress - NoTrim", func(t *testing.T) { rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress = "stable-ingress" - canaryIngress := GetCanaryIngressName(rollout) + canaryIngress := GetCanaryIngressName(rollout.GetName(), rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress) assert.Equal(t, "myrollout-stable-ingress-canary", canaryIngress) }) - t.Run("Trim", func(t *testing.T) { + t.Run("StableIngress - Trim", func(t *testing.T) { rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress = fmt.Sprintf("stable-ingress%s", strings.Repeat("a", 260)) - canaryIngress := GetCanaryIngressName(rollout) + canaryIngress := GetCanaryIngressName(rollout.GetName(), rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.StableIngress) assert.Equal(t, 253, len(canaryIngress), "canary ingress truncated to 253") assert.Equal(t, true, strings.HasSuffix(canaryIngress, "-canary"), "canary ingress has -canary suffix") }) + t.Run("AdditionalStableIngresses - NoTrim", func(t *testing.T) { + for _, ing := range rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses { + canaryIngress := GetCanaryIngressName(rollout.GetName(), ing) + assert.Equal(t, "myrollout-stable-ingress-additional-canary", canaryIngress) + } + }) + t.Run("AdditionalStableIngresses - Trim", func(t *testing.T) { + rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses = []string{fmt.Sprintf("stable-ingress%s", strings.Repeat("a", 260))} + for _, ing := range rollout.Spec.Strategy.Canary.TrafficRouting.Nginx.AdditionalStableIngresses { + canaryIngress := GetCanaryIngressName(rollout.GetName(), ing) + assert.Equal(t, 253, len(canaryIngress), "canary ingress truncated to 253") + assert.Equal(t, true, strings.HasSuffix(canaryIngress, "-canary"), "canary ingress has -canary suffix") + } + }) t.Run("NoStableIngress", func(t *testing.T) { rollout.Spec.Strategy.Canary.TrafficRouting.Nginx = nil - canaryIngress := GetCanaryIngressName(rollout) + canaryIngress := GetCanaryIngressName(rollout.GetName(), "") assert.Equal(t, "", canaryIngress, "canary ingress is empty") }) }