Add secrets for Helm release upgrade#16787
Conversation
…tching - Pass basicAuthSecretName through UpgradeReleaseAsync to support auth credentials during chart upgrades - Use __none__ sentinel to allow users to explicitly clear a secret - Preserve installation annotation on upgrade to maintain URL-install tracking across revisions - Return errors instead of logging on auth credential failures so the frontend can surface them - Add 401/unauthorized detection in GetChartFromURL, InstallChartFromURL, and UpgradeReleaseAsync with actionable error messages - Update handler signature, mock, and tests for the new parameter
…orms - Add HelmCreateBasicAuthSecretModal for creating kubernetes.io/basic-auth secrets with username/password from within the Helm forms - Add auth secret dropdown with 'None' and 'Create Secret' options to HelmInstallUpgradeForm, shown when upgrading a URL-installed chart - Detect URL-installed charts via 'installation' annotation and read initial auth secret from 'helm.openshift.io/auth-secret' annotation - Pass basicAuthSecretName through to the upgrade API payload - Use __none__ sentinel to allow explicitly clearing a previously set secret - Show warning when a previously referenced secret no longer exists - Add auth secret dropdown to HelmURLChartForm for URL-based chart installs
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughHelm URL installations and upgrades now support selecting or creating Kubernetes basic-auth Secrets. Secret names flow from chart annotations through frontend forms and handlers into Helm actions, which add authentication-specific errors and explicit Secret-clearing behavior. ChangesHelm basic-auth authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant HelmInstallUpgradePage
participant HelmInstallUpgradeForm
participant HelmHandler
participant HelmUpgradeRelease
User->>HelmInstallUpgradePage: open URL-based Helm install or upgrade
HelmInstallUpgradePage->>HelmInstallUpgradeForm: provide Secret name and URL-install state
HelmInstallUpgradeForm->>HelmHandler: submit basic-auth Secret name
HelmHandler->>HelmUpgradeRelease: pass basicAuthSecretName
HelmUpgradeRelease->>HelmUpgradeRelease: locate chart with Secret authentication
HelmUpgradeRelease-->>HelmHandler: return release or authentication error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)frontend/packages/helm-plugin/locales/en/helm-plugin.jsonTraceback (most recent call last): Comment |
openshift-ci
Bot
added
component/backend
![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/install_chart.go (1)
339-347: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMissing "none" sentinel handling — mirrors a gap fixed in
upgrade_release.go.
upgrade_release.go'sUpgradeReleaseAsynctreatsbasicAuthSecretName == "__none__"as an explicit "no auth" request and clears it before doing anything else.InstallChartFromURLhas no equivalent handling, so if a user selects "None" inHelmInstallUpgradeForm.tsxduring a fresh Create/install flow (the dropdown's "None" action item isn't gated to Upgrade-only), the literal string"__none__"is sent asbasicAuthSecretNamehere, andGetUserCredentials(coreClient, ns, "__none__")will fail looking up a Secret literally named"__none__"— producing a confusing error instead of the intended "no basic auth" behavior.🐛 Suggested fix (mirrors upgrade_release.go)
+ if basicAuthSecretName == "__none__" { + basicAuthSecretName = "" + } if basicAuthSecretName != "" { userCredentials, err := GetUserCredentials(coreClient, ns, basicAuthSecretName)🤖 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/install_chart.go` around lines 339 - 347, Update InstallChartFromURL before the basic-auth credential lookup to treat basicAuthSecretName == "__none__" as an explicit no-auth request by clearing it. Preserve the existing credential retrieval and applyBasicAuthFromUserCredentials flow for all other non-empty secret names.
🧹 Nitpick comments (2)
pkg/helm/actions/upgrade_release_test.go (1)
703-793: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test coverage for the
"__none__"explicit-clear sentinel.
TestUpgradeAfterURLInstallWithSecretsverifies the auth-secret annotation is preserved across an upgrade, but there's no test asserting that passingbasicAuthSecretName = "__none__"actually clearsauth_secretand skips credential lookup (the newexplicitlyClearedSecretlogic inupgrade_release.go). A regression there would silently restore the old secret via the annotation fallback instead of honoring the user's explicit "None" selection.🤖 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_test.go` around lines 703 - 793, The test suite needs coverage for the "__none__" explicit-clear path in TestUpgradeAfterURLInstallWithSecrets. Add a case that passes basicAuthSecretName as "__none__", verifies the upgrade does not look up credentials, and asserts the resulting chart metadata clears auth_secret instead of restoring the annotated secret through fallback.pkg/helm/actions/get_chart.go (1)
87-89: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExtract the duplicated "registry requires authentication" detection into a shared helper.
The same
strings.Contains(err.Error(), "401") || strings.Contains(err.Error(), "unauthorized")check, guarded by an emptybasicAuthSecretName, is copy-pasted into three files. A singleisAuthRequiredError(err error) bool(or similar) helper in theactionspackage would keep the heuristic (and any future refinement, e.g. adding "403"/"forbidden") consistent across all three call sites instead of relying on manual sync.
pkg/helm/actions/get_chart.go#L87-L89: replace the inlinestrings.Containscheck inGetChartFromURLwith a call to the shared helper.pkg/helm/actions/install_chart.go#L361-L363: replace the inline check inInstallChartFromURLwith the same helper.pkg/helm/actions/upgrade_release.go#L225-L241: replace the inline check inUpgradeReleaseAsyncwith the same helper.🤖 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/get_chart.go` around lines 87 - 89, The authentication-required error detection is duplicated across three call sites; extract it into one shared actions-package helper and reuse it while preserving the existing empty basicAuthSecretName guard. Update GetChartFromURL in pkg/helm/actions/get_chart.go (lines 87-89), InstallChartFromURL in pkg/helm/actions/install_chart.go (lines 361-363), and UpgradeReleaseAsync in pkg/helm/actions/upgrade_release.go (lines 225-241) to call the helper instead of inline strings.Contains checks.
🤖 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
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx`:
- Around line 83-84: Update the Helm install path to handle the __none__ secret
sentinel consistently with the upgrade path. In the install chart action,
recognize the NONE_SECRET_KEY value and skip secret creation or attachment when
selected, while preserving existing behavior for CREATE_SECRET_KEY and actual
secret keys.
---
Outside diff comments:
In `@pkg/helm/actions/install_chart.go`:
- Around line 339-347: Update InstallChartFromURL before the basic-auth
credential lookup to treat basicAuthSecretName == "__none__" as an explicit
no-auth request by clearing it. Preserve the existing credential retrieval and
applyBasicAuthFromUserCredentials flow for all other non-empty secret names.
---
Nitpick comments:
In `@pkg/helm/actions/get_chart.go`:
- Around line 87-89: The authentication-required error detection is duplicated
across three call sites; extract it into one shared actions-package helper and
reuse it while preserving the existing empty basicAuthSecretName guard. Update
GetChartFromURL in pkg/helm/actions/get_chart.go (lines 87-89),
InstallChartFromURL in pkg/helm/actions/install_chart.go (lines 361-363), and
UpgradeReleaseAsync in pkg/helm/actions/upgrade_release.go (lines 225-241) to
call the helper instead of inline strings.Contains checks.
In `@pkg/helm/actions/upgrade_release_test.go`:
- Around line 703-793: The test suite needs coverage for the "__none__"
explicit-clear path in TestUpgradeAfterURLInstallWithSecrets. Add a case that
passes basicAuthSecretName as "__none__", verifies the upgrade does not look up
credentials, and asserts the resulting chart metadata clears auth_secret instead
of restoring the annotated secret through fallback.
🪄 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: 048661ba-b60c-46bf-aafa-b785af51ccd5
📒 Files selected for processing (11)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsxfrontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsxpkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/upgrade_release.gopkg/helm/actions/upgrade_release_test.gopkg/helm/handlers/handler_test.gopkg/helm/handlers/handlers.go
| const CREATE_SECRET_KEY = 'create-secret'; | ||
| const NONE_SECRET_KEY = '__none__'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
"None" option reachable during fresh install, but install path lacks the "none" sentinel handling that upgrade has.
This will be covered together with the backend gap in a consolidated comment anchored at pkg/helm/actions/install_chart.go, since the root fix belongs there.
Also applies to: 234-243
🤖 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
`@frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx`
around lines 83 - 84, Update the Helm install path to handle the __none__ secret
sentinel consistently with the upgrade path. In the install chart action,
recognize the NONE_SECRET_KEY value and skip secret creation or attachment when
selected, while preserving existing behavior for CREATE_SECRET_KEY and actual
secret keys.
- Make 401/unauthorized error detection case-insensitive across get_chart, install_chart, and upgrade_release - Capitalize 'Basic' in 'Secret for Basic authentication' label - Extract secretName.trim() to a single variable in the modal - Remove duplicate locale key for 'Secret for Basic authentication' Co-authored-by: Cursor <cursoragent@cursor.com>
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: martinszuc, 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 |
|
@sowmya-sl: The following tests 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. |
2 participants
Analysis / Root cause:
Kubernetes Secret object is used to store username and password for authentication when trying to create a Helm release. If a Helm release is created using a URL and selecting a particular secret for authentication, the users are unable to change the secrets during upgrade. This poses a problem if the secret's name has been changed or removed.
Solution description:
Create a dropdown where the secret can be entered during upgrade. If no secret is entered, it will go with the existing secret name if it exists, otherwise the new secret will override the old one.
Screenshots / screen recording:
https://docs.google.com/document/d/1LDRXScwr_jOnvdWU0siqW1gBMM8hrhm3D0oz831gUPA/edit?tab=t.mcv80hqh092h#heading=h.2jo1jtiyytic
Test setup:
An OCI registry and a Helm repository requiring authentication via username and password.
Additional Information:
Rebased on #16676
Reviewers and assignees:
Summary by CodeRabbit