Merge pull request #253 from faiq/fix-null-ptr-checks

🐛 pointer checks if user doesn't specify CASecret
kubernetes-sigs · Jun 3, 2024 · 681ddfa · 681ddfa
2 parents a95e8c5 + f801efc
commit 681ddfa
Show file tree

Hide file tree

Showing 4 changed files with 177 additions and 2 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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)