merge rallyhealth support for N Nginx Ingress controllerrs into v1.3.1

Signed-off-by: Travis Perdue <[email protected]>

.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

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()

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,15 +86,29 @@ 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 {
 		client = fake.NewSimpleClientset(rollout)
 	}
 	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"])
+}

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:

manifests/install.yaml

@@ -11817,6 +11817,10 @@ spec:
                                 additionalProperties:
                                   type: string
                                 type: object
+                              additionalStableIngresses:
+                                items:
+                                  type: string
+                                type: array
                               annotationPrefix:
                                 type: string
                               stableIngress:

manifests/namespace-install.yaml

@@ -11817,6 +11817,10 @@ spec:
                                 additionalProperties:
                                   type: string
                                 type: object
+                              additionalStableIngresses:
+                                items:
+                                  type: string
+                                type: array
                               annotationPrefix:
                                 type: string
                               stableIngress:

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"

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

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

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))

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())