Add validation for rollout if selector is not specified

[harikrongali]

Similar to deployment, Rollout should throw error and shouldn't create the object if the selector is not specified.

Current behavior: Rollout object gets created with Error status

Expected behavior: Rollout object shouldn't be allowed to create and throw a validation error

Following file doesn't have selector defined

apiVersion: argoproj.io/v1alpha1
kind: Rollout
metadata:
  name: rollout-canary
spec:
  replicas: 2
  revisionHistoryLimit: 2
  template:
    metadata:
      labels:
        app: rollouts-demo
    spec:
      containers:
      - name: rollouts-demo
        image: argoproj/rollouts-demo:blue
        imagePullPolicy: Always
        ports:
        - containerPort: 8080
  strategy:
    canary:
      canaryService: rollouts-demo-canary

[sourikghosh]

Can i work on this issue

[sourikghosh]

@harikrongali @perenesenko Can you give me a hint from where to start looking, to make this enhancement?

[huikang]

@sourikghosh , try starting from https://github.com/argoproj/argo-rollouts/blob/575a619d68fdd0e21e6dfa80aeea1c6c34009ef9/pkg/apis/rollouts/validation/validation.go#L95

[sourikghosh]

thank you very much @huikang.

[vishal-yadav]

Below code section comment assumes that it is expected behavior if this is caused by dependency between Rollout and Service resources. I am a newbie to this project so not sure if such condition/order would be caused by client/user or some race condition. If the wrong order is caused by client/user then it should see such validation error instead of queuing the request to controller for creating the rollout object with incorrect spec. The correct order of operation can be explicitly specified in the documentation.

argo-rollouts/rollout/context.go

	if err != nil {
		if vErr, ok := err.(*field.Error); ok {
			// We want to frequently requeue rollouts with InvalidSpec errors, because the error
			// condition might be timing related (e.g. the Rollout was applied before the Service).
			c.enqueueRolloutAfter(c.rollout, 20*time.Second)
			return c.createInvalidRolloutCondition(vErr, c.rollout)
		}
	return err

Hi @perenesenko if it's okay I can work on this issue. Thanks!

[vishal-yadav]

While tracking above change bit more it appears it was part of https://github.com/argoproj/argo-rollouts/pull/814 PR and related to referenced resource. However IMO we can refactor/separate both scenarios (waiting/queuing for referenced resource to be created vs general invalid rollout spec issue like this one).

@jessesuen let me know the background and your opinion about above and I'll work on it.

[nirvanagit]

@harikrongali - can this be converted to a bug, as selector field should be mandatory for rollouts. Without selector, a rollout is of no use.

[vishal-yadav]

@harikrongali, Can you please assign this to me?

[vishal-yadav]

Above validation needs to be done on CRD schema level. After some further deep-dive, got to know that rollout.spec.selector can also resolve the value from rollout.spec.workloadRef section that's why it is optional. So the correct validation for this case would be that one of these fields (selector or workloadRef) should be required/non-empty. Such validation is possible with CRD structural schema oneOf or union constructs however it is yet to be supported from kubebuilder (controller-tools: https://github.com/kubernetes-sigs/controller-tools/issues/461). For the time planning to add some hack for CRD generation to handle this case.

[nikhil1raghav]

Is this issue still being worked upon @vishal-yadav. As @vishal-yadav said spec.selector can also get resolved from spec.workloadRef and while doing validation we are already checking if both of them are nil.

Then why this is not being caught here

https://github.com/argoproj/argo-rollouts/blob/575a619d68fdd0e21e6dfa80aeea1c6c34009ef9/pkg/apis/rollouts/validation/validation.go#L102

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity.

[github-actions[bot]]

This issue is stale because it has been open 60 days with no activity.