Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong globbing in Git file generator #13313

Closed
3 tasks done
Calchan opened this issue Apr 21, 2023 · 1 comment · Fixed by #13314
Closed
3 tasks done

Wrong globbing in Git file generator #13313

Calchan opened this issue Apr 21, 2023 · 1 comment · Fixed by #13314
Labels
bug Something isn't working

Comments

@Calchan
Copy link
Contributor

Calchan commented Apr 21, 2023

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

The Git file generator currently implements globbing in a way which can trigger errors or catch users off-guard. For example, consider the following repository layout:

└── cluster-charts/
    ├── cluster1
    │   ├── mychart/
    │   │   ├── charts/
    │   │   │    └── mysubchart/
    │   │   │        ├── values.yaml
    │   │   │        └── etc…
    │   │   ├── values.yaml
    │   │   └── etc…
    │   └── myotherchart/
    │       ├── values.yaml
    │       └── etc…
    └── cluster2
        └── etc…

In cluster1 we have two charts, one of them with a subchart.

Assuming we need the ApplicationSet to template values in the values.yaml, then we need to use a Git file generator instead of a directory generator. The value of the path key of the Git file generator should be set to:

path: cluster-charts/*/*/values.yaml

However, the current implementation will interpret the above pattern as:

path: cluster-charts/**/values.yaml

Meaning, for mychart in cluster1, that it will pick up both the chart's values.yaml but also the one from its subchart. This will most likely fail, and even if it didn't it would be wrong.

This is the failure users are most likely to encounter but there are multiple other ways this undesirable globbing can fail. For example:

path: some-path/*.yaml

This will return all YAML files in any directory at any level under some-path, instead of only those directly under it.

To Reproduce

I have created a repository to reproduce the second type of failure above (https://github.com/Calchan/monorepo-local), based on an actual configuration we currently run in production. The repository layout is in the form:
prod/clusters/<cluster name>/<namespace>
We're using a cluster named local and deploy in the default namespace because it's easier, so the path to our charts in the repository is:
prod/clusters/local/default/

In there, we have two types of charts. The first one is in the usual form of a directory with the usual files and sub-directories in it: Chart.yaml, values.yaml, templates/, etc… The example for that is at prod/clusters/local/default/hello-world/.

The second type of chart is a simple YAML file pointing to a chart on another directory. The use case here is to store charts that are commonly used in all clusters which increases DRY when dealing with dozens of clusters. In our example we point to the guestbook chart at prod/clusters/virtual/default/.

Both type of charts are managed by two ApplicationSets at prod/bootstrap/local/ in the repository. The implementation details can be perused in the example repository but are not important for this bug.

Save the following file as bootstrap.yaml:

apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
  annotations:
    argocd.argoproj.io/tracking-id: argocd:argoproj.io/Application:argocd/bootstrap
  name: bootstrap
  namespace: argocd
spec:
  destination:
    name: in-cluster
    namespace: argocd
  project: default
  source:
    path: prod/bootstrap/local
    repoURL: https://github.com/Calchan/monorepo-local.git
  syncPolicy:
    automated:
      prune: true
      selfHeal: true

Then bootstrap the cluster with kubectl -n argocd apply -f bootstrap.yaml.

The regular ApplicationSet generating applications for charts in the directory form will work, but not the one for charts in link YAML files (see screenshots below). In the latter case, the value of the path key in the Git file generator spec is set to prod/clusters/local/*/*.yaml.

The reason is instead of picking up YAML files directly under prod/clusters/local/*/ as the ApplicationSet expects, the Git file generator returns all *.yaml files at any level under prod/clusters/local/.

The log shows this (simplified for readability, see full log below):

msg="git ls-files --full-name -z -- prod/clusters/local/*/*.yaml"
msg="prod/clusters/local/default/hello-world/Chart.yaml\x00prod/clusters/local/default/hello-world/templates/deployment.yaml\x00prod/clusters/local/default/hello-world/templates/service.yaml\x00prod/clusters/local/default/hello-world/templates/serviceaccount.yaml\x00prod/clusters/local/default/hello-world/values.yaml\x00prod/clusters/local/default/helm-guestbook.yaml\x00"

Expected behavior

The bootstrap-applications-link ApplicationSet is not in degraded mode and the log shows only this:

msg="git ls-files --full-name -z -- prod/clusters/local/*/*.yaml"
msg="prod/clusters/local/default/helm-guestbook.yaml\x00"

Screenshots

image

image

Version

