Skip to content

Migrate HelmRepository to runtime/secrets #1849

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

Conversation

cappyzawa
Copy link
Contributor

@cappyzawa cappyzawa commented Jul 13, 2025

This PR migrates HelmRepository authentication to use runtime/secrets API as part of fluxcd/flux2#5433.

Introduces pkg/runtime v0.66.0 and refactors HelmRepository authentication accordingly.

Breaking Changes (security improvement)

  • Empty TLS secrets now return validation errors instead of being silently ignored
  • Invalid CA certificates with public repos now fail

@cappyzawa cappyzawa force-pushed the feat/helm-repository-runtime-secrets-migration branch from 4733f8f to 2d22893 Compare July 13, 2025 17:38
@cappyzawa cappyzawa marked this pull request as draft July 13, 2025 17:39
@cappyzawa cappyzawa force-pushed the feat/helm-repository-runtime-secrets-migration branch from 2d22893 to 11522d2 Compare July 13, 2025 17:54
@cappyzawa cappyzawa changed the title feat: migrate HelmRepository to runtime/secrets Migrate HelmRepository to runtime/secrets Jul 13, 2025
@cappyzawa cappyzawa marked this pull request as ready for review July 13, 2025 18:28
@cappyzawa cappyzawa marked this pull request as draft July 17, 2025 12:55
@cappyzawa cappyzawa force-pushed the feat/helm-repository-runtime-secrets-migration branch from d8c26cc to fd2a017 Compare July 17, 2025 18:34
@cappyzawa cappyzawa force-pushed the feat/helm-repository-runtime-secrets-migration branch 2 times, most recently from 866f087 to 3cb0187 Compare July 17, 2025 19:29
@cappyzawa cappyzawa marked this pull request as ready for review July 17, 2025 19:52
Copy link
Member

@matheuscscp matheuscscp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

This is the best refactor I have reviewed so far in source-controller, and the most impressive part is that we are not breaking any APIs!

hrOpts.Insecure = obj.Spec.Insecure
// Handle TLS certificate files for OCI
var tempCertDir string
if opts.TlsConfig != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed helm/helm#31075 and helm/helm#31076 so we can stop doing this crazy dance and start leveraging our shiny new secrets library ;)

This commit upgrades pkg/runtime to v0.73.0 and adopts the unified
AuthMethodsFromSecret API for HelmRepository authentication handling.
The change replaces complex manual authentication detection with a
single API call and improves error handling consistency.

Breaking Changes:
- TLS certificate validation is now strictly enforced. Invalid CA
  certificates will cause authentication failures even for public
  repositories, where they were previously ignored.
- Empty TLS certificate secrets now trigger validation errors instead
  of being silently ignored. This affects certSecretRef with empty
  Data map - previously ignored, now causes proper error.

Signed-off-by: cappyzawa <[email protected]>
@cappyzawa cappyzawa force-pushed the feat/helm-repository-runtime-secrets-migration branch from 3cb0187 to 9950f56 Compare July 18, 2025 12:30
@cappyzawa
Copy link
Contributor Author

@matheuscscp Thanks a lot for your review and kind words!
I've addressed all the points you mentioned.
Since you had already approved the PR, I updated the commits with force-with-lease so it can be merged right away.

@matheuscscp matheuscscp merged commit 173a1cc into fluxcd:main Jul 18, 2025
8 checks passed
@cappyzawa cappyzawa deleted the feat/helm-repository-runtime-secrets-migration branch July 18, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants