Changes after review 2

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

internal/bundlereader/auth.go

@@ -2,6 +2,7 @@ package bundlereader
 
 import (
 	"context"
+	"fmt"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -27,16 +28,23 @@ func ReadHelmAuthFromSecret(ctx context.Context, c client.Client, req types.Name
 	}
 
 	auth := Auth{}
-	username, ok := secret.Data[corev1.BasicAuthUsernameKey]
-	if ok {
+	username, okUsername := secret.Data[corev1.BasicAuthUsernameKey]
+	if okUsername {
 		auth.Username = string(username)
 	}
 
-	password, ok := secret.Data[corev1.BasicAuthPasswordKey]
-	if ok {
+	password, okPasswd := secret.Data[corev1.BasicAuthPasswordKey]
+	if okPasswd {
 		auth.Password = string(password)
 	}
 
+	// check that username and password are both set or none is set
+	if okUsername && !okPasswd {
+		return Auth{}, fmt.Errorf("%s is set in the secret, but %s isn't", corev1.BasicAuthUsernameKey, corev1.BasicAuthPasswordKey)
+	} else if !okUsername && okPasswd {
+		return Auth{}, fmt.Errorf("%s is set in the secret, but %s isn't", corev1.BasicAuthPasswordKey, corev1.BasicAuthUsernameKey)
+	}
+
 	caBundle, ok := secret.Data["cacerts"]
 	if ok {
 		auth.CABundle = caBundle

internal/bundlereader/auth_test.go

@@ -0,0 +1,148 @@
+package bundlereader_test
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"go.uber.org/mock/gomock"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
+
+	"github.com/rancher/fleet/internal/bundlereader"
+	"github.com/rancher/fleet/internal/mocks"
+)
+
+// nolint: funlen
+func TestReadHelmAuthFromSecret(t *testing.T) {
+	cases := []struct {
+		name              string
+		secretData        map[string][]byte
+		getError          string
+		expectedAuth      bundlereader.Auth
+		expectedErrNotNil bool
+		expectedError     string
+	}{
+		{
+			name:         "nothing is set",
+			secretData:   map[string][]byte{},
+			getError:     "",
+			expectedAuth: bundlereader.Auth{
+				// default values
+			},
+			expectedErrNotNil: false,
+			expectedError:     "",
+		},
+		{
+			name: "username, password and caBundle are set",
+			secretData: map[string][]byte{
+				corev1.BasicAuthUsernameKey: []byte("user"),
+				corev1.BasicAuthPasswordKey: []byte("passwd"),
+				"cacerts":                   []byte("test_cabundle"),
+			},
+			getError: "",
+			expectedAuth: bundlereader.Auth{
+				Username: "user",
+				Password: "passwd",
+				CABundle: []byte("test_cabundle"),
+			},
+			expectedErrNotNil: false,
+			expectedError:     "",
+		},
+		{
+			name: "username, password are set, caBundle is not",
+			secretData: map[string][]byte{
+				corev1.BasicAuthUsernameKey: []byte("user"),
+				corev1.BasicAuthPasswordKey: []byte("passwd"),
+			},
+			getError: "",
+			expectedAuth: bundlereader.Auth{
+				Username: "user",
+				Password: "passwd",
+			},
+			expectedErrNotNil: false,
+			expectedError:     "",
+		},
+		{
+			name: "caBundle is set, username and password are not",
+			secretData: map[string][]byte{
+				"cacerts": []byte("test_cabundle"),
+			},
+			getError: "",
+			expectedAuth: bundlereader.Auth{
+				CABundle: []byte("test_cabundle"),
+			},
+			expectedErrNotNil: false,
+			expectedError:     "",
+		},
+		{
+			name: "username, caBundle are set, password is not",
+			secretData: map[string][]byte{
+				corev1.BasicAuthUsernameKey: []byte("user"),
+				"cacerts":                   []byte("test_cabundle"),
+			},
+			getError:          "",
+			expectedAuth:      bundlereader.Auth{},
+			expectedErrNotNil: true,
+			expectedError:     "username is set in the secret, but password isn't",
+		},
+		{
+			name: "username is set, password and caBundle are not",
+			secretData: map[string][]byte{
+				corev1.BasicAuthUsernameKey: []byte("user"),
+			},
+			getError:          "",
+			expectedAuth:      bundlereader.Auth{},
+			expectedErrNotNil: true,
+			expectedError:     "username is set in the secret, but password isn't",
+		},
+		{
+			name: "password, caBundle are set, username is not",
+			secretData: map[string][]byte{
+				corev1.BasicAuthPasswordKey: []byte("passwd"),
+				"cacerts":                   []byte("test_cabundle"),
+			},
+			getError:          "",
+			expectedAuth:      bundlereader.Auth{},
+			expectedErrNotNil: true,
+			expectedError:     "password is set in the secret, but username isn't",
+		},
+		{
+			name: "password is set, username and caBundle are not",
+			secretData: map[string][]byte{
+				corev1.BasicAuthPasswordKey: []byte("passwd"),
+			},
+			getError:          "",
+			expectedAuth:      bundlereader.Auth{},
+			expectedErrNotNil: true,
+			expectedError:     "password is set in the secret, but username isn't",
+		},
+	}
+
+	mockCtrl := gomock.NewController(t)
+	defer mockCtrl.Finish()
+	mockClient := mocks.NewMockClient(mockCtrl)
+
+	assert := assert.New(t)
+	for _, c := range cases {
+		if c.getError != "" {
+			mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Do(fmt.Errorf(c.getError)) // nolint:govet
+		} else {
+			mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).DoAndReturn(
+				func(_ context.Context, _ types.NamespacedName, secret *corev1.Secret, _ ...interface{}) error {
+					secret.Data = c.secretData
+					return nil
+				},
+			)
+		}
+
+		nsName := types.NamespacedName{Name: "test", Namespace: "test"}
+		auth, err := bundlereader.ReadHelmAuthFromSecret(context.TODO(), mockClient, nsName)
+		assert.Equal(c.expectedErrNotNil, err != nil)
+		if err != nil && c.expectedErrNotNil {
+			assert.Equal(c.expectedError, err.Error())
+		}
+		assert.Equal(auth, c.expectedAuth)
+	}
+}

internal/bundlereader/charturl.go

@@ -41,9 +41,9 @@ func ChartVersion(location fleet.HelmOptions, auth Auth) (string, error) {
 	return chart.Version, nil
 }
 
-// ChartURL returns the URL to the helm chart from a helm repo server, by
+// chartURL returns the URL to the helm chart from a helm repo server, by
 // inspecting the repo's index.yaml
-func ChartURL(location fleet.HelmOptions, auth Auth) (string, error) {
+func chartURL(location fleet.HelmOptions, auth Auth) (string, error) {
 	if hasOCIURL.MatchString(location.Chart) {
 		return location.Chart, nil
 	}

internal/bundlereader/helm.go

@@ -31,7 +31,7 @@ func GetManifestFromHelmChart(ctx context.Context, c client.Client, bd *fleet.Bu
 	}
 	auth.InsecureSkipVerify = bd.Spec.HelmChartOptions.InsecureSkipTLSverify
 
-	chartURL, err := ChartURL(*helm, auth)
+	chartURL, err := chartURL(*helm, auth)
 	if err != nil {
 		return nil, err
 	}

internal/bundlereader/resources.go

@@ -160,7 +160,7 @@ func addRemoteCharts(directories []directory, base string, charts []*fleet.HelmO
 				auth = Auth{}
 			}
 
-			chartURL, err := ChartURL(*chart, auth)
+			chartURL, err := chartURL(*chart, auth)
 			if err != nil {
 				return nil, err
 			}

internal/cmd/controller/helmops/operator.go

@@ -145,7 +145,7 @@ func (g *HelmOperator) Run(cmd *cobra.Command, args []string) error {
 			return err
 		}
 
-		setupLog.Info("starting gitops controller")
+		setupLog.Info("starting helmops controller")
 		if err = helmAppReconciler.SetupWithManager(mgr); err != nil {
 			return err
 		}