From d0096cf2a84d803a907bf888089eb042624258b9 Mon Sep 17 00:00:00 2001 From: Yannick Struyf Date: Wed, 6 Dec 2023 15:45:01 +0100 Subject: [PATCH 1/2] Remove limitation of only allowing one ownerRef for the credential secret --- controllers/nutanixcluster_controller.go | 13 +- controllers/nutanixcluster_controller_test.go | 191 +++++++++++++++++- 2 files changed, 193 insertions(+), 11 deletions(-) diff --git a/controllers/nutanixcluster_controller.go b/controllers/nutanixcluster_controller.go index 78a578db43..22f0029ae3 100644 --- a/controllers/nutanixcluster_controller.go +++ b/controllers/nutanixcluster_controller.go @@ -389,16 +389,21 @@ func (r *NutanixClusterReconciler) reconcileCredentialRef(ctx context.Context, n log.Error(errorMsg, "error occurred fetching cluster") return errorMsg } + // Check if ownerRef is already set on nutanixCluster object if !capiutil.IsOwnedByObject(secret, nutanixCluster) { - if len(secret.GetOwnerReferences()) > 0 { - return fmt.Errorf("secret for cluster %s already has other owners set", nutanixCluster.Name) + // Check if another nutanixCluster already has set ownerRef. Secret can only be owned by one nutanixCluster object + if capiutil.HasOwner(secret.OwnerReferences, infrav1.GroupVersion.String(), []string{ + nutanixCluster.Kind, + }) { + return fmt.Errorf("secret %s already owned by another nutanixCluster object", secret.Name) } - secret.SetOwnerReferences([]metav1.OwnerReference{{ + // Set nutanixCluster ownerRef on the secret + secret.OwnerReferences = capiutil.EnsureOwnerRef(secret.OwnerReferences, metav1.OwnerReference{ APIVersion: infrav1.GroupVersion.String(), Kind: nutanixCluster.Kind, UID: nutanixCluster.UID, Name: nutanixCluster.Name, - }}) + }) } if !ctrlutil.ContainsFinalizer(secret, infrav1.NutanixClusterCredentialFinalizer) { ctrlutil.AddFinalizer(secret, infrav1.NutanixClusterCredentialFinalizer) diff --git a/controllers/nutanixcluster_controller_test.go b/controllers/nutanixcluster_controller_test.go index 266095d970..b0c471685b 100644 --- a/controllers/nutanixcluster_controller_test.go +++ b/controllers/nutanixcluster_controller_test.go @@ -24,9 +24,13 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/uuid" + capiv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util" + capiutil "sigs.k8s.io/cluster-api/util" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + ctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" infrav1 "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/api/v1beta1" nctx "github.com/nutanix-cloud-native/cluster-api-provider-nutanix/pkg/context" @@ -41,8 +45,10 @@ func TestNutanixClusterReconciler(t *testing.T) { _ = Describe("NutanixClusterReconciler", func() { const ( - fd1Name = "fd-1" - fd2Name = "fd-2" + fd1Name = "fd-1" + fd2Name = "fd-2" + nutanixClusterKind = "NutanixCluster" + clusterKind = "Cluster" ) var ( @@ -50,16 +56,31 @@ func TestNutanixClusterReconciler(t *testing.T) { ctx context.Context fd1 infrav1.NutanixFailureDomain reconciler *NutanixClusterReconciler + ntnxSecret *corev1.Secret r string ) BeforeEach(func() { ctx = context.Background() - r := util.RandomString(10) + r = util.RandomString(10) + ntnxSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: r, + Namespace: corev1.NamespaceDefault, + }, + StringData: map[string]string{ + r: r, + }, + } ntnxCluster = &infrav1.NutanixCluster{ + TypeMeta: metav1.TypeMeta{ + Kind: nutanixClusterKind, + APIVersion: infrav1.GroupVersion.String(), + }, ObjectMeta: metav1.ObjectMeta{ Name: "test", - Namespace: "default", + Namespace: corev1.NamespaceDefault, + UID: utilruntime.NewUUID(), }, Spec: infrav1.NutanixClusterSpec{ PrismCentral: &credentialTypes.NutanixPrismEndpoint{ @@ -88,8 +109,8 @@ func TestNutanixClusterReconciler(t *testing.T) { }) AfterEach(func() { - err := k8sClient.Delete(ctx, ntnxCluster) - Expect(err).NotTo(HaveOccurred()) + // Delete ntnxCluster if exists. + k8sClient.Delete(ctx, ntnxCluster) }) Context("Reconcile an NutanixCluster", func() { @@ -110,7 +131,7 @@ func TestNutanixClusterReconciler(t *testing.T) { }) Context("ReconcileNormal for a NutanixCluster", func() { - It("should not requeue if failure message is set on nutanixCluster", func() { + It("should not requeue if failure message is set on NutanixCluster", func() { g.Expect(k8sClient.Create(ctx, ntnxCluster)).To(Succeed()) ntnxCluster.Status.FailureMessage = &r result, err := reconciler.reconcileNormal(&nctx.ClusterContext{ @@ -215,5 +236,161 @@ func TestNutanixClusterReconciler(t *testing.T) { g.Expect(appliedNtnxCluster.Status.FailureDomains).To(BeEmpty()) }) }) + Context("Reconcile credentialRef for a NutanixCluster", func() { + It("should return error if already owned by another NutanixCluster", func() { + // Create an additional NutanixCluster object + additionalNtnxCluster := &infrav1.NutanixCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: r, + Namespace: corev1.NamespaceDefault, + }, + Spec: infrav1.NutanixClusterSpec{ + PrismCentral: &credentialTypes.NutanixPrismEndpoint{ + // Adding port info to override default value (0) + Port: 9440, + }, + }, + } + g.Expect(k8sClient.Create(ctx, additionalNtnxCluster)).To(Succeed()) + + // Add credential ref to the ntnxCluster resource + ntnxCluster.Spec.PrismCentral.CredentialRef = &credentialTypes.NutanixCredentialReference{ + Kind: credentialTypes.SecretKind, + Name: ntnxSecret.Name, + Namespace: ntnxSecret.Namespace, + } + + // Add an ownerReference for the additional NutanixCluster object + ntnxSecret.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: infrav1.GroupVersion.String(), + Kind: nutanixClusterKind, + UID: additionalNtnxCluster.UID, + Name: additionalNtnxCluster.Name, + }, + } + g.Expect(k8sClient.Create(ctx, ntnxSecret)).To(Succeed()) + + // Reconcile credentialRef + err := reconciler.reconcileCredentialRef(ctx, ntnxCluster) + g.Expect(err).To(HaveOccurred()) + }) + It("should add credentialRef and finalizer if not owned by other cluster", func() { + // Add credential ref to the ntnxCluster resource + ntnxCluster.Spec.PrismCentral.CredentialRef = &credentialTypes.NutanixCredentialReference{ + Kind: credentialTypes.SecretKind, + Name: ntnxSecret.Name, + Namespace: ntnxSecret.Namespace, + } + + // Create secret + g.Expect(k8sClient.Create(ctx, ntnxSecret)).To(Succeed()) + + // Reconcile credentialRef + err := reconciler.reconcileCredentialRef(ctx, ntnxCluster) + g.Expect(err).ToNot(HaveOccurred()) + + // Get latest secret status + g.Expect(k8sClient.Get(ctx, client.ObjectKey{ + Namespace: ntnxSecret.Namespace, + Name: ntnxSecret.Name, + }, ntnxSecret)).To(Succeed()) + + // Check if secret is owned by the NutanixCluster + g.Expect(capiutil.IsOwnedByObject(ntnxSecret, ntnxCluster)).To(BeTrue()) + + // check finalizer + g.Expect(ctrlutil.ContainsFinalizer(ntnxSecret, infrav1.NutanixClusterCredentialFinalizer)) + }) + It("does not add another credentialRef if it is already set", func() { + // Add credential ref to the ntnxCluster resource + ntnxCluster.Spec.PrismCentral.CredentialRef = &credentialTypes.NutanixCredentialReference{ + Kind: credentialTypes.SecretKind, + Name: ntnxSecret.Name, + Namespace: ntnxSecret.Namespace, + } + + // Add an ownerReference for the NutanixCluster object + ntnxSecret.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: infrav1.GroupVersion.String(), + Kind: nutanixClusterKind, + UID: ntnxCluster.UID, + Name: ntnxCluster.Name, + }, + } + + g.Expect(k8sClient.Create(ctx, ntnxSecret)).To(Succeed()) + + // Reconcile credentialRef + err := reconciler.reconcileCredentialRef(ctx, ntnxCluster) + g.Expect(err).ToNot(HaveOccurred()) + + // Get latest secret status + g.Expect(k8sClient.Get(ctx, client.ObjectKey{ + Namespace: ntnxSecret.Namespace, + Name: ntnxSecret.Name, + }, ntnxSecret)).To(Succeed()) + + // Check if secret is owned by the NutanixCluster + g.Expect(capiutil.IsOwnedByObject(ntnxSecret, ntnxCluster)).To(BeTrue()) + + // check if only one ownerReference has been added + g.Expect(len(ntnxSecret.OwnerReferences)).To(Equal(1)) + }) + + It("allows multiple ownerReferences with different kinds", func() { + // Add credential ref to the ntnxCluster resource + ntnxCluster.Spec.PrismCentral.CredentialRef = &credentialTypes.NutanixCredentialReference{ + Kind: credentialTypes.SecretKind, + Name: ntnxSecret.Name, + Namespace: ntnxSecret.Namespace, + } + + // Add an ownerReference for a fake object + ntnxSecret.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: capiv1.GroupVersion.String(), + Kind: clusterKind, + UID: ntnxCluster.UID, + Name: r, + }, + } + + g.Expect(k8sClient.Create(ctx, ntnxSecret)).To(Succeed()) + + // Reconcile credentialRef + err := reconciler.reconcileCredentialRef(ctx, ntnxCluster) + g.Expect(err).ToNot(HaveOccurred()) + + // Get latest secret status + g.Expect(k8sClient.Get(ctx, client.ObjectKey{ + Namespace: ntnxSecret.Namespace, + Name: ntnxSecret.Name, + }, ntnxSecret)).To(Succeed()) + + // Check if secret is owned by the NutanixCluster + g.Expect(capiutil.IsOwnedByObject(ntnxSecret, ntnxCluster)).To(BeTrue()) + + g.Expect(len(ntnxSecret.OwnerReferences)).To(Equal(2)) + }) + It("should error if secret does not exist", func() { + // Add credential ref to the ntnxCluster resource + ntnxCluster.Spec.PrismCentral.CredentialRef = &credentialTypes.NutanixCredentialReference{ + Kind: credentialTypes.SecretKind, + Name: ntnxSecret.Name, + Namespace: ntnxSecret.Namespace, + } + + // Reconcile credentialRef + err := reconciler.reconcileCredentialRef(ctx, ntnxCluster) + g.Expect(err).To(HaveOccurred()) + }) + It("should error if NutanixCluster is nil", func() { + // Reconcile credentialRef + err := reconciler.reconcileCredentialRef(ctx, nil) + g.Expect(err).To(HaveOccurred()) + }) + }) }) } From 678afbe7047b4a4b337cc64b3c915a7421131dc3 Mon Sep 17 00:00:00 2001 From: Yannick Struyf Date: Fri, 8 Dec 2023 12:47:10 +0100 Subject: [PATCH 2/2] Add Kind constants for CAPX --- api/v1beta1/nutanixcluster_types.go | 3 +++ api/v1beta1/nutanixmachine_types.go | 3 +++ api/v1beta1/nutanixmachinetemplate_types.go | 5 +++++ controllers/nutanixcluster_controller_test.go | 14 +++++++------- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/api/v1beta1/nutanixcluster_types.go b/api/v1beta1/nutanixcluster_types.go index b19dd24fb5..d197cdc23d 100644 --- a/api/v1beta1/nutanixcluster_types.go +++ b/api/v1beta1/nutanixcluster_types.go @@ -24,6 +24,9 @@ import ( ) const ( + // NutanixClusterKind represents the Kind of NutanixCluster + NutanixClusterKind = "NutanixClusterKind" + // NutanixClusterFinalizer allows NutanixClusterReconciler to clean up AHV // resources associated with NutanixCluster before removing it from the // API Server. diff --git a/api/v1beta1/nutanixmachine_types.go b/api/v1beta1/nutanixmachine_types.go index ff7d1523ef..a4ddeb86ee 100644 --- a/api/v1beta1/nutanixmachine_types.go +++ b/api/v1beta1/nutanixmachine_types.go @@ -28,6 +28,9 @@ import ( // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. const ( + // NutanixMachineKind represents the Kind of NutanixMachine + NutanixMachineKind = "NutanixMachineKind" + // NutanixMachineFinalizer allows NutanixMachineReconciler to clean up AHV // resources associated with NutanixMachine before removing it from the // API Server. diff --git a/api/v1beta1/nutanixmachinetemplate_types.go b/api/v1beta1/nutanixmachinetemplate_types.go index 540a7e81ce..b08f4e7b05 100644 --- a/api/v1beta1/nutanixmachinetemplate_types.go +++ b/api/v1beta1/nutanixmachinetemplate_types.go @@ -23,6 +23,11 @@ import ( // NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. +const ( + // NutanixMachineTemplateKind represents the Kind of NutanixMachineTemplate + NutanixMachineTemplateKind = "NutanixMachineTemplateKind" +) + // NutanixMachineTemplateSpec defines the desired state of NutanixMachineTemplate type NutanixMachineTemplateSpec struct { Template NutanixMachineTemplateResource `json:"template"` diff --git a/controllers/nutanixcluster_controller_test.go b/controllers/nutanixcluster_controller_test.go index b0c471685b..16b98403a1 100644 --- a/controllers/nutanixcluster_controller_test.go +++ b/controllers/nutanixcluster_controller_test.go @@ -45,10 +45,10 @@ func TestNutanixClusterReconciler(t *testing.T) { _ = Describe("NutanixClusterReconciler", func() { const ( - fd1Name = "fd-1" - fd2Name = "fd-2" - nutanixClusterKind = "NutanixCluster" - clusterKind = "Cluster" + fd1Name = "fd-1" + fd2Name = "fd-2" + // To be replaced with capiv1.ClusterKind + clusterKind = "Cluster" ) var ( @@ -74,7 +74,7 @@ func TestNutanixClusterReconciler(t *testing.T) { } ntnxCluster = &infrav1.NutanixCluster{ TypeMeta: metav1.TypeMeta{ - Kind: nutanixClusterKind, + Kind: infrav1.NutanixClusterKind, APIVersion: infrav1.GroupVersion.String(), }, ObjectMeta: metav1.ObjectMeta{ @@ -264,7 +264,7 @@ func TestNutanixClusterReconciler(t *testing.T) { ntnxSecret.OwnerReferences = []metav1.OwnerReference{ { APIVersion: infrav1.GroupVersion.String(), - Kind: nutanixClusterKind, + Kind: infrav1.NutanixClusterKind, UID: additionalNtnxCluster.UID, Name: additionalNtnxCluster.Name, }, @@ -314,7 +314,7 @@ func TestNutanixClusterReconciler(t *testing.T) { ntnxSecret.OwnerReferences = []metav1.OwnerReference{ { APIVersion: infrav1.GroupVersion.String(), - Kind: nutanixClusterKind, + Kind: infrav1.NutanixClusterKind, UID: ntnxCluster.UID, Name: ntnxCluster.Name, },