From f801efc4df9a9df1e9c69af414ce4bcf40191904 Mon Sep 17 00:00:00 2001 From: faiq Date: Mon, 3 Jun 2024 08:55:29 -0600 Subject: [PATCH] fix: pointer checks if user doesn't specify CASecret --- .../helmchartproxy_controller_phases.go | 2 +- .../helmchartproxy_controller_phases_test.go | 79 ++++++++++++++++ .../helmreleaseproxy_controller.go | 4 +- .../helmreleaseproxy_controller_test.go | 94 +++++++++++++++++++ 4 files changed, 177 insertions(+), 2 deletions(-) diff --git a/controllers/helmchartproxy/helmchartproxy_controller_phases.go b/controllers/helmchartproxy/helmchartproxy_controller_phases.go index 1c5f3180..9d9712f3 100644 --- a/controllers/helmchartproxy/helmchartproxy_controller_phases.go +++ b/controllers/helmchartproxy/helmchartproxy_controller_phases.go @@ -237,7 +237,7 @@ func constructHelmReleaseProxy(existing *addonsv1alpha1.HelmReleaseProxy, helmCh helmReleaseProxy.Spec.TLSConfig = helmChartProxy.Spec.TLSConfig - if helmReleaseProxy.Spec.TLSConfig != nil { + if helmReleaseProxy.Spec.TLSConfig != nil && helmReleaseProxy.Spec.TLSConfig.CASecretRef != nil { // If the namespace is not set, set it to the namespace of the HelmChartProxy if helmReleaseProxy.Spec.TLSConfig.CASecretRef.Namespace == "" { helmReleaseProxy.Spec.TLSConfig.CASecretRef.Namespace = helmChartProxy.Namespace diff --git a/controllers/helmchartproxy/helmchartproxy_controller_phases_test.go b/controllers/helmchartproxy/helmchartproxy_controller_phases_test.go index 84f03ad7..2cd15343 100644 --- a/controllers/helmchartproxy/helmchartproxy_controller_phases_test.go +++ b/controllers/helmchartproxy/helmchartproxy_controller_phases_test.go @@ -1010,6 +1010,85 @@ func TestConstructHelmReleaseProxy(t *testing.T) { }, }, }, + { + name: "construct helm release proxy with insecure TLS", + existing: nil, + helmChartProxy: &addonsv1alpha1.HelmChartProxy{ + TypeMeta: metav1.TypeMeta{ + APIVersion: addonsv1alpha1.GroupVersion.String(), + Kind: "HelmChartProxy", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-hcp", + Namespace: "test-namespace", + }, + Spec: addonsv1alpha1.HelmChartProxySpec{ + ReleaseName: "test-release-name", + ChartName: "test-chart-name", + RepoURL: "https://test-repo-url", + ReleaseNamespace: "test-release-namespace", + Options: addonsv1alpha1.HelmOptions{ + EnableClientCache: true, + Timeout: &metav1.Duration{ + Duration: 10 * time.Minute, + }, + }, + TLSConfig: &addonsv1alpha1.TLSConfig{ + InsecureSkipTLSVerify: true, + }, + }, + }, + parsedValues: "test-parsed-values", + cluster: &clusterv1.Cluster{ + TypeMeta: metav1.TypeMeta{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + }, + expected: &addonsv1alpha1.HelmReleaseProxy{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-chart-name-test-cluster-", + Namespace: "test-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: addonsv1alpha1.GroupVersion.String(), + Kind: "HelmChartProxy", + Name: "test-hcp", + Controller: ptr.To(true), + BlockOwnerDeletion: ptr.To(true), + }, + }, + Labels: map[string]string{ + clusterv1.ClusterNameLabel: "test-cluster", + addonsv1alpha1.HelmChartProxyLabelName: "test-hcp", + }, + }, + Spec: addonsv1alpha1.HelmReleaseProxySpec{ + ClusterRef: corev1.ObjectReference{ + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: "test-cluster", + }, + ReleaseName: "test-release-name", + ChartName: "test-chart-name", + RepoURL: "https://test-repo-url", + ReleaseNamespace: "test-release-namespace", + Values: "test-parsed-values", + Options: addonsv1alpha1.HelmOptions{ + EnableClientCache: true, + Timeout: &metav1.Duration{ + Duration: 10 * time.Minute, + }, + }, + TLSConfig: &addonsv1alpha1.TLSConfig{ + InsecureSkipTLSVerify: true, + }, + }, + }, + }, } for _, tc := range testCases { diff --git a/controllers/helmreleaseproxy/helmreleaseproxy_controller.go b/controllers/helmreleaseproxy/helmreleaseproxy_controller.go index 7a30f645..1b40126f 100644 --- a/controllers/helmreleaseproxy/helmreleaseproxy_controller.go +++ b/controllers/helmreleaseproxy/helmreleaseproxy_controller.go @@ -399,7 +399,9 @@ func (r *HelmReleaseProxyReconciler) getCredentials(ctx context.Context, helmRel // getCAFile fetches the CA certificate from a Secret and writes it to a temporary file, returning the path to the temporary file. func (r *HelmReleaseProxyReconciler) getCAFile(ctx context.Context, helmReleaseProxy *addonsv1alpha1.HelmReleaseProxy) (string, error) { caFilePath := "" - if helmReleaseProxy.Spec.TLSConfig != nil && helmReleaseProxy.Spec.TLSConfig.CASecretRef.Name != "" { + if helmReleaseProxy.Spec.TLSConfig != nil && + helmReleaseProxy.Spec.TLSConfig.CASecretRef != nil && + helmReleaseProxy.Spec.TLSConfig.CASecretRef.Name != "" { // By default, the secret is in the same namespace as the HelmReleaseProxy if helmReleaseProxy.Spec.TLSConfig.CASecretRef.Namespace == "" { helmReleaseProxy.Spec.TLSConfig.CASecretRef.Namespace = helmReleaseProxy.Namespace diff --git a/controllers/helmreleaseproxy/helmreleaseproxy_controller_test.go b/controllers/helmreleaseproxy/helmreleaseproxy_controller_test.go index e47b9169..8b0d9216 100644 --- a/controllers/helmreleaseproxy/helmreleaseproxy_controller_test.go +++ b/controllers/helmreleaseproxy/helmreleaseproxy_controller_test.go @@ -131,6 +131,34 @@ var ( }, } + defaultProxyWithSkipTLS = &addonsv1alpha1.HelmReleaseProxy{ + TypeMeta: metav1.TypeMeta{ + Kind: "HelmReleaseProxy", + APIVersion: "addons.cluster.x-k8s.io/v1alpha1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-proxy", + Namespace: "default", + }, + Spec: addonsv1alpha1.HelmReleaseProxySpec{ + ClusterRef: corev1.ObjectReference{ + APIVersion: "cluster.x-k8s.io/v1beta1", + Kind: "Cluster", + Namespace: "default", + Name: "test-cluster", + }, + RepoURL: "https://test-repo", + ChartName: "test-chart", + Version: "test-version", + ReleaseName: "test-release", + ReleaseNamespace: "default", + Values: "test-values", + TLSConfig: &addonsv1alpha1.TLSConfig{ + InsecureSkipTLSVerify: true, + }, + }, + } + generateNameProxy = &addonsv1alpha1.HelmReleaseProxy{ TypeMeta: metav1.TypeMeta{ Kind: "HelmReleaseProxy", @@ -543,6 +571,72 @@ func TestReconcileDelete(t *testing.T) { } } +func TestTLSSettings(t *testing.T) { + t.Parallel() + + testcases := []struct { + name string + helmReleaseProxy *addonsv1alpha1.HelmReleaseProxy + clientExpect func(g *WithT, c *mocks.MockClientMockRecorder) + expect func(g *WithT, hrp *addonsv1alpha1.HelmReleaseProxy) + expectedError string + }{ + { + name: "test", + helmReleaseProxy: defaultProxyWithSkipTLS.DeepCopy(), + clientExpect: func(g *WithT, c *mocks.MockClientMockRecorder) { + c.InstallOrUpgradeHelmRelease(ctx, restConfig, "", "", defaultProxyWithSkipTLS.Spec).Return(&helmRelease.Release{ + Name: "test-release", + Version: 1, + Info: &helmRelease.Info{ + Status: helmRelease.StatusDeployed, + }, + }, nil).Times(1) + }, + expect: func(g *WithT, hrp *addonsv1alpha1.HelmReleaseProxy) { + _, ok := hrp.Annotations[addonsv1alpha1.IsReleaseNameGeneratedAnnotation] + g.Expect(ok).To(BeFalse()) + g.Expect(hrp.Spec.ReleaseName).To(Equal("test-release")) + g.Expect(hrp.Status.Revision).To(Equal(1)) + g.Expect(hrp.Status.Status).To(BeEquivalentTo(helmRelease.StatusDeployed)) + + g.Expect(conditions.Has(hrp, addonsv1alpha1.HelmReleaseReadyCondition)).To(BeTrue()) + g.Expect(conditions.IsTrue(hrp, addonsv1alpha1.HelmReleaseReadyCondition)).To(BeTrue()) + }, + expectedError: "", + }, + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + t.Parallel() + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + clientMock := mocks.NewMockClient(mockCtrl) + tc.clientExpect(g, clientMock.EXPECT()) + + r := &HelmReleaseProxyReconciler{ + Client: fake.NewClientBuilder(). + WithScheme(fakeScheme). + WithStatusSubresource(&addonsv1alpha1.HelmReleaseProxy{}). + Build(), + } + caFilePath, err := r.getCAFile(ctx, tc.helmReleaseProxy) + g.Expect(err).ToNot(HaveOccurred(), "did not expect error to get CA file") + err = r.reconcileNormal(ctx, tc.helmReleaseProxy, clientMock, "", caFilePath, restConfig) + if tc.expectedError != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(MatchError(tc.expectedError), err.Error()) + } else { + g.Expect(err).NotTo(HaveOccurred()) + tc.expect(g, tc.helmReleaseProxy) + } + }) + } +} + func init() { _ = scheme.AddToScheme(fakeScheme) _ = clusterv1.AddToScheme(fakeScheme)