From 9ca7cfc1c140db8522252590cd4bc2c3286f4c38 Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Thu, 18 Apr 2024 12:20:59 -0600 Subject: [PATCH] csi: ensure correct csi config map owner during creation In the primary CSI reconcile, ensure the CSI config map (`rook-ceph-csi-config`) has the correct owner info. This corrects any pre-existing config maps that might have incorrect owner info, which has observed to include references to CephClusters. The config map should only have a single reference, and it should refer to the operator deployment. If any existing Rook clusters have a CSI config map which has a CephCluster as an owner, this change will ensure that the config map is not deleted when the CephCluster is deleted. This is especially important for any environments with multiple CephClusters installed. Signed-off-by: Blaine Gardner (cherry picked from commit 605b9633eaf3ec562c1e04cc69a1798d609e6ec4) --- pkg/operator/ceph/csi/cluster_config.go | 38 ++++++ pkg/operator/ceph/csi/cluster_config_test.go | 131 ++++++++++++++++++- pkg/operator/k8sutil/resources.go | 8 +- 3 files changed, 174 insertions(+), 3 deletions(-) diff --git a/pkg/operator/ceph/csi/cluster_config.go b/pkg/operator/ceph/csi/cluster_config.go index 076614f63..e0681e62f 100644 --- a/pkg/operator/ceph/csi/cluster_config.go +++ b/pkg/operator/ceph/csi/cluster_config.go @@ -254,12 +254,50 @@ func CreateCsiConfigMap(ctx context.Context, namespace string, clientset kuberne if !k8serrors.IsAlreadyExists(err) { return errors.Wrapf(err, "failed to create initial csi config map %q (in %q)", configMap.Name, namespace) } + // CM already exists; update owner refs to it if needed + // this corrects issues where the csi config map was sometimes created with CephCluster + // owner ref, which would result in the cm being deleted if that cluster was deleted + if err := updateCsiConfigMapOwnerRefs(ctx, namespace, clientset, ownerInfo); err != nil { + return errors.Wrapf(err, "failed to ensure csi config map %q (in %q) owner references", configMap.Name, namespace) + } } logger.Infof("successfully created csi config map %q", configMap.Name) return nil } +// check the owner references on the csi config map, and fix incorrect references if needed +func updateCsiConfigMapOwnerRefs(ctx context.Context, namespace string, clientset kubernetes.Interface, expectedOwnerInfo *k8sutil.OwnerInfo) error { + cm, err := clientset.CoreV1().ConfigMaps(namespace).Get(ctx, ConfigName, metav1.GetOptions{}) + if err != nil { + return errors.Wrapf(err, "failed to fetch csi config map %q (in %q) which already exists", ConfigName, namespace) + } + + existingOwners := cm.GetOwnerReferences() + var currentOwner *metav1.OwnerReference = nil + if len(existingOwners) == 1 { + currentOwner = &existingOwners[0] // currentOwner is nil unless there is exactly one owner on the cm + } + // if there is exactly one owner, and it is correct --> no fix needed + if currentOwner != nil && (currentOwner.UID == expectedOwnerInfo.GetUID()) { + logger.Debugf("csi config map %q (in %q) has the expected owner; owner id: %q", ConfigName, namespace, currentOwner.UID) + return nil + } + + // must fix owner refs + logger.Infof("updating csi configmap %q (in %q) owner info", ConfigName, namespace) + cm.OwnerReferences = []metav1.OwnerReference{} + if err := expectedOwnerInfo.SetControllerReference(cm); err != nil { + return errors.Wrapf(err, "failed to set updated owner reference on csi config map %q (in %q)", ConfigName, namespace) + } + _, err = clientset.CoreV1().ConfigMaps(namespace).Update(ctx, cm, metav1.UpdateOptions{}) + if err != nil { + return errors.Wrapf(err, "failed to update csi config map %q (in %q) to update its owner reference", ConfigName, namespace) + } + + return nil +} + // SaveClusterConfig updates the config map used to provide ceph-csi with // basic cluster configuration. The clusterNamespace and clusterInfo are // used to determine what "cluster" in the config map will be updated and diff --git a/pkg/operator/ceph/csi/cluster_config_test.go b/pkg/operator/ceph/csi/cluster_config_test.go index fefc036ba..dca5e4425 100644 --- a/pkg/operator/ceph/csi/cluster_config_test.go +++ b/pkg/operator/ceph/csi/cluster_config_test.go @@ -17,17 +17,22 @@ limitations under the License. package csi import ( + "context" "encoding/json" "reflect" "strings" "testing" + cephcsi "github.com/ceph/ceph-csi/api/deploy/kubernetes" cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1" cephclient "github.com/rook/rook/pkg/daemon/ceph/client" "github.com/rook/rook/pkg/operator/ceph/cluster/osd/topology" + "github.com/rook/rook/pkg/operator/k8sutil" + "github.com/rook/rook/pkg/operator/test" "github.com/stretchr/testify/assert" - - cephcsi "github.com/ceph/ceph-csi/api/deploy/kubernetes" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" ) func unmarshal(s string) ([]CSIClusterConfigEntry, error) { @@ -892,3 +897,125 @@ func TestUpdateNetNamespaceFilePath(t *testing.T) { } }) } + +func Test_updateCsiConfigMapOwnerRefs(t *testing.T) { + ctx := context.TODO() + ns := "test-ns" + ownerController := true + blockOwnerDel := true + opDeployRef := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "Deployment", + Name: "rook-ceph-operator", + UID: "e55604f2-710c-4353-9a3e-9d23ea2d6eb9", // random uuid + Controller: &ownerController, + BlockOwnerDeletion: &blockOwnerDel, + } + expectedOwnerInfo := k8sutil.NewOwnerInfoWithOwnerRef(&opDeployRef, ns) + + minimalCsiConfigMap := func() *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: ConfigName, + Namespace: ns, + OwnerReferences: []metav1.OwnerReference{}, + }, + } + } + + t.Run("no configmap", func(t *testing.T) { + clientset := test.New(t, 1) + err := updateCsiConfigMapOwnerRefs(ctx, ns, clientset, expectedOwnerInfo) + assert.ErrorContains(t, err, "failed to fetch csi config map") + assert.ErrorContains(t, err, "which already exists") + }) + + assertOwner := func(t *testing.T, clientset kubernetes.Interface) { + t.Helper() + + cm, err := clientset.CoreV1().ConfigMaps(ns).Get(ctx, ConfigName, metav1.GetOptions{}) + assert.NoError(t, err) + assert.Len(t, cm.GetOwnerReferences(), 1) + cmOwner := cm.GetOwnerReferences()[0] + assert.Equal(t, "v1", cmOwner.APIVersion) + assert.Equal(t, "Deployment", cmOwner.Kind) + assert.Equal(t, "rook-ceph-operator", cmOwner.Name) + assert.Equal(t, "e55604f2-710c-4353-9a3e-9d23ea2d6eb9", string(cmOwner.UID)) + assert.True(t, *cmOwner.Controller) + assert.True(t, *cmOwner.BlockOwnerDeletion) + } + + t.Run("no existing owner ref", func(t *testing.T) { + clientset := test.New(t, 1) + cm := minimalCsiConfigMap() + _, err := clientset.CoreV1().ConfigMaps(ns).Create(ctx, cm, metav1.CreateOptions{}) + assert.NoError(t, err) + + err = updateCsiConfigMapOwnerRefs(ctx, ns, clientset, expectedOwnerInfo) + assert.NoError(t, err) + assertOwner(t, clientset) + }) + + t.Run("correct existing owner ref", func(t *testing.T) { + clientset := test.New(t, 1) + cm := minimalCsiConfigMap() + cm.OwnerReferences = []metav1.OwnerReference{ + *opDeployRef.DeepCopy(), // correct ref + } + _, err := clientset.CoreV1().ConfigMaps(ns).Create(ctx, cm, metav1.CreateOptions{}) + assert.NoError(t, err) + + err = updateCsiConfigMapOwnerRefs(ctx, ns, clientset, expectedOwnerInfo) + assert.NoError(t, err) + assertOwner(t, clientset) + }) + + t.Run("single incorrect existing owner ref", func(t *testing.T) { + clientset := test.New(t, 1) + cm := minimalCsiConfigMap() + + ownerController := true + blockOwnerDel := true + cm.OwnerReferences = []metav1.OwnerReference{ + { + APIVersion: "ceph.rook.io/v1", + Kind: "CephCluster", + Name: "my-cluster", + UID: "a77777a7-777a-7777-7a7a-7a77aa7a7aa7", // random uuid + Controller: &ownerController, + BlockOwnerDeletion: &blockOwnerDel, + }, + } + _, err := clientset.CoreV1().ConfigMaps(ns).Create(ctx, cm, metav1.CreateOptions{}) + assert.NoError(t, err) + + err = updateCsiConfigMapOwnerRefs(ctx, ns, clientset, expectedOwnerInfo) + assert.NoError(t, err) + assertOwner(t, clientset) + }) + + t.Run("multiple existing owner refs, one correct", func(t *testing.T) { + clientset := test.New(t, 1) + cm := minimalCsiConfigMap() + + ownerController := true + blockOwnerDel := true + cm.OwnerReferences = []metav1.OwnerReference{ + *opDeployRef.DeepCopy(), // correct ref + { + APIVersion: "ceph.rook.io/v1", + Kind: "CephCluster", + Name: "my-cluster", + UID: "a77777a7-777a-7777-7a7a-7a77aa7a7aa7", // random uuid + Controller: &ownerController, + BlockOwnerDeletion: &blockOwnerDel, + }, + } + _, err := clientset.CoreV1().ConfigMaps(ns).Create(ctx, cm, metav1.CreateOptions{}) + assert.NoError(t, err) + + err = updateCsiConfigMapOwnerRefs(ctx, ns, clientset, expectedOwnerInfo) + assert.NoError(t, err) + assertOwner(t, clientset) + }) +} diff --git a/pkg/operator/k8sutil/resources.go b/pkg/operator/k8sutil/resources.go index d4b141f99..01e6c377e 100644 --- a/pkg/operator/k8sutil/resources.go +++ b/pkg/operator/k8sutil/resources.go @@ -144,7 +144,13 @@ func (info *OwnerInfo) SetControllerReference(object metav1.Object) error { // GetUID gets the UID of the owner func (info *OwnerInfo) GetUID() types.UID { - return info.owner.GetUID() + if info.owner != nil { + return info.owner.GetUID() + } + if info.ownerRef != nil { + return info.ownerRef.UID + } + return types.UID("") } func MergeResourceRequirements(first, second v1.ResourceRequirements) v1.ResourceRequirements {