Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sowmya-sl wants to merge 2 commits into
base: main
Choose a base branch
from
+217 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
37075cf
fix(helm): add OCI registry client for chart operations
sowmya-sl Dec 9, 2025
db77391
fix(helm): address review comments for OCI registry client
sowmya-sl Jan 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/helm/actions/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ func GetActionConfigurations(host, ns, token string, transport *http.RoundTrippe
}
conf := new(action.Configuration)
conf.Init(confFlags, ns, "secrets", klog.Infof)

err = GetDefaultOCIRegistry(conf)
if err != nil {
klog.V(4).Infof("Failed to get default OCI registry: %v", err)
}
return conf
}
44 changes: 44 additions & 0 deletions pkg/helm/actions/get_registry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package actions

import (
"crypto/tls"
"fmt"
"net/http"

"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/registry"
)

// newRegistryClient is a package-level variable to allow mocking in tests
var newRegistryClient = registry.NewClient

func GetDefaultOCIRegistry(conf *action.Configuration) error {
return GetOCIRegistry(conf, false, false)
}

func GetOCIRegistry(conf *action.Configuration, skipTLSVerify bool, plainHTTP bool) error {
if conf == nil {
return fmt.Errorf("action configuration cannot be nil")
}
opts := []registry.ClientOption{
registry.ClientOptDebug(false),
}
if plainHTTP {
opts = append(opts, registry.ClientOptPlainHTTP())
}
if skipTLSVerify {
opts = append(opts, registry.ClientOptHTTPClient(&http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
},
},
}))
Comment on lines +29 to +36
Copy link

@coderabbitai coderabbitai bot Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

git ls-files | head -20

Repository: openshift/console

Length of output: 500

🏁 Script executed:

fd -t f "get_registry.go" -x wc -l {}

Repository: openshift/console

Length of output: 98

🏁 Script executed:

cat -n pkg/helm/actions/get_registry.go | head -50

Repository: openshift/console

Length of output: 1426

🏁 Script executed:

# Check the minimum Go version supported by this project
if [ -f "go.mod" ]; then
  head -5 go.mod
fi

Repository: openshift/console

Length of output: 127

🌐 Web query:

Go http.DefaultTransport http.Transport Clone method

💡 Result:

Yes — http.DefaultTransport is a RoundTripper whose concrete type is *http.Transport, and http.Transport has a Clone method you can use to copy the default transport instead of mutating it.

Example:

  • safe: t := http.DefaultTransport.(*http.Transport).Clone()
  • then modify and use: t.MaxIdleConns = 50; client := &http.Client{Transport: t}

Notes:

  • Do not set fields on http.DefaultTransport directly (that mutates the global). Clone was added to Transport to make copying easy (Go 1.13+). [1][2][3]

Sources:
[1] Stack Overflow — “How to use http.DefaultTransport …” (example using Clone).
[2] Go 1.13 release notes (Transport changes; Clone added).
[3] net/http docs (DefaultTransport is &Transport{...}).

🏁 Script executed:

rg -t go "ClientOptHTTPClient|http\.Client" --context 3 | head -50

Repository: openshift/console

Length of output: 2576

🏁 Script executed:

rg -t go "registry\.NewClient|registry\.Client" --context 2 | head -40

Repository: openshift/console

Length of output: 2575

🏁 Script executed:

cat -n pkg/helm/actions/get_registry_test.go

Repository: openshift/console

Length of output: 4961

🏁 Script executed:

rg -t go "Proxy|proxy|ProxyFromEnvironment" pkg/helm/actions/

Repository: openshift/console

Length of output: 251

🌐 Web query:

http.DefaultTransport ProxyFromEnvironment proxy support Go

💡 Result:

Short answer

  • net/http's DefaultTransport uses ProxyFromEnvironment, so it will honor HTTP_PROXY/http_proxy, HTTPS_PROXY/https_proxy and NO_PROXY/no_proxy (the exact parsing/semantics come from golang.org/x/net/http/httpproxy). [1][2]

