HELM-763: Add secrets for Helm release upgrade#16787
Conversation
|
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 (5)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
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 URL basic-auth flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HelmInstallUpgradePage
participant HelmInstallUpgradeForm
participant HelmHandlers
participant UpgradeReleaseAsync
participant HelmRegistry
HelmInstallUpgradePage->>HelmInstallUpgradeForm: provide URL-install state and Secret name
HelmInstallUpgradeForm->>HelmHandlers: submit basicAuthSecretName
HelmHandlers->>UpgradeReleaseAsync: pass Secret name
UpgradeReleaseAsync->>HelmRegistry: locate chart with authentication
HelmRegistry-->>UpgradeReleaseAsync: chart or authentication error
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
|
/lgtm |
|
Scheduling tests matching the |
sowmya-sl
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sowmya-sl: This pull request references HELM-763 which is a valid jira issue. 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.
@sowmya-sl a few things to address, see inline comments for details.
| onClose?: () => void; | ||
| } | ||
|
|
||
| const HelmCreateBasicAuthSecretModal: OverlayComponent<HelmCreateBasicAuthSecretModalProps> = ({ |
There was a problem hiding this comment.
This modal calls k8sCreate to create Kubernetes Secrets but has no unit tests. Would be good to cover at least:
- Required field validation (Create button disabled when fields empty)
- Correct Secret shape passed to
k8sCreate(kubernetes.io/basic-authtype,stringDatawith username/password) savecallback invoked with the created secret name on success- Error state rendered on
k8sCreatefailure - Cancel/close behavior
Same gap for the secret dropdown + missing-secret warning in HelmInstallUpgradeForm.tsx.... that form already has a test file at __tests__/HelmInstallUpgradeForm.spec.tsx that could be extended.
There was a problem hiding this comment.
Added unit tests for HelmCreateBasicAuthSecretModal in a new test file at __tests__/HelmCreateBasicAuthSecretModal.spec.tsx
| isOpen | ||
| onClose={() => closeModal(true)} | ||
| title={t('Create authentication Secret')} | ||
| variant={ModalVariant.medium} |
There was a problem hiding this comment.
PF6 Modal doesn't have a title prop, this ends up as an HTML title attribute creating an unwanted browser tooltip on hover. The heading comes from <ModalHeader> below. Remove this prop and add aria-labelledby + labelId on ModalHeader to match codebase convention (see CreateProjectModal, HelmReadmeModal, etc).
|
|
||
| <FormGroup label={t('Secret username')} isRequired fieldId="helm-secret-username"> | ||
| <TextInput | ||
| id="helm-secret-username" |
There was a problem hiding this comment.
Missing isRequired on this TextInput, unlike the Secret name and password fields. The FormGroup shows a required asterisk but without isRequired on the input, screen readers won't announce aria-required. Add it for a11y parity.
| const launchHelmCreateBasicAuthSecretModal = useHelmCreateBasicAuthSecretModal(); | ||
| const [isCreateSecretModalOpen, setIsCreateSecretModalOpen] = useState(false); | ||
|
|
||
| const CREATE_SECRET_KEY = 'create-secret'; |
There was a problem hiding this comment.
CREATE_SECRET_KEY and NONE_SECRET_KEY are defined inside the component body, recreated each render. NONE_SECRET_KEY is also used in the secretMissing useMemo without being in the deps array - react-hooks/exhaustive-deps would flag this. Hoist both to module scope.
| setFieldValue('basicAuthSecretName', name); | ||
| }; | ||
|
|
||
| const handleSecretChange = (key: string) => { |
There was a problem hiding this comment.
This handleSecretChange / handleSecretSave / modal-launch logic is nearly identical to HelmURLChartForm.tsx (~25 lines). Consider extracting a shared hook (e.g. useBasicAuthSecretDropdown).... consistent with how useSecretResources was already pulled out.
|
|
||
| const handleSecretChange = (key: string) => { | ||
| if (key === NONE_SECRET_KEY) { | ||
| window.setTimeout(() => setFieldValue('basicAuthSecretName', NONE_SECRET_KEY), 0); |
There was a problem hiding this comment.
window.setTimeout(fn, 0) to race ResourceDropdownField's internal Formik write is a coupling to its implementation detail. If the dropdown ever updates via useEffect or batches differently, this silently breaks. Worth checking if ResourceDropdownField supports a controlled mode or pre-update callback to avoid this.
There was a problem hiding this comment.
Good catch. I checked the ResourceDropdownField. Its onChange calls props.Onchange() first and then calls setFieldValue(..) synchronously on the same line. So the setTimeout(..) fires after write.
A cleaner fix would be to add a preventFormikWrite return value or controlled mode to ResourceDropdownField, but that's a change to a shared component and out of scope for this PR.
| secretsDriver := driver.NewSecrets(coreClient.Secrets(tt.releaseNamespace)) | ||
| go func() { | ||
| upgradeResult, upgradeErr := UpgradeReleaseAsync(tt.releaseNamespace, tt.releaseName, "", nil, actionConfig, dynamicClient, coreClient, true, "") | ||
| upgradeResult, upgradeErr := UpgradeReleaseAsync(tt.releaseNamespace, tt.releaseName, "", nil, actionConfig, dynamicClient, coreClient, true, "", "") |
There was a problem hiding this comment.
This test passes "" as basicAuthSecretName, only exercising the annotation-fallback path. Missing test cases for:
- Non-empty
basicAuthSecretNameoverriding the annotation value "__none__"sentinel clearing the annotation-based secret- The
klog.Errorf->return errorchange forGetUserCredentials/applyBasicAuthFromUserCredentialsfailures (behavior change, errors now propagate instead of being silently logged) - 401/unauthorized error enrichment
| ...(chartURL ? { chart_url: chartURL } : {}), // eslint-disable-line @typescript-eslint/naming-convention | ||
| ...(indexEntry ? { indexEntry } : { indexEntry: chartIndexEntry }), | ||
| ...(valuesObj ? { values: valuesObj } : {}), | ||
| ...(values.isURLInstall ? { basic_auth_secret_name: basicAuthSecretName } : {}), // eslint-disable-line @typescript-eslint/naming-convention |
There was a problem hiding this comment.
The auth secret selection/clearing during upgrade has no e2e coverage. The existing Cypress features (install-url-chart.feature) test the general upgrade flow but not the new secret interactions. Worth adding at least a basic scenario.
There was a problem hiding this comment.
Agreed this would be valuable. However, adding e2e coverage for the auth secret interactions requires a private chart registry in the test environment (to trigger the auth flow end-to-end), which is new infrastructure not currently available in CI.
I'll file a follow-up ticket to add this once we have a test registry setup.
openshift-ci
Bot
added
needs-rebase
…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
- 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>
- Add unit tests for HelmCreateBasicAuthSecretModal covering field validation, k8sCreate shape, save callback, error state, and cancel - Add backend tests for explicit secret override, __none__ sentinel clearing, and credential lookup error propagation - Extract useBasicAuthSecretDropdown shared hook from duplicated logic in HelmInstallUpgradeForm and HelmURLChartForm - Hoist CREATE_SECRET_KEY and NONE_SECRET_KEY to module scope - Use NONE_SECRET_KEY constant in HelmInstallUpgradePage payload - Fix PF6 Modal: remove title prop, add aria-labelledby + labelId - Add missing isRequired on username TextInput for a11y parity - Export HelmCreateBasicAuthSecretModal for test imports Co-authored-by: Cursor <cursoragent@cursor.com>
16c16b3 to
a90ac5e
Compare
openshift-ci
Bot
removed
the
needs-rebase
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)
frontend/packages/helm-plugin/locales/en/helm-plugin.json (1)
2-42: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRemove the duplicate localization keys.
The inserted blocks redeclare many keys already present later in the same JSON object (
None,Select,Next,Chart URL,Helm release, and others). Biome reports these as errors, so the locale file can fail linting and different parsers may retain different values. Keep one definition per key in the canonical locale block.Also applies to: 55-89
🤖 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/locales/en/helm-plugin.json` around lines 2 - 42, Remove the newly inserted duplicate localization entries from the English Helm plugin locale object, including keys such as “Chart URL”, “None”, “Select”, “Next”, and “Helm release” that already exist in the canonical locale block. Preserve one definition of each key and retain the existing canonical values so the JSON contains no duplicate keys.Source: Linters/SAST tools
🤖 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 109-117: Guard rel.Chart.Metadata before accessing its Annotations
when copying the installation value in both upgrade paths:
pkg/helm/actions/upgrade_release.go lines 109-117 and UpgradeReleaseAsync at
lines 263-265. Preserve the existing chart annotation updates, but only read
rel.Chart.Metadata.Annotations when metadata is non-nil.
---
Outside diff comments:
In `@frontend/packages/helm-plugin/locales/en/helm-plugin.json`:
- Around line 2-42: Remove the newly inserted duplicate localization entries
from the English Helm plugin locale object, including keys such as “Chart URL”,
“None”, “Select”, “Next”, and “Helm release” that already exist in the canonical
locale block. Preserve one definition of each key and retain the existing
canonical values so the JSON contains no duplicate keys.
🪄 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: bca7a3b0-0889-45d1-a6d8-b6103060270b
📒 Files selected for processing (13)
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.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/__tests__/HelmCreateBasicAuthSecretModal.spec.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/useBasicAuthSecretDropdown.tspkg/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
🚧 Files skipped from review as they are similar to previous changes (11)
- pkg/helm/actions/get_chart.go
- pkg/helm/handlers/handler_test.go
- frontend/packages/helm-plugin/src/components/forms/url-chart/useBasicAuthSecretDropdown.ts
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx
- pkg/helm/handlers/handlers.go
- pkg/helm/actions/install_chart.go
- frontend/packages/helm-plugin/src/components/forms/url-chart/tests/HelmCreateBasicAuthSecretModal.spec.tsx
- frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradePage.tsx
- frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx
- pkg/helm/actions/upgrade_release_test.go
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx
Fix i18n locale file. Also add guard for rel.Chart.Metadata reference.
db0dfc0 to
03abe52
Compare
|
/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. |
jhadvig
left a comment
jhadvig
left a comment
There was a problem hiding this comment.
Two minor items from a second pass:
-
The old i18n key
"A secret with \"username\" and \"password\" keys for OCI/HTTP(S) authentication"(line 9 ofhelm-plugin.json) is orphaned - no code references it anymore after the switch to the{{username}}/{{password}}interpolated variant. Runningyarn i18nshould clean it up. -
See inline comment on
useBasicAuthSecretDropdown.ts.
| import { useState, useCallback } from 'react'; | ||
| import { useHelmCreateBasicAuthSecretModal } from './HelmCreateBasicAuthSecretModal'; | ||
|
|
||
| const CREATE_SECRET_KEY = 'create-secret'; |
There was a problem hiding this comment.
'create-secret' is a valid Kubernetes secret name. If a user happens to have a secret with that exact name, clicking it in the dropdown matches key === CREATE_SECRET_KEY and opens the Create Secret modal instead of selecting the existing secret. NONE_SECRET_KEY avoids this with '__none__'.... same convention here would fix it, e.g. '__create_secret__'.
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
Looks good...just one nit for your consideration.
/lgtm
| // "__none__" is a sentinel used by the upgrade path to clear auth; reject it as a secret name. | ||
| if (trimmedSecretName === '__none__') { |
There was a problem hiding this comment.
Should this be referencing NONE_SECRET_KEY instead of the literal '__none__'?
There was a problem hiding this comment.
Yes, but we can't import NONE_SECRET_KEY here because it would create a circular dependency
|
Scheduling tests matching the |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
|
@sowmya-sl: This pull request references HELM-763 which is a valid jira issue. 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.
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)
230-234: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up TLS files on authentication failures.
setUpAuthentication*can populatetlsFilesbefore these returns, but cleanup is deferred only inside the goroutine at Lines 283-291. WithfileCleanUpenabled, credential lookup or application failures therefore leak temporary files. Use a shared cleanup closure for these early returns, while transferring cleanup ownership to the goroutine only after it starts.🤖 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 230 - 234, Ensure authentication failures in the upgrade flow clean up any TLS files created by setUpAuthentication* before returning. Define a shared cleanup closure for the early credential lookup and applyBasicAuthFromUserCredentials error paths, then transfer cleanup ownership to the goroutine only after it starts so cleanup occurs exactly once.
🤖 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 230-234: Ensure authentication failures in the upgrade flow clean
up any TLS files created by setUpAuthentication* before returning. Define a
shared cleanup closure for the early credential lookup and
applyBasicAuthFromUserCredentials error paths, then transfer cleanup ownership
to the goroutine only after it starts so cleanup occurs exactly once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 792bb7b6-a112-49e2-bea0-91fc8669dc12
📒 Files selected for processing (6)
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/url-chart/HelmCreateBasicAuthSecretModal.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/useBasicAuthSecretDropdown.tspkg/helm/actions/upgrade_release.go
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/packages/helm-plugin/src/components/forms/url-chart/useBasicAuthSecretDropdown.ts
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx
- frontend/packages/helm-plugin/src/components/forms/install-upgrade/HelmInstallUpgradeForm.tsx
- frontend/packages/helm-plugin/src/components/forms/url-chart/HelmCreateBasicAuthSecretModal.tsx
✅ Action performedReviews resumed. |
- Fix autocompleteFilter crash when fuzzysearch receives undefined haystack for non-React-element items; fall back to string value or key - Rename CREATE_SECRET_KEY from 'create-secret' to '__create_secret__' to prevent collision with a real Kubernetes secret of that name - Align HelmURLInstallForm help text to use interpolated i18n variant and remove orphaned locale key Co-authored-by: Cursor <cursoragent@cursor.com>
openshift-ci-robot
removed
the
verified
|
/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. |
|
@jhadvig Addressed your comments. Please check. |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: martinszuc, sowmya-sl, webbnh 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 |
|
Did you see Coderabbitai's concern?
It sounds like some of your additions contribute to this problem. |
|
/test backend |
|
/retest |
|
@webbnh The path where we use repository and TLS files and the path where we use authentication secret are mutually exclusive. So there is no fear of TLS files being created and not deleted later. |
|
@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. |
5 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
kubernetes.io/basic-authSecrets with required validation and reserved-name protection, integrated into the wizard flow.usernameandpassword.__none__clearing and more defensive annotation copying.