CLOUDP-294105: remove kube lookups in Atlas Provider (#2158)

* remove kube lookups in atlas provider

* refactor existing controllers

* address review comments

* fix nil value

* remove v20231115008.SdkClient from workflow context, use client set only

* remove sdk client in favor of client set

* wire global secret in network peering controller

Author: s-urbaniak

internal/controller/atlas/provider.go

@@ -13,8 +13,6 @@ import (
 	adminv20241113001 "go.mongodb.org/atlas-sdk/v20241113001/admin"
 	"go.mongodb.org/atlas/mongodbatlas"
 	"go.uber.org/zap"
-	corev1 "k8s.io/api/core/v1"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
@@ -25,14 +23,11 @@ import (
 
 const (
 	govAtlasDomain = "mongodbgov.com"
-	orgIDKey       = "orgId"
-	publicAPIKey   = "publicApiKey"
-	privateAPIKey  = "privateApiKey"
 )
 
 type Provider interface {
-	Client(ctx context.Context, secretRef *client.ObjectKey, log *zap.SugaredLogger) (*mongodbatlas.Client, string, error)
-	SdkClientSet(ctx context.Context, secretRef *client.ObjectKey, log *zap.SugaredLogger) (*ClientSet, string, error)
+	Client(ctx context.Context, creds *Credentials, log *zap.SugaredLogger) (*mongodbatlas.Client, error)
+	SdkClientSet(ctx context.Context, creds *Credentials, log *zap.SugaredLogger) (*ClientSet, error)
 	IsCloudGov() bool
 	IsResourceSupported(resource api.AtlasCustomResource) bool
 }
@@ -43,24 +38,33 @@ type ClientSet struct {
 }
 
 type ProductionProvider struct {
-	k8sClient       client.Client
-	domain          string
-	globalSecretRef client.ObjectKey
-	dryRun          bool
+	domain string
+	dryRun bool
 }
 
-type credentialsSecret struct {
-	OrgID      string
+// ConnectionConfig is the type that contains connection configuration to Atlas, including credentials.
+type ConnectionConfig struct {
+	OrgID       string
+	Credentials *Credentials
+}
+
+// Credentials is the type that holds credentials to authenticate against the Atlas API.
+// Currently, only API keys are support but more credential types could be added,
+// see https://www.mongodb.com/docs/atlas/configure-api-access/.
+type Credentials struct {
+	APIKeys *APIKeys
+}
+
+// APIKeys is the type that holds Public/Private API keys to authenticate against the Atlas API.
+type APIKeys struct {
 	PublicKey  string
 	PrivateKey string
 }
 
-func NewProductionProvider(atlasDomain string, globalSecretRef client.ObjectKey, k8sClient client.Client, dryRun bool) *ProductionProvider {
+func NewProductionProvider(atlasDomain string, dryRun bool) *ProductionProvider {
 	return &ProductionProvider{
-		k8sClient:       k8sClient,
-		domain:          atlasDomain,
-		globalSecretRef: globalSecretRef,
-		dryRun:          dryRun,
+		domain: atlasDomain,
+		dryRun: dryRun,
 	}
 }
 
@@ -102,35 +106,25 @@ func (p *ProductionProvider) IsResourceSupported(resource api.AtlasCustomResourc
 	return false
 }
 
-func (p *ProductionProvider) Client(ctx context.Context, secretRef *client.ObjectKey, log *zap.SugaredLogger) (*mongodbatlas.Client, string, error) {
-	secretData, err := getSecrets(ctx, p.k8sClient, secretRef, &p.globalSecretRef)
-	if err != nil {
-		return nil, "", err
-	}
-
+func (p *ProductionProvider) Client(ctx context.Context, creds *Credentials, log *zap.SugaredLogger) (*mongodbatlas.Client, error) {
 	clientCfg := []httputil.ClientOpt{
-		httputil.Digest(secretData.PublicKey, secretData.PrivateKey),
+		httputil.Digest(creds.APIKeys.PublicKey, creds.APIKeys.PrivateKey),
 		httputil.LoggingTransport(log),
 	}
 
 	transport := p.newDryRunTransport(http.DefaultTransport)
 	httpClient, err := httputil.DecorateClient(&http.Client{Transport: transport}, clientCfg...)
 	if err != nil {
-		return nil, "", err
+		return nil, err
 	}
 
 	c, err := mongodbatlas.New(httpClient, mongodbatlas.SetBaseURL(p.domain), mongodbatlas.SetUserAgent(operatorUserAgent()))
 
-	return c, secretData.OrgID, err
+	return c, err
 }
 
-func (p *ProductionProvider) SdkClientSet(ctx context.Context, secretRef *client.ObjectKey, log *zap.SugaredLogger) (*ClientSet, string, error) {
-	secretData, err := getSecrets(ctx, p.k8sClient, secretRef, &p.globalSecretRef)
-	if err != nil {
-		return nil, "", err
-	}
-
-	var transport http.RoundTripper = digest.NewTransport(secretData.PublicKey, secretData.PrivateKey)
+func (p *ProductionProvider) SdkClientSet(ctx context.Context, creds *Credentials, log *zap.SugaredLogger) (*ClientSet, error) {
+	var transport http.RoundTripper = digest.NewTransport(creds.APIKeys.PublicKey, creds.APIKeys.PrivateKey)
 	transport = p.newDryRunTransport(transport)
 	transport = httputil.NewLoggingTransport(log, false, transport)
 
@@ -141,21 +135,21 @@ func (p *ProductionProvider) SdkClientSet(ctx context.Context, secretRef *client
 		adminv20231115008.UseHTTPClient(httpClient),
 		adminv20231115008.UseUserAgent(operatorUserAgent()))
 	if err != nil {
-		return nil, "", err
+		return nil, err
 	}
 
 	clientv20241113001, err := adminv20241113001.NewClient(
 		adminv20241113001.UseBaseURL(p.domain),
 		adminv20241113001.UseHTTPClient(httpClient),
 		adminv20241113001.UseUserAgent(operatorUserAgent()))
 	if err != nil {
-		return nil, "", err
+		return nil, err
 	}
 
 	return &ClientSet{
 		SdkClient20231115008: clientv20231115008,
 		SdkClient20241113001: clientv20241113001,
-	}, secretData.OrgID, nil
+	}, nil
 }
 
 func (p *ProductionProvider) newDryRunTransport(delegate http.RoundTripper) http.RoundTripper {
@@ -166,51 +160,6 @@ func (p *ProductionProvider) newDryRunTransport(delegate http.RoundTripper) http
 	return delegate
 }
 
-func getSecrets(ctx context.Context, k8sClient client.Client, secretRef, fallbackRef *client.ObjectKey) (*credentialsSecret, error) {
-	if secretRef == nil {
-		secretRef = fallbackRef
-	}
-
-	secret := &corev1.Secret{}
-	if err := k8sClient.Get(ctx, *secretRef, secret); err != nil {
-		return nil, fmt.Errorf("failed to read Atlas API credentials from the secret %s: %w", secretRef.String(), err)
-	}
-
-	secretData := credentialsSecret{
-		OrgID:      string(secret.Data[orgIDKey]),
-		PublicKey:  string(secret.Data[publicAPIKey]),
-		PrivateKey: string(secret.Data[privateAPIKey]),
-	}
-
-	if missingFields, valid := validateSecretData(&secretData); !valid {
-		return nil, fmt.Errorf("the following fields are missing in the secret %v: %v", secretRef, missingFields)
-	}
-
-	return &secretData, nil
-}
-
-func validateSecretData(secretData *credentialsSecret) ([]string, bool) {
-	missingFields := make([]string, 0, 3)
-
-	if secretData.OrgID == "" {
-		missingFields = append(missingFields, orgIDKey)
-	}
-
-	if secretData.PublicKey == "" {
-		missingFields = append(missingFields, publicAPIKey)
-	}
-
-	if secretData.PrivateKey == "" {
-		missingFields = append(missingFields, privateAPIKey)
-	}
-
-	if len(missingFields) > 0 {
-		return missingFields, false
-	}
-
-	return nil, true
-}
-
 func operatorUserAgent() string {
 	return fmt.Sprintf("%s/%s (%s;%s)", "MongoDBAtlasKubernetesOperator", version.Version, runtime.GOOS, runtime.GOARCH)
 }

internal/controller/atlas/provider_test.go

@@ -1,79 +1,29 @@
 package atlas
 
 import (
-	"context"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"go.uber.org/zap/zaptest"
-	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/api"
 	akov2 "github.com/mongodb/mongodb-atlas-kubernetes/v2/api/v1"
 	"github.com/mongodb/mongodb-atlas-kubernetes/v2/internal/version"
 )
 
-func TestProvider_Client(t *testing.T) {
-	secret := &corev1.Secret{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "api-secret",
-			Namespace: "default",
-			Labels: map[string]string{
-				"atlas.mongodb.com/type": "credentials",
-			},
-		},
-		Data: map[string][]byte{
-			"orgId":         []byte("1234567890"),
-			"publicApiKey":  []byte("a1b2c3"),
-			"privateApiKey": []byte("abcdef123456"),
-		},
-		Type: "Opaque",
-	}
-
-	sch := runtime.NewScheme()
-	sch.AddKnownTypes(corev1.SchemeGroupVersion, &corev1.Secret{})
-	k8sClient := fake.NewClientBuilder().
-		WithScheme(sch).
-		WithObjects(secret).
-		Build()
-
-	t.Run("should return Atlas API client and organization id using global secret", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{Name: "api-secret", Namespace: "default"}, k8sClient, false)
-
-		c, id, err := p.Client(context.Background(), nil, zaptest.NewLogger(t).Sugar())
-		assert.NoError(t, err)
-		assert.Equal(t, "1234567890", id)
-		assert.NotNil(t, c)
-	})
-
-	t.Run("should return Atlas API client and organization id using connection secret", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{Name: "global-secret", Namespace: "default"}, k8sClient, false)
-
-		c, id, err := p.Client(context.Background(), &client.ObjectKey{Name: "api-secret", Namespace: "default"}, zaptest.NewLogger(t).Sugar())
-		assert.NoError(t, err)
-		assert.Equal(t, "1234567890", id)
-		assert.NotNil(t, c)
-	})
-}
-
 func TestProvider_IsCloudGov(t *testing.T) {
 	t.Run("should return false for invalid domain", func(t *testing.T) {
-		p := NewProductionProvider("http://x:namedport", client.ObjectKey{}, nil, false)
+		p := NewProductionProvider("http://x:namedport", false)
 		assert.False(t, p.IsCloudGov())
 	})
 
 	t.Run("should return false for commercial Atlas domain", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodb.com/", client.ObjectKey{}, nil, false)
+		p := NewProductionProvider("https://cloud.mongodb.com/", false)
 		assert.False(t, p.IsCloudGov())
 	})
 
 	t.Run("should return true for Atlas for government domain", func(t *testing.T) {
-		p := NewProductionProvider("https://cloud.mongodbgov.com/", client.ObjectKey{}, nil, false)
+		p := NewProductionProvider("https://cloud.mongodbgov.com/", false)
 		assert.True(t, p.IsCloudGov())
 	})
 }
