diff --git a/pkg/helm/actions/auth.go b/pkg/helm/actions/auth.go index 1aab9a79fb3..c7c646ba508 100644 --- a/pkg/helm/actions/auth.go +++ b/pkg/helm/actions/auth.go @@ -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 tlsKeyFile, tlsCertFile, err := setupTlsCertFile(connectionConfig.TLSClientConfig.Name, configNamespace, coreClient) if err != nil { return nil, err @@ -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 @@ -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 @@ -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 diff --git a/pkg/helm/actions/auth_test.go b/pkg/helm/actions/auth_test.go new file mode 100644 index 00000000000..405dd5c9485 --- /dev/null +++ b/pkg/helm/actions/auth_test.go @@ -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, + }, + { + 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) + } + + 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 { + f.Close() + } +} diff --git a/pkg/helm/actions/get_chart.go b/pkg/helm/actions/get_chart.go index 5d219352a44..40fb48301b2 100644 --- a/pkg/helm/actions/get_chart.go +++ b/pkg/helm/actions/get_chart.go @@ -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) diff --git a/pkg/helm/actions/get_chart_test.go b/pkg/helm/actions/get_chart_test.go index a89524d0cc6..1dd994d5674 100644 --- a/pkg/helm/actions/get_chart_test.go +++ b/pkg/helm/actions/get_chart_test.go @@ -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") } }) } diff --git a/pkg/helm/actions/install_chart.go b/pkg/helm/actions/install_chart.go index d6b087e7472..9776b5ae3b6 100644 --- a/pkg/helm/actions/install_chart.go +++ b/pkg/helm/actions/install_chart.go @@ -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) @@ -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 { diff --git a/pkg/helm/actions/upgrade_release.go b/pkg/helm/actions/upgrade_release.go index aaf5d100d6e..36f1e563a19 100644 --- a/pkg/helm/actions/upgrade_release.go +++ b/pkg/helm/actions/upgrade_release.go @@ -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 { @@ -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) if err != nil {