Skip to content

OCPBUGS-44235: Fix Helm chart installation with CA/TLS certificates #15607

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

Merged
openshift-merge-bot merged 2 commits into from
Nov 5, 2025
Merged

OCPBUGS-44235: Fix Helm chart installation with CA/TLS certificates #15607

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
4d472f0
OCPBUGS-44235: Fix Helm chart installation with CA/TLS certificates
martinszuc Oct 14, 2025
aed7a1e
OCPBUGS-44235: Add tests for fix
martinszuc Oct 16, 2025
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
4 changes: 0 additions & 4 deletions pkg/helm/actions/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ func setUpAuthentication(chartPathOptions *action.ChartPathOptions, connectionCo
tlsFiles := []*os.File{}
//set up tls cert and key
if connectionConfig.TLSClientConfig != (configv1.SecretNameReference{}) {
chartPathOptions.RepoURL = connectionConfig.URL
Copy link

@webbnh webbnh Nov 4, 2025

Choose a reason for hiding this comment

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

After you remove these four (write) references, are there any remaining references (read or write) to this field? If not, shouldn't you remove the field itself?

The field claims to be associated with the --repo option. Is that option used (either as an input to the package or in a generated command line executed by the package)?

Copy link
Contributor

@baijum baijum Nov 5, 2025

Choose a reason for hiding this comment

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

The ChartPathOptions is not defined in our code. It's part of helm API. It's imported from: helm.sh/helm/v3/pkg/action.

tlsKeyFile, tlsCertFile, err := setupTlsCertFile(connectionConfig.TLSClientConfig.Name, configNamespace, coreClient)
if err != nil {
return nil, err
Expand All @@ -31,7 +30,6 @@ func setUpAuthentication(chartPathOptions *action.ChartPathOptions, connectionCo
}
//set up ca certificate
if connectionConfig.CA != (configv1.ConfigMapNameReference{}) {
chartPathOptions.RepoURL = connectionConfig.URL
caFile, err := setupCaCertFile(connectionConfig.CA.Name, configNamespace, coreClient)
if err != nil {
return nil, err
Expand All @@ -47,7 +45,6 @@ func setUpAuthenticationProject(chartPathOptions *action.ChartPathOptions, conne
var secretNamespace string
//set up tls cert and key
if connectionConfig.TLSClientConfig != (configv1.SecretNameReference{}) {
chartPathOptions.RepoURL = connectionConfig.URL
tlsKeyFile, tlsCertFile, err := setupTlsCertFile(connectionConfig.TLSClientConfig.Name, namespace, coreClient)
if err != nil {
return nil, err
Expand Down Expand Up @@ -77,7 +74,6 @@ func setUpAuthenticationProject(chartPathOptions *action.ChartPathOptions, conne
}
//set up ca certificate
if connectionConfig.CA != (configv1.ConfigMapNameReference{}) {
chartPathOptions.RepoURL = connectionConfig.URL
caFile, err := setupCaCertFile(connectionConfig.CA.Name, namespace, coreClient)
if err != nil {
return nil, err
Expand Down
141 changes: 141 additions & 0 deletions pkg/helm/actions/auth_test.go
View file
Open in desktop
Copy link

@webbnh webbnh Nov 4, 2025

Choose a reason for hiding this comment

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

I have the impression that there are no references to ChartPathOptions.RepoURL outside of this (test) file. If that's the case, is there any point to adding tests which make sure that it is not set? (That is, we expend CPU time and disk space just to have this test, when it seems like it doesn't matter whether the field is set or not, since it seems that nothing reads it...or am I missing something?)

Copy link
Contributor Author

@martinszuc martinszuc Nov 10, 2025

Choose a reason for hiding this comment

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

Great question! I thought of these tests as regression prevention. The bug happened specifically because we were setting RepoURL, which forced Helm to look for cache files that don't exist in the console pod.
Since nothing currently reads RepoURL, the tests act as a sort of safe guard against adding chartPathOptions.RepoURL = connectionConfig.URL again, which would break installing charts with CA/TLS configuration.

Copy link
Contributor Author

@martinszuc martinszuc Nov 10, 2025

Choose a reason for hiding this comment

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

But your question brought up a idea that this should be documented in code with a comment.
Link to the jira issue maybe.
What do you think @webbnh? I'll try to add it in my refactor pull request if you agree😄

Copy link

@webbnh webbnh Nov 10, 2025

Choose a reason for hiding this comment

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

As a general rule, I'm on-board with preventative safeguards. In this case, I'm not sure where in the code to put your suggested comment. 😞 We would want it in some place where developers always look (which probably doesn't exist...), or we want it in each place where a developer would be tempted to add the offending code (which would be at least the places that you edited, and probably more) which is likely to create a "litter problem". 😝

So, given those as options, your idea of adding a test looks like a much superior solution. 😆

Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package actions

import (
"io/ioutil"
"testing"

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/api/helm/v1beta1"
"github.com/stretchr/testify/require"
"helm.sh/helm/v3/pkg/action"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
k8sfake "k8s.io/client-go/kubernetes/fake"
)

func TestAuthenticationDoesNotSetRepoURL(t *testing.T) {
tests := []struct {
name string
config *v1beta1.ConnectionConfig
hasCA bool
hasTLS bool
expectCaFile bool
expectCertFile bool
}{
{
name: "CA certificate only",
config: &v1beta1.ConnectionConfig{
URL: "https://charts.example.com",
CA: configv1.ConfigMapNameReference{Name: "test-ca"},
},
hasCA: true,
expectCaFile: true,
},
Comment on lines +26 to +34
Copy link

@webbnh webbnh Nov 4, 2025

Choose a reason for hiding this comment

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

You have the hasCA case, the hasTLS case, and the has-both case...should we have a case for has-neither?

Copy link
Contributor Author

@martinszuc martinszuc Nov 10, 2025

Choose a reason for hiding this comment

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

I completely agree. This test case is valid and should be added for completeness and regression guard I mentioned in another review comment.

{
name: "Client TLS only",
config: &v1beta1.ConnectionConfig{
URL: "https://charts.example.com",
TLSClientConfig: configv1.SecretNameReference{Name: "test-tls"},
},
hasTLS: true,
expectCertFile: true,
},
{
name: "Both CA and client TLS",
config: &v1beta1.ConnectionConfig{
URL: "https://charts.example.com",
CA: configv1.ConfigMapNameReference{Name: "test-ca"},
TLSClientConfig: configv1.SecretNameReference{Name: "test-tls"},
},
hasCA: true,
hasTLS: true,
expectCaFile: true,
expectCertFile: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
opts := &action.ChartPathOptions{}
objs := []runtime.Object{}

if tt.hasCA {
caConfigMap := &v1.ConfigMap{
Data: map[string]string{caBundleKey: "FAKE_CA_DATA"},
ObjectMeta: metav1.ObjectMeta{Name: "test-ca", Namespace: configNamespace},
}
objs = append(objs, caConfigMap)
}

if tt.hasTLS {
cert, _ := ioutil.ReadFile("./server.crt")
key, _ := ioutil.ReadFile("./server.key")
tlsSecret := &v1.Secret{
Data: map[string][]byte{
tlsSecretCertKey: cert,
tlsSecretKey: key,
},
ObjectMeta: metav1.ObjectMeta{Name: "test-tls", Namespace: configNamespace},
}
objs = append(objs, tlsSecret)
}

coreClient := k8sfake.NewSimpleClientset(objs...).CoreV1()

tlsFiles, err := setUpAuthentication(opts, tt.config, coreClient)
require.NoError(t, err)

require.Empty(t, opts.RepoURL)

// Verify auth fields are set
if tt.expectCaFile {
require.NotEmpty(t, opts.CaFile)
}
if tt.expectCertFile {
require.NotEmpty(t, opts.CertFile)
require.NotEmpty(t, opts.KeyFile)
}
Comment on lines +92 to +98
Copy link

@webbnh webbnh Nov 4, 2025

Choose a reason for hiding this comment

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

"Not empty" is a pretty low bar...should we be checking the actual value? (Ditto for line 135.)

Copy link
Contributor Author

@martinszuc martinszuc Nov 10, 2025

Choose a reason for hiding this comment

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

You're right that we could be more thorough. I am open to strengthening the assertions. Maybe add a check if they point to a readable file or check the contents match expected.
Nice catch again Webb!


expectedFileCount := 0
if tt.hasCA {
expectedFileCount++
}
if tt.hasTLS {
expectedFileCount += 2
}
require.Len(t, tlsFiles, expectedFileCount)

// Cleanup
for _, f := range tlsFiles {
f.Close()
}
})
}
}

func TestAuthenticationProjectDoesNotSetRepoURL(t *testing.T) {
opts := &action.ChartPathOptions{}
config := &v1beta1.ConnectionConfigNamespaceScoped{
URL: "https://charts.example.com",
CA: configv1.ConfigMapNameReference{Name: "test-ca"},
}

caConfigMap := &v1.ConfigMap{
Data: map[string]string{caBundleKey: "FAKE_CA_DATA"},
ObjectMeta: metav1.ObjectMeta{Name: "test-ca", Namespace: "test-ns"},
}

coreClient := k8sfake.NewSimpleClientset(caConfigMap).CoreV1()

tlsFiles, err := setUpAuthenticationProject(opts, config, coreClient, "test-ns")
require.NoError(t, err)

require.Empty(t, opts.RepoURL)
require.NotEmpty(t, opts.CaFile)

// Cleanup
for _, f := range tlsFiles {
Comment on lines +135 to +138
Copy link

@webbnh webbnh Nov 4, 2025

Choose a reason for hiding this comment

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

No check on the number of TLS files?

Copy link
Contributor Author

@martinszuc martinszuc Nov 10, 2025

Choose a reason for hiding this comment

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

Good eye @webbnh!
I'll add it in my refactor pr asap :)

f.Close()
}
}
6 changes: 1 addition & 5 deletions pkg/helm/actions/get_chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,7 @@ func GetChart(url string, conf *action.Configuration, repositoryNamespace string
}
}

if len(tlsFiles) == 0 {
chartLocation = url
} else {
chartLocation = chartInfo.Name
}
chartLocation = url

cmd.ChartPathOptions.Version = chartInfo.Version
chartPath, err = cmd.ChartPathOptions.LocateChart(chartLocation, settings)
Expand Down
1 change: 1 addition & 0 deletions pkg/helm/actions/get_chart_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ func TestGetChartWithTlsData(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, chart.Metadata)
require.Equal(t, chart.Metadata.Name, test.chartName)
require.NotEmpty(t, chart.Templates, "Chart must have templates")
}
})
}
Expand Down
12 changes: 2 additions & 10 deletions pkg/helm/actions/install_chart.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,7 @@ func InstallChart(ns, name, url string, vals map[string]interface{}, conf *actio
}
}
cmd.ReleaseName = name
if len(tlsFiles) == 0 {
chartLocation = url
} else {
chartLocation = chartInfo.Name
}
chartLocation = url

cmd.ChartPathOptions.Version = chartInfo.Version
cp, err = cmd.ChartPathOptions.LocateChart(chartLocation, settings)
Expand Down Expand Up @@ -150,11 +146,7 @@ func InstallChartAsync(ns, name, url string, vals map[string]interface{}, conf *
}
}
cmd.ReleaseName = name
if len(tlsFiles) == 0 {
chartLocation = url
} else {
chartLocation = chartInfo.Name
}
chartLocation = url
cmd.ChartPathOptions.Version = chartInfo.Version
cp, err = cmd.ChartPathOptions.LocateChart(chartLocation, settings)
if err != nil {
Expand Down
12 changes: 2 additions & 10 deletions pkg/helm/actions/upgrade_release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,7 @@ func UpgradeRelease(
return nil, fmt.Errorf("error setting up authentication: %v", err)
}
}
if len(tlsFiles) == 0 {
chartLocation = chartUrl
} else {
chartLocation = chartInfo.Name
}
chartLocation = chartUrl
client.ChartPathOptions.Version = chartInfo.Version
cp, err = client.ChartPathOptions.LocateChart(chartLocation, settings)
if err != nil {
Expand Down Expand Up @@ -199,11 +195,7 @@ func UpgradeReleaseAsync(
return nil, fmt.Errorf("error setting up authentication: %v", err)
}
}
if len(tlsFiles) == 0 {
chartLocation = chartUrl
} else {
chartLocation = chartInfo.Name
}
chartLocation = chartUrl
client.ChartPathOptions.Version = chartInfo.Version
cp, err = client.ChartPathOptions.LocateChart(chartLocation, settings)
Comment on lines +198 to 200
Copy link

@webbnh webbnh Nov 4, 2025

Choose a reason for hiding this comment

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

Unless we are trying to minimize the changes (and, possibly, even if we're not!...), I recommend removing the chartLocation variable: it no longer has a life which is independent of chartUrl, so we should just replace it with the latter.

A similar comment applies to the occurrences in pkg/helm/actions/install_chart.go and pkg/helm/actions/get_chart.go.

Copy link
Contributor Author

@martinszuc martinszuc Nov 10, 2025

Choose a reason for hiding this comment

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

You are correct @webbnh! I fully agree that chartLocation is redudant now. I will open a new pr for cleaning up the code asap.
Great catch! This definitely would be an improvement towards cleaner code! 😄

if err != nil {
Expand Down