feat: Render Helm ValuesObject as YAML in log output instead of binary (#18342) by subhramit · Pull Request #27649 · argoproj/argo-cd

subhramit · 2026-05-02T09:58:59Z

I have used a different approach compared to #23717 by keeping the fix entirely server-side.
In short, ValuesObject dumped raw bytes like [123 34 ...] which was both unreadable and large.
So by rendering it as YAML instead, the output is human-readable and compact, so it stops flooding Loki storage.
This was achieved by suppressing the stringer at proto-level and providing a custom implementation for String().

Also fixed some wrong log texts in repository.go where cache was being cleared instead of set (and the wording conveyed otherwise).

Checklist:

Signed-off-by: subhramit <subhramit.bb@live.in>

bunnyshell · 2026-05-02T09:59:04Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

Signed-off-by: subhramit <subhramit.bb@live.in>

codecov · 2026-05-02T10:47:17Z

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.08%. Comparing base (5471c94) to head (3748168).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
reposerver/repository/repository.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27649      +/-   ##
==========================================
+ Coverage   64.04%   64.08%   +0.04%     
==========================================
  Files         422      422              
  Lines       57828    57865      +37     
==========================================
+ Hits        37037    37084      +47     
+ Misses      17305    17295      -10     
  Partials     3486     3486

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

subhramit · 2026-05-02T10:49:25Z

ccing @agaudreault for a review, if this approach makes sense.

Copilot

Pull request overview

This PR addresses repo-server log bloat caused by Helm valuesObject being rendered as raw byte arrays in manifest-cache log messages. It adds log-specific formatting for application sources so cache-related repo-server logs emit readable values text instead of binary-looking output.

Changes:

Add LogString() helpers for ApplicationSourceHelm and ApplicationSource to render Helm ValuesObject through the existing values helpers.
Update manifest cache hit/miss/error log statements in reposerver/repository/repository.go to use the new log formatter.
Add unit tests covering the new logging string behavior for Helm/application sources.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
reposerver/repository/repository.go	Swaps manifest-cache log messages over to the new log-specific source formatter.
pkg/apis/application/v1alpha1/values.go	Adds `LogString()` helpers that rewrite `ValuesObject` into log-friendly text.
pkg/apis/application/v1alpha1/values_test.go	Adds unit tests for the new `LogString()` behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

subhramit · 2026-05-02T18:29:08Z

+	if s.Helm == nil || s.Helm.ValuesObject == nil {
+		return s.String()
+	}
+	sourceCopy := s.DeepCopy()


Addressed in d74a732

subhramit · 2026-05-02T18:33:01Z

+			log.Infof("manifest error cache miss: %s/%s", q.ApplicationSource.LogString(), cacheKey)
 			return false, res.ManifestResponse, nil
 		}

-		log.Infof("manifest cache hit: %s/%s", q.ApplicationSource.String(), cacheKey)
+		log.Infof("manifest cache hit: %s/%s", q.ApplicationSource.LogString(), cacheKey)


This has merit, but was not introduced as a regression in this PR.
Happy to address separately in another PR, if that's fine? @agaudreault

subhramit · 2026-05-02T18:34:19Z

 						err = s.cache.DeleteManifests(cacheKey, q.ApplicationSource, q.RefSources, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, refSourceCommitSHAs, q.InstallationID)
 						if err != nil {
-							log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
+							log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.LogString(), cacheKey, err)


This wording is also pre-existing, hence out of the PR's scope.
But I think the concern is valid.

Edit: this was a tiny one, so addressed in 0fd6f6b

subhramit · 2026-05-02T18:34:35Z

 						err = s.cache.DeleteManifests(cacheKey, q.ApplicationSource, q.RefSources, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, refSourceCommitSHAs, q.InstallationID)
 						if err != nil {
-							log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
+							log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.LogString(), cacheKey, err)


Same as #27649 (comment)

Addressed in 0fd6f6b

subhramit · 2026-05-02T18:39:33Z

+	helmCopy := *h
+	helmCopy.Values = h.ValuesString()
+	helmCopy.ValuesObject = nil


