Introduce a nutanix prism client cache (#415)

* Introduce a nutanix prism client cache

The cache stores a prismgoclient.V3 client instance for each NutanixCluster instance.
The cache is shared between nutanixcluster and nutanixmachine controllers.

* Address review comments

Author: thunderboltsid

Makefile

@@ -306,8 +306,10 @@ mocks: ## Generate mocks for the project
 	mockgen -destination=mocks/ctlclient/cache_mock.go -package=mockctlclient sigs.k8s.io/controller-runtime/pkg/cache Cache
 	mockgen -destination=mocks/k8sclient/cm_informer.go -package=mockk8sclient k8s.io/client-go/informers/core/v1 ConfigMapInformer
 	mockgen -destination=mocks/k8sclient/secret_informer.go -package=mockk8sclient k8s.io/client-go/informers/core/v1 SecretInformer
+	mockgen -destination=mocks/k8sclient/secret_lister.go -package=mockk8sclient k8s.io/client-go/listers/core/v1 SecretLister
+	mockgen -destination=mocks/k8sclient/secret_namespace_lister.go -package=mockk8sclient k8s.io/client-go/listers/core/v1 SecretNamespaceLister
 
-GOTESTPKGS = $(shell go list ./... | grep -v /mocks | grep -v /templates)
+GOTESTPKGS = $(shell go list ./... | grep -v /mocks | grep -v /templates | grep -v /v1alpha4)
 
 .PHONY: unit-test
 unit-test: mocks  ## Run unit tests.

api/v1beta1/nutanixcluster_types.go

@@ -17,9 +17,11 @@ limitations under the License.
 package v1beta1
 
 import (
+	"cmp"
 	"fmt"
 
 	credentialTypes "github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	capiv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/errors"
@@ -162,6 +164,12 @@ func (ncl *NutanixCluster) GetPrismCentralCredentialRef() (*credentialTypes.Nuta
 	return prismCentralInfo.CredentialRef, nil
 }
 
+// GetNamespacedName returns the namespaced name of the NutanixCluster.
+func (ncl *NutanixCluster) GetNamespacedName() string {
+	namespace := cmp.Or(ncl.Namespace, corev1.NamespaceDefault)
+	return fmt.Sprintf("%s/%s", namespace, ncl.Name)
+}
+
 // +kubebuilder:object:root=true
 
 // NutanixClusterList contains a list of NutanixCluster

api/v1beta1/nutanixcluster_types_test.go

@@ -112,3 +112,39 @@ func TestGetCredentialRefForCluster(t *testing.T) {
 		})
 	}
 }
+
+func TestGetNamespacedName(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name             string
+		nutanixCluster   *NutanixCluster
+		expectedFullName string
+	}{
+		{
+			name: "namespace and name are set",
+			nutanixCluster: &NutanixCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "test-namespace",
+				},
+			},
+			expectedFullName: "test-namespace/test",
+		},
+		{
+			name: "namespace is not set, should use default",
+			nutanixCluster: &NutanixCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			expectedFullName: "default/test",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			fullName := tt.nutanixCluster.GetNamespacedName()
+			assert.Equal(t, tt.expectedFullName, fullName)
+		})
+	}
+}

controllers/helpers.go

@@ -18,16 +18,24 @@ package controllers
 
 import (
 	"context"
+	"encoding/json"
+	"errors"
 	"testing"
 
-	credentialTypes "github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
+	"github.com/golang/mock/gomock"
+	credentialtypes "github.com/nutanix-cloud-native/prism-go-client/environment/credentials"
+	prismclientv3 "github.com/nutanix-cloud-native/prism-go-client/v3"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/cluster-api/util"
 
 	infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
-
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
+	mockk8sclient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/mocks/k8sclient"
+	nutanixclient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/pkg/client"
 )
 
 func TestControllerHelpers(t *testing.T) {
@@ -52,7 +60,7 @@ func TestControllerHelpers(t *testing.T) {
 					Namespace: "default",
 				},
 				Spec: infrav1.NutanixClusterSpec{
-					PrismCentral: &credentialTypes.NutanixPrismEndpoint{
+					PrismCentral: &credentialtypes.NutanixPrismEndpoint{
 						// Adding port info to override default value (0)
 						Port: 9440,
 					},
@@ -121,3 +129,103 @@ func TestControllerHelpers(t *testing.T) {
 		})
 	})
 }
