fix: controller panic with ephemeral containers with bad resource qty (…

…#1055)

Signed-off-by: Jesse Suen <[email protected]>

rollout/controller.go

@@ -221,7 +221,7 @@ func NewController(cfg ControllerConfig) *Controller {
 					controller.IstioController.EnqueueDestinationRule(key)
 				}
 			}
-			controller.enqueueRollout(newRollout)
+			controller.enqueueRollout(new)
 		},
 		DeleteFunc: func(obj interface{}) {
 			if ro := unstructuredutil.ObjectToRollout(obj); ro != nil {

test/e2e/expectedfailures/malformed-rollout-ephemeral.yaml

@@ -0,0 +1,36 @@
+# This example reproduces a cornercase bug where there are multiple errors in the rollout causing a panic:
+# 1. duplicated name in the ephemeralContainer
+# 2. invalid resource request in ephemeral container
+# See: https://github.com/argoproj/argo-rollouts/issues/1045
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+#apiVersion: apps/v1
+#kind: Deployment
+metadata:
+  name: malformed-rollout-ephemeral
+spec:
+  selector:
+    matchLabels:
+      app: malformed-rollout-ephemeral
+  template:
+    metadata:
+      labels:
+        app: malformed-rollout-ephemeral
+    spec:
+      containers:
+      - name: malformed-rollout-ephemeral
+        image: argoproj/rollouts-demo:blue
+        resources:
+          requests:
+            memory: invalid # invalid
+      # NOTE: kubernetes drops the ephemeral containers list completely when one fails to unmarshal
+      # (e.g. when one has an invalid resource quantity). So for better or worse, this rollout will
+      # become healthy.
+      ephemeralContainers: 
+      - name: malformed-rollout-ephemeral   # duplicated name
+        image: argoproj/rollouts-demo:blue
+        resources:
+          requests:
+            memory: invalid # invalid
+  strategy:
+    canary: {}

test/e2e/functional_test.go

@@ -260,6 +260,11 @@ func (s *FunctionalSuite) TestMalformedRollout() {
 		HealthyRollout(`@expectedfailures/malformed-rollout.yaml`)
 }
 
+func (s *FunctionalSuite) TestMalformedRolloutEphemeralCtr() {
+	s.Given().
+		HealthyRollout(`@expectedfailures/malformed-rollout-ephemeral.yaml`)
+}
+
 // TestContainerResourceFormats verifies resource requests are accepted in multiple formats and not
 // rejected by validation
 func (s *FunctionalSuite) TestContainerResourceFormats() {

utils/tolerantinformer/tolerantinformer_test.go

@@ -81,6 +81,47 @@ func TestMalformedRollout(t *testing.T) {
 	verify(list[0])
 }
 
+func TestMalformedRolloutEphemeralCtr(t *testing.T) {
+	good := testutil.ObjectFromPath("examples/rollout-canary.yaml")
+	good.SetNamespace("default")
+	bad := testutil.ObjectFromPath("test/e2e/expectedfailures/malformed-rollout-ephemeral.yaml")
+	bad.SetNamespace(dummyNamespace)
+	dynInformerFactory := newFakeDynamicInformer(good, bad)
+	informer := NewTolerantRolloutInformer(dynInformerFactory)
+
+	verify := func(ro *v1alpha1.Rollout) {
+		assert.True(t, ro.Spec.Strategy.Canary != nil)
+		assert.Len(t, ro.Spec.Template.Spec.Containers[0].Resources.Requests, 0)
+
+		// NOTE: kubernetes drops the ephemeral containers list completely when one fails to unmarshal
+		// (e.g. when one has an invalid resource quantity). The following assertion is just to detect
+		// if this assumption continues to hold true over the course of time (as we update k8s libraries)
+		assert.Len(t, ro.Spec.Template.Spec.EphemeralContainers, 0)
+		// assert.Len(t, ro.Spec.Template.Spec.EphemeralContainers[0].Resources.Requests, 0)
+	}
+
+	// test cluster scoped list
+	list, err := informer.Lister().List(labels.NewSelector())
+	assert.NoError(t, err)
+	assert.Len(t, list, 2)
+	for _, obj := range list {
+		if obj.Name == "malformed-rollout-ephemeral" {
+			verify(obj)
+		}
+	}
+
+	// test namespaced scoped get
+	obj, err := informer.Lister().Rollouts(dummyNamespace).Get("malformed-rollout-ephemeral")
+	assert.NoError(t, err)
+	verify(obj)
+
+	// test namespaced scoped list
+	list, err = informer.Lister().Rollouts(dummyNamespace).List(labels.NewSelector())
+	assert.NoError(t, err)
+	assert.Len(t, list, 1)
+	verify(list[0])
+}
+
 func verifyAnalysisSpec(t *testing.T, s interface{}) {
 	//   metrics:
 	//   - name: test

utils/unstructured/unstructured.go

@@ -3,11 +3,13 @@ package unstructured
 import (
 	"regexp"
 
-	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
 	"github.com/ghodss/yaml"
 	log "github.com/sirupsen/logrus"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
+
+	"github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1"
+	logutil "github.com/argoproj/argo-rollouts/utils/log"
 )
 
 func StrToUnstructuredUnsafe(jsonStr string) *unstructured.Unstructured {
@@ -34,14 +36,15 @@ func ObjectToRollout(obj interface{}) *v1alpha1.Rollout {
 		var ro v1alpha1.Rollout
 		err := runtime.DefaultUnstructuredConverter.FromUnstructured(un.Object, &ro)
 		if err != nil {
-			log.Warnf("Failed to convert Rollout from Unstructured object: %v", err)
+			logCtx := logutil.WithUnstructured(un)
+			logCtx.Warnf("Failed to convert Rollout from Unstructured object: %v", err)
 			return nil
 		}
 		return &ro
 	}
 	ro, ok := obj.(*v1alpha1.Rollout)
 	if !ok {
-		log.Warn("Object is neither a rollout or unstructured")
+		log.Warnf("Object is neither a rollout or unstructured: %v", obj)
 	}
 	return ro
 }