Good idea.
Covered in 1bdcc8f

Signed-off-by: subhramit <subhramit.bb@live.in>

Qodo-Free-For-OSS · 2026-05-03T07:00:33Z

Hi, Switching reposerver cache logs from .String() to .LogString() makes Helm ValuesObject content human-readable YAML, increasing the likelihood of accidental secret disclosure via Info/Warn logs if users store sensitive data in Helm values.

Severity: remediation recommended | Category: security

How to fix: Redact/truncate logged Helm values

Agent prompt to fix - you can give this to your LLM of choice:

Issue description

LogString() makes Helm values readable in reposerver logs. If Helm values contain secrets, this increases accidental exposure risk.

Issue Context

The previous logging also contained the data (as bytes), but the new format is substantially easier to read/search/copy.

Fix Focus Areas

pkg/apis/application/v1alpha1/values.go[56-88]

reposerver/repository/repository.go[939-1072]

Suggested approach

Implement a conservative max-length truncation for the rendered values in LogString().

Optionally redact common sensitive keys (e.g., password, token, secret, key) from the YAML string before logging.

Consider lowering verbosity of cache hit/miss logs or avoiding logging full values at Info/Warn levels.

Found by Qodo. Free code review for open-source maintainers.

subhramit · 2026-05-03T07:50:43Z

Hi, Switching reposerver cache logs from .String() to .LogString() makes Helm ValuesObject content human-readable YAML, increasing the likelihood of accidental secret disclosure via Info/Warn logs if users store sensitive data in Helm values.

Severity: remediation recommended | Category: security

The information was disclosed previously as well, just in a different format and also trivially decodable into JSON back.
For a solution to the original issue which I intend to fix, I think either truncation or the YAML based approach suffice, with the latter being the neater one, but both together may be overkill (in the context of that issue).

Truncation/security concerns can be raised as a separate issue.

Optionally redact common sensitive keys (e.g., password, token, secret, key) from the YAML string before logging.

I don't agree - it would miss everything that doesn't match the wordlist. Helm values are user-defined arbitrary YAML which we should not concern ourselves with. It would also produce false positives that destroy debuggability. For example key: matches kafka_bootstrap_key:, partition_key:, private_key_path:, accessKeyId etc. would be blocked.

Consider lowering verbosity of cache hit/miss logs or avoiding logging full values at Info/Warn levels.

This may be a reasonable thing to consider but is pre-existing and orthogonal to this fix.
@agaudreault what do you think?

agaudreault · 2026-05-06T21:19:32Z