+
+func TestGetPrismCentralClientForCluster(t *testing.T) {
+	ctx := context.Background()
+	cluster := &infrav1.NutanixCluster{
+		Spec: infrav1.NutanixClusterSpec{
+			PrismCentral: &credentialtypes.NutanixPrismEndpoint{
+				Address: "prismcentral.nutanix.com",
+				Port:    9440,
+				CredentialRef: &credentialtypes.NutanixCredentialReference{
+					Kind:      credentialtypes.SecretKind,
+					Name:      "test-credential",
+					Namespace: "test-ns",
+				},
+			},
+		},
+	}
+
+	t.Run("BuildManagementEndpoint Fails", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+
+		secretNamespaceLister := mockk8sclient.NewMockSecretNamespaceLister(ctrl)
+		secretNamespaceLister.EXPECT().Get("test-credential").Return(nil, errors.New("failed to get secret"))
+		secretLister := mockk8sclient.NewMockSecretLister(ctrl)
+		secretLister.EXPECT().Secrets("test-ns").Return(secretNamespaceLister)
+		secretInformer := mockk8sclient.NewMockSecretInformer(ctrl)
+		mapInformer := mockk8sclient.NewMockConfigMapInformer(ctrl)
+		secretInformer.EXPECT().Lister().Return(secretLister)
+
+		_, err := getPrismCentralClientForCluster(ctx, cluster, secretInformer, mapInformer)
+		assert.Error(t, err)
+	})
+
+	t.Run("GetOrCreate Fails", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+
+		creds := []credentialtypes.Credential{
+			{
+				Type: credentialtypes.BasicAuthCredentialType,
+				Data: []byte(`{"prismCentral":{"username":"user","password":"password"}}`),
+			},
+		}
+		credsMarshal, err := json.Marshal(creds)
+		require.NoError(t, err)
+
+		secret := &corev1.Secret{
+			Data: map[string][]byte{
+				credentialtypes.KeyName: credsMarshal,
+			},
+		}
+
+		secretNamespaceLister := mockk8sclient.NewMockSecretNamespaceLister(ctrl)
+		secretNamespaceLister.EXPECT().Get("test-credential").Return(secret, nil)
+		secretLister := mockk8sclient.NewMockSecretLister(ctrl)
+		secretLister.EXPECT().Secrets("test-ns").Return(secretNamespaceLister)
+		secretInformer := mockk8sclient.NewMockSecretInformer(ctrl)
+		mapInformer := mockk8sclient.NewMockConfigMapInformer(ctrl)
+		secretInformer.EXPECT().Lister().Return(secretLister)
+
+		_, err = getPrismCentralClientForCluster(ctx, cluster, secretInformer, mapInformer)
+		assert.Error(t, err)
+	})
+
+	t.Run("GetOrCreate succeeds", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+
+		oldNutanixClientCache := nutanixclient.NutanixClientCache
+		defer func() {
+			nutanixclient.NutanixClientCache = oldNutanixClientCache
+		}()
+
+		// Create a new client cache with session auth disabled to avoid network calls in tests
+		nutanixclient.NutanixClientCache = prismclientv3.NewClientCache()
+
+		creds := []credentialtypes.Credential{
+			{
+				Type: credentialtypes.BasicAuthCredentialType,
+				Data: []byte(`{"prismCentral":{"username":"user","password":"password"}}`),
+			},
+		}
+
+		credsMarshal, err := json.Marshal(creds)
+		require.NoError(t, err)
+		secret := &corev1.Secret{
+			Data: map[string][]byte{
+				credentialtypes.KeyName: credsMarshal,
+			},
+		}
+
+		secretNamespaceLister := mockk8sclient.NewMockSecretNamespaceLister(ctrl)
+		secretNamespaceLister.EXPECT().Get("test-credential").Return(secret, nil)
+		secretLister := mockk8sclient.NewMockSecretLister(ctrl)
+		secretLister.EXPECT().Secrets("test-ns").Return(secretNamespaceLister)
+		secretInformer := mockk8sclient.NewMockSecretInformer(ctrl)
+		mapInformer := mockk8sclient.NewMockConfigMapInformer(ctrl)
+		secretInformer.EXPECT().Lister().Return(secretLister)
+
+		_, err = getPrismCentralClientForCluster(ctx, cluster, secretInformer, mapInformer)
+		assert.NoError(t, err)
+	})
+}

controllers/helpers_test.go

@@ -44,6 +44,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1"
+	nutanixClient "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/pkg/client"
 	nctx "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/pkg/context"
 )
 
