HELM-728: Persist secrets for Helm upgrade#16432
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
component/backend
sowmya-sl
force-pushed
the
add-secrets-for-helm-upgrade
branch
3 times, most recently
from
c2dac6d to
92da9e3
Compare
openshift-ci
Bot
added
the
needs-rebase
sowmya-sl
force-pushed
the
add-secrets-for-helm-upgrade
branch
2 times, most recently
from
e935d09 to
19338e5
Compare
openshift-ci
Bot
removed
the
needs-rebase
WalkthroughRefactors Helm basic-auth handling: introduce RegistryClientSetter and chart annotation, change helper to set ChartPathOptions and inject an OCI registry client, update install/get to use it, and re-apply persisted auth during upgrades (logged on failure). ChangesHelm basic-auth credential lifecycle
Sequence Diagram(s)sequenceDiagram
participant Client
participant InstallChartFromURL
participant applyBasicAuthFromUserCredentials
participant GetOCIRegistry
participant cmd_SetRegistryClient
Client->>InstallChartFromURL: request install from URL
InstallChartFromURL->>applyBasicAuthFromUserCredentials: pass &ChartPathOptions, cmd (setter), credentials
applyBasicAuthFromUserCredentials->>GetOCIRegistry: GetOCIRegistry(auth)
GetOCIRegistry-->>applyBasicAuthFromUserCredentials: registry.Client
applyBasicAuthFromUserCredentials->>cmd_SetRegistryClient: SetRegistryClient(registry.Client)
participant UpgradeReleaseAsync
participant GetUserCredentials
Client->>UpgradeReleaseAsync: request upgrade
UpgradeReleaseAsync->>UpgradeReleaseAsync: read helmAuthSecretAnnotation
UpgradeReleaseAsync->>GetUserCredentials: fetch credentials
GetUserCredentials-->>UpgradeReleaseAsync: credentials
UpgradeReleaseAsync->>applyBasicAuthFromUserCredentials: apply to ChartPathOptions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Use a RegistryClientSetter interface so applyBasicAuthFromUserCredentials works with both action.Install and action.Upgrade via their shared ChartPathOptions. Persist the basic-auth secret name as a chart annotation (helm.openshift.io/auth-secret) during install and propagate it on upgrade so authenticated registries remain accessible across the release lifecycle.
sowmya-sl
force-pushed
the
add-secrets-for-helm-upgrade
branch
from
19338e5 to
f08c6e8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helm/actions/upgrade_release.go (1)
225-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add the auth-secret annotation to the upgraded chart.
When
chartUrlis set,loader.Loadgives you a fresh chart object. This code restoreschart_urlbut never writes backhelm.openshift.io/auth-secret, so the upgraded release drops the secret reference and the next upgrade can no longer rehydrate credentials.Suggested fix
// Ensure chart URL is properly set in the upgrade chart + if ch.Metadata == nil { + ch.Metadata = &chart.Metadata{} + } + if ch.Metadata.Annotations == nil { + ch.Metadata.Annotations = make(map[string]string) + } if chartUrl != "" { - if ch.Metadata.Annotations == nil { - ch.Metadata.Annotations = make(map[string]string) - } ch.Metadata.Annotations["chart_url"] = chartUrl } + addAuthSecretAnnotation(ch, auth_secret) if auth_secret != "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helm/actions/upgrade_release.go` around lines 225 - 245, The upgrade logic restores the chart_url annotation on the freshly loaded chart (chartUrl -> ch.Metadata.Annotations["chart_url"]) but never re-adds the persisted auth-secret annotation, so upgraded releases lose the "helm.openshift.io/auth-secret" reference; fix by, when auth_secret != "" and after ensuring ch.Metadata.Annotations is non-nil, set ch.Metadata.Annotations["helm.openshift.io/auth-secret"] = auth_secret (the same place where you already set "chart_url" and where you call GetUserCredentials/applyBasicAuthFromUserCredentials) so the secret reference is preserved across upgrades.
🧹 Nitpick comments (1)
pkg/helm/actions/upgrade_release.go (1)
233-240: ⚡ Quick winUse repo-standard klog levels here.
The "found persisted auth secret" message is debug-level, while credential lookup failures should be logged as errors instead of
Infof.Suggested fix
- klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) + klog.V(4).Infof("Found persisted auth secret for release %s/%s, applying credentials for upgrade", releaseNamespace, releaseName) userCredentials, err := GetUserCredentials(coreClient, releaseNamespace, auth_secret) if err != nil { - klog.Infof("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err) + klog.Errorf("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err)As per coding guidelines, "Use
klogwith appropriate levels (V(4) for debug, Error, Fatal) for Go logging".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helm/actions/upgrade_release.go` around lines 233 - 240, The log level usage is inconsistent: change the "Found persisted auth secret ..." Infof to a debug-level call using klog.V(4).Infof, and change the GetUserCredentials failure log from klog.Infof to klog.Errorf so credential lookup failures are logged as errors; keep the existing klog.Errorf for applyBasicAuthFromUserCredentials as-is. Update the logging calls near GetUserCredentials, auth_secret, releaseNamespace, releaseName, and applyBasicAuthFromUserCredentials accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 232-245: UpgradeReleaseAsync currently calls
client.ChartPathOptions.LocateChart before applying persisted auth, so private
OCI/direct chart fetch can’t use the credentials; move the block that checks
auth_secret and calls GetUserCredentials + applyBasicAuthFromUserCredentials to
run before the call to client.ChartPathOptions.LocateChart. Keep the same error
handling/logging (klog.Infof on GetUserCredentials failure, klog.Errorf on apply
failure) and ensure you reference the same variables (auth_secret,
releaseNamespace, releaseName, client.ChartPathOptions) so LocateChart runs with
the applied credentials.
---
Outside diff comments:
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 225-245: The upgrade logic restores the chart_url annotation on
the freshly loaded chart (chartUrl -> ch.Metadata.Annotations["chart_url"]) but
never re-adds the persisted auth-secret annotation, so upgraded releases lose
the "helm.openshift.io/auth-secret" reference; fix by, when auth_secret != ""
and after ensuring ch.Metadata.Annotations is non-nil, set
ch.Metadata.Annotations["helm.openshift.io/auth-secret"] = auth_secret (the same
place where you already set "chart_url" and where you call
GetUserCredentials/applyBasicAuthFromUserCredentials) so the secret reference is
preserved across upgrades.
---
Nitpick comments:
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 233-240: The log level usage is inconsistent: change the "Found
persisted auth secret ..." Infof to a debug-level call using klog.V(4).Infof,
and change the GetUserCredentials failure log from klog.Infof to klog.Errorf so
credential lookup failures are logged as errors; keep the existing klog.Errorf
for applyBasicAuthFromUserCredentials as-is. Update the logging calls near
GetUserCredentials, auth_secret, releaseNamespace, releaseName, and
applyBasicAuthFromUserCredentials accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e4c0f55-cdf2-4023-8777-ae5444398f6a
📒 Files selected for processing (3)
pkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/upgrade_release.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/helm/actions/upgrade_release.go (1)
225-245:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-add the auth-secret annotation to the upgraded chart.
auth_secretis read from the previous release, but the new chart metadata only writeschart_url. After this upgrade, the next revision loseshelm.openshift.io/auth-secret, so a subsequent private upgrade can no longer recover the persisted credentials.💡 Minimal fix
// Ensure chart URL is properly set in the upgrade chart - if chartUrl != "" { - if ch.Metadata.Annotations == nil { - ch.Metadata.Annotations = make(map[string]string) - } + if ch.Metadata == nil { + ch.Metadata = &chart.Metadata{} + } + if ch.Metadata.Annotations == nil { + ch.Metadata.Annotations = make(map[string]string) + } + if chartUrl != "" { ch.Metadata.Annotations["chart_url"] = chartUrl } + addAuthSecretAnnotation(ch, auth_secret) if auth_secret != "" {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helm/actions/upgrade_release.go` around lines 225 - 245, The upgraded chart currently only writes chart_url and omits persisting auth_secret, causing future revisions to lose helm.openshift.io/auth-secret; modify the upgrade logic (around chartUrl, ch.Metadata.Annotations, auth_secret handling) to ensure ch.Metadata.Annotations is initialized whenever either chartUrl or auth_secret is non-empty and set both ch.Metadata.Annotations["chart_url"] = chartUrl (if present) and ch.Metadata.Annotations["helm.openshift.io/auth-secret"] = auth_secret (if present); keep the existing GetUserCredentials/applyBasicAuthFromUserCredentials flow unchanged.
🧹 Nitpick comments (1)
pkg/helm/actions/upgrade_release.go (1)
232-240: ⚡ Quick winUse
V(4)/Errorffor theseklogcalls.The “found persisted auth secret” message is debug-level noise, while the credential lookup failure is an error path.
Infoffor both does not match the repo’s logging rule.As per coding guidelines, `Use klog with appropriate levels (V(4) for debug, Error, Fatal) for Go logging`.💡 Suggested change
- klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) + klog.V(4).Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) userCredentials, err := GetUserCredentials(coreClient, releaseNamespace, auth_secret) if err != nil { - klog.Infof("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err) + klog.Errorf("Failed to get user credentials for release upgrade %s/%s: %v", releaseNamespace, releaseName, err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helm/actions/upgrade_release.go` around lines 232 - 240, Replace the inappropriate Info-level logs around auth secret handling: change the "Found persisted auth secret ..." klog.Infof to debug-level klog.V(4).Infof, and change the credential lookup failure log inside the GetUserCredentials error branch from klog.Infof to klog.Errorf; the functions/vars to edit are auth_secret, GetUserCredentials, applyBasicAuthFromUserCredentials, and the klog calls shown in the snippet so the debug message is V(4) and lookup failures are logged as errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 225-245: The upgraded chart currently only writes chart_url and
omits persisting auth_secret, causing future revisions to lose
helm.openshift.io/auth-secret; modify the upgrade logic (around chartUrl,
ch.Metadata.Annotations, auth_secret handling) to ensure ch.Metadata.Annotations
is initialized whenever either chartUrl or auth_secret is non-empty and set both
ch.Metadata.Annotations["chart_url"] = chartUrl (if present) and
ch.Metadata.Annotations["helm.openshift.io/auth-secret"] = auth_secret (if
present); keep the existing GetUserCredentials/applyBasicAuthFromUserCredentials
flow unchanged.
---
Nitpick comments:
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 232-240: Replace the inappropriate Info-level logs around auth
secret handling: change the "Found persisted auth secret ..." klog.Infof to
debug-level klog.V(4).Infof, and change the credential lookup failure log inside
the GetUserCredentials error branch from klog.Infof to klog.Errorf; the
functions/vars to edit are auth_secret, GetUserCredentials,
applyBasicAuthFromUserCredentials, and the klog calls shown in the snippet so
the debug message is V(4) and lookup failures are logged as errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3467ec1d-cfbb-47ad-a798-2dc23b6c5a34
📒 Files selected for processing (3)
pkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/upgrade_release.go
sowmya-sl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sowmya-sl: This pull request references HELM-728 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| if rel.Chart.Metadata.Annotations != nil { | ||
| if chart_url, ok := rel.Chart.Metadata.Annotations["chart_url"]; chartUrl == "" && ok { | ||
| chartUrl = chart_url | ||
| } |
There was a problem hiding this comment.
Should the sync UpgradeRelease path also read helmAuthSecretAnnotation here?
There was a problem hiding this comment.
It's being read in the UpgradeReleaseAsync function which is the one being called from the frontend. UpgradeRelease is not being called anywhere.
| @@ -224,6 +229,20 @@ | |||
| } | |||
| ch.Metadata.Annotations["chart_url"] = chartUrl | |||
There was a problem hiding this comment.
Should helmAuthSecretAnnotation also be written here alongside chart_url? My thinking is that on a second upgrade, the annotation would already be gone from the stored release, so the secret lookup would fail. Could be wrong though, curious what you think. I have not tested out the changes myself.
There was a problem hiding this comment.
Good catch, I have added it now.
Ensure addAuthSecretAnnotation is called when rewriting chart metadata so subsequent upgrades continue to resolve registry credentials.
Credentials were being set after LocateChart, which is the call that actually contacts the registry. Move applyBasicAuthFromUserCredentials before LocateChart so the registry client has auth headers when pulling.
|
/verified by @sowmya-sl |
openshift-ci-robot
added
the
verified
|
@sowmya-sl: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@sowmya-sl: This pull request references HELM-728 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/helm/actions/upgrade_release.go (1)
207-208: ⚡ Quick winDemote this to debug and avoid logging the secret reference.
This runs on every authenticated upgrade, so emitting
auth_secretat info level is noisier than needed and exposes secret metadata in normal logs.Suggested change
- klog.Infof("Found persisted auth secret %s for release %s/%s, applying credentials for upgrade", auth_secret, releaseNamespace, releaseName) + klog.V(4).Infof("Applying persisted auth credentials for release %s/%s", releaseNamespace, releaseName)As per coding guidelines, "Use klog with appropriate levels (V(4) for debug, Error, Fatal) for logging in Go".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/helm/actions/upgrade_release.go` around lines 207 - 208, Change the info-level log that currently prints the auth_secret variable to a debug-level log and stop emitting the secret reference: replace the klog.Infof call that mentions auth_secret with klog.V(4).Infof and remove the auth_secret interpolation so it only logs context like "found persisted auth secret for release %s/%s, applying credentials for upgrade" (refer to the klog.Infof line and the auth_secret variable in upgrade_release.go).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/helm/actions/upgrade_release.go`:
- Around line 207-208: Change the info-level log that currently prints the
auth_secret variable to a debug-level log and stop emitting the secret
reference: replace the klog.Infof call that mentions auth_secret with
klog.V(4).Infof and remove the auth_secret interpolation so it only logs context
like "found persisted auth secret for release %s/%s, applying credentials for
upgrade" (refer to the klog.Infof line and the auth_secret variable in
upgrade_release.go).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b0c8b733-c16f-4d36-b565-d0da5ffed64f
📒 Files selected for processing (1)
pkg/helm/actions/upgrade_release.go
|
@sowmya-sl: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Analysis / Root cause:
Helm releases are installed via URL charts which use authentication. This authentication is stored as a generic secret. But this is not persisted during upgrade, which leads to upgrade failing.
Solution description:
The secret for authentication is added as a metadata for the Helm release. So while upgrade, it is fetched and used for authentication.
Summary by Coderabbit
New Features
Bug Fixes
Summary by CodeRabbit
Bug Fixes
New Features