+// LogString returns a human-readable string representation of ApplicationSourceHelm
+// suitable for logging. It renders ValuesObject as its YAML equivalent instead of
+// raw bytes, preventing binary data from flooding log output.
+func (h *ApplicationSourceHelm) LogString() string {


Why not using the String function directly? This makes it error prone since most people will not think of using LogString

Suggested change

// LogString returns a human-readable string representation of ApplicationSourceHelm

// suitable for logging. It renders ValuesObject as its YAML equivalent instead of

// raw bytes, preventing binary data from flooding log output.

func (h *ApplicationSourceHelm) LogString() string {

func (h ApplicationSourceHelm) String() string {

I had also thought about it, but generated.pb.go already has func (this *ApplicationSourceHelm) String() string. Since go doesn't allow value-receiver and pointer-receiver methods with the same name on a single type, it will not compile.

IMO in hindsight, chances are non-zero, but unlikely that developers make that mistake with today's AI-assisted code suggestions, development tools and code reviews.

Out of curiosity, I dug a little deeper - there seems to be no workaround for this except proto suppression protobuf.options.(gogoproto.goproto_stringer)=false, which IMO is not clean at all.

@agaudreault just a gentle ping in case you can go through the above and think the current state is good to go.

(Whenever you get time - I know you may be busy, I'm an OSS maintainer myself, but looking at the sheer number of PRs open I'd prefer this doesn't get buried if it adds value)

I think setting the stringer=false annotation is best, so you can provide an implementation that prints the RawExtension type correctly.

Signed-off-by: subhramit <subhramit.bb@live.in>

Signed-off-by: subhramit-suki <sbasu@suki.ai>

Signed-off-by: subhramit <subhramit.bb@live.in>

subhramit · 2026-05-12T05:59:01Z

+// only observable difference is the ValuesObject rendering. If new fields are
+// added to ApplicationSourceHelm, they must be added here as well - there is
+// no codegen for this method.
+func (h *ApplicationSourceHelm) String() string {


All the additional "syntactical" differences from the codegen implementation are enhancements to satisfy the linters.

Semantically it's exactly the same (except for our fix on ValuesObject).

Signed-off-by: subhramit <subhramit.bb@live.in>

subhramit · 2026-05-12T11:30:24Z

    "v1alpha1ApplicationSourceHelm": {
      "type": "object",
-      "title": "ApplicationSourceHelm holds helm specific options",
+      "title": "ApplicationSourceHelm holds helm specific options\n+protobuf.options.(gogoproto.goproto_stringer)=false",


Couldn't find a workaround for this, but the codebase contains many other similar examples:

argo-cd/assets/swagger.json

Lines 6521 to 6522 in 682de3e

"v1GroupKind": {

"description": "+protobuf.options.(gogoproto.goproto_stringer)=false",

argo-cd/assets/swagger.json

Lines 5831 to 5832 in 682de3e

"intstrIntOrString": {

"description": "+protobuf=true\n+protobuf.options.(gogoproto.goproto_stringer)=false\n+k8s:openapi-gen=true",

etc.

agaudreault

LGTM, but it might be better if we can wrap the RawExtension type

fix: render ValuesObject as YAML in log output

d375c5a

Signed-off-by: subhramit <subhramit.bb@live.in>

subhramit requested a review from a team as a code owner May 2, 2026 09:59

Use assert.NotContains

a4af96b

Signed-off-by: subhramit <subhramit.bb@live.in>

agaudreault requested a review from Copilot May 2, 2026 16:47

Copilot started reviewing on behalf of agaudreault May 2, 2026 16:48 View session

Copilot AI reviewed May 2, 2026

View reviewed changes

subhramit added 4 commits May 3, 2026 00:01

Use shallow copy

d74a732

Signed-off-by: subhramit <subhramit.bb@live.in>

Add YAML testcase

1bdcc8f

Signed-off-by: subhramit <subhramit.bb@live.in>

Merge branch 'master' into truncate-large-binary-logs

3b70f0f

Change wording of warning from set to delete

0fd6f6b

Signed-off-by: subhramit <subhramit.bb@live.in>

agaudreault reviewed May 6, 2026

View reviewed changes

Merge branch 'master' into truncate-large-binary-logs

c7d9bc2

subhramit requested a review from agaudreault May 7, 2026 17:27

subhramit added 2 commits May 8, 2026 06:17

Merge branch 'master' into truncate-large-binary-logs

2cbd024

Merge branch 'master' into truncate-large-binary-logs

623d51a

subhramit changed the title ~~fix: Render Helm ValuesObject as YAML in log output instead of binary (#18342)~~ feat: Render Helm ValuesObject as YAML in log output instead of binary (#18342) May 8, 2026

subhramit and others added 9 commits May 8, 2026 22:04

Merge branch 'master' into truncate-large-binary-logs

fca81b4

Merge branch 'master' into truncate-large-binary-logs

356f5d2

Merge branch 'master' into truncate-large-binary-logs

980f231

Revert LogString changes, add proto suppression annotation

cca2722

Signed-off-by: subhramit <subhramit.bb@live.in>

Swap position of suppress comment

6ae2ceb

Signed-off-by: subhramit-suki <sbasu@suki.ai>

Override String() implementation

0a947c0

Signed-off-by: subhramit-suki <sbasu@suki.ai>

Override String() implementation

ad7fb45

Signed-off-by: subhramit-suki <sbasu@suki.ai>

Lint

9ce435d

Signed-off-by: subhramit-suki <sbasu@suki.ai>

Lint

743ca94

Signed-off-by: subhramit-suki <sbasu@suki.ai>

subhramit added 3 commits May 12, 2026 00:08

Improve comment

62909cb

Signed-off-by: subhramit <subhramit.bb@live.in>

Merge branch 'master' into truncate-large-binary-logs

0342486

Remove extra space from comment

642b17f

Signed-off-by: subhramit <subhramit.bb@live.in>

This comment was marked as resolved.

Sign in to view

subhramit added 2 commits May 12, 2026 02:14

Merge branch 'master' into truncate-large-binary-logs

5f91e2e

Merge branch 'master' into truncate-large-binary-logs

0f386da

subhramit commented May 12, 2026

View reviewed changes

subhramit added 2 commits May 12, 2026 16:49

Remove redundant info in comment

1528afc

Signed-off-by: subhramit <subhramit.bb@live.in>

Merge branch 'master' into truncate-large-binary-logs

4456bdc

subhramit commented May 12, 2026

View reviewed changes

subhramit added 3 commits May 12, 2026 21:06

Merge branch 'master' into truncate-large-binary-logs

a232ac4

Merge branch 'master' into truncate-large-binary-logs

a66e633

Merge branch 'master' into truncate-large-binary-logs

3748168

agaudreault reviewed May 14, 2026

View reviewed changes

Comment thread pkg/apis/application/v1alpha1/types.go

agaudreault mentioned this pull request May 14, 2026

Breaking: Wrap RawExtension type in a custom type #27860

Open

agaudreault approved these changes May 14, 2026

View reviewed changes

agaudreault merged commit c61c1dc into argoproj:master May 14, 2026
28 checks passed

subhramit deleted the truncate-large-binary-logs branch May 22, 2026 16:34

	"v1GroupKind": {
	"description": "+protobuf.options.(gogoproto.goproto_stringer)=false",

	"intstrIntOrString": {
	"description": "+protobuf=true\n+protobuf.options.(gogoproto.goproto_stringer)=false\n+k8s:openapi-gen=true",

Uh oh!

Conversation

subhramit commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bunnyshell Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Preview Environment deleted from Bunnyshell

Uh oh!

codecov Bot commented May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

subhramit commented May 2, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

subhramit May 2, 2026

Choose a reason for hiding this comment

Uh oh!

subhramit May 2, 2026

Choose a reason for hiding this comment

Uh oh!

subhramit May 2, 2026

Choose a reason for hiding this comment

Uh oh!

subhramit May 2, 2026 • 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!

subhramit May 2, 2026 • 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!

subhramit May 2, 2026

Choose a reason for hiding this comment

Uh oh!

Qodo-Free-For-OSS commented May 3, 2026

Issue description

Issue Context

Fix Focus Areas

Suggested approach

Uh oh!

subhramit commented May 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agaudreault May 6, 2026

Choose a reason for hiding this comment

Uh oh!

subhramit May 7, 2026 • 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!

subhramit May 8, 2026 • 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!

subhramit May 9, 2026

Choose a reason for hiding this comment

Uh oh!

agaudreault May 11, 2026

Choose a reason for hiding this comment

Uh oh!

subhramit May 12, 2026 • 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!

This comment was marked as resolved.

subhramit May 12, 2026 • 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!

subhramit May 12, 2026 • 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!

agaudreault left a comment

subhramit commented May 2, 2026 •

edited

Loading

bunnyshell Bot commented May 2, 2026 •

edited

Loading

codecov Bot commented May 2, 2026 •

edited

Loading

subhramit May 2, 2026 •

edited

Loading

subhramit May 2, 2026 •

edited

Loading

subhramit commented May 3, 2026 •

edited

Loading

subhramit May 7, 2026 •

edited

Loading

subhramit May 8, 2026 •

edited

Loading

subhramit May 12, 2026 •

edited

Loading

subhramit May 12, 2026 •

edited

Loading

subhramit May 12, 2026 •

edited

Loading