diff --git a/pkg/operator/ceph/csi/secrets.go b/pkg/operator/ceph/csi/secrets.go index d6c183710..b33960354 100644 --- a/pkg/operator/ceph/csi/secrets.go +++ b/pkg/operator/ceph/csi/secrets.go @@ -110,15 +110,17 @@ func createCSIKeyring( return "", "", 0, shouldRotate, errors.Wrapf(err, "failed to check if keys should to be rotated for CSI key %s", csiKeyrigName) } + // add the new key to the list of all keys + // do this before deleting old keys so that delete logic is the same when keys are first rotated + // and afterwards when they are not rotated + allKeyWithBaseName = append(allKeyWithBaseName, latestClientId) + allKeyWithBaseName = deduplicate(allKeyWithBaseName) + keysDeleted, err := deleteOldKeyGen(context, clusterInfo, allKeyWithBaseName, cephCluster.Spec.Security.CephX.CSI.KeepPriorKeyCountMax) if err != nil { logger.Errorf("failed to delete keys during CSI key rotation. %v. Continuing with key rotation", err) } - // add the new key to the list of all keys - allKeyWithBaseName = append(allKeyWithBaseName, latestClientId) - allKeyWithBaseName = deduplicate(allKeyWithBaseName) - currentKeyCount := len(allKeyWithBaseName) - len(keysDeleted) if currentKeyCount <= 0 { logger.Warningf("currentKeyCount is %d is less than or equal to 0; this could be due to an error in key generation", currentKeyCount) @@ -435,16 +437,8 @@ func deleteOldKeyGen(context *clusterd.Context, clusterInfo *client.ClusterInfo, return []string{}, nil } - keyDeleteCount := max(0, len(sortedClientList)-int(count)) // can't delete negative count - if len(sortedClientList) == keyDeleteCount && keyDeleteCount > 0 { - // Above condition will delete all the keys related to specific CSI driver keys, this will break CSI and Ceph connection. - // To avoid above we'll delete `count-1` - logger.Warningf("current `keepPriorCountMax` value %d will delete all the related CSI driver key count %d. Deleting `keepPriorCountMax +1` keys", count, keyDeleteCount) - keyDeleteCount -= 1 - } - listOfKeysDeleted := []string{} - for i := range keyDeleteCount { + for i := range deleteCount(len(sortedClientList), int(count)) { err := client.AuthDelete(context, clusterInfo, sortedClientList[i]) if err != nil { // in case of partial success, return successfully deleted list on error @@ -455,6 +449,17 @@ func deleteOldKeyGen(context *clusterd.Context, clusterInfo *client.ClusterInfo, return listOfKeysDeleted, nil } +// calculate the number of keys to delete, given current num keys and desired (max) prior count +func deleteCount(numKeys, keepPriorCount int) int { + // assumption: not called with numKeys < 2 + // current key is counted in numKeys, so ignore it in delete count + c := numKeys - keepPriorCount - 1 + if c < 0 { + return 0 // can't delete negative number of keys + } + return c +} + func deleteOwnedCSISecretsByCephCluster(context *clusterd.Context, clusterInfo *client.ClusterInfo) error { ownerRef := metav1.OwnerReference{ APIVersion: "ceph.rook.io/v1", diff --git a/pkg/operator/ceph/csi/secrets_test.go b/pkg/operator/ceph/csi/secrets_test.go index ebfe71910..10c4222eb 100644 --- a/pkg/operator/ceph/csi/secrets_test.go +++ b/pkg/operator/ceph/csi/secrets_test.go @@ -37,11 +37,22 @@ import ( const ( mockAuthLs = `{"auth_dump":[ - {"entity":"osd.0"},{"entity":"client.admin"},{"entity":"client.bootstrap-mds"},{"entity":"client.bootstrap-rbd-mirror"},{"entity":"client.csi-rbd-node.10"}, - {"entity":"client.csi-rbd-node.4"},{"entity":"client.csi-rbd-node.2"},{"entity":"client.csi-rbd-node.1"},{"entity":"client.csi-cepfs-node"}, - {"entity":"client.csi-rbd-node"},{"entity":"client.csi-rbd-provisioner"},{"entity":"client.rbd-mirror-peer"},{"entity":"mgr.a"}, - {"entity":"client.csi-cepfs-node"},{"entity":"client.csi-cephfs-node.2"} - ]}` +{"entity":"osd.0"}, +{"entity":"client.admin"}, +{"entity":"client.bootstrap-mds"}, +{"entity":"client.bootstrap-rbd-mirror"}, +{"entity":"client.csi-rbd-node.10"}, +{"entity":"client.csi-rbd-node.4"}, +{"entity":"client.csi-rbd-node.2"}, +{"entity":"client.csi-rbd-node.1"}, +{"entity":"client.csi-cepfs-node"}, +{"entity":"client.csi-rbd-node"}, +{"entity":"client.csi-rbd-provisioner"}, +{"entity":"client.rbd-mirror-peer"}, +{"entity":"mgr.a"}, +{"entity":"client.csi-cepfs-node"}, +{"entity":"client.csi-cephfs-node.2"} +]}` ) func TestCephCSIKeyringRBDNodeCaps(t *testing.T) { @@ -183,16 +194,31 @@ func TestDeleteOldKeyGen(t *testing.T) { "client.csi-rbd-node.10", "client.csi-rbd-node.1", "client.csi-rbd-node.4", "client.csi-rbd-node.2", "client.csi-rbd-node", } - // Only the oldest keys ("client.csi-rbd-node" and "client.csi-rbd-node.1") are deleted to retain the latest 3 generations. - keyDeleted, err := deleteOldKeyGen(ctx, clusterInfo, keys, 3) + var keyDeleted []string + var err error + + // deleting nothing when keep prior count more than current number of existing prior keys + keyDeleted, err = deleteOldKeyGen(ctx, clusterInfo, keys, 5) + assert.NoError(t, err) + assert.Equal(t, []string{}, keyDeleted) + + // deleting nothing when keep prior count is same as current number of existing prior keys + keyDeleted, err = deleteOldKeyGen(ctx, clusterInfo, keys, 4) + assert.NoError(t, err) + assert.Equal(t, []string{}, keyDeleted) + + // Delete 1 oldest key ("client.csi-rbd-node") is deleted to retain the prior 3 generations. + keyDeleted, err = deleteOldKeyGen(ctx, clusterInfo, keys, 3) + assert.NoError(t, err) + assert.Equal(t, []string{"client.csi-rbd-node"}, keyDeleted) + + keyDeleted, err = deleteOldKeyGen(ctx, clusterInfo, keys, 2) assert.NoError(t, err) assert.Equal(t, []string{"client.csi-rbd-node", "client.csi-rbd-node.1"}, keyDeleted) - // When keepCount is 1, only the most recent key ("client.csi-rbd-node.10") is kept; older keys ("client.csi-rbd-node.1", "client.csi-rbd-node.4") are deleted. - keys = []string{"client.csi-rbd-node.10", "client.csi-rbd-node.1", "client.csi-rbd-node.4"} keyDeleted, err = deleteOldKeyGen(ctx, clusterInfo, keys, 1) assert.NoError(t, err) - assert.Equal(t, []string{"client.csi-rbd-node.1", "client.csi-rbd-node.4"}, keyDeleted) + assert.Equal(t, []string{"client.csi-rbd-node", "client.csi-rbd-node.1", "client.csi-rbd-node.2"}, keyDeleted) // No keys should be deleted when the input key list is empty. keys = []string{} @@ -370,11 +396,16 @@ func TestGetCSIKeyInfoAndDeleteOldKey(t *testing.T) { keepCount uint8 wantDeleted []string }{ - {"test deletion", keyWithBaseName, 3, []string{"client.csi-rbd-node", "client.csi-rbd-node.1"}}, - {"keep count = to total key count", keyWithBaseName, 10, []string{}}, - {"keep count > to total key count", keyWithBaseName, 11, []string{}}, + {"test deletion", keyWithBaseName, 2, []string{"client.csi-rbd-node", "client.csi-rbd-node.1"}}, + {"keep count = current gen", keyWithBaseName, 10, []string{}}, + {"keep count > current gen", keyWithBaseName, 11, []string{}}, {"keep count 0", keyWithBaseName, 0, []string{"client.csi-rbd-node", "client.csi-rbd-node.1", "client.csi-rbd-node.2", "client.csi-rbd-node.4"}}, - {"keep count 1", keyWithBaseName, 1, []string{"client.csi-rbd-node", "client.csi-rbd-node.1", "client.csi-rbd-node.2", "client.csi-rbd-node.4"}}, + {"keep count 1", keyWithBaseName, 1, []string{"client.csi-rbd-node", "client.csi-rbd-node.1", "client.csi-rbd-node.2"}}, + {"keep count 2", keyWithBaseName, 2, []string{"client.csi-rbd-node", "client.csi-rbd-node.1"}}, + {"keep count 3", keyWithBaseName, 3, []string{"client.csi-rbd-node"}}, + {"keep count = current count - 1 = 4", keyWithBaseName, 4, []string{}}, + {"keep count = current count = 5", keyWithBaseName, 5, []string{}}, + {"keep count > current count", keyWithBaseName, 6, []string{}}, {"key list empty, keep count 0", []string{}, 0, []string{}}, {"key list empty, keep count 1", []string{}, 1, []string{}}, {"key list empty, keep count 3", []string{}, 3, []string{}}, @@ -541,3 +572,28 @@ func TestGetPriorKeyCount(t *testing.T) { }) } } + +func Test_deleteCount(t *testing.T) { + tests := []struct { + numKeys int + keepPriorCount int + want int + }{ + {2, 0, 1}, + {3, 0, 2}, + {4, 0, 3}, + {2, 1, 0}, + {3, 1, 1}, + {4, 1, 2}, + {2, 2, 0}, + {3, 2, 0}, + {4, 2, 1}, + } + for _, tt := range tests { + t.Run(fmt.Sprintf("%d,%d", tt.numKeys, tt.keepPriorCount), func(t *testing.T) { + if got := deleteCount(tt.numKeys, tt.keepPriorCount); got != tt.want { + t.Errorf("deleteCount() = %v, want %v", got, tt.want) + } + }) + } +}