helmrepo: fix Secret type check for TLS via .spec.secretRef

This is a regression fix introduced in a302c71 which would wrongly check
for the type of the Secret specified in `.spec.secretRef` while
configuring TLS data.

This moves the Secret type check after checking whether the Secret has
any TLS data, to avoid unnecessary type enforcement.

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

internal/controller/helmrepository_controller_test.go

@@ -550,6 +550,38 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) {
 				t.Expect(artifact.Revision).ToNot(BeEmpty())
 			},
 		},
+		{
+			// Regression test for: https://github.com/fluxcd/source-controller/issues/1218
+			name:     "HTTP with docker config secretRef sets Reconciling=True",
+			protocol: "http",
+			server: options{
+				username: "git",
+				password: "1234",
+			},
+			secret: &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "basic-auth",
+				},
+				Data: map[string][]byte{
+					"username": []byte("git"),
+					"password": []byte("1234"),
+				},
+				Type: corev1.SecretTypeDockerConfigJson,
+			},
+			beforeFunc: func(t *WithT, obj *helmv1.HelmRepository, rev digest.Digest) {
+				obj.Spec.SecretRef = &meta.LocalObjectReference{Name: "basic-auth"}
+			},
+			want: sreconcile.ResultSuccess,
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReconcilingCondition, meta.ProgressingReason, "building artifact: new index revision"),
+				*conditions.UnknownCondition(meta.ReadyCondition, meta.ProgressingReason, "building artifact: new index revision"),
+			},
+			afterFunc: func(t *WithT, obj *helmv1.HelmRepository, artifact sourcev1.Artifact, chartRepo *repository.ChartRepository) {
+				t.Expect(chartRepo.Path).ToNot(BeEmpty())
+				t.Expect(chartRepo.Index).ToNot(BeNil())
+				t.Expect(artifact.Revision).ToNot(BeEmpty())
+			},
+		},
 		{
 			name:     "HTTPS with invalid CAFile in certSecretRef makes FetchFailed=True and returns error",
 			protocol: "https",

internal/tls/config.go

@@ -45,8 +45,18 @@ type TLSBytes struct {
 // - ca.crt, for the CA certificate
 //
 // Secrets with no certificate, private key, AND CA cert are ignored. If only a
-// certificate OR private key is found, an error is returned.
+// certificate OR private key is found, an error is returned. The Secret type
+// can be blank, otherwise Opaque or kubernetes.io/tls.
 func KubeTLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *TLSBytes, error) {
+	// Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank
+	// type, to avoid having to specify the type of the Secret for every test case.
+	// Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this.
+	switch secret.Type {
+	case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "":
+	default:
+		return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type)
+	}
+
 	return tlsClientConfigFromSecret(secret, url, true)
 }
 
@@ -76,15 +86,6 @@ func TLSClientConfigFromSecret(secret corev1.Secret, url string) (*tls.Config, *
 // The keys should adhere to a single convention, i.e. a Secret with tls.key
 // and certFile is invalid.
 func tlsClientConfigFromSecret(secret corev1.Secret, url string, kubernetesTLSKeys bool) (*tls.Config, *TLSBytes, error) {
-	// Only Secrets of type Opaque and TLS are allowed. We also allow Secrets with a blank
-	// type, to avoid having to specify the type of the Secret for every test case.
-	// Since a real Kubernetes Secret is of type Opaque by default, its safe to allow this.
-	switch secret.Type {
-	case corev1.SecretTypeOpaque, corev1.SecretTypeTLS, "":
-	default:
-		return nil, nil, fmt.Errorf("cannot use secret '%s' to construct TLS config: invalid secret type: '%s'", secret.Name, secret.Type)
-	}
-
 	var certBytes, keyBytes, caBytes []byte
 	if kubernetesTLSKeys {
 		certBytes, keyBytes, caBytes = secret.Data[corev1.TLSCertKey], secret.Data[corev1.TLSPrivateKeyKey], secret.Data[CACrtKey]

internal/tls/config_test.go

@@ -85,12 +85,6 @@ func Test_tlsClientConfigFromSecret(t *testing.T) {
 			tlsKeys: true,
 			wantNil: true,
 		},
-		{
-			name:    "invalid secret type",
-			secret:  corev1.Secret{Type: corev1.SecretTypeDockerConfigJson},
-			wantErr: true,
-			wantNil: true,
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -111,6 +105,20 @@ func Test_tlsClientConfigFromSecret(t *testing.T) {
 	}
 }
 
+func TestKubeTLSConfigFromSecret(t *testing.T) {
+	kubernetesTlsSecretFixture := validTlsSecret(t, true)
+	g := NewWithT(t)
+
+	config, _, err := KubeTLSClientConfigFromSecret(kubernetesTlsSecretFixture, "")
+	g.Expect(config).ToNot(BeNil())
+	g.Expect(err).ToNot(HaveOccurred())
+
+	kubernetesTlsSecretFixture.Type = corev1.SecretTypeDockerConfigJson
+	_, _, err = KubeTLSClientConfigFromSecret(kubernetesTlsSecretFixture, "")
+	g.Expect(err).To(HaveOccurred())
+	g.Expect(err.Error()).To(ContainSubstring("invalid secret type"))
+}
+
 // validTlsSecret creates a secret containing key pair and CA certificate that are
 // valid from a syntax (minimum requirements) perspective.
 func validTlsSecret(t *testing.T, kubernetesTlsKeys bool) corev1.Secret {