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 {