Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance ownerReferences for Secrets used by CAPX #340

Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions api/v1beta1/nutanixcluster_types.go
Original file line number Diff line number Diff line change
@@ -24,6 +24,9 @@ import (
)

const (
// NutanixClusterKind represents the Kind of NutanixCluster
NutanixClusterKind = "NutanixClusterKind"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CAPI ClusterKind is simply "Cluster":
https://github.com/kubernetes-sigs/cluster-api/blob/9d36ddcbf8381574a86cebeacdcdfc85bafa71e9/api/v1beta1/cluster_types.go#L39

Do we really need "NutanixClusterKind" but not "NutanixCluster"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot @adiantum! These should definitely not have the Kind suffix in the string values.


// NutanixClusterFinalizer allows NutanixClusterReconciler to clean up AHV
// resources associated with NutanixCluster before removing it from the
// API Server.
3 changes: 3 additions & 0 deletions api/v1beta1/nutanixmachine_types.go
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need "NutanixMachineKind" instead of "NutanixMachine"?


// NutanixMachineFinalizer allows NutanixMachineReconciler to clean up AHV
// resources associated with NutanixMachine before removing it from the
// API Server.
5 changes: 5 additions & 0 deletions api/v1beta1/nutanixmachinetemplate_types.go
Original file line number Diff line number Diff line change
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: do we really need "Kind" suffix?

)

// NutanixMachineTemplateSpec defines the desired state of NutanixMachineTemplate
type NutanixMachineTemplateSpec struct {
Template NutanixMachineTemplateResource `json:"template"`
13 changes: 9 additions & 4 deletions controllers/nutanixcluster_controller.go
Original file line number Diff line number Diff line change
@@ -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)
187 changes: 182 additions & 5 deletions controllers/nutanixcluster_controller_test.go
Original file line number Diff line number Diff line change
@@ -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"
@@ -43,23 +47,40 @@ func TestNutanixClusterReconciler(t *testing.T) {
const (
fd1Name = "fd-1"
fd2Name = "fd-2"
// To be replaced with capiv1.ClusterKind
clusterKind = "Cluster"
)

var (
ntnxCluster *infrav1.NutanixCluster
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: infrav1.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: infrav1.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: infrav1.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())
})
})
})
}
Loading