Repository: rook/rook
Pull request #14089 (2024-04-18 17:14:07)

csi: only create CSI config configmap in CSI reconciler
There have been some issues with non-CSI Rook controllers that are creating the CSI config configmap (`rook-ceph-csi-config`). This causes problems with the K8s OwnerReference. The primary CSI reconciler (controller) creates the configmap with the correct owner reference, which is supposed to be the operator deployment.

Other instances were creating the configmap with owner references set to the CephCluster. This is a minor bug, but it can result in this configmap being deleted along with the first CephCluster that initially created it.

To fix this issue, remove all instances of `CreateCsiConfigMap()` except the single usage which the CSI reconcile uses to initially create the configmap. Other controllers that might have attempted to create this configmap previously will return an error indicating that it is waiting for the configmap to be created.

<!-- Thank you for contributing to Rook! -->

<!-- STEPS TO FOLLOW:
  1. Add a description of the changes (frequently the same as the commit description)
  2. Enter the issue number next to "Resolves #" below (if there is no tracking issue resolved, **remove that section**)
  3. Review our Contributing documentation at https://rook.io/docs/rook/latest/Contributing/development-flow/
  4. Follow the steps in the checklist below, starting with the **Commit Message Formatting**.
-->

<!-- Uncomment this section with the issue number if an issue is being resolved
**Issue resolved by this Pull Request:**
Resolves #
--->

**Checklist:**

