Skip to content

Commit aa30da3

Browse files
authored
HELM-611: add OCI registry client for chart operations (#15925)
* fix(helm): add OCI registry client for chart operations 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 * fix(helm): address review comments for OCI registry client - Remove getDefaultRegistryClient from helm handler file. - Rename insecure param to skipTLSVerify for clarity - Add TLS skip verification and plainHTTP support - Use mockable newRegistryClient for testability - Fix variable naming (registryClient) - Return nil directly instead of err variable
1 parent c947e17 commit aa30da3

File tree

3 files changed

+178
-1
lines changed

3 files changed

+178
-1
lines changed

pkg/helm/actions/config.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,9 @@ func GetActionConfigurations(host, ns, token string, transport *http.RoundTrippe
5050
}
5151
conf := new(action.Configuration)
5252
conf.Init(confFlags, ns, "secrets", klog.Infof)
53-
53+
err = GetDefaultOCIRegistry(conf)
54+
if err != nil {
55+
klog.V(4).Infof("Failed to get default OCI registry: %v", err)
56+
}
5457
return conf
5558
}

pkg/helm/actions/get_registry.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package actions
2+
3+
import (
4+
"crypto/tls"
5+
"fmt"
6+
"net/http"
7+
8+
"helm.sh/helm/v3/pkg/action"
9+
"helm.sh/helm/v3/pkg/registry"
10+
)
11+
12+
// newRegistryClient is a package-level variable to allow mocking in tests
13+
var newRegistryClient = registry.NewClient
14+
15+
func GetDefaultOCIRegistry(conf *action.Configuration) error {
16+
return GetOCIRegistry(conf, false, false)
17+
}
18+
19+
func GetOCIRegistry(conf *action.Configuration, skipTLSVerify bool, plainHTTP bool) error {
20+
if conf == nil {
21+
return fmt.Errorf("action configuration cannot be nil")
22+
}
23+
opts := []registry.ClientOption{
24+
registry.ClientOptDebug(false),
25+
}
26+
if plainHTTP {
27+
opts = append(opts, registry.ClientOptPlainHTTP())
28+
}
29+
if skipTLSVerify {
30+
transport := http.DefaultTransport.(*http.Transport).Clone()
31+
transport.TLSClientConfig = &tls.Config{
32+
InsecureSkipVerify: true,
33+
}
34+
opts = append(opts, registry.ClientOptHTTPClient(&http.Client{Transport: transport}))
35+
}
36+
registryClient, err := newRegistryClient(opts...)
37+
if err != nil {
38+
return fmt.Errorf("failed to create registry client: %w", err)
39+
}
40+
conf.RegistryClient = registryClient
41+
return nil
42+
}
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
package actions
2+
3+
import (
4+
"errors"
5+
"io"
6+
"testing"
7+
8+
"github.com/stretchr/testify/require"
9+
"helm.sh/helm/v3/pkg/action"
10+
"helm.sh/helm/v3/pkg/chartutil"
11+
kubefake "helm.sh/helm/v3/pkg/kube/fake"
12+
"helm.sh/helm/v3/pkg/registry"
13+
"helm.sh/helm/v3/pkg/storage"
14+
"helm.sh/helm/v3/pkg/storage/driver"
15+
)
16+
17+
func TestGetDefaultOCIRegistry_Success(t *testing.T) {
18+
store := storage.Init(driver.NewMemory())
19+
conf := &action.Configuration{
20+
RESTClientGetter: FakeConfig{},
21+
Releases: store,
22+
KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
23+
Capabilities: chartutil.DefaultCapabilities,
24+
}
25+
require.Nil(t, conf.RegistryClient, "Registry Client should be nil")
26+
27+
// Store original values
28+
originalReleases := conf.Releases
29+
originalKubeClient := conf.KubeClient
30+
originalCapabilities := conf.Capabilities
31+
32+
err := GetDefaultOCIRegistry(conf)
33+
require.NoError(t, err)
34+
require.NotNil(t, conf.RegistryClient, "Registry Client should not be nil")
35+
36+
// Verify other configuration fields are not modified.
37+
require.Equal(t, originalReleases, conf.Releases, "Releases should not be modified")
38+
require.Equal(t, originalKubeClient, conf.KubeClient, "KubeClient should not be modified")
39+
require.Equal(t, originalCapabilities, conf.Capabilities, "Capabilities should not be modified")
40+
41+
}
42+
43+
func TestGetOCIRegistry_NilConfig(t *testing.T) {
44+
err := GetOCIRegistry(nil, false, false)
45+
require.Error(t, err)
46+
require.Contains(t, err.Error(), "action configuration cannot be nil")
47+
}
48+
49+
func TestGetOCIRegistry_Success(t *testing.T) {
50+
tests := []struct {
51+
name string
52+
skipTLSVerify bool
53+
plainHTTP bool
54+
}{
55+
{
56+
name: "default options",
57+
skipTLSVerify: false,
58+
plainHTTP: false,
59+
},
60+
{
61+
name: "with skipTLSVerify",
62+
skipTLSVerify: true,
63+
plainHTTP: false,
64+
},
65+
{
66+
name: "with plainHTTP",
67+
skipTLSVerify: false,
68+
plainHTTP: true,
69+
},
70+
{
71+
name: "with both skipTLSVerify and plainHTTP",
72+
skipTLSVerify: true,
73+
plainHTTP: true,
74+
},
75+
}
76+
originalNewRegistryClient := newRegistryClient
77+
defer func() {
78+
newRegistryClient = originalNewRegistryClient
79+
}()
80+
81+
for _, tt := range tests {
82+
newRegistryClient = func(options ...registry.ClientOption) (*registry.Client, error) {
83+
count := 0
84+
if tt.plainHTTP {
85+
count += 1
86+
}
87+
if tt.skipTLSVerify {
88+
count += 1
89+
}
90+
require.Equal(t, count, len(options)-1, "Expected %d options, got %d", count, len(options))
91+
return &registry.Client{}, nil
92+
}
93+
t.Run(tt.name, func(t *testing.T) {
94+
store := storage.Init(driver.NewMemory())
95+
conf := &action.Configuration{
96+
RESTClientGetter: FakeConfig{},
97+
Releases: store,
98+
KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
99+
Capabilities: chartutil.DefaultCapabilities,
100+
}
101+
require.Nil(t, conf.RegistryClient, "Registry Client should be nil initially")
102+
103+
err := GetOCIRegistry(conf, tt.skipTLSVerify, tt.plainHTTP)
104+
require.NoError(t, err)
105+
require.NotNil(t, conf.RegistryClient, "Registry Client should not be nil after GetOCIRegistry")
106+
})
107+
}
108+
}
109+
110+
func TestGetOCIRegistry_NewClientError(t *testing.T) {
111+
// Save original function and restore after test
112+
originalNewRegistryClient := newRegistryClient
113+
defer func() { newRegistryClient = originalNewRegistryClient }()
114+
115+
// Mock newRegistryClient to return an error
116+
newRegistryClient = func(options ...registry.ClientOption) (*registry.Client, error) {
117+
return nil, errors.New("mock registry client error")
118+
}
119+
120+
store := storage.Init(driver.NewMemory())
121+
conf := &action.Configuration{
122+
RESTClientGetter: FakeConfig{},
123+
Releases: store,
124+
KubeClient: &kubefake.PrintingKubeClient{Out: io.Discard},
125+
Capabilities: chartutil.DefaultCapabilities,
126+
}
127+
128+
err := GetOCIRegistry(conf, false, false)
129+
require.Error(t, err)
130+
require.Contains(t, err.Error(), "failed to create registry client")
131+
require.Contains(t, err.Error(), "mock registry client error")
132+
}

0 commit comments

Comments
 (0)