argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.92k stars 5.46k forks source link

ApplicationSet CRD should not validate `template` or `generators[].{generator}.template` fields #9919

Open crenshaw-dev opened 2 years ago

crenshaw-dev commented 2 years ago

Summary

The ApplicationSet CRD inherits validation rules from the Application spec. We should clear out the "required" validation rules for all the template fields (i.e. the generator templates and the main template).

Motivation

The problem is that, if you want to provide part of the Application template in the main template field and the rest of it in a generator template, you'll get validation errors. To pass validation, you have to provide a fully-specified, valid Application in each template field. For example:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: my-appset
spec:
  generators:
    - git:
        repoURL: https://github.com/some/repo
        revision:e2e
        directories:
          - path: '*'
        template:
          metadata:
            name: '{{path.basenameNormalized}}'
          spec:
            project: "my-project"
            destination:
              server: https://kubernetes.default.svc
            source:
              targetRevision: "e2e"
              repoURL: https://github.com/some/repo
              path: '{{path.basenameNormalized}}'
    - git:
        repoURL: https://github.com/some/repo
        revision: release/prd
        directories:
          - path: '*'
        template:
          metadata:
            name: '{{path.basenameNormalized}}'
          spec:
            project: "my-project"
            destination:
              server: https://kubernetes.default.svc
            source:
              targetRevision: "prd"
              repoURL: https://github.com/some/repo
              path: '{{path.basenameNormalized}}'
  template:
    metadata:
      name: '{{path.basenameNormalized}}'
    spec:
      project: "my-project"
      destination:
        server: https://kubernetes.default.svc
      syncPolicy:
        syncOptions:
          - ApplyOutOfSyncOnly=true
      source:
        repoURL: https://github.com/some/repo
        path: '{{path.basenameNormalized}}'
        directory:
          jsonnet: {}
          recurse: true

The more generators you have, the more duplication there is. In my actual use case, I have four generators.

Ideally I'd like to be able to do this:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: my-appset
spec:
  generators:
    - git:
        repoURL: https://github.com/some/repo
        revision:e2e
        directories:
          - path: '*'
        template:
          metadata:
            name: '{{path.basenameNormalized}}'
          spec:
            source:
              targetRevision: "e2e"
    - git:
        repoURL: https://github.com/some/repo
        revision: release/prd
        directories:
          - path: '*'
        template:
          spec:
            source:
              targetRevision: "prd"
  template:
    metadata:
      name: '{{path.basenameNormalized}}'
    spec:
      project: "my-project"
      destination:
        server: https://kubernetes.default.svc
      syncPolicy:
        syncOptions:
          - ApplyOutOfSyncOnly=true
      source:
        repoURL: https://github.com/some/repo
        path: '{{path.basenameNormalized}}'
        directory:
          jsonnet: {}
          recurse: true

Proposal

I think this would require a post-processor on the ApplicationSet CRD generation to remove "required" validation from the OpenAPI spec.

This pushes validation errors to runtime. I think it's worthwhile for the simplified spec.

hobti01 commented 2 years ago

We are seeing that ApplicationSets also incorrectly remain in 'Degraded' state when using a templated field as the template.metadata.name.

ApplicationSet my-appset contains applications with duplicate name: {{path.basenameNormalized}} (and 19 more)
crenshaw-dev commented 2 years ago

@hobti01 that error seems legitimate to me. {{path.basenameNormalized}} should not appear in that error message unless variable substitution did not happen. In other words, {{path.basenameNormalized}} is unresolved.

hobti01 commented 2 years ago

OK thanks for the confirmation. I think I see the issue: There is no mechanism for a custom generator (allowing a filter of invalid results) - so this means that if the generator does not supply full objects for the templating I see this error. I used your example for my situation, but {{path.basenameNormalized}} would always exist and therefore isn't appropriate. In my case I'm using a files generator and it looks like the error is raised when the files are missing values used in the template, so that makes sense with your explanation, thank you!