- [ ] **Commit Message Formatting**: Commit titles and messages follow guidelines in the [developer guide](https://rook.io/docs/rook/latest/Contributing/development-flow/#commit-structure).
- [ ] Reviewed the developer guide on [Submitting a Pull Request](https://rook.io/docs/rook/latest/Contributing/development-flow/#submitting-a-pull-request)
- [ ] [Pending release notes](https://github.com/rook/rook/blob/master/PendingReleaseNotes.md) updated with breaking and/or notable changes for the next minor release.
- [ ] Documentation has been updated, if necessary.
- [ ] Unit tests have been added, if necessary.
- [ ] Integration tests have been added, if necessary.



Patch:
diff --git a/pkg/operator/ceph/cluster/cluster_external.go b/pkg/operator/ceph/cluster/cluster_external.go
index f0845b82d..0c0749b7e 100644
--- a/pkg/operator/ceph/cluster/cluster_external.go
+++ b/pkg/operator/ceph/cluster/cluster_external.go
@@ -108,12 +108,6 @@ func (c *ClusterController) configureExternalCephCluster(cluster *cluster) error
 		}
 	}
 
-	// Create CSI config map
-	err = csi.CreateCsiConfigMap(c.OpManagerCtx, c.namespacedName.Namespace, c.context.Clientset, cluster.ownerInfo)
-	if err != nil {
-		return errors.Wrap(err, "failed to create csi config map")
-	}
-
 	// update the msgr2 flag
 	for _, m := range cluster.ClusterInfo.Monitors {
 		// m.Endpoint=10.1.115.104:3300
diff --git a/pkg/operator/ceph/csi/cluster_config.go b/pkg/operator/ceph/csi/cluster_config.go
index 132ca5350..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
@@ -292,10 +330,7 @@ func SaveClusterConfig(clientset kubernetes.Interface, clusterNamespace string,
 	configMap, err := clientset.CoreV1().ConfigMaps(csiNamespace).Get(clusterInfo.Context, ConfigName, metav1.GetOptions{})
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
-			err = CreateCsiConfigMap(clusterInfo.Context, csiNamespace, clientset, clusterInfo.OwnerInfo)
-			if err != nil {
-				return errors.Wrap(err, "failed creating csi config map")
-			}
+			return errors.Wrap(err, "waiting for CSI config map to be created")
 		}
 		return errors.Wrap(err, "failed to fetch current csi config map")
 	}
diff --git a/pkg/operator/ceph/csi/controller.go b/pkg/operator/ceph/csi/controller.go
index 8a31f36f7..f4874ed55 100644
--- a/pkg/operator/ceph/csi/controller.go
+++ b/pkg/operator/ceph/csi/controller.go
@@ -200,6 +200,34 @@ func (r *ReconcileCSI) reconcile(request reconcile.Request) (reconcile.Result, e
 		return reconcile.Result{}, nil
 	}
 
+	ownerRef, err := k8sutil.GetDeploymentOwnerReference(r.opManagerContext, r.context.Clientset, os.Getenv(k8sutil.PodNameEnvVar), r.opConfig.OperatorNamespace)
+	if err != nil {
+		logger.Warningf("could not find deployment owner reference to assign to csi drivers. %v", err)
+	}
+	if ownerRef != nil {
+		blockOwnerDeletion := false
+		ownerRef.BlockOwnerDeletion = &blockOwnerDeletion
+	}
+
+	ownerInfo := k8sutil.NewOwnerInfoWithOwnerRef(ownerRef, r.opConfig.OperatorNamespace)
+	// create an empty config map. config map will be filled with data
+	// later when clusters have mons
+	err = CreateCsiConfigMap(r.opManagerContext, r.opConfig.OperatorNamespace, r.context.Clientset, ownerInfo)
+	if err != nil {
+		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed creating csi config map")
+	}
+
+	err = peermap.CreateOrUpdateConfig(r.opManagerContext, r.context, &peermap.PeerIDMappings{})
+	if err != nil {
+		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed to create pool ID mapping config map")
+	}
+
+	exists, err := checkCsiCephConfigMapExists(r.opManagerContext, r.context.Clientset, r.opConfig.OperatorNamespace)
+	if err != nil {
+		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed to get csi ceph.conf configmap")
+	}
+	CustomCSICephConfigExists = exists
+
 	csiHostNetworkEnabled, err := strconv.ParseBool(k8sutil.GetValue(r.opConfig.Parameters, "CSI_ENABLE_HOST_NETWORK", "true"))
 	if err != nil {
 		return reconcile.Result{}, errors.Wrap(err, "failed to parse value for 'CSI_ENABLE_HOST_NETWORK'")
@@ -264,34 +292,6 @@ func (r *ReconcileCSI) reconcile(request reconcile.Request) (reconcile.Result, e
 		}
 	}
 
-	ownerRef, err := k8sutil.GetDeploymentOwnerReference(r.opManagerContext, r.context.Clientset, os.Getenv(k8sutil.PodNameEnvVar), r.opConfig.OperatorNamespace)
-	if err != nil {
-		logger.Warningf("could not find deployment owner reference to assign to csi drivers. %v", err)
-	}
-	if ownerRef != nil {
-		blockOwnerDeletion := false
-		ownerRef.BlockOwnerDeletion = &blockOwnerDeletion
-	}
-
-	ownerInfo := k8sutil.NewOwnerInfoWithOwnerRef(ownerRef, r.opConfig.OperatorNamespace)
-	// create an empty config map. config map will be filled with data
-	// later when clusters have mons
-	err = CreateCsiConfigMap(r.opManagerContext, r.opConfig.OperatorNamespace, r.context.Clientset, ownerInfo)
-	if err != nil {
-		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed creating csi config map")
-	}
-
-	err = peermap.CreateOrUpdateConfig(r.opManagerContext, r.context, &peermap.PeerIDMappings{})
-	if err != nil {
-		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed to create pool ID mapping config map")
-	}
-
-	exists, err := checkCsiCephConfigMapExists(r.opManagerContext, r.context.Clientset, r.opConfig.OperatorNamespace)
-	if err != nil {
-		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed to get csi ceph.conf configmap")
-	}
-	CustomCSICephConfigExists = exists
-
 	err = r.validateAndConfigureDrivers(serverVersion, ownerInfo)
 	if err != nil {
 		return opcontroller.ImmediateRetryResult, errors.Wrap(err, "failed to configure ceph csi")
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 {


Test patch:
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)
+	})
+}