[ogulcanaydogan]

What's changed

Adds the repository URL to the message of the index parse failure in the HelmRepository reconciler. When the index is fetched but cannot be parsed (for example the server returns HTML instead of YAML), the FetchFailed message now is:

failed to load Helm repository from index YAML at https://example.com: <root cause>

The URL is passed through (*url.URL).Redacted() first, so a user:pass@ in spec.url does not end up in conditions or events.

The existing messages are kept as they were. In particular the CacheIndex error is untouched, because the Helm getter error already contains the full index URL.

Tests

• TestHelmRepositoryReconciler_reconcileSource_cachedIndexFile: the invalid index case now asserts the new message in the returned error and in the FetchFailed condition.
• A new case checks that a URL with credentials is redacted (user:xxxxx@, the password never shows up in the error or in the condition).

Both fail on main without the change.

Related

Closes #654

[ogulcanaydogan]

Friendly ping on this one. It is a small error message change in the HelmRepository reconciler. If it is welcome I can rebase it, and if it would be better to open an issue first or to drop it, just let me know

[stefanprodan]

Thanks for picking this up. A few issues before this can go in:

1. The URL is duplicated on fetch errors. Helm's HTTP getter error already includes the full index URL, so a 404 ends up as:

   looks like "https://example.com" is not a valid chart repository or cannot be reached: failed to fetch https://example.com/index.yaml : 404 Not Found
   

   Please keep the CacheIndex error as it is.

2. Credentials can leak. spec.url may contain user:pass@. Conditions and events must not contain secrets, so any URL we add to a message has to be redacted first, e.g. with (*url.URL).Redacted().

3. Keep the existing wording. "failed to fetch Helm repository index" and "failed to load Helm repository from index YAML" tell users which step failed. Please don't swap them for one generic message. Tools and alerts may also match on these strings.

4. Tests. Please add a test case to TestHelmRepositoryReconciler_reconcileSource that asserts the new FetchFailed message. For example, an index.yaml that's served with a 200 but isn't valid YAML.

The one case where the URL is missing is the index parse failure. Something like this would cover #654:

fmt.Errorf("failed to load Helm repository from index YAML at %s: %w", redactedURL, err)

Please also rebase on main.

[ogulcanaydogan]

Thanks @stefanprodan, this is a much better scope. I kept the CacheIndex error and both existing messages as they were, and the only change is the URL in the index parse failure, redacted with Redacted() as you suggested. You were right about the credentials, my first version would have put user:pass@ in the condition. I added the test to the invalid index case plus one that checks the password is redacted, and rebased on main

[stefanprodan]

LGTM

Thanks @ogulcanaydogan