Repository: argoproj/argo-rollouts

Issue:
controller: recovered from panic when `workloadRef` does not exist
<!-- If you are trying to resolve an environment-specific issue or have a one-off question about the edge case that does not require a feature then please consider asking a question in argo rollouts slack [channel](https://argoproj.github.io/community/join-slack). -->

Checklist:

* [x] I've included steps to reproduce the bug.
* [x] I've included the version of argo rollouts.

**Describe the bug**

<!-- A clear and concise description of what the bug is. -->
When using `workloadRef`, controller will end up recovering from panic if referenced workload does not exist.

```
Recovered from panic: runtime error: invalid memory address or nil pointer dereference
goroutine 436 [running]:
    runtime/debug.Stack()
    /usr/local/go/src/runtime/debug/stack.go:26 +0x5e
github.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1.1.1()
    /go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:151 +0x49
panic({0x2c7d280?, 0x51d0410?})
    /usr/local/go/src/runtime/panic.go:785 +0x132
k8s.io/kubernetes/pkg/util/labels.CloneSelectorAndAddLabel(0x0, {0x31c7be3, 0x1a}, {0xc0011090b0, 0xa})
    /go/pkg/mod/k8s.io/kubernetes@v1.29.3/pkg/util/labels/labels.go:81 +0xa3
github.com/argoproj/argo-rollouts/rollout.(*rolloutContext).createDesiredReplicaSet(0xc0013bb808)
    /go/src/github.com/argoproj/argo-rollouts/rollout/sync.go:148 +0x318
github.com/argoproj/argo-rollouts/rollout.(*Controller).newRolloutContext(0xc0006fb340, 0xc0009f3808)
    /go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:561 +0xa4c
github.com/argoproj/argo-rollouts/rollout.(*Controller).syncHandler(0xc0006fb340, {0x37d5a30, 0xc00069a820}, {0xc002950a22, 0x7})
    /go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:410 +0x3b8
github.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1.1()
    /go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:155 +0x6f
github.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1({0x37e9560, 0xc000ba0840}, {0x3194bd8, 0x7}, 0xc002b5feb0, {0x37d5a30, 0xc00069a820}, 0xc000b8b9e0, {0x2abd860, 0xc001484500})
    /go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:159 +0x38a
github.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem({0x37d5a30, 0xc00069a820}, {0x37e9560, 0xc000ba0840}, {0x3194bd8, 0x7}, 0xc002b5feb0, 0xc000b8b9e0)
    /go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:178 +0xb6
github.com/argoproj/argo-rollouts/utils/controller.RunWorker(...)
    /go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:106
github.com/argoproj/argo-rollouts/rollout.(*Controller).Run.func1()
    /go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:352 +0xa6
k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)
    /go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:226 +0x33
k8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc00144e930, {0x37a39a0, 0xc0015312f0}, 0x1, 0xc0006fecb0)
    /go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:227 +0xaf
k8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc00144e930, 0x3b9aca00, 0x0, 0x1, 0xc0006fecb0)
    /go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:204 +0x7f
k8s.io/apimachinery/pkg/util/wait.Until(...)
    /go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:161
created by github.com/argoproj/argo-rollouts/rollout.(*Controller).Run in goroutine 362
    /go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:351 +0x98
``` 

Due to the panic the `status` on the `Rollout` is also never updated. 

**To Reproduce**

<!-- A list of the steps required to reproduce the issue. Best of all, give us the URL to a repository that exhibits this issue. -->

Apply following YAML to cluster of your choice.

``` yaml
apiVersion: v1
kind: Namespace
metadata:
  name: foo
---
apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
  name: foo
  namespace: foo
spec:
  strategy:
    canary:
      steps:
        - setWeight: 50
        - pause:
            duration: "1m"
  workloadRef:
    apiVersion: apps/v1
    kind: Deployment
    name: foo
```

Monitor controller logs to observe "recover from panic" (see logs below).

Describe rollout

```bash
$ kubectl describe rollout foo   
Name:         foo
Namespace:    foo
Labels:       <none>
Annotations:  <none>
API Version:  argoproj.io/v1alpha1
Kind:         Rollout
Metadata:
  Creation Timestamp:  2025-03-17T13:43:39Z
  Generation:          1
  Resource Version:    273422247
  UID:                 0689cfe6-b2ea-4ab6-a99a-5121919868be
Spec:
  Strategy:
    Canary:
      Steps:
        Set Weight:  50
        Pause:
          Duration:  1m
  Workload Ref:
    API Version:  apps/v1
    Kind:         Deployment
    Name:         foo
Events:
  Type    Reason                  Age    From                 Message
  ----    ------                  ----   ----                 -------
  Normal  RolloutAddedToInformer  6m11s  rollouts-controller  Rollout resource added to informer: foo/foo
```

**Expected behavior**

<!-- A clear and concise description of what you expected to happen. -->
Controller should gracefully handle this situation and set appropriate status (could reuse [`InvalidSpec`](https://github.com/argoproj/argo-rollouts/blob/507dc0c233e2866d4bc76d2fbcedfb31675250f2/pkg/apis/rollouts/v1alpha1/types.go#L1137)) and message detailing the problem.

**Screenshots**

<!-- If applicable, add screenshots to help explain your problem. -->

**Version**

<!-- What version of argo rollouts controller are you running? -->
`v1.8.0`

**Logs**

```
# Paste the logs from the rollout controller

# Logs for the entire controller:
time="2025-03-17T13:50:45Z" level=info msg="Started syncing rollout" generation=1 namespace=foo resourceVersion=273422247 rollout=foo
time="2025-03-17T13:50:45Z" level=info msg="Reconciliation completed" generation=1 namespace=foo resourceVersion=273422247 rollout=foo time_ms=0.964304
time="2025-03-17T13:50:45Z" level=error msg="Recovered from panic: runtime error: invalid memory address or nil pointer dereference\ngoroutine 437 [running]:\nruntime/debug.Stack()\n\t/usr/local/go/src/runtime/debug/stack.go:26 +0x5e\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1.1.1()\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:151 +0x49\npanic({0x2c7d280?, 0x51d0410?})\n\t/usr/local/go/src/runtime/panic.go:785 +0x132\nk8s.io/kubernetes/pkg/util/labels.CloneSelectorAndAddLabel(0x0, {0x31c7be3, 0x1a}, {0xc0013c9360, 0xa})\n\t/go/pkg/mod/k8s.io/kubernetes@v1.29.3/pkg/util/labels/labels.go:81 +0xa3\ngithub.com/argoproj/argo-rollouts/rollout.(*rolloutContext).createDesiredReplicaSet(0xc00083cc08)\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/sync.go:148 +0x318\ngithub.com/argoproj/argo-rollouts/rollout.(*Controller).newRolloutContext(0xc0006fb340, 0xc0013c2308)\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:561 +0xa4c\ngithub.com/argoproj/argo-rollouts/rollout.(*Controller).syncHandler(0xc0006fb340, {0x37d5a30, 0xc00069a820}, {0xc002950a22, 0x7})\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:410 +0x3b8\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1.1()\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:155 +0x6f\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem.func1({0x37e9560, 0xc000ba0840}, {0x3194bd8, 0x7}, 0xc002d43eb0, {0x37d5a30, 0xc00069a820}, 0xc000b8b9e0, {0x2abd860, 0xc002045ec0})\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:159 +0x38a\ngithub.com/argoproj/argo-rollouts/utils/controller.processNextWorkItem({0x37d5a30, 0xc00069a820}, {0x37e9560, 0xc000ba0840}, {0x3194bd8, 0x7}, 0xc002d43eb0, 0xc000b8b9e0)\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:178 +0xb6\ngithub.com/argoproj/argo-rollouts/utils/controller.RunWorker(...)\n\t/go/src/github.com/argoproj/argo-rollouts/utils/controller/controller.go:106\ngithub.com/argoproj/argo-rollouts/rollout.(*Controller).Run.func1()\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:352 +0xa6\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1(0x30?)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:226 +0x33\nk8s.io/apimachinery/pkg/util/wait.BackoffUntil(0xc00144e960, {0x37a39a0, 0xc001531320}, 0x1, 0xc0006fecb0)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:227 +0xaf\nk8s.io/apimachinery/pkg/util/wait.JitterUntil(0xc00144e960, 0x3b9aca00, 0x0, 0x1, 0xc0006fecb0)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:204 +0x7f\nk8s.io/apimachinery/pkg/util/wait.Until(...)\n\t/go/pkg/mod/k8s.io/apimachinery@v0.29.3/pkg/util/wait/backoff.go:161\ncreated by github.com/argoproj/argo-rollouts/rollout.(*Controller).Run in goroutine 362\n\t/go/src/github.com/argoproj/argo-rollouts/rollout/controller.go:351 +0x98\n" namespace=foo rollout=foo
time="2025-03-17T13:50:45Z" level=error msg="rollout syncHandler error: Recovered from Panic" namespace=foo rollout=foo
time="2025-03-17T13:50:45Z" level=info msg="rollout syncHandler queue retries: 58 : key \"foo/foo\"" namespace=foo rollout=foo
time="2025-03-17T13:50:45Z" level=error msg="Recovered from Panic" error="<nil>"
```

---
<!-- Issue Author: Don't delete this message to encourage other users to support your issue! -->
**Message from the maintainers**:

Impacted by this bug? Give it a 👍. We prioritize the issues with the most 👍.


Discussion:
@onematchfox: This behaviour is ultimately the result of `rollout.Spec.Selector` being nil when calling [`labelsutil.CloneSelectorAndAddLabel`](https://github.com/argoproj/argo-rollouts/blob/507dc0c233e2866d4bc76d2fbcedfb31675250f2/rollout/sync.go#L148) and seems to be a result of the logic within `controller.go` where any error returned by [`c.refResolver.Resolve(r)`](https://github.com/argoproj/argo-rollouts/blob/507dc0c233e2866d4bc76d2fbcedfb31675250f2/rollout/controller.go#L411) is initially ignored (only handled later [here](https://github.com/argoproj/argo-rollouts/blob/507dc0c233e2866d4bc76d2fbcedfb31675250f2/rollout/controller.go#L428-L430))

Solution patch:
diff --git a/pkg/apis/rollouts/validation/validation.go b/pkg/apis/rollouts/validation/validation.go
index d9c31a37..bebbcb3f 100644
--- a/pkg/apis/rollouts/validation/validation.go
+++ b/pkg/apis/rollouts/validation/validation.go
@@ -228,6 +228,11 @@ func ValidateRolloutStrategyBlueGreen(rollout *v1alpha1.Rollout, fldPath *field.
 	return allErrs
 }
 
+func InvalidWorkloadRef(rollout *v1alpha1.Rollout, err error) error {
+	fldPath := field.NewPath("spec")
+	return field.Invalid(fldPath.Child("workloadRef"), rollout.Spec.WorkloadRef, err.Error())
+}
+
 // requireCanaryStableServices returns true if the rollout requires canary.stableService and
 // canary.canaryService to be defined
 func requireCanaryStableServices(rollout *v1alpha1.Rollout) bool {
diff --git a/rollout/controller.go b/rollout/controller.go
index 8a5ec674..aa51a3a2 100644
--- a/rollout/controller.go
+++ b/rollout/controller.go
@@ -419,8 +419,6 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error {
 		logCtx.WithField("time_ms", duration.Seconds()*1e3).Info("Reconciliation completed")
 	}()
 
-	resolveErr := c.refResolver.Resolve(r)
-	// We could maybe lose setting the error condition from the below if resolveErr != nil {}, and just log the error to clean up the logic
 	roCtx, err := c.newRolloutContext(r)
 	if roCtx == nil {
 		logCtx.Error("newRolloutContext returned nil")
@@ -438,15 +436,6 @@ func (c *Controller) syncHandler(ctx context.Context, key string) error {
 		logCtx.Errorf("newRolloutContext err %v", err)
 		return err
 	}
-	// We should probably delete this if block and just log the error to clean up the logic, a bigger change would be to add a new
-	// field to the status maybe (reconcileErrMsg) and store the errors there from the processNextWorkItem function in controller/controller.go
-	if resolveErr != nil {
-		err := roCtx.createInvalidRolloutCondition(resolveErr, r)
-		if err != nil {
-			return fmt.Errorf("failed to create invalid rollout condition during resolving the rollout: %w", err)
-		}
-		return fmt.Errorf("failed to resolve rollout: %w", resolveErr)
-	}
 
 	// In order to work with HPA, the rollout.Spec.Replica field cannot be nil. As a result, the controller will update
 	// the rollout to have the replicas field set to the default value. see https://github.com/argoproj/argo-rollouts/issues/119
@@ -508,6 +497,14 @@ func (c *Controller) writeBackToInformer(ro *v1alpha1.Rollout) {
 }
 
 func (c *Controller) newRolloutContext(rollout *v1alpha1.Rollout) (*rolloutContext, error) {
+	// This needs to be run before replicasets are found/resolved below.
+	// Additionally, for whatever reason, the tests also have assertions that
+	// fail if we delay finding the replicasets, analysisruns and experiments
+	// until after the roll context is created. Thus any eventual error in
+	// resolving the workload ref will be handled immediately after the
+	// rollcontext is created.
+	resolveErr := c.refResolver.Resolve(rollout)
+
 	rsList, err := c.getReplicaSetsForRollouts(rollout)
 	if err != nil {
 		return nil, err
@@ -560,6 +557,15 @@ func (c *Controller) newRolloutContext(rollout *v1alpha1.Rollout) (*rolloutConte
 		reconcilerBase: c.reconcilerBase,
 	}
 
+	if resolveErr != nil {
+		err := roCtx.createInvalidRolloutCondition(resolveErr, rollout)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create invalid rollout condition when resolving the rollout: %w", err)
+		}
+
+		return &roCtx, validation.InvalidWorkloadRef(roCtx.rollout, resolveErr)
+	}
+
 	// Get Rollout Validation errors
 	err = roCtx.getRolloutValidationErrors()
 	if err != nil {



Test patch:
diff --git a/rollout/controller_test.go b/rollout/controller_test.go
index ef998ebe..c8f1e7b7 100644
--- a/rollout/controller_test.go
+++ b/rollout/controller_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"net/http"
 	"reflect"
 	"strconv"
 	"strings"
@@ -17,6 +18,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -73,7 +75,22 @@ const (
 type FakeWorkloadRefResolver struct {
 }
 
-func (f *FakeWorkloadRefResolver) Resolve(_ *v1alpha1.Rollout) error {
+func (f *FakeWorkloadRefResolver) Resolve(r *v1alpha1.Rollout) error {
+	if r.Spec.WorkloadRef == nil {
+		return nil
+	}
+
+	if r.Spec.WorkloadRef.Kind == "Error" {
+		return &errors.StatusError{
+			ErrStatus: metav1.Status{
+				Status:  metav1.StatusFailure,
+				Code:    http.StatusNotFound,
+				Reason:  metav1.StatusReasonNotFound,
+				Message: "not found",
+			},
+		}
+	}
+
 	return nil
 }
 
@@ -1400,6 +1417,40 @@ func TestSwitchInvalidSpecMessage(t *testing.T) {
 	assert.JSONEq(t, calculatePatch(r, expectedPatch), patch)
 }
 
+func TestInvalidWorkloadRef(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	r := newCanaryRollout("foo", 1, nil, nil, nil, intstr.FromInt(0), intstr.FromInt(1))
+	r.Spec.Selector = nil
+	r.Spec.Template = corev1.PodTemplateSpec{}
+	r.Spec.WorkloadRef = &v1alpha1.ObjectRef{
+		Kind: "Error",
+	}
+
+	f.rolloutLister = append(f.rolloutLister, r)
+	f.objects = append(f.objects, r)
+
+	patchIndex := f.expectPatchRolloutAction(r)
+	f.run(getKey(r, t))
+
+	expectedPatchWithoutSub := `{
+		"status": {
+			"conditions": [%s,%s],
+			"message": "%s: %s",
+			"phase": "Degraded"
+		}
+	}`
+	errmsg := "The Rollout \"foo\" is invalid: not found"
+	_, progressingCond := newProgressingCondition(conditions.ReplicaSetUpdatedReason, r, "")
+	invalidSpecCond := conditions.NewRolloutCondition(v1alpha1.InvalidSpec, corev1.ConditionTrue, conditions.InvalidSpecReason, errmsg)
+	invalidSpecBytes, _ := json.Marshal(invalidSpecCond)
+	expectedPatch := fmt.Sprintf(expectedPatchWithoutSub, progressingCond, string(invalidSpecBytes), conditions.InvalidSpecReason, strings.ReplaceAll(errmsg, "\"", "\\\""))
+
+	patch := f.getPatchedRollout(patchIndex)
+	assert.JSONEq(t, calculatePatch(r, expectedPatch), patch)
+}
+
 // TestPodTemplateHashEquivalence verifies the hash is computed consistently when there are slight
 // variations made to the pod template in equivalent ways.
 func TestPodTemplateHashEquivalence(t *testing.T) {