Add new server endpoint for installing OCI charts without Helm repository #15863

sowmya-sl · 2025-12-18T08:57:54Z

Add dedicated endpoint and handler for installing Helm charts from OCI
registries. This provides a simplified installation flow for OCI charts
that doesn't require HelmChartRepository lookup.

Changes:

Add InstallOCIChart function in actions for OCI-only installations
Add HandleInstallOCIChart handler with OCI URL validation
Register /api/helm/oci-chart endpoint in server routes
Add tests for both the methods

HELM-598

coderabbitai · 2025-12-18T08:58:01Z

Important

Review skipped

Auto reviews are limited based on label configuration.

🚫 Excluded labels (none allowed) (1)

do-not-merge/work-in-progress

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2025-12-18T08:58:32Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-12-18T09:05:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sowmya-sl
Once this PR has been reviewed and has the lgtm label, please assign jhadvig for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

pkg/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

OCI-based Helm charts were failing to install because the action configuration lacked a registry client. This change: - Add GetDefaultOCIRegistry() to create and attach a registry client to the Helm action configuration - Integrate registry client initialization into all Helm handlers: install, upgrade, uninstall, rollback, and chart get operations - Add unit tests for the new registry client function - Update older tests to use mock registry Client function Without a registry client, operations on OCI charts (oci://) would fail with errors about missing registry support. Fixes: HELM-611

Add dedicated endpoint and handler for installing Helm charts from OCI registries. This provides a simplified installation flow for OCI charts that doesn't require HelmChartRepository lookup. Changes: - Add InstallOCIChart function in actions for OCI-only installations - Add HandleInstallOCIChart handler with OCI URL validation - Register /api/helm/oci-chart endpoint in server routes - Add tests for both the methods HELM-598

webbnh

I realize that this is still a draft PR, but I have a few comments.

Also, please ensure that all files end with a newline.

webbnh · 2026-01-08T19:39:55Z

pkg/helm/actions/testdata/cleanup.sh

-rm -rf ./chartmuseum.tar.gz
+rm -rf ./chartmuseum.tar.gz
+rm -rf ./helm.tar.gz
+# Zot cleanup


Since the Zot cleanup comprises only a single line, and since we don't have similar comments elsewhere in this file, I would omit this comment.

webbnh · 2026-01-08T19:42:43Z

pkg/helm/actions/testdata/downloadHelm.sh

+set -e
+GOOS=${GOOS:-$(go env GOOS)}
+GOARCH=${GOARCH:-$(go env GOARCH)}
+HELM_VERSION=${HELM_VERSION:-3.19.0}


Having a fixed and silent fallback if HELM_VERSION is undefined is an invitation to trouble in the future (unless we keep the fallback version updated, which is its own kind of trouble).

I strongly recommend exiting with a message and error status if HELM_VERSION is undefined rather than supplying a default value.

I strongly recommend exiting with a message and error status if HELM_VERSION is undefined rather than supplying a default value.

I did not get you? Are you talking about installing helm chart?

In line 7 of the code cited above, we set the value of the local variable $HELM_VERSON to the value of the existing environment variable with the same name, if the environment variable exists; otherwise, we force the value to be a fixed version, which will become increasingly inappropriate as time passes (unless we remember to come back and update this file in the future).

Instead of using the ${ :- } (default value) syntax, we should test that $HELM_VERSION is set, and, if it is not set, then exit the script with an error status.

webbnh · 2026-01-08T19:51:16Z

pkg/helm/actions/testdata/downloadHelm.sh

+
+mkdir -p "$GOOS-$GOARCH"
+
+if [ ! -f "$GOOS-$GOARCH/helm" ]; then


In Bash scripts, [[ is prefered over [.

webbnh · 2026-01-08T19:56:26Z

pkg/helm/actions/testdata/downloadZot.sh

+#!/bin/bash
+
+set -e


Since you don't turn the -e option back off, I believe you can combine these two lines together:

Suggested change

#!/bin/bash

set -e

#!/bin/bash -e

Ditto for the other scripts.

webbnh · 2026-01-08T22:24:13Z

pkg/helm/actions/testdata/downloadZot.sh

+GOOS=${GOOS:-$(go env GOOS)}
+GOARCH=${GOARCH:-$(go env GOARCH)}
+ZOT_VERSION=${ZOT_VERSION:-v2.1.6}
+ZOT_ARTIFACT_URL="https://github.com/project-zot/zot/releases/download/$ZOT_VERSION/zot-$GOOS-$GOARCH"


Are you sure that this URL is correct? I think the filename should begin with zb instead of zot.

Also, in downloadHelm.sh, the value of HELM_VERSION doesn't include the v (instead, it is added when the URL is constructed); we should probably be consistent in the treatment of ZOT_VERSION.

Its zot. Docs say zb is zot benchmark build.

Changed downloadHelm to include v in the HELM_VERSION

Changed downloadHelm to include v in the HELM_VERSION

I happened to look at pkg/helm/actions/testdata/downloadChartmuseum.sh, and it omits the v from the variable value and adds it when constructing the URL. It would be best if we were consistent across all our files.

Is there a case where we would want a prefix other than v? If not, I suggest omitting it from the value.

webbnh · 2026-01-08T22:36:29Z

pkg/helm/actions/testdata/uploadOciCharts.sh

+# Push charts to OCI registry using helm push
+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 --ca-file=$CACERT
+
+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


Why do we push two charts? Is there some difference between them which is significant? Or, are we just trying to ensure that the target registry can hold more than one? Or, is it something else?

I planned to use the other chart for the test with TLS. I will remove now, I can add when we actually do the tests.

webbnh · 2026-01-08T22:41:58Z

pkg/helm/actions/testdata/uploadOciChartsWithoutTls.sh

@@ -0,0 +1,34 @@
+#!/bin/bash
+# Upload Helm charts as OCI artifacts to zot registry (without TLS)


This script is identical to uploadOciCharts.sh except for the flag on the $HELM command and the port on the registry, right? I recommend combining the scripts and either using a command line argument to select the functionality or just making a single script which runs both sets of tests.

webbnh · 2026-01-08T22:47:43Z

pkg/helm/actions/testdata/zot-stop.sh

@@ -0,0 +1,4 @@
+#!/bin/bash
+
+/usr/bin/pkill -15 zot


I recommend against using a numeric value for the signal and instead suggest using a symbolic value (or just omitting it and using the default signal). (I think -15 is probably -SIGTERM, but I'm not sure, since it can vary between "Unixes"; and, I think that SIGTERM is the default.)

webbnh · 2026-01-08T23:04:20Z

pkg/helm/actions/get_registry.go

+	return GetOCIRegistry(conf, false, false)
+}
+
+func GetOCIRegistry(conf *action.Configuration, insecure bool, plainHTTP bool) error {


"insecure" isn't really a good name: I don't think that the mere fact that we skip the TLS verification necessarily means that the connection is insecure. (At most, I think it means that we cannot be sure who we are connecting to, but the connection itself is, I think, still secure against third-parties.)

I think skipVerify or something similar would be better.

@webbnh I am thinking of moving this change to when we introduce GetDefaultOCIRegistry. (#15830). What do you say?

That seems reasonable.

webbnh · 2026-01-08T23:16:42Z

pkg/helm/actions/install_chart_test.go

 			releaseName:  "valid-chart-path",
 			chartPath:    "http://localhost:9181/charts/influxdb-3.0.2.tgz",
 			chartName:    "influxdb",
 			chartVersion: "3.0.2",
 		},
+		{
+			releaseName:  "valid-chart-path",
+			chartPath:    "oci://localhost:5000/helm-charts/mychart:0.1.0",
+			chartName:    "mychart",
+			chartVersion: "0.1.0",
+			plainHTTP:    true,
+		},
+		{
+			releaseName:  "valid-chart-path",
+			chartPath:    "oci://localhost:5443/helm-charts/mariadb:7.3.5",
+			chartName:    "mariadb",
+			chartVersion: "7.3.5",
+			insecureTLS:  true,
+		},


It strikes me as odd that all three of these scenarios use the same releaseName value. Prior to your addition, the scenarios each had unique names, which seems appropriate.

The significance of the first scenario is that it uses an HTTP-based chart path, while the new scenarios use OCI-based paths. The difference between the two new paths is that one uses plain HTTP and the other uses TLS without verification. So, there's basis for making the names unique.

However, I have to wonder whether we aren't missing a few cells in the matrix: in terms of variables, we have:

HTTP vs OCI path schemes

plain HTTP vs. TLS(?)

verified TLS vs. unverified TLS, and

valid vs. invalid paths.

Presumably, for invalid paths, we don't need to test all of the other dimensions. And, presumably, for plain HTTP, we don't need to test TLS verification. But, for a valid path using TLS, presumably we should test both verified and unverified connections with both HTTP and OCI schemes. So, I think that there should be about seven cases:

(1) invalid path

valid path

plain HTTP

(2) HTTP scheme

(3) OCI scheme

TLS

verified

(4) HTTP scheme

(5) OCI scheme

unverified

(6) HTTP scheme

(7) OCI scheme

But, it's possible that not all of those make sense.

The test cases make sense. But I am not planning to add TLS test cases till the basic oci support is done end to end. Hope that is fine?

My recommendation is to add the testing when the CUT (code under test) is added. (Technically, you're supposed to add the tests first, observe that they fail, then update the CUT, and observe that the tests pass...but I'm not sure that I've ever actually done that.... 🙃)

So, if the TLS support isn't going in, then you can omit adding the TLS testing; however, if you're adding latent support for TLS, then you should add the tests which correspond to whatever support you're adding to the CUT.

- Add zot server scripts (zot.sh, zotWithoutTls.sh, zot-stop.sh) and zot configuration files for TLS and non-TLS modes. - Add downloadHelm.sh to download helm v3.19.0 for OCI chart uploads. - Update GetOCIRegistry to support insecure TLS and plain HTTP modes. These modes are necessary as we are self hosting the oci registry for test. - Add TestInstallOCIChart test cases for OCI registries.

webbnh · 2026-01-09T18:41:21Z

pkg/helm/actions/testdata/zot-stop.sh

@@ -0,0 +1,4 @@
+#!/bin/bash -e
+
+/usr/bin/pkill -SIGTERM zot


Note that, in #15830, an attempt to use pkill is currently failing.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 18, 2025

openshift-ci bot added the component/backend Related to backend label Dec 18, 2025

sowmya-sl added 2 commits January 5, 2026 11:55

webbnh reviewed Jan 8, 2026

View reviewed changes

sowmya-sl force-pushed the new-oci-endpoints branch from 8ad9717 to c8c3aec Compare January 9, 2026 16:28

webbnh mentioned this pull request Jan 9, 2026

HELM-611: add OCI registry client for chart operations #15830

Draft

webbnh reviewed Jan 9, 2026

View reviewed changes


		mkdir -p "$GOOS-$GOARCH"

		if [ ! -f "$GOOS-$GOARCH/helm" ]; then

		@@ -0,0 +1,34 @@
		#!/bin/bash
		# Upload Helm charts as OCI artifacts to zot registry (without TLS)

Add new server endpoint for installing OCI charts without Helm repository #15863

Are you sure you want to change the base?

Add new server endpoint for installing OCI charts without Helm repository #15863

Conversation

sowmya-sl commented Dec 18, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

openshift-ci bot commented Dec 18, 2025

Uh oh!

openshift-ci bot commented Dec 18, 2025

Uh oh!

webbnh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sowmya-sl commented Dec 18, 2025 •

edited by openshift-ci bot

Loading

coderabbitai bot commented Dec 18, 2025 •

edited

Loading