argocd: v2.6.7+5bcd846
  BuildDate: 2023-03-23T14:57:27Z
  GitCommit: 5bcd846fa16e4b19d8f477de7da50ec0aef320e5
  GitTreeState: clean
  GoVersion: go1.18.10
  Compiler: gc
  Platform: linux/amd64
argocd-server: v2.6.7+5bcd846
  BuildDate: 2023-03-23T14:57:27Z
  GitCommit: 5bcd846fa16e4b19d8f477de7da50ec0aef320e5
  GitTreeState: clean
  GoVersion: go1.18.10
  Compiler: gc
  Platform: linux/amd64
  Kustomize Version: v4.5.7 2022-08-02T16:35:54Z
  Helm Version: v3.10.3+g835b733
  Kubectl Version: v0.24.2
  Jsonnet Version: v0.19.1

Logs

18:16:51 applicationset-controller | time="2023-04-21T18:16:51Z" level=info msg=Trace args="[git clean -fdx]" dir=/tmp/https___github.com_calchan_monorepo-local operation_name="exec git" time_ms=1.791212                                                                                                                                                                                                                                                                                    
18:16:51 applicationset-controller | time="2023-04-21T18:16:51Z" level=info msg="git ls-files --full-name -z -- prod/clusters/local/*/*.yaml" dir=/tmp/https___github.com_calchan_monorepo-local execID=80997                                                                                                                                                                                                                                                                                  
18:16:51 applicationset-controller | time="2023-04-21T18:16:51Z" level=debug msg="prod/clusters/local/default/hello-world/Chart.yaml\x00prod/clusters/local/default/hello-world/templates/deployment.yaml\x00prod/clusters/local/default/hello-world/templates/service.yaml\x00prod/clusters/local/default/hello-world/templates/serviceaccount.yaml\x00prod/clusters/local/default/hello-world/values.yaml\x00prod/clusters/local/default/helm-guestbook.yaml\x00" duration=1.531613ms execID=80997                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          
18:16:51 applicationset-controller | time="2023-04-21T18:16:51Z" level=info msg=Trace args="[git ls-files --full-name -z -- prod/clusters/local/*/*.yaml]" dir=/tmp/https___github.com_calchan_monorepo-local operation_name="exec git" time_ms=1.618197                                                                                                        
18:16:51 applicationset-controller | time="2023-04-21T18:16:51Z" level=error msg="error generating params" error="unable to process file 'prod/clusters/local/default/hello-world/templates/deployment.yaml': unable to parse file: error converting YAML to JSON: yaml: line 5: did not find expected node content" generator="&{0xc0012eaa50 true}"                                                                                                                                          
18:16:51 applicationset-controller | time="2023-04-21T18:16:51Z" level=error msg="error generating application from params" error="unable to process file 'prod/clusters/local/default/hello-world/templates/deployment.yaml': unable to parse file: error converting YAML to JSON: yaml: line 5: did not find expected node content" generator="{nil nil &GitGenerator{RepoURL:https://github.com/Calchan/monorepo-local.git,Directories:[]GitDirectoryGeneratorItem{},Files:[]GitFileGeneratorItem{GitFileGeneratorItem{Path:prod/clusters/local/*/*.yaml,},},Revision:HEAD,RequeueAfterSeconds:nil,Template:ApplicationSetTemplate{ApplicationSetTemplateMeta:ApplicationSetTemplateMeta{Name:,Namespace:,Labels:map[string]string{},Annotations:map[string]string{},Finalizers:[],},Spec:ApplicationSpec{Source:nil,Destination:ApplicationDestination{Server:,Namespace:,Name:,},Project:,SyncPolicy:nil,IgnoreDifferences:[]ResourceIgnoreDifferences{},Info:[]Info{},RevisionHistoryLimit:nil,Sources:[]ApplicationSource{},},},PathParamPrefix:,} nil nil nil nil nil nil}"

Slack discussion with Michael Crenshaw about this issue:
https://cloud-native.slack.com/archives/C01TSERG0KZ/p1681230308737899

I have a PR coming up.

@xashr
Copy link
Contributor

xashr commented Apr 25, 2023

path: some-path/*.yaml

We just ran into that exact issue. Glad there is a fix coming up.

Calchan added a commit to Calchan/argo-cd that referenced this issue May 23, 2023
Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com>
crenshaw-dev added a commit that referenced this issue May 28, 2023
Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this issue Aug 9, 2023
…rgoproj#13314)

Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
…rgoproj#13314)

Signed-off-by: Denis Dupeyron <denis.dupeyron@gmail.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants