feat(controller): show invalid spec on cronwf res

[carlosrodfern]

Fixes: #6781

Motivation

Help the user see the validation errors on the cronwf resources early, before they find out later that the workflow didn't trigger.

Modifications

Add the event to the description of the cronwf, e.g.

Events:
  Type     Reason     Age   From                 Message
  ----     ------     ----  ----                 -------
  Warning  Invalid  14s   workflow-controller  cron workflow name "test-cron-wf-very-very-very-very-very-very-very-very-very-very-very-very-long" must not be more than 52 characters long (currently 77)

Verification

I created my own image of workflow-controller and deployed it in a local k3s kubernetes with argo-workflow.

I used this artifact as a test:

apiVersion: argoproj.io/v1alpha1
kind: CronWorkflow
metadata:
  name: test-cron-wf-very-very-very-very-very-very-very-very-very-very-very-very-long
spec:
  schedule: "5 * * * *"
  concurrencyPolicy: "Replace"
  startingDeadlineSeconds: 0
  workflowSpec:
    entrypoint: date
    templates:
    - name: date
      container:
        image: alpine:3.6
        command: [sh, -c]
        args: ["date; sleep 90"]

[carlosrodfern]

I'm still investigating the possibiilty to actually make it a status, e.g. Ready if the cronworkflow is good, or something else otherwise. The goal is that if I deploy an invalid cronworkflow with ArgoCD, for example, the health status of the cronworkflow would be failed because of invalid spec, and ArgoCD would mark the app syncing as failed.

[tooptoop4]

if woc.controller.Config.WorkflowEvents.IsEnabled() { like https://github.com/argoproj/argo-workflows/pull/13746/files

[Joibel]

There is already a metric for this.

The best solution is an admission controller which will reject them. This is started in #13879

[carlosrodfern]

There is already a metric for this.

The best solution is an admission controller which will reject them. This is started in #13879

That looks like the best solution. That specific PR is for the Workflow artifact. We would need another validating webhook for CronWorkflow, and even for the WorkflowTemplates as well, correct?

[Joibel]

That looks like the best solution. That specific PR is for the Workflow artifact. We would need another validating webhook for CronWorkflow, and even for the WorkflowTemplates as well, correct?

Yes. Initially getting it in with the framework is the hard part, but then it should be extended to cover those two, ClusterWFTemplates, and EventBindings to cover all the "user facing" custom resources.

[carlosrodfern]

That looks like the best solution. That specific PR is for the Workflow artifact. We would need another validating webhook for CronWorkflow, and even for the WorkflowTemplates as well, correct?

Yes. Initially getting it in with the framework is the hard part, but then it should be extended to cover those two, ClusterWFTemplates, and EventBindings to cover all the "user facing" custom resources.

Awesome :). I'll close this PR then, and I'll just watch the one being worked on, to then bring a new PR extending the new webhook 👍