fix: do not fail on manifest-like yaml files - fixes #21934 #22043

CefBoud · 2025-02-27T00:19:39Z

Description

Fixes #21934

Repositories may contain YAML files that resemble valid k8s manifests, as they include apiVersion, metadata, and kind, but are not necessarily valid k8s manifests. An example highlighted in this GitHub issue involves a Helm values file that could be mistaken for a manifest.

Currently, these manifest-like YAMLs cause an error in the form of Failed to unmarshal "{filename}": <nil> that stops the Application from continuing.

This PR replaces the error with a warning without stopping execution. The rationale being that manifest-like files are allowed to exist but a warning is issued to provide information on true positives (manifest that are malformed).

Alternative

A potential downside of this change is that invalid manifests (true positives) will only trigger a warning rather than causing a failure. An alternative approach to address this could be to introduce an annotation or comment, such as # ArgoCD: IgnoreNotAManifest, which can be added to non-manifest YAML files. This would allow those manifests to be ignored if their unmarshalling fails.

Testing

#21934 contains a fairly simple way to reproduce the problem. Using this PR's branch should allow the Application to proceed while logging the warning.

Checklist:

bunnyshell · 2025-02-27T00:19:46Z

❗ Preview Environment undeploy from Bunnyshell failed

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to try again to remove the environment

nitishfy

Couple of things that I'd suggest here doing:

Please add a test case to validate the behaviour.
Please fix the failing tests. They are failing because we are not returning an error when the manifest is invalid.

=== FAIL: reposerver/repository TestInvalidManifestsInDir (0.00s)
time="2025-02-27T00:26:33Z" level=info msg="manifest cache miss: &ApplicationSource{RepoURL:,Path:./testdata/invalid-manifests,TargetRevision:,Helm:nil,Kustomize:nil,Directory:&ApplicationSourceDirectory{Recurse:true,Jsonnet:ApplicationSourceJsonnet{ExtVars:[]JsonnetVar{},TLAs:[]JsonnetVar{},Libs:[],},Exclude:,Include:,},Plugin:nil,Chart:,Ref:,Name:,}/mock.Anything"
time="2025-02-27T00:26:33Z" level=info msg="manifest cache miss: &ApplicationSource{RepoURL:,Path:./testdata/invalid-manifests,TargetRevision:,Helm:nil,Kustomize:nil,Directory:&ApplicationSourceDirectory{Recurse:true,Jsonnet:ApplicationSourceJsonnet{ExtVars:[]JsonnetVar{},TLAs:[]JsonnetVar{},Libs:[],},Exclude:,Include:,},Plugin:nil,Chart:,Ref:,Name:,}/mock.Anything"
time="2025-02-27T00:26:33Z" level=warning msg="Failed to unmarshal \"bad.yaml\": file contains 'apiVersion:', 'kind:', and 'metadata:', but is not a valid manifest: failed to unmarshal manifest: error converting YAML to JSON: yaml: line 3: did not find expected key" application=
    repository_test.go:680: 
        	Error Trace:	/home/runner/work/argo-cd/argo-cd/reposerver/repository/repository_test.go:680
        	Error:      	An error is expected but got nil.
        	Test:       	TestInvalidManifestsInDir

=== FAIL: reposerver/repository Test_findManifests/invalid_manifest_throws_an_error (0.00s)
time="2025-02-27T00:26:36Z" level=warning msg="Failed to unmarshal \"bad.yaml\": file contains 'apiVersion:', 'kind:', and 'metadata:', but is not a valid manifest: failed to unmarshal manifest: error converting YAML to JSON: yaml: line 3: did not find expected key" test=test
    repository_test.go:2810: 
        	Error Trace:	/home/runner/work/argo-cd/argo-cd/reposerver/repository/repository_test.go:2810
        	Error:      	An error is expected but got nil.
        	Test:       	Test_findManifests/invalid_manifest_throws_an_error

codecov · 2025-02-28T00:11:01Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.91%. Comparing base (b39ca15) to head (1f702f3).
⚠️ Report is 434 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #22043      +/-   ##
==========================================
- Coverage   60.01%   59.91%   -0.11%     
==========================================
  Files         342      342              
  Lines       57845    58582     +737     