@@ -173,44 +123,12 @@ func TestProvider_IsResourceSupported(t *testing.T) {
 
 	for desc, data := range dataProvider {
 		t.Run(desc, func(t *testing.T) {
-			p := NewProductionProvider(data.domain, client.ObjectKey{}, nil, false)
+			p := NewProductionProvider(data.domain, false)
 			assert.Equal(t, data.expectation, p.IsResourceSupported(data.resource))
 		})
 	}
 }
 
-func TestValidateSecretData(t *testing.T) {
-	t.Run("should be invalid and all missing data", func(t *testing.T) {
-		missing, ok := validateSecretData(&credentialsSecret{})
-		assert.False(t, ok)
-		assert.Equal(t, missing, []string{"orgId", "publicApiKey", "privateApiKey"})
-	})
-
-	t.Run("should be invalid and organization id is missing", func(t *testing.T) {
-		missing, ok := validateSecretData(&credentialsSecret{PublicKey: "abcdef", PrivateKey: "123456"})
-		assert.False(t, ok)
-		assert.Equal(t, missing, []string{"orgId"})
-	})
-
-	t.Run("should be invalid and public key id is missing", func(t *testing.T) {
-		missing, ok := validateSecretData(&credentialsSecret{OrgID: "abcdef", PrivateKey: "123456"})
-		assert.False(t, ok)
-		assert.Equal(t, missing, []string{"publicApiKey"})
-	})
-
-	t.Run("should be invalid and private key id is missing", func(t *testing.T) {
-		missing, ok := validateSecretData(&credentialsSecret{PublicKey: "abcdef", OrgID: "123456"})
-		assert.False(t, ok)
-		assert.Equal(t, missing, []string{"privateApiKey"})
-	})
-
-	t.Run("should be valid", func(t *testing.T) {
-		missing, ok := validateSecretData(&credentialsSecret{OrgID: "my-org", PublicKey: "abcdef", PrivateKey: "123456"})
-		assert.True(t, ok)
-		assert.Empty(t, missing)
-	})
-}
-
 func TestOperatorUserAgent(t *testing.T) {
 	userAgent := operatorUserAgent()
 

internal/controller/atlascustomrole/atlascustomrole_controller.go

@@ -50,11 +50,13 @@ func NewAtlasCustomRoleReconciler(
 	deletionProtection bool,
 	independentSyncPeriod time.Duration,
 	logger *zap.Logger,
+	globalSecretRef client.ObjectKey,
 ) *AtlasCustomRoleReconciler {
 	return &AtlasCustomRoleReconciler{
 		AtlasReconciler: reconciler.AtlasReconciler{
-			Client: c.GetClient(),
-			Log:    logger.Named("controllers").Named("AtlasCustomRoles").Sugar(),
+			Client:          c.GetClient(),
+			Log:             logger.Named("controllers").Named("AtlasCustomRoles").Sugar(),
+			GlobalSecretRef: globalSecretRef,
 		},
 		Scheme:                   c.GetScheme(),
 		EventRecorder:            c.GetEventRecorderFor("AtlasCustomRoles"),
@@ -125,11 +127,11 @@ func (r *AtlasCustomRoleReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 			fmt.Errorf("the %T is not supported by Atlas for government", atlasCustomRole)), nil
 	}
 
-	credentials, err := r.ResolveCredentials(ctx, atlasCustomRole)
+	connectionConfig, err := r.ResolveConnectionConfig(ctx, atlasCustomRole)
 	if err != nil {
 		return r.fail(req, err), nil
 	}
-	atlasSdkClientSet, _, err := r.AtlasProvider.SdkClientSet(workflowCtx.Context, credentials, workflowCtx.Log)
+	atlasSdkClientSet, err := r.AtlasProvider.SdkClientSet(workflowCtx.Context, connectionConfig.Credentials, workflowCtx.Log)
 	if err != nil {
 		return r.terminate(workflowCtx, atlasCustomRole, api.ProjectCustomRolesReadyType, workflow.AtlasAPIAccessNotConfigured, true, err), nil
 	}

internal/controller/atlascustomrole/atlascustomrole_controller_test.go

@@ -12,6 +12,7 @@ import (
 	"go.mongodb.org/atlas-sdk/v20231115008/admin"
 	"go.mongodb.org/atlas-sdk/v20231115008/mockadmin"
 	"go.uber.org/zap"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -333,9 +334,20 @@ func TestAtlasCustomRoleReconciler_Reconcile(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			testScheme := runtime.NewScheme()
 			assert.NoError(t, akov2.AddToScheme(testScheme))
+			assert.NoError(t, corev1.AddToScheme(testScheme))
 			k8sClient := fake.NewClientBuilder().
 				WithScheme(testScheme).
-				WithObjects(tt.akoCustomRole).
+				WithObjects(tt.akoCustomRole, &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "test",
+						Namespace: "default",
+					},
+					Data: map[string][]byte{
+						"orgId":         []byte("orgId"),
+						"publicApiKey":  []byte("publicApiKey"),
+						"privateApiKey": []byte("privateApiKey"),
+					},
+				}).
 				WithInterceptorFuncs(tt.interceptors).
 				Build()
 			r := &AtlasCustomRoleReconciler{
@@ -346,9 +358,9 @@ func TestAtlasCustomRoleReconciler_Reconcile(t *testing.T) {
 				Scheme:        testScheme,
 				EventRecorder: record.NewFakeRecorder(10),
 				AtlasProvider: &atlasmocks.TestProvider{
-					SdkSetClientFunc: func(secretRef *client.ObjectKey, log *zap.SugaredLogger) (*atlas.ClientSet, string, error) {
+					SdkClientSetFunc: func(ctx context.Context, creds *atlas.Credentials, log *zap.SugaredLogger) (*atlas.ClientSet, error) {
 						if tt.sdkShouldError {
-							return nil, "", fmt.Errorf("failed to create sdk")
+							return nil, fmt.Errorf("failed to create sdk")
 						}
 						cdrAPI := mockadmin.NewCustomDatabaseRolesApi(t)
 						cdrAPI.EXPECT().GetCustomDatabaseRole(mock.Anything, "testProjectID", "TestRoleName").
@@ -375,7 +387,7 @@ func TestAtlasCustomRoleReconciler_Reconcile(t *testing.T) {
 						return &atlas.ClientSet{SdkClient20231115008: &admin.APIClient{
 							CustomDatabaseRolesApi: cdrAPI,
 							ProjectsApi:            pAPI,
-						}}, "", nil
+						}}, nil
 					},
 					IsCloudGovFunc: func() bool {
 						return false