@@ -170,20 +171,18 @@ func (r *NutanixClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		log.V(1).Info(fmt.Sprintf("Patched NutanixCluster. Status: %+v", cluster.Status))
 	}()
 
-	err = r.reconcileCredentialRef(ctx, cluster)
-	if err != nil {
+	if err := r.reconcileCredentialRef(ctx, cluster); err != nil {
 		log.Error(err, fmt.Sprintf("error occurred while reconciling credential ref for cluster %s", capiCluster.Name))
 		conditions.MarkFalse(cluster, infrav1.CredentialRefSecretOwnerSetCondition, infrav1.CredentialRefSecretOwnerSetFailed, capiv1.ConditionSeverityError, err.Error())
 		return reconcile.Result{}, err
 	}
 	conditions.MarkTrue(cluster, infrav1.CredentialRefSecretOwnerSetCondition)
 
-	v3Client, err := CreateNutanixClient(ctx, r.SecretInformer, r.ConfigMapInformer, cluster)
+	v3Client, err := getPrismCentralClientForCluster(ctx, cluster, r.SecretInformer, r.ConfigMapInformer)
 	if err != nil {
-		conditions.MarkFalse(cluster, infrav1.PrismCentralClientCondition, infrav1.PrismCentralClientInitializationFailed, capiv1.ConditionSeverityError, err.Error())
-		return ctrl.Result{Requeue: true}, fmt.Errorf("nutanix client error: %v", err)
+		log.Error(err, "error occurred while fetching prism central client")
+		return reconcile.Result{}, err
 	}
-	conditions.MarkTrue(cluster, infrav1.PrismCentralClientCondition)
 
 	rctx := &nctx.ClusterContext{
 		Context:        ctx,
@@ -224,6 +223,10 @@ func (r *NutanixClusterReconciler) reconcileDelete(rctx *nctx.ClusterContext) (r
 		return reconcile.Result{}, err
 	}
 
+	// delete the client from the cache
+	log.Info(fmt.Sprintf("deleting nutanix prism client for cluster %s from cache", rctx.NutanixCluster.GetNamespacedName()))
+	nutanixClient.NutanixClientCache.Delete(&nutanixClient.CacheParams{NutanixCluster: rctx.NutanixCluster})
+
 	err = r.reconcileCredentialRefDelete(rctx.Context, rctx.NutanixCluster)
 	if err != nil {
 		log.Error(err, fmt.Sprintf("error occurred while reconciling credential ref deletion for cluster %s", rctx.Cluster.Name))
@@ -376,21 +379,24 @@ func (r *NutanixClusterReconciler) reconcileCredentialRef(ctx context.Context, n
 	if err != nil {
 		return err
 	}
+
+	secret := &corev1.Secret{}
 	if credentialRef == nil {
 		return nil
 	}
+
 	log.V(1).Info(fmt.Sprintf("credential ref is kind Secret for cluster %s", nutanixCluster.Name))
-	secret := &corev1.Secret{}
 	secretKey := client.ObjectKey{
 		Namespace: nutanixCluster.Namespace,
 		Name:      credentialRef.Name,
 	}
-	err = r.Client.Get(ctx, secretKey, secret)
-	if err != nil {
+
+	if err := r.Client.Get(ctx, secretKey, secret); err != nil {
 		errorMsg := fmt.Errorf("error occurred while fetching cluster %s secret for credential ref: %v", nutanixCluster.Name, err)
 		log.Error(errorMsg, "error occurred fetching cluster")
 		return errorMsg
 	}
+
 	// Check if ownerRef is already set on nutanixCluster object
 	if !capiutil.IsOwnedByObject(secret, nutanixCluster) {
 		// Check if another nutanixCluster already has set ownerRef. Secret can only be owned by one nutanixCluster object
@@ -407,15 +413,18 @@ func (r *NutanixClusterReconciler) reconcileCredentialRef(ctx context.Context, n
 			Name:       nutanixCluster.Name,
 		})
 	}
+
 	if !ctrlutil.ContainsFinalizer(secret, infrav1.NutanixClusterCredentialFinalizer) {
 		ctrlutil.AddFinalizer(secret, infrav1.NutanixClusterCredentialFinalizer)
 	}
+
 	err = r.Client.Update(ctx, secret)
 	if err != nil {
 		errorMsg := fmt.Errorf("failed to update secret for cluster %s: %v", nutanixCluster.Name, err)
 		log.Error(errorMsg, "failed to update secret")
 		return errorMsg
 	}
+
 	return nil
 }