==========================================
+ Hits        34718    35100     +382     
- Misses      20352    20635     +283     
- Partials     2775     2847      +72

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

andrii-korotkov-verkada · 2025-02-28T05:39:25Z

reposerver/repository/repository.go

 				bytes.Contains(out, []byte("kind:")) &&
 				bytes.Contains(out, []byte("metadata:")) {
-				return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", filename, err)
+				logCtx.Warnf("Failed to unmarshal %q: file contains 'apiVersion:', 'kind:', and 'metadata:', but is not a valid manifest: %v", filename, err)


What if it's an actual invalid yaml which is a manifest?

It would simply log a warning, as mentioned in the PR description.
There's also an "Alternative" section where I suggest adding an annotation to mark non-manifest YAML files. This would maintain the existing behavior and also allow to fix the issue this PR is trying to address.

ishitasequeira · 2025-03-02T22:59:13Z

reposerver/repository/repository.go

+			out, _ := utfutil.ReadFile(manifestPath, utfutil.UTF8)
+			// Otherwise, let's see if it looks like a resource, if yes, we log a warning.
 			if bytes.Contains(out, []byte("apiVersion:")) &&
 				bytes.Contains(out, []byte("kind:")) &&
 				bytes.Contains(out, []byte("metadata:")) {
-				return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", filename, err)
+				logCtx.Warnf("Failed to unmarshal %q: file contains 'apiVersion:', 'kind:', and 'metadata:', but is not a valid manifest: %v", filename, err)


Let's keep the err on line 1749 and log it as part of the warning.

@ishitasequeira Thanks for review.
If the file read fails, I think it doesn't make much sense to run the following if bytes.Contains(out, []byte(...)).

I suggest returning an error instead:

out, rerr := utfutil.ReadFile(manifestPath, utfutil.UTF8) if rerr != nil { return status.Errorf(codes.FailedPrecondition, "Failed to read %q: %v", filename, rerr) }

Let me know what you think.

Thanks @CefBoud, that would actually be better and I see that you have already addressed that in your latest commits.

ishitasequeira

Overall change looks good to me. @CefBoud can you resolve the conflicts?

agaudreault · 2025-05-07T22:56:53Z

reposerver/repository/repository.go

I think I disagree with the idea of having this feature merged because the risk of ignoring malformed yaml error that could cause resources to be automatically pruned is just to big.

In my opinion, the only way we can safely ignore "kubernetes-like" file is with an explicit instruction to ignore the file, such as a comment anywhere in the file that would include +argocd:skip-rendering or something like that.

Let me know what you think, maybe i over evaluate the risks here.

Thanks for chiming in, @agaudreault. I mentioned this in the Alternatives section of the PR description, and now that you say it, I believe it's best to stay on the safe side. Adding support for skipping files containing +argocd:skip-rendering seems like the right approach.
I've made the changes. Let me know what you think.

CC @ishitasequeira @andrii-korotkov-verkada

agaudreault

This should also be documented somewhere in the documentation, probably in https://argo-cd.readthedocs.io/en/stable/user-guide/directory/

agaudreault · 2025-06-04T16:49:03Z

reposerver/repository/repository.go

+	if rerr != nil {
+		return nil, "", fmt.Errorf("failed to read %q: %w", relPath, rerr)
+	}
+	//	skip file if it contains "+argocd:skip-rendering"


Suggested change

// skip file if it contains "+argocd:skip-rendering"

// skip file if it contains "+argocd:skip-rendering"

agaudreault · 2025-06-04T16:51:56Z

reposerver/repository/repository.go

+		return nil, "", fmt.Errorf("failed to read %q: %w", relPath, rerr)
+	}
+	//	skip file if it contains "+argocd:skip-rendering"
+	if bytes.Contains(out, []byte("+argocd:skip-rendering")) {


Extract as constant and document in go code.

I would also rename to "+argocd:skip-file-rendering" because it might be confusing if this is in a file containing (---) with multiple objects

CefBoud · 2025-06-07T16:56:16Z

Thanks for the review @agaudreault . I've made the changes and added this to the documentation.

Signed-off-by: Moncef Abboud <[email protected]>

Signed-off-by: Moncef Abboud <[email protected]> Signed-off-by: dsuhinin <[email protected]>

Signed-off-by: Moncef Abboud <[email protected]> Signed-off-by: enneitex <[email protected]>

CefBoud requested a review from a team as a code owner February 27, 2025 00:19

nitishfy requested changes Feb 27, 2025

View reviewed changes

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch from adc130b to 07c3dec Compare February 27, 2025 23:32

andrii-korotkov-verkada reviewed Feb 28, 2025

View reviewed changes

andrii-korotkov-verkada approved these changes Mar 1, 2025

View reviewed changes

andrii-korotkov-verkada requested a review from nitishfy March 1, 2025 18:43

andrii-korotkov-verkada added the ready-for-review An approver should give a final review and merge the PR label Mar 1, 2025

github-project-automation bot added this to Argo CD Review Mar 1, 2025

github-project-automation bot moved this to Ready for final review in Argo CD Review Mar 1, 2025

ishitasequeira reviewed Mar 2, 2025

View reviewed changes

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch 2 times, most recently from fa97cf5 to 95d7be4 Compare March 4, 2025 14:59

ishitasequeira reviewed Apr 27, 2025

View reviewed changes

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch 3 times, most recently from 1d7b0d9 to d6fe76b Compare April 30, 2025 19:57

CefBoud requested a review from ishitasequeira April 30, 2025 20:08

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch from d6fe76b to 5713321 Compare May 1, 2025 14:40

agaudreault requested changes May 7, 2025

View reviewed changes

github-project-automation bot moved this from Ready for final review to Changes Requested in Argo CD Review May 7, 2025

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch from 5713321 to ec3ac3e Compare June 2, 2025 16:57

CefBoud requested a review from agaudreault June 2, 2025 17:02

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch 2 times, most recently from f0ee6cc to 23ae46e Compare June 2, 2025 17:13

agaudreault requested changes Jun 4, 2025

View reviewed changes

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch from 23ae46e to 9f7b23c Compare June 7, 2025 16:53

CefBoud requested a review from a team as a code owner June 7, 2025 16:53

feat: support skipping file rendering via '+argocd:skip-rendering'

1f702f3

Signed-off-by: Moncef Abboud <[email protected]>

CefBoud force-pushed the fix/ignore-k8s-manifest-like-yamls branch from 9f7b23c to 1f702f3 Compare June 7, 2025 16:57

agaudreault approved these changes Jun 7, 2025

View reviewed changes

agaudreault merged commit 01a882d into argoproj:master Jun 7, 2025
28 checks passed

github-project-automation bot moved this from Changes Requested to Done in Argo CD Review Jun 7, 2025

dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025

fix: do not fail on manifest-like yaml argoproj#21934 (argoproj#22043)

1487f30

Signed-off-by: Moncef Abboud <[email protected]> Signed-off-by: dsuhinin <[email protected]>

dsuhinin pushed a commit to dsuhinin/argo-cd that referenced this pull request Jun 16, 2025

fix: do not fail on manifest-like yaml argoproj#21934 (argoproj#22043)

315cb6b

Signed-off-by: Moncef Abboud <[email protected]> Signed-off-by: dsuhinin <[email protected]>

enneitex pushed a commit to enneitex/argo-cd that referenced this pull request Aug 24, 2025

fix: do not fail on manifest-like yaml argoproj#21934 (argoproj#22043)

5c740c5

Signed-off-by: Moncef Abboud <[email protected]> Signed-off-by: enneitex <[email protected]>

	// skip file if it contains "+argocd:skip-rendering"
	// skip file if it contains "+argocd:skip-rendering"

fix: do not fail on manifest-like yaml files - fixes #21934 #22043

fix: do not fail on manifest-like yaml files - fixes #21934 #22043

Uh oh!

Conversation

CefBoud commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Alternative

Testing

Uh oh!

bunnyshell bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❗ Preview Environment undeploy from Bunnyshell failed

Uh oh!

nitishfy left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ishitasequeira left a comment

Choose a reason for hiding this comment

Uh oh!

agaudreault May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agaudreault left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CefBoud commented Jun 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CefBoud commented Feb 27, 2025 •

edited

Loading

bunnyshell bot commented Feb 27, 2025 •

edited

Loading

codecov bot commented Feb 28, 2025 •

edited

Loading

agaudreault May 7, 2025 •

edited

Loading