Notes / gotchas

  • If you create a Transport with an empty struct (e.g. &http.Transport{}), Proxy is nil and environment proxying won't be used — DefaultTransport sets Proxy: ProxyFromEnvironment for you. [1][3]

Sources

  • DefaultTransport and its Proxy field in net/http docs. [1]
  • httpproxy implementation (FromEnvironment / rules for HTTP(S)_PROXY and NO_PROXY). [2]
  • Common pitfall: empty Transport vs DefaultTransport (discussion/example). [3]

Clone http.DefaultTransport to preserve proxy and timeout settings.

The bare http.Transport loses ProxyFromEnvironment support, breaking registry access in proxied environments when skipTLSVerify is true—a critical gap in enterprise deployments where corporate proxies are standard. Additionally, timeouts and HTTP/2 configuration from the default transport are dropped.

🔧 Proposed fix 
-    opts = append(opts, registry.ClientOptHTTPClient(&http.Client{
-        Transport: &http.Transport{
-            TLSClientConfig: &tls.Config{
-                InsecureSkipVerify: true,
-            },
-        },
-    }))
+    transport := http.DefaultTransport.(*http.Transport).Clone()
+    transport.TLSClientConfig = &tls.Config{
+        InsecureSkipVerify: true,
+        // MinVersion: tls.VersionTLS12, // or TLS13 if your registries support it
+    }
+    opts = append(opts, registry.ClientOptHTTPClient(&http.Client{Transport: transport}))
🧰 Tools
🪛 ast-grep (0.40.5)

[warning] 31-33: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures

(missing-ssl-minversion-go)

🤖 Prompt for AI Agents 
In `@pkg/helm/actions/get_registry.go` around lines 29 - 36, When skipTLSVerify is
true, don't create a bare http.Transport (which loses ProxyFromEnvironment and
default timeouts/HTTP2 settings); instead clone the default transport, assert
http.DefaultTransport.(*http.Transport), call Clone() to get a copy, set its
TLSClientConfig.InsecureSkipVerify = true, and use that cloned transport in the
http.Client passed to registry.ClientOptHTTPClient so Proxy and other default
behaviors are preserved.

}
registryClient, err := newRegistryClient(opts...)
if err != nil {
return fmt.Errorf("failed to create registry client: %w", err)
}
conf.RegistryClient = registryClient
return nil
}
132 changes: 132 additions & 0 deletions pkg/helm/actions/get_registry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
package actions

import (
"errors"
"io"
"testing"

"github.com/stretchr/testify/require"
"helm.sh/helm/v3/pkg/action"
"helm.sh/helm/v3/pkg/chartutil"
kubefake "helm.sh/helm/v3/pkg/kube/fake"
"helm.sh/helm/v3/pkg/registry"
"helm.sh/helm/v3/pkg/storage"
"helm.sh/helm/v3/pkg/storage/driver"
)

func TestGetDefaultOCIRegistry_Success(t *testing.T) {
store := storage.Init(driver.NewMemory())
conf := &action.Configuration{
RESTClientGetter: FakeConfig{},
Releases: store,
KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
Capabilities: chartutil.DefaultCapabilities,
}
require.Nil(t, conf.RegistryClient, "Registry Client should be nil")

// Store original values
originalReleases := conf.Releases
originalKubeClient := conf.KubeClient
originalCapabilities := conf.Capabilities

err := GetDefaultOCIRegistry(conf)
require.NoError(t, err)
require.NotNil(t, conf.RegistryClient, "Registry Client should not be nil")

// Verify other configuration fields are not modified.
require.Equal(t, originalReleases, conf.Releases, "Releases should not be modified")
require.Equal(t, originalKubeClient, conf.KubeClient, "KubeClient should not be modified")
require.Equal(t, originalCapabilities, conf.Capabilities, "Capabilities should not be modified")

}

func TestGetOCIRegistry_NilConfig(t *testing.T) {
err := GetOCIRegistry(nil, false, false)
require.Error(t, err)
require.Contains(t, err.Error(), "action configuration cannot be nil")
}

func TestGetOCIRegistry_Success(t *testing.T) {
tests := []struct {
name string
skipTLSVerify bool
plainHTTP bool
}{
{
name: "default options",
skipTLSVerify: false,
plainHTTP: false,
},
{
name: "with skipTLSVerify",
skipTLSVerify: true,
plainHTTP: false,
},
{
name: "with plainHTTP",
skipTLSVerify: false,
plainHTTP: true,
},
{
name: "with both skipTLSVerify and plainHTTP",
skipTLSVerify: true,
plainHTTP: true,
},
}

for _, tt := range tests {
originalNewRegistryClient := newRegistryClient
defer func() {
newRegistryClient = originalNewRegistryClient
}()
Comment on lines +77 to +81
Copy link

@webbnh webbnh Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this is going to work as intended: the defer operation is going to be done four times, since it is inside the loop, and so it will be executed four times when the function returns; however, on each iteration, originalNewRegistryClient will be overwritten with the mock from the previous iteration, and so on each execution of the deferred function, it will restore newRegistryClient to the third iteration's mock.

There is a way to make each deferral restore the previous state, such that, after the last completes, the state is back to the original value, but it would be much easier/simpler/more-effective to simply move lines 78-81 to before the loop.

Also, there is something you should check: since your test is using t.Run() to execute the scenarios in separate go-routines, you should make sure that the deferred function(s) doesn't run before all the scenarios are complete. (That is, can control return from TestGetOCIRegistry_Success() and trigger the deferred actions before all of the t.Run() functions have finished and are no longer using newRegistryClient?...I don't recall that detail.)

newRegistryClient = func(options ...registry.ClientOption) (*registry.Client, error) {
count := 0
if tt.plainHTTP {
count += 1
}
if tt.skipTLSVerify {
count += 1
}
require.Equal(t, count, len(options)-1, "Expected %d options, got %d", count, len(options))
return &registry.Client{}, nil
}
t.Run(tt.name, func(t *testing.T) {
store := storage.Init(driver.NewMemory())
conf := &action.Configuration{
RESTClientGetter: FakeConfig{},
Releases: store,
KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
Capabilities: chartutil.DefaultCapabilities,
}
require.Nil(t, conf.RegistryClient, "Registry Client should be nil initially")

err := GetOCIRegistry(conf, tt.skipTLSVerify, tt.plainHTTP)
require.NoError(t, err)
require.NotNil(t, conf.RegistryClient, "Registry Client should not be nil after GetOCIRegistry")
})
}
}

func TestGetOCIRegistry_NewClientError(t *testing.T) {
Comment on lines +48 to +110

This comment was marked as resolved.

Sign in to view

Copy link
Contributor Author

@sowmya-sl sowmya-sl Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this in the test.

// Save original function and restore after test
originalNewRegistryClient := newRegistryClient
defer func() { newRegistryClient = originalNewRegistryClient }()

// Mock newRegistryClient to return an error
newRegistryClient = func(options ...registry.ClientOption) (*registry.Client, error) {
return nil, errors.New("mock registry client error")
}

store := storage.Init(driver.NewMemory())
conf := &action.Configuration{
RESTClientGetter: FakeConfig{},
Releases: store,
KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
Capabilities: chartutil.DefaultCapabilities,
}

err := GetOCIRegistry(conf, false, false)
require.Error(t, err)
require.Contains(t, err.Error(), "failed to create registry client")
require.Contains(t, err.Error(), "mock registry client error")
}
5 changes: 5 additions & 0 deletions pkg/helm/handlers/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ var fakeReleaseManifest = "manifest-data"
func fakeHelmHandler() helmHandlers {
return helmHandlers{
getActionConfigurations: getFakeActionConfigurations,
getDefaultOCIRegistry: fakeGetDefaultOCIRegistry,
}
}

Expand Down Expand Up @@ -201,6 +202,10 @@ func getFakeActionConfigurations(string, string, string, *http.RoundTripper) *ac
}
}

func fakeGetDefaultOCIRegistry(conf *action.Configuration) error {
return nil
}

func TestHelmHandlers_HandleHelmList(t *testing.T) {
tests := []struct {
name string
Expand Down
32 changes: 32 additions & 0 deletions pkg/helm/handlers/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func New(apiUrl string, transport http.RoundTripper, kubeversionGetter version.K
uninstallReleaseAsync: actions.UninstallReleaseAsync,
rollbackRelease: actions.RollbackRelease,
getReleaseHistory: actions.GetReleaseHistory,
getDefaultOCIRegistry: actions.GetDefaultOCIRegistry,
}

h.newProxy = func(bearerToken string) (getter chartproxy.Proxy, err error) {
Expand Down Expand Up @@ -72,6 +73,7 @@ type helmHandlers struct {
getChart func(chartUrl string, conf *action.Configuration, namespace string, client dynamic.Interface, coreClient corev1client.CoreV1Interface, filesCleanup bool, indexEntry string) (*chart.Chart, error)
getReleaseHistory func(releaseName string, conf *action.Configuration) ([]*release.Release, error)
newProxy func(bearerToken string) (chartproxy.Proxy, error)
getDefaultOCIRegistry func(*action.Configuration) error
}

func (h *helmHandlers) restConfig(bearerToken string) *rest.Config {
Expand Down Expand Up @@ -117,6 +119,11 @@ func (h *helmHandlers) HandleHelmInstall(user *auth.User, w http.ResponseWriter,
}

conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
err = h.getDefaultOCIRegistry(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)})
return
Comment on lines +123 to +125
Copy link

@webbnh webbnh Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a unit test scenario which exercises this code? (Ditto for the other changes to this module?)

It looks like we probably don't have tests for any of the error cases in this (or the other) functions...but it would be good if the new code brought with it new tests.... 😇

Copy link
Contributor Author

@sowmya-sl sowmya-sl Jan 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No UTs for this right now. But I will check on that.

}
handlerClients, err := NewHandlerClients(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: err.Error()})
Expand All @@ -143,6 +150,11 @@ func (h *helmHandlers) HandleHelmInstallAsync(user *auth.User, w http.ResponseWr
}

conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
err = h.getDefaultOCIRegistry(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)})
return
}
handlerClients, err := NewHandlerClients(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: err.Error()})
Expand Down Expand Up @@ -210,6 +222,11 @@ func (h *helmHandlers) HandleChartGet(user *auth.User, w http.ResponseWriter, r
indexEntry := params.Get("indexEntry")
// scope request to default namespace
conf := h.getActionConfigurations(h.ApiServerHost, "default", user.Token, &h.Transport)
err := h.getDefaultOCIRegistry(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)})
return
}
handlerClients, err := NewHandlerClients(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: err.Error()})
Expand Down Expand Up @@ -237,6 +254,11 @@ func (h *helmHandlers) HandleUpgradeRelease(user *auth.User, w http.ResponseWrit
}

conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
err = h.getDefaultOCIRegistry(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)})
return
}
handlerClients, err := NewHandlerClients(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: err.Error()})
Expand Down Expand Up @@ -267,6 +289,11 @@ func (h *helmHandlers) HandleUpgradeReleaseAsync(user *auth.User, w http.Respons
}

conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
err = h.getDefaultOCIRegistry(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)})
return
}
handlerClients, err := NewHandlerClients(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: err.Error()})
Expand Down Expand Up @@ -314,6 +341,11 @@ func (h *helmHandlers) HandleRollbackRelease(user *auth.User, w http.ResponseWri
}

conf := h.getActionConfigurations(h.ApiServerHost, req.Namespace, user.Token, &h.Transport)
err = h.getDefaultOCIRegistry(conf)
if err != nil {
serverutils.SendResponse(w, http.StatusBadGateway, serverutils.ApiError{Err: fmt.Sprintf("Failed to get default registry: %v", err)})
return
}
rel, err := h.rollbackRelease(req.Name, req.Version, conf)
if err != nil {
if err.Error() == actions.ErrReleaseRevisionNotFound.Error() {
Expand Down