HELM-703: Add authentication for Helm chart URLs#16360
HELM-703: Add authentication for Helm chart URLs#16360sowmya-sl wants to merge 4 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
|
@sowmya-sl: This pull request references HELM-703 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
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sowmya-sl The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/helm
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
sowmya-sl
force-pushed
the
add-auth-helm
branch
from
139ca66 to
355844a
Compare
📝 WalkthroughWalkthroughAdds optional OCI/HTTP basic-auth via Kubernetes Secrets across frontend and backend. Frontend: new localization entries, a namespaced Secret typeahead that filters for Secrets with Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@CodeRabbit help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/helm/handlers/handlers.go (1)
452-471:⚠️ Potential issue | 🟠 MajorInconsistent namespace usage in action configuration.
HandleURLChartGetreadsnamespacefrom query params (lines 454-457) but still passes hardcoded"default"togetActionConfigurationson line 465. This is inconsistent withHandleChartGet(line 238) which was fixed in this PR. Ifbasic_auth_secret_namereferences a Secret in the user-provided namespace, the action config needs the correct namespace to resolve it.🔧 Proposed fix
- conf := h.getActionConfigurations(h.ApiServerHost, "default", user.Token, &h.Transport) + conf := h.getActionConfigurations(h.ApiServerHost, namespace, user.Token, &h.Transport)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/handlers/handlers.go` around lines 452 - 471, HandleURLChartGet reads namespace from query params into the local variable namespace but still calls getActionConfigurations with the hardcoded "default"; update the call to getActionConfigurations inside HandleURLChartGet to pass the parsed namespace variable (namespace) instead of the string "default" so action configuration (used by NewHandlerClients and getChartFromURL) resolves Secrets like basic_auth_secret_name in the correct namespace.pkg/helm/actions/install_chart.go (1)
327-335:⚠️ Potential issue | 🟡 MinorPreserve original URL for traceability in OCI install path.
In
InstallChartFromURL, thechart_urlannotation stores the modified URL (version-stripped at line 315) rather than the original user-provided URL. While version trimming is intentional for OCI handling (stored separately inChartPathOptions.Version), the original URL is lost. For consistency withInstallChart/InstallChartAsyncand to maintain full traceability across release upgrades, capture the original URL before trimming and store it in the annotation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/install_chart.go` around lines 327 - 335, In InstallChartFromURL capture the original user-provided URL before you trim/version-strip it (e.g., save url to originalURL) and set ch.Metadata.Annotations["chart_url"] = originalURL while keeping the trimmed url used for OCI handling and ChartPathOptions.Version; ensure you still set ch.Metadata.Annotations["installation"] = "url_install" and that trimming logic (the code that mutates url and populates ChartPathOptions.Version) remains unchanged so traceability is preserved.
🧹 Nitpick comments (6)
pkg/helm/handlers/handler_test.go (1)
204-205: Assert the newbasicAuthSecretNameparameter in this mock.The new argument is accepted but ignored, so forwarding regressions in the handler path would still pass tests.
Suggested test hardening
-func fakeInstallChartFromURL(mockedSecret *kv1.Secret, err error) func(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration, coreClient corev1client.CoreV1Interface, version string, basicAuthSecretName string) (*kv1.Secret, error) { - return func(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration, coreClient corev1client.CoreV1Interface, version string, basicAuthSecretName string) (*kv1.Secret, error) { +func fakeInstallChartFromURL(mockedSecret *kv1.Secret, expectedBasicAuthSecretName string, t *testing.T, err error) func(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration, coreClient corev1client.CoreV1Interface, version string, basicAuthSecretName string) (*kv1.Secret, error) { + return func(ns string, name string, url string, values map[string]interface{}, conf *action.Configuration, coreClient corev1client.CoreV1Interface, version string, basicAuthSecretName string) (*kv1.Secret, error) { + if basicAuthSecretName != expectedBasicAuthSecretName { + t.Errorf("basicAuthSecretName mismatch: expected %q got %q", expectedBasicAuthSecretName, basicAuthSecretName) + } return mockedSecret, err } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/handlers/handler_test.go` around lines 204 - 205, The mock factory fakeInstallChartFromURL currently accepts basicAuthSecretName but ignores it; modify the factory to take an expectedBasicAuthSecretName (or capture it via closure) and assert the incoming basicAuthSecretName equals that expected value inside the returned function (using t.Fatalf/t.Errorf or testify require/assert) before proceeding to return the fake secret/error; update tests that call fakeInstallChartFromURL to pass the expected basicAuthSecretName so the mock will fail if the handler forwards an incorrect value.pkg/helm/actions/setup_test.go (1)
178-180: Prefer readiness polling over fixed sleep for OCI basic-auth setup.Line 178 introduces a fixed 5s delay, which is a common source of CI flakes when startup is slower than expected.
Suggested direction
- time.Sleep(5 * time.Second) + if err := waitForTCP("127.0.0.1:5001", 30*time.Second); err != nil { + return fmt.Errorf("zot basic-auth registry not ready: %w", err) + }// add helper in this file func waitForTCP(addr string, timeout time.Duration) error { deadline := time.Now().Add(timeout) for time.Now().Before(deadline) { c, err := net.DialTimeout("tcp", addr, 1*time.Second) if err == nil { _ = c.Close() return nil } time.Sleep(250 * time.Millisecond) } return fmt.Errorf("timeout waiting for %s", addr) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/setup_test.go` around lines 178 - 180, Replace the fixed 5s sleep before calling ExecuteScript with a readiness polling helper (e.g., add waitForTCP(addr string, timeout time.Duration)) and call it to wait for the OCI/basic-auth service TCP port to become available; locate the test block containing time.Sleep(5 * time.Second) and ExecuteScript("./testdata/uploadOciCharts.sh", ...) and swap the sleep for waitForTCP("<host:port>", 30*time.Second) (or appropriate timeout) so the test proceeds only after the service is reachable, returning any error from waitForTCP instead of relying on a fixed delay.pkg/helm/actions/install_chart_test.go (1)
450-475: Add a malformed-secret negative case (username/passwordkey missing).Current additions cover valid and wrong credentials, but not the required secret shape contract. A case where the secret exists but misses one required key would prevent regressions in validation behavior.
Also applies to: 491-502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/install_chart_test.go` around lines 450 - 475, Add a negative test case to the install_chart tests that uses the same OCI chart inputs but supplies a secret name that exists with a malformed shape (missing the "username" and/or "password" keys) to validate secret shape validation; update the table of test cases (the struct entries with fields testName, releaseName, chartPath, chartName, chartVersion, plainHTTP, skipTLSVerify, basicAuthSecretName, basicAuthUser, basicAuthPass, expectError) by adding an entry named like "OCI chart with malformed auth secret" that points basicAuthSecretName at the malformed secret and sets expectError=true so the test ensures the code path in the install logic (where basic auth secret is parsed/validated) rejects secrets missing "username"/"password".frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx (1)
168-179: Prefer reusing sharedinstallChartFromURLhelper for request construction.This block duplicates the API payload contract already captured in
frontend/packages/helm-plugin/src/utils/helm-utils.ts(includingbasic_auth_secret_name). Reusing the helper reduces drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx` around lines 168 - 179, The payload construction in HelmURLChartInstallPage.tsx duplicates the API contract; replace the manual payload object and direct coFetchJSON.post call with the shared helper installChartFromURL from frontend/packages/helm-plugin/src/utils/helm-utils.ts: call installChartFromURL({ namespace, releaseName, fullChartURL, chartVersion, valuesObj, basicAuthSecretName, noRepo: true }) (map parameter names to the helper's expected keys, ensuring basic_auth_secret_name and chart_version are set by the helper) and use its returned promise instead of coFetchJSON.post to avoid naming-convention eslint disables and drift.frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx (2)
187-191: Minor: Remove trailing period from form label.Form labels typically don't end with punctuation. This also differs from the PR screenshots showing "Secret for basic authentication" without a period.
✏️ Proposed fix
<FormGroup - label={t('helm-plugin~Secret for basic authentication.')} + label={t('helm-plugin~Secret for basic authentication')} fieldId="basic-auth-secret" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx` around lines 187 - 191, In HelmURLChartForm.tsx update the FormGroup label prop (inside the GridItem md={12} block) to remove the trailing period so the label reads "Secret for basic authentication" instead of "Secret for basic authentication."; locate the FormGroup component where label={t('helm-plugin~Secret for basic authentication.')} and remove the final period within the translation string to match the screenshots and form conventions.
54-68: Consider handling watch errors and optimizing secret filtering.The
useK8sWatchResourcehook returns a third value for errors. Currently, if the watch fails (e.g., RBAC issues), users see no feedback. Additionally, fetching all secrets and filtering client-side may be inefficient in namespaces with many secrets.♻️ Proposed improvement for error handling
- const [secrets, secretsLoaded] = useK8sWatchResource<SecretKind[]>({ + const [secrets, secretsLoaded, secretsError] = useK8sWatchResource<SecretKind[]>({ kind: SecretModel.kind, namespace, isList: true, }); + + // Consider showing secretsError in UI if watch failsYou could display an error state in the
SelectListwhensecretsErroris truthy, similar to the loading state pattern already implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx` around lines 54 - 68, The watch currently ignores the third return value (errors) from useK8sWatchResource and always fetches all secrets then filters client-side; update the hook call to capture the error (e.g., const [secrets, secretsLoaded, secretsError] = useK8sWatchResource(...)) and surface secretsError to the UI (show an error state/message in the SelectList like you do for loading). Also limit server-side load by adding a selector/fieldSelector in the useK8sWatchResource options (or a label selector on SecretModel) so you only request potential basic-auth secrets instead of fetching all secrets, and include secretsError and any new selector fields in the basicAuthSecrets dependencies to avoid stale results (referencing useK8sWatchResource, secretsLoaded, secrets, secretsError, basicAuthSecrets, SecretModel, SelectList).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/helm/actions/get_chart.go`:
- Around line 78-84: The code currently creates an OCI registry client whenever
basicAuthSecretName is set; change this to only build the OCI client when the
chart source is an OCI URL (e.g., starts with "oci://") to avoid creating the
client for plain HTTP/HTTPS .tgz URLs. In the block referencing
basicAuthSecretName, first obtain the chart source URL used for fetching (the
chart URL value used in this function), check it with a simple guard like
strings.HasPrefix(chartURL, "oci://") (or use an existing helper that detects
OCI charts if available), and only call RegistryClientWithBasicAuth(...) and
cmd.SetRegistryClient(...) when that guard passes; otherwise skip OCI client
creation. Use the symbols basicAuthSecretName, RegistryClientWithBasicAuth, and
cmd.SetRegistryClient to locate and modify the code.
In `@pkg/helm/actions/install_chart.go`:
- Around line 311-316: The current unconditional strings.TrimSuffix(url,
":"+version) can corrupt non-OCI HTTP archive URLs; update the logic in
install_chart.go so the URL trimming is only applied for OCI-style URLs (e.g.,
check strings.HasPrefix(url, "oci://") or an equivalent OCI detection used by
LocateChart/Helm), leaving HTTP/HTTPS archive URLs untouched; keep the existing
behavior of setting cmd.ChartPathOptions.Version = version and continue to use
chartVersionFromURL(url) when version is empty.
In `@pkg/helm/actions/testdata/uploadOciCharts.sh`:
- Line 3: The script currently accepts flags and silently falls back to no-TLS
on unknown input; update the argument parsing (the flag handling around lines
24-38 in pkg/helm/actions/testdata/uploadOciCharts.sh) to explicitly validate
flags and fail fast: detect any unknown flag, print a clear error and usage
hint, and exit non‑zero instead of defaulting to no‑TLS. Ensure the parser only
accepts the advertised modes (--tls, --no-tls, --basic-auth) and refers to the
existing mode variable(s) used in the script so behavior remains unchanged for
valid flags.
In `@pkg/helm/actions/testdata/zot-stop.sh`:
- Around line 5-6: The kill invocation using the unquoted command substitution
kill -TERM $(< zot-basicauth.pid) should be changed to quote the substitution so
the PID string won't be subject to word splitting (i.e., read and use the
contents of zot-basicauth.pid inside quotes when invoking kill -TERM). Locate
the line containing kill -TERM $(< zot-basicauth.pid) and wrap the substitution
in quotes, or equivalently use a quoted cat of zot-basicauth.pid; optionally,
add an existence/readability check for zot-basicauth.pid before attempting to
kill to avoid errors when the file is missing.
In `@pkg/helm/actions/testdata/zotWithBasicAuth.sh`:
- Around line 6-9: The script pkg/helm/actions/testdata/zotWithBasicAuth.sh uses
relative paths for the zot binary, config file
(testdata/zot-config-basicauth.json) and PID file (zot-basicauth.pid), which
break when run from a different CWD; update the script to compute the script
directory at runtime (based on the script's location) and use that absolute path
when referencing ./$GOOS-$GOARCH/zot, ./testdata/zot-config-basicauth.json and
the ./zot-storage-5001 and zot-basicauth.pid locations so all file lookups and
the PID file are created relative to the script location rather than the
caller's working directory.
---
Outside diff comments:
In `@pkg/helm/actions/install_chart.go`:
- Around line 327-335: In InstallChartFromURL capture the original user-provided
URL before you trim/version-strip it (e.g., save url to originalURL) and set
ch.Metadata.Annotations["chart_url"] = originalURL while keeping the trimmed url
used for OCI handling and ChartPathOptions.Version; ensure you still set
ch.Metadata.Annotations["installation"] = "url_install" and that trimming logic
(the code that mutates url and populates ChartPathOptions.Version) remains
unchanged so traceability is preserved.
In `@pkg/helm/handlers/handlers.go`:
- Around line 452-471: HandleURLChartGet reads namespace from query params into
the local variable namespace but still calls getActionConfigurations with the
hardcoded "default"; update the call to getActionConfigurations inside
HandleURLChartGet to pass the parsed namespace variable (namespace) instead of
the string "default" so action configuration (used by NewHandlerClients and
getChartFromURL) resolves Secrets like basic_auth_secret_name in the correct
namespace.
---
Nitpick comments:
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx`:
- Around line 187-191: In HelmURLChartForm.tsx update the FormGroup label prop
(inside the GridItem md={12} block) to remove the trailing period so the label
reads "Secret for basic authentication" instead of "Secret for basic
authentication."; locate the FormGroup component where
label={t('helm-plugin~Secret for basic authentication.')} and remove the final
period within the translation string to match the screenshots and form
conventions.
- Around line 54-68: The watch currently ignores the third return value (errors)
from useK8sWatchResource and always fetches all secrets then filters
client-side; update the hook call to capture the error (e.g., const [secrets,
secretsLoaded, secretsError] = useK8sWatchResource(...)) and surface
secretsError to the UI (show an error state/message in the SelectList like you
do for loading). Also limit server-side load by adding a selector/fieldSelector
in the useK8sWatchResource options (or a label selector on SecretModel) so you
only request potential basic-auth secrets instead of fetching all secrets, and
include secretsError and any new selector fields in the basicAuthSecrets
dependencies to avoid stale results (referencing useK8sWatchResource,
secretsLoaded, secrets, secretsError, basicAuthSecrets, SecretModel,
SelectList).
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx`:
- Around line 168-179: The payload construction in HelmURLChartInstallPage.tsx
duplicates the API contract; replace the manual payload object and direct
coFetchJSON.post call with the shared helper installChartFromURL from
frontend/packages/helm-plugin/src/utils/helm-utils.ts: call
installChartFromURL({ namespace, releaseName, fullChartURL, chartVersion,
valuesObj, basicAuthSecretName, noRepo: true }) (map parameter names to the
helper's expected keys, ensuring basic_auth_secret_name and chart_version are
set by the helper) and use its returned promise instead of coFetchJSON.post to
avoid naming-convention eslint disables and drift.
In `@pkg/helm/actions/install_chart_test.go`:
- Around line 450-475: Add a negative test case to the install_chart tests that
uses the same OCI chart inputs but supplies a secret name that exists with a
malformed shape (missing the "username" and/or "password" keys) to validate
secret shape validation; update the table of test cases (the struct entries with
fields testName, releaseName, chartPath, chartName, chartVersion, plainHTTP,
skipTLSVerify, basicAuthSecretName, basicAuthUser, basicAuthPass, expectError)
by adding an entry named like "OCI chart with malformed auth secret" that points
basicAuthSecretName at the malformed secret and sets expectError=true so the
test ensures the code path in the install logic (where basic auth secret is
parsed/validated) rejects secrets missing "username"/"password".
In `@pkg/helm/actions/setup_test.go`:
- Around line 178-180: Replace the fixed 5s sleep before calling ExecuteScript
with a readiness polling helper (e.g., add waitForTCP(addr string, timeout
time.Duration)) and call it to wait for the OCI/basic-auth service TCP port to
become available; locate the test block containing time.Sleep(5 * time.Second)
and ExecuteScript("./testdata/uploadOciCharts.sh", ...) and swap the sleep for
waitForTCP("<host:port>", 30*time.Second) (or appropriate timeout) so the test
proceeds only after the service is reachable, returning any error from
waitForTCP instead of relying on a fixed delay.
In `@pkg/helm/handlers/handler_test.go`:
- Around line 204-205: The mock factory fakeInstallChartFromURL currently
accepts basicAuthSecretName but ignores it; modify the factory to take an
expectedBasicAuthSecretName (or capture it via closure) and assert the incoming
basicAuthSecretName equals that expected value inside the returned function
(using t.Fatalf/t.Errorf or testify require/assert) before proceeding to return
the fake secret/error; update tests that call fakeInstallChartFromURL to pass
the expected basicAuthSecretName so the mock will fail if the handler forwards
an incorrect value.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 282a7534-9cff-4cf6-8313-2e7e319e10ea
📒 Files selected for processing (19)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLInstallForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/types.tsfrontend/packages/helm-plugin/src/utils/helm-utils.tspkg/helm/actions/get_chart.gopkg/helm/actions/get_registry.gopkg/helm/actions/install_chart.gopkg/helm/actions/install_chart_test.gopkg/helm/actions/setup_test.gopkg/helm/actions/testdata/htpasswdpkg/helm/actions/testdata/uploadOciCharts.shpkg/helm/actions/testdata/zot-config-basicauth.jsonpkg/helm/actions/testdata/zot-stop.shpkg/helm/actions/testdata/zotWithBasicAuth.shpkg/helm/handlers/handler_test.gopkg/helm/handlers/handlers.gopkg/helm/handlers/request.go
📜 Review details
🧰 Additional context used
🪛 Shellcheck (0.11.0)
pkg/helm/actions/testdata/zotWithBasicAuth.sh
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
pkg/helm/actions/testdata/zot-stop.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (16)
pkg/helm/actions/testdata/htpasswd (1)
1-1: Test fixture addition looks good.This keeps the basic-auth test setup self-contained and reproducible.
pkg/helm/actions/get_registry.go (1)
15-31: Nice refactor and credential wiring.Centralizing client option construction and reusing it for basic-auth client creation is clean and reduces drift risk.
Also applies to: 33-41, 47-52
frontend/packages/helm-plugin/src/components/forms/url-chart/types.ts (1)
9-9: Type extension is clean and backward-compatible.Using an optional field here is the right choice for incremental rollout.
pkg/helm/actions/testdata/zot-config-basicauth.json (1)
1-19: Config setup for basic-auth Zot test instance looks good.Dedicated port, storage root, and htpasswd path keep this fixture isolated from other registry test modes.
pkg/helm/handlers/request.go (1)
12-13: Good request-model extension for optional auth secret.Line 12 and Line 13 are clear and align with the API payload naming used across handlers.
frontend/packages/helm-plugin/locales/en/helm-plugin.json (1)
127-132: i18n entries for secret selection look good.These additions are clear and match the new URL install/authentication flow.
Also applies to: 139-139
frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLInstallForm.tsx (1)
149-159: Read-only Basic Auth Secret summary is wired correctly.Line 149 to Line 159 cleanly exposes the selected secret in the confirm/configure form.
frontend/packages/helm-plugin/src/utils/helm-utils.ts (1)
353-360: Backward-compatible API payload update looks correct.The optional parameter and conditional
basic_auth_secret_namepayload mapping are implemented cleanly.Also applies to: 367-367
pkg/helm/handlers/handlers.go (3)
67-77: LGTM on signature updates.The function signatures have been correctly extended to accept
basicAuthSecretNameas the final parameter, maintaining backward compatibility and consistent positioning across bothinstallChartFromURLandgetChartFromURL. Clean integration with the existing handler infrastructure.
161-169: Proper threading ofbasicAuthSecretNamein async install flow.The
req.BasicAuthSecretNameis correctly passed through toinstallChartFromURLfor theNoRepopath. The HTTP status codes (400 for chart errors, 201 for success) are appropriate.
226-251: Good fix: namespace now used for action configuration.Line 238 correctly uses the resolved
namespacevariable instead of a hardcoded"default"when building the action configuration. This ensures the correct namespace context when fetching secrets for basic auth. ThebasicAuthSecretNamequery parameter is properly extracted and forwarded.frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx (2)
192-263: Well-implemented PatternFly 6 typeahead Select with good a11y coverage.The Select component correctly implements:
- PatternFly 6 patterns with
MenuToggle+TextInputGroupfor typeahead- Proper ARIA attributes (
role="combobox",aria-controls,aria-label,isExpanded)- Loading and empty states with disabled options
- Clear button with accessible label
- Consistent i18n usage throughout
One minor a11y enhancement: consider adding
aria-describedbylinking to the helper text for screen readers.
80-94: Good UX: Focus management after selection/clear.The
textInputRef?.current?.focus()calls ensure keyboard users can continue typing after selecting or clearing, maintaining good focus management patterns.pkg/helm/actions/install_chart.go (3)
262-283: Solid implementation ofapplyBasicAuthFromSecret.Good defensive programming:
- Early return for empty secret name
- Wrapped errors with
%wfor proper error chaining- Clear, actionable error messages including namespace and secret name
- Uses the existing
username/passwordconstants for key names (consistency with HelmChartRepository convention)
298-309: Correct handling of OCI registry client credentials.Good catch on Helm's OCI getter behavior—it doesn't merge
ChartPathOptionsusername/password onto an existingRegistryClient. Rebuilding the client withClientOptBasicAuthis the right approach. The conditional check (basicAuthSecretName != "") correctly gates this to only when credentials are provided.
336-351: Async installation follows established patterns.The goroutine implementation correctly:
- Logs success/failure with contextual info
- Creates error-tracking secret on failure
- Cleans up error secret after 15-second delay
- Records metrics on success
This aligns with the existing
InstallChartAsyncpattern.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
pkg/helm/actions/testdata/zot-stop.sh (1)
5-5:⚠️ Potential issue | 🟡 MinorQuote the PID substitution in
kill.Line 5 still uses unquoted command substitution (
$(< zot-basicauth.pid)), which triggers ShellCheck SC2046 and can cause word-splitting/globbing surprises.Suggested fix
-kill -TERM $(< zot-basicauth.pid) || echo "Zot (basic auth) is not currently running." +kill -TERM "$(<zot-basicauth.pid)" || echo "Zot (basic auth) is not currently running."#!/bin/bash set -euo pipefail # Verify current warning and validate fix result. if command -v shellcheck >/dev/null 2>&1; then shellcheck pkg/helm/actions/testdata/zot-stop.sh else rg -n 'kill -TERM \$\(<\s*zot-basicauth\.pid\)' pkg/helm/actions/testdata/zot-stop.sh fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/testdata/zot-stop.sh` at line 5, The kill invocation in zot-stop.sh uses unquoted command substitution (kill -TERM $(< zot-basicauth.pid)) which can trigger word-splitting/globbing; update the kill call to use a quoted substitution or assign the PID to a variable and pass it quoted (e.g., read the PID from zot-basicauth.pid into a variable and call kill -TERM "$PID") while keeping the existing || echo fallback unchanged.pkg/helm/actions/testdata/uploadOciCharts.sh (1)
24-38:⚠️ Potential issue | 🟡 MinorReject unknown flags instead of silently defaulting to no-TLS mode.
Line 24-Line 38 still allows typoed flags to fall into default behavior, which hides setup mistakes.
Proposed fix
-# Push charts to OCI registry using helm push -if [[ $1 == "--tls" ]]; then - REGISTRY="localhost:5443" - echo "Pushing mariadb-7.3.5.tgz to oci://$REGISTRY/helm-charts..." - $HELM push $CHARTS_DIR/mariadb-7.3.5.tgz oci://$REGISTRY/helm-charts --ca-file=$CACERT -elif [[ $1 == "--basic-auth" ]]; then - REGISTRY="localhost:5001" - echo "Logging in to oci://$REGISTRY with basic auth..." - echo "hunter2" | $HELM registry login $REGISTRY --username AzureDiamond --password-stdin --plain-http - echo "Pushing mychart-0.1.0.tgz to oci://$REGISTRY/helm-charts..." - $HELM push $CHARTS_DIR/mychart-0.1.0.tgz oci://$REGISTRY/helm-charts --plain-http -else - REGISTRY="localhost:5000" - echo "Pushing mychart-0.1.0.tgz to oci://$REGISTRY/helm-charts..." - $HELM push $CHARTS_DIR/mychart-0.1.0.tgz oci://$REGISTRY/helm-charts --plain-http -fi +mode="${1:---no-tls}" +case "$mode" in + --tls) + REGISTRY="localhost:5443" + echo "Pushing mariadb-7.3.5.tgz to oci://$REGISTRY/helm-charts..." + $HELM push $CHARTS_DIR/mariadb-7.3.5.tgz oci://$REGISTRY/helm-charts --ca-file=$CACERT + ;; + --basic-auth) + REGISTRY="localhost:5001" + echo "Logging in to oci://$REGISTRY with basic auth..." + echo "hunter2" | $HELM registry login $REGISTRY --username AzureDiamond --password-stdin --plain-http + echo "Pushing mychart-0.1.0.tgz to oci://$REGISTRY/helm-charts..." + $HELM push $CHARTS_DIR/mychart-0.1.0.tgz oci://$REGISTRY/helm-charts --plain-http + ;; + --no-tls|"") + REGISTRY="localhost:5000" + echo "Pushing mychart-0.1.0.tgz to oci://$REGISTRY/helm-charts..." + $HELM push $CHARTS_DIR/mychart-0.1.0.tgz oci://$REGISTRY/helm-charts --plain-http + ;; + *) + echo "Usage: uploadOciCharts.sh --tls | --no-tls | --basic-auth" >&2 + exit 2 + ;; +esac🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/testdata/uploadOciCharts.sh` around lines 24 - 38, The script currently treats any unrecognized first argument ($1) as the no-TLS default; change the conditional to explicitly validate allowed flags ("--tls" and "--basic-auth") and make the else branch print a clear usage/error message and exit non-zero instead of proceeding; reference the existing checks for "--tls" and "--basic-auth" and the variables REGISTRY, $HELM and $CHARTS_DIR so you update the conditional logic to reject unknown flags (echo "Usage: ... --tls | --basic-auth" >&2; exit 1) before attempting any pushes.pkg/helm/actions/testdata/zotWithBasicAuth.sh (1)
6-9:⚠️ Potential issue | 🟠 MajorUse script-relative paths; current CWD-dependent paths are fragile.
Line 6, Line 8, and Line 9 still depend on caller working directory, so startup/pid placement can break outside repo root.
Proposed fix
#!/bin/bash -e # Start zot OCI registry server with basic auth (htpasswd) GOOS=${GOOS:-$(go env GOOS)} GOARCH=${GOARCH:-$(go env GOARCH)} +SCRIPT_DIR="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" +ACTIONS_DIR="$(cd -- "$SCRIPT_DIR/.." && pwd)" -mkdir -p ./zot-storage-5001 +mkdir -p "$ACTIONS_DIR/zot-storage-5001" -./$GOOS-$GOARCH/zot serve ./testdata/zot-config-basicauth.json & -echo $! > ./zot-basicauth.pid +"$ACTIONS_DIR/$GOOS-$GOARCH/zot" serve "$SCRIPT_DIR/zot-config-basicauth.json" & +echo "$!" > "$ACTIONS_DIR/zot-basicauth.pid"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/testdata/zotWithBasicAuth.sh` around lines 6 - 9, The script uses relative paths for the storage dir, config, binary and PID file (mkdir -p ./zot-storage-5001, ./$GOOS-$GOARCH/zot serve ./testdata/zot-config-basicauth.json &, echo $! > ./zot-basicauth.pid) which breaks when run outside the repo root; modify the script to compute the script directory (e.g. SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" or equivalent) and then replace those relative paths with absolute paths prefixed by that directory (use "$SCRIPT_DIR"/zot-storage-5001, "$SCRIPT_DIR"/$GOOS-$GOARCH/zot, "$SCRIPT_DIR"/testdata/zot-config-basicauth.json, and "$SCRIPT_DIR"/zot-basicauth.pid) so startup and PID placement are script-location independent.pkg/helm/actions/get_chart.go (1)
78-84:⚠️ Potential issue | 🟠 MajorOnly create an OCI registry client for
oci://sources.Line 78 still builds a Helm registry client for any authenticated URL. For direct
http(s)archives,LocateChartcan usecmd.Username/cmd.Passworddirectly, so this adds an avoidable failure path unrelated to the requested chart source.Proposed fix
import ( "fmt" "os" + "strings" @@ - if basicAuthSecretName != "" { + if basicAuthSecretName != "" && strings.HasPrefix(strings.ToLower(url), "oci://") { rc, err := RegistryClientWithBasicAuth(false, false, cmd.Username, cmd.Password) if err != nil { return nil, fmt.Errorf("failed to configure OCI registry client: %w", err) } cmd.SetRegistryClient(rc) }
🧹 Nitpick comments (4)
pkg/helm/handlers/handler_test.go (1)
204-205: Strengthen test helper by assertingbasicAuthSecretNamepropagation.The new parameter is accepted but not validated, so handler wiring regressions could pass unnoticed. Please assert the expected value in the fake used by the no-repo install tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/handlers/handler_test.go` around lines 204 - 205, Update the fakeInstallChartFromURL test helper to validate the propagated basicAuthSecretName: change fakeInstallChartFromURL signature to accept an expectedBasicAuthSecretName string (in addition to mockedSecret and err) and inside the returned func assert that the incoming basicAuthSecretName equals expectedBasicAuthSecretName (use the test helper's t/require/fatal to fail if not). This ensures the returned fake (fakeInstallChartFromURL) checks the basicAuthSecretName parameter passed into the handler wiring.pkg/helm/actions/setup_test.go (1)
107-109: Replace fixed sleep with TCP readiness polling to reduce CI flakiness.The
setupTestOCIBasicAuth()function (line 178) uses a hardcoded 5-second delay before uploading OCI charts. SincezotWithBasicAuth.shstarts the registry in the background without readiness signaling, slow startup times in CI can cause intermittent failures. Polling the registry port is more reliable.Suggested implementation
import ( "fmt" "io/ioutil" + "net" "os" "os/exec" "regexp" "runtime/debug" "strings" "testing" "time" @@ func setupTestOCIBasicAuth() error { if err := ExecuteScript("./testdata/zotWithBasicAuth.sh", false); err != nil { return err } - time.Sleep(5 * time.Second) + if err := waitForTCP("127.0.0.1:5001", 30*time.Second); err != nil { + return err + } if err := ExecuteScript("./testdata/uploadOciCharts.sh", true, "--basic-auth"); err != nil { return err } return nil } + +func waitForTCP(addr string, timeout time.Duration) error { + deadline := time.Now().Add(timeout) + for time.Now().Before(deadline) { + conn, err := net.DialTimeout("tcp", addr, 1*time.Second) + if err == nil { + _ = conn.Close() + return nil + } + time.Sleep(250 * time.Millisecond) + } + return fmt.Errorf("timed out waiting for %s", addr) +}Note: Similar patterns exist in
setupTestWithTls(),setupTestWithoutTls(), andsetupTestBasicAuth()that could benefit from the same treatment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/setup_test.go` around lines 107 - 109, The test setup uses a fixed sleep in setupTestOCIBasicAuth (and similarly in setupTestWithTls, setupTestWithoutTls, setupTestBasicAuth) which causes CI flakiness; replace the hardcoded time.Sleep with a TCP readiness poll that repeatedly dials the registry address/port (with a short backoff and an overall timeout) and only proceeds to upload OCI charts once the TCP connection succeeds, failing the setup if the timeout elapses; implement this polling logic inside setupTestOCIBasicAuth (and mirror into the other setup* functions) so the upload occurs after a successful dial rather than after a fixed delay.frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx (1)
78-85: PreferURLSearchParamsover manual query-string concatenation.The current string assembly is brittle and harder to maintain as params grow.
Proposed refactor
- let authParam = ''; - if (basicAuthSecretName) { - authParam = `&basic_auth_secret_name=${encodeURIComponent(basicAuthSecretName)}`; - } - const apiUrl = `/api/helm/chart?url=${encodeURIComponent( - fullChartURL, - )}&noRepo=true&namespace=${namespace}${authParam}`; + const params = new URLSearchParams({ + url: fullChartURL, + noRepo: 'true', + namespace: namespace ?? '', + }); + if (basicAuthSecretName) { + params.set('basic_auth_secret_name', basicAuthSecretName); + } + const apiUrl = `/api/helm/chart?${params.toString()}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx` around lines 78 - 85, The current apiUrl assembly using string concatenation (variables authParam, basicAuthSecretName, fullChartURL, namespace, and apiUrl) is brittle; replace it with URLSearchParams: create a params = new URLSearchParams(), set url=fullChartURL, noRepo=true, namespace=namespace, and conditionally set basic_auth_secret_name when basicAuthSecretName is present, then build apiUrl = `/api/helm/chart?${params.toString()}` so encoding is handled automatically and the code is easier to extend and maintain.pkg/helm/actions/install_chart_test.go (1)
450-475: Add one malformed-secret test case to lock key-contract behavior.You now cover valid creds and wrong creds; consider adding a case where the referenced secret is present but missing
usernameorpassword, and assert an error.Also applies to: 491-502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/helm/actions/install_chart_test.go` around lines 450 - 475, Add a test case to the table that exercises a malformed basic-auth secret (present but missing either the username or password key) and expect the install to fail; e.g. add an entry similar to the existing OCI cases with basicAuthSecretName: "malformed-auth-secret", set one of basicAuthUser or basicAuthPass empty (or both) and expectError: true, and ensure the test harness/fixture that creates secrets for tests creates a secret with the missing key for "malformed-auth-secret"; repeat the same malformed-secret case in the other test table referenced around the second block (the section you noted at 491-502) so both places assert an error when the secret is missing username/password.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsx`:
- Around line 54-59: The secret watch currently only destructures two return
values from useK8sWatchResource (const [secrets, secretsLoaded] =
useK8sWatchResource<SecretKind[]>()), which hides watch errors and makes
RBAC/network failures look like “no matching secrets”; update the destructuring
to capture the third return value (e.g., const [secrets, secretsLoaded,
secretsError] = useK8sWatchResource<SecretKind[]>()), then add explicit handling
in HelmURLChartForm to check secretsError and render a clear error state/message
(instead of or alongside the empty-state UI) so users see
permission/network/watch failures; repeat the same change for the other secret
watch instance currently around the later block that uses the same hook.
In `@pkg/helm/actions/install_chart.go`:
- Around line 303-309: The code always creates an OCI registry client when
basicAuthSecretName is set, causing failures for non-OCI (plain http(s) chart
archive) installs; change the logic so RegistryClientWithBasicAuth is only
called for OCI installs by adding a guard that detects OCI installs (e.g., check
the chart reference/URL or an existing isOCI flag for this install) before
invoking RegistryClientWithBasicAuth and before calling cmd.SetRegistryClient;
keep the same error handling but skip creating/setting the registry client when
the install is not OCI (use basicAuthSecretName, RegistryClientWithBasicAuth,
and cmd.SetRegistryClient to locate where to apply the conditional).
---
Duplicate comments:
In `@pkg/helm/actions/testdata/uploadOciCharts.sh`:
- Around line 24-38: The script currently treats any unrecognized first argument
($1) as the no-TLS default; change the conditional to explicitly validate
allowed flags ("--tls" and "--basic-auth") and make the else branch print a
clear usage/error message and exit non-zero instead of proceeding; reference the
existing checks for "--tls" and "--basic-auth" and the variables REGISTRY, $HELM
and $CHARTS_DIR so you update the conditional logic to reject unknown flags
(echo "Usage: ... --tls | --basic-auth" >&2; exit 1) before attempting any
pushes.
In `@pkg/helm/actions/testdata/zot-stop.sh`:
- Line 5: The kill invocation in zot-stop.sh uses unquoted command substitution
(kill -TERM $(< zot-basicauth.pid)) which can trigger word-splitting/globbing;
update the kill call to use a quoted substitution or assign the PID to a
variable and pass it quoted (e.g., read the PID from zot-basicauth.pid into a
variable and call kill -TERM "$PID") while keeping the existing || echo fallback
unchanged.
In `@pkg/helm/actions/testdata/zotWithBasicAuth.sh`:
- Around line 6-9: The script uses relative paths for the storage dir, config,
binary and PID file (mkdir -p ./zot-storage-5001, ./$GOOS-$GOARCH/zot serve
./testdata/zot-config-basicauth.json &, echo $! > ./zot-basicauth.pid) which
breaks when run outside the repo root; modify the script to compute the script
directory (e.g. SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" or
equivalent) and then replace those relative paths with absolute paths prefixed
by that directory (use "$SCRIPT_DIR"/zot-storage-5001,
"$SCRIPT_DIR"/$GOOS-$GOARCH/zot,
"$SCRIPT_DIR"/testdata/zot-config-basicauth.json, and
"$SCRIPT_DIR"/zot-basicauth.pid) so startup and PID placement are
script-location independent.
---
Nitpick comments:
In
`@frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsx`:
- Around line 78-85: The current apiUrl assembly using string concatenation
(variables authParam, basicAuthSecretName, fullChartURL, namespace, and apiUrl)
is brittle; replace it with URLSearchParams: create a params = new
URLSearchParams(), set url=fullChartURL, noRepo=true, namespace=namespace, and
conditionally set basic_auth_secret_name when basicAuthSecretName is present,
then build apiUrl = `/api/helm/chart?${params.toString()}` so encoding is
handled automatically and the code is easier to extend and maintain.
In `@pkg/helm/actions/install_chart_test.go`:
- Around line 450-475: Add a test case to the table that exercises a malformed
basic-auth secret (present but missing either the username or password key) and
expect the install to fail; e.g. add an entry similar to the existing OCI cases
with basicAuthSecretName: "malformed-auth-secret", set one of basicAuthUser or
basicAuthPass empty (or both) and expectError: true, and ensure the test
harness/fixture that creates secrets for tests creates a secret with the missing
key for "malformed-auth-secret"; repeat the same malformed-secret case in the
other test table referenced around the second block (the section you noted at
491-502) so both places assert an error when the secret is missing
username/password.
In `@pkg/helm/actions/setup_test.go`:
- Around line 107-109: The test setup uses a fixed sleep in
setupTestOCIBasicAuth (and similarly in setupTestWithTls, setupTestWithoutTls,
setupTestBasicAuth) which causes CI flakiness; replace the hardcoded time.Sleep
with a TCP readiness poll that repeatedly dials the registry address/port (with
a short backoff and an overall timeout) and only proceeds to upload OCI charts
once the TCP connection succeeds, failing the setup if the timeout elapses;
implement this polling logic inside setupTestOCIBasicAuth (and mirror into the
other setup* functions) so the upload occurs after a successful dial rather than
after a fixed delay.
In `@pkg/helm/handlers/handler_test.go`:
- Around line 204-205: Update the fakeInstallChartFromURL test helper to
validate the propagated basicAuthSecretName: change fakeInstallChartFromURL
signature to accept an expectedBasicAuthSecretName string (in addition to
mockedSecret and err) and inside the returned func assert that the incoming
basicAuthSecretName equals expectedBasicAuthSecretName (use the test helper's
t/require/fatal to fail if not). This ensures the returned fake
(fakeInstallChartFromURL) checks the basicAuthSecretName parameter passed into
the handler wiring.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f68dfb6b-c9c5-4f6d-93ed-2b42a181fc57
📒 Files selected for processing (19)
frontend/packages/helm-plugin/locales/en/helm-plugin.jsonfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLChartInstallPage.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLInstallForm.tsxfrontend/packages/helm-plugin/src/components/forms/url-chart/types.tsfrontend/packages/helm-plugin/src/utils/helm-utils.tspkg/helm/actions/get_chart.gopkg/helm/actions/get_registry.gopkg/helm/actions/install_chart.gopkg/helm/actions/install_chart_test.gopkg/helm/actions/setup_test.gopkg/helm/actions/testdata/htpasswdpkg/helm/actions/testdata/uploadOciCharts.shpkg/helm/actions/testdata/zot-config-basicauth.jsonpkg/helm/actions/testdata/zot-stop.shpkg/helm/actions/testdata/zotWithBasicAuth.shpkg/helm/handlers/handler_test.gopkg/helm/handlers/handlers.gopkg/helm/handlers/request.go
📜 Review details
🧰 Additional context used
🪛 Shellcheck (0.11.0)
pkg/helm/actions/testdata/zotWithBasicAuth.sh
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
[info] 8-8: Double quote to prevent globbing and word splitting.
(SC2086)
pkg/helm/actions/testdata/zot-stop.sh
[warning] 5-5: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (11)
pkg/helm/actions/testdata/htpasswd (1)
1-1: Looks good for test fixture usage.The added
htpasswdentry is appropriate for the new basic-auth test flow.pkg/helm/actions/testdata/zot-stop.sh (1)
6-6: Good cleanup update.Including
zot-basicauth.pidin the teardown cleanup is the right addition.frontend/packages/helm-plugin/src/components/forms/url-chart/types.ts (1)
9-9: Type shape change is clean and backward-compatible.
basicAuthSecretNamebeing optional is the right choice for existing URL chart flows.pkg/helm/actions/testdata/zot-config-basicauth.json (1)
1-19: Config wiring looks consistent for the new basic-auth test registry.Address, port, auth backend, and storage isolation are aligned with the companion test scripts.
frontend/packages/helm-plugin/locales/en/helm-plugin.json (1)
127-132: i18n additions are complete for the new secret-selection UX.Nice coverage of label, CTA text, loading/empty states, and install-summary label.
Also applies to: 139-139
pkg/helm/handlers/request.go (1)
12-13: Request model update is clear and API-compatible.The optional
basic_auth_secret_namefield and inline doc make the contract explicit for URL-based installs.frontend/packages/helm-plugin/src/components/forms/url-chart/HelmURLInstallForm.tsx (1)
149-159: Good conditional read-only rendering for selected auth secret.This is a clean way to surface the selected secret in the summary/configure step without allowing accidental edits.
frontend/packages/helm-plugin/src/utils/helm-utils.ts (1)
353-368: Nice backward-compatible extension of install payload.Optional
basic_auth_secret_nameis threaded cleanly without affecting existing callers.pkg/helm/handlers/handlers.go (1)
67-76: No concerns on handler-side secret propagation.The no-repo flow consistently threads
basic_auth_secret_namethrough request parsing and action invocation.Also applies to: 162-163, 232-252, 471-471
pkg/helm/actions/get_registry.go (1)
15-57: Nice consolidation of registry client setup.Centralizing the TLS/plain-HTTP options keeps the default and basic-auth OCI client paths aligned and reduces drift between them.
pkg/helm/actions/install_chart.go (1)
262-283: Good helper extraction for Secret-backed auth.Pulling the Secret lookup and key validation into one helper keeps the URL install/get flows consistent and gives clear errors when the Secret is malformed.
sowmya-sl
force-pushed
the
add-auth-helm
branch
from
355844a to
517dc88
Compare
|
/retest |
- Add basicAuthSecretName parameter to GetChartFromURL and InstallChartFromURL - Build RegistryClient with basic auth credentials for OCI registry pulls - Extract shared registryClientOptions from GetOCIRegistry for reuse - Add applyBasicAuthFromSecret helper to read credentials from K8s Secret - Strip OCI version tags before LocateChart to prevent duplicate tag errors - Wire basic_auth_secret_name through HandleChartGet and HandleHelmInstallAsync - Add OCI basic auth test cases with zot registry and htpasswd auth
openshift-ci
Bot
added
the
needs-rebase
- Add typeahead secret dropdown to HelmURLChartForm
- Surface errors for invalid secrets with explanatory description
- Pass basicAuthSecretName through fetchChartData and install API calls
- Append namespace and basic_auth_secret_name to chart fetch query string
- Display selected secret as read-only field on the install confirmation step
- Add basicAuthSecretName to HelmURLChartFormData type and helm-utils
openshift-ci
Bot
removed
the
needs-rebase
|
/retest |
…stallForm - Remove stale useTheme import that caused TS6133 build error - Drop theme property from useHelmReadmeModalLauncher (not in Props type) - Pass namespace to HelmURLInstallForm for ResourceDropdownField
|
@sowmya-sl: all tests passed! 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. |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
Looks generally good. I have a bunch of polishing suggestions for you. Also, you should look at CodeRabbit's nitpick suggestions and see if any are appropriate to fix (it came up with the same test concern that I did).
Also, it would be good to update the description with fresh screenshots, since you've changed the selector.
| let authParam = ''; | ||
| if (basicAuthSecretName) { | ||
| authParam = `&basic_auth_secret_name=${encodeURIComponent(basicAuthSecretName)}`; | ||
| } |
There was a problem hiding this comment.
Alternatively, I believe you can use a ternary,
| let authParam = ''; | |
| if (basicAuthSecretName) { | |
| authParam = `&basic_auth_secret_name=${encodeURIComponent(basicAuthSecretName)}`; | |
| } | |
| const authParam = basicAuthSecretName ? `&basic_auth_secret_name=${encodeURIComponent(basicAuthSecretName)}` : ''; |
which avoids the need for using let.
| const apiUrl = `/api/helm/chart?url=${encodeURIComponent( | ||
| fullChartURL, | ||
| )}&noRepo=true&namespace=${namespace}${authParam}`; |
There was a problem hiding this comment.
This might be prettier if you went ahead and used one more local variable:
| const apiUrl = `/api/helm/chart?url=${encodeURIComponent( | |
| fullChartURL, | |
| )}&noRepo=true&namespace=${namespace}${authParam}`; | |
| const urlParam = encodeURIComponent(fullChartURL) | |
| const apiUrl = `/api/helm/chart?url=${urlParam}&noRepo=true&namespace=${namespace}${authParam}`; |
| name="basicAuthSecretName" | ||
| label={t('helm-plugin~Secret for basic authentication.')} |
There was a problem hiding this comment.
The label at HelmURLInstallForm.tsx line 185 doesn't have a trailing period...should we remove the one here?
There was a problem hiding this comment.
These changes seem very similar to the ones in HelmURLChartForm.tsx, would it be appropriate to refactor these into common code?
| echo "Logging in to oci://$REGISTRY with basic auth..." | ||
| echo "hunter2" | $HELM registry login $REGISTRY --username AzureDiamond --password-stdin --plain-http |
There was a problem hiding this comment.
Given that you're hard-coding the password into the script, you might as well put it on the command line and save the pipe-gymnastics:
| echo "Logging in to oci://$REGISTRY with basic auth..." | |
| echo "hunter2" | $HELM registry login $REGISTRY --username AzureDiamond --password-stdin --plain-http | |
| echo "Logging in to oci://$REGISTRY with basic auth..." | |
| $HELM registry login $REGISTRY --username AzureDiamond --password "hunter2" --plain-http |
| // applyBasicAuthFromSecret sets cmd.Username and cmd.Password from a Secret in ns with | ||
| // keys "username" and "password" (same convention as HelmChartRepository connectionConfig). | ||
| func applyBasicAuthFromSecret(cmd *action.Install, coreClient corev1client.CoreV1Interface, ns, secretName string) error { |
There was a problem hiding this comment.
This function seems like it should be a method receiving a *action.Install. Is that feasible?
| { | ||
| testName: "OCI chart with basic auth", |
There was a problem hiding this comment.
What about HTTP(S) chart with basic auth? (And, if that is feasible, what about the same with wrong credentials?)
| basicAuthSecretName: "oci-auth-secret", | ||
| basicAuthUser: "AzureDiamond", | ||
| basicAuthPass: "hunter2", |
There was a problem hiding this comment.
What about a malformed secrets? (I.e., ones missing the username, password, or both.)
| rel, err := InstallChartFromURL("test-namespace", tt.releaseName, tt.chartPath, nil, actionConfig, coreClient, tt.chartVersion, tt.basicAuthSecretName) | ||
| require.Error(t, err) | ||
| require.Nil(t, rel) |
There was a problem hiding this comment.
Simply requiring "an error" is a very weak assertion, one which could result if a false positive if the test fails for the wrong reason (i.e., if there's a bug in the test). Can we do better, here? (E.g., if expectError is a pointer to an error or a string, instead of a boolean, then, if it is non-nil/empty, we can check for the correct error.)
| // For valid URLs: create the release secret in a background goroutine | ||
| // to simulate what Helm's cmd.Run would do, unblocking getSecret's Watch. |
There was a problem hiding this comment.
Was this explanation wrong? If not, it seems like it would be good to have an explanation of why we're using a goroutine instead of doing this synchronously.
3 participants
Analysis / Root cause:
The Helm installation using URLs is currently supported without any authentication. This becomes difficult when charts are in registries which require username and password. So add support for the same.
Solution description:
The solution is creating a generic secret having username and password as keys. This secret is read and shared with the Helm registry in the backend.
Screenshots / screen recording:
First tab:
On clicking Next:
Clicking on Install installs the Helm release
If secret is not needed, then this is the flow:
On clicking Next
Test setup:
Reviewers and assignees:
@openshift/team-ux-review
Summary by CodeRabbit
New Features
Tests