util: cache the controller publish secret by black-dragon74 · Pull Request #5497 · ceph/ceph-csi

black-dragon74 · 2025-08-13T07:37:31Z

Describe what this PR does

This patch introduces a caching mechanism based on the shared informers for ControllerPublish secrets.

Is there anything that requires special attention

Do you have any questions?

Should we watch for all namespaces?
Is the default cache re-sync interval of 10 minutes ideal?

Rakshith-R

just a small nit,
please open the pr for review

nixpanic · 2025-09-04T10:45:53Z

 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/informers"
+	"k8s.io/client-go/tools/cache"
+	"sigs.k8s.io/controller-runtime/pkg/manager/signals"


controller-runtime is very large, I don't think you need it here.

The signals package is only used for this:

<-signals.SetupSignalHandler().Done()

I think you can just write an empty struct to the channel too?

<-struct{}{}

Or, consider replacing stopCh chan struct{} by stopCh chan bool and write <-true or something to it.

The idea was to avoid leaking the unclosed channel. Handled it via signal.Notify.

nixpanic · 2025-09-04T10:52:37Z

+	"sigs.k8s.io/controller-runtime/pkg/manager/signals"
+)
+
+type cachedSecret struct {


Why introduce this type, and not use the Kubernetes Secret type?

To avoid iterating, casting and copying secret.Data

I guess nothing uses secret.StringData, but only the base64 encoded values?

map[string][]byte vs map[string]string. While storing in cache we iterate over the bytes and cast them to string. Storing them like that is more convenient as the code elsewhere expects a map[string]string

nixpanic · 2025-09-04T10:59:52Z

Do you have any questions?
* Should we watch for all namespaces?

Only the namespaces+secrets that are requested. There will be many Secrets in a Kubernetes cluster, we definitely should not cache all of them. Even all Secrets in the namespace where Ceph-CSI is deployed may be way too much. We also don't want auditing to fire alarms when Ceph-CSI is caching secrets it would not use.

* Is the default cache re-sync interval of 10 minutes ideal?

A non-changeable default is never ideal. For some users it will be fine, others may want/need to tune it. Ideally there is an option to adjust the behavior.

nixpanic

Looks good, just a few nits that should be corrected. The OOP change would be nice, but isn't absolutely required.

nixpanic · 2025-09-09T07:33:54Z

+	"sigs.k8s.io/controller-runtime/pkg/manager/signals"
+)
+
+type cachedSecret struct {


I guess nothing uses secret.StringData, but only the base64 encoded values?

nixpanic

Looks good. Just waiting for a response on the outstanding questions/comments.

nixpanic · 2025-09-09T14:34:27Z

+	input := fmt.Sprintf("%s/%s", ns, name)
+	h := sha256.Sum256([]byte(input))
+
+	// Only use 16bytes


I understand that the code only uses 16 bytes. The comment should better explain why only 16 bytes should be used. Is there no issue with potential abbreviated-hash-collisions?

Added a comment. For a cache key, 128bits is collision is improbable until order of 10^9 entries. We'd be good with more than a billion entries.

nixpanic · 2025-09-09T14:37:41Z

+}
+
+// secretCache is a thread safe cache for secrets.
+// /!\ The `cache` must be read/modified with the lock held.


Just wondering why you decided to use sync.RWMutex and not sync.Map. Can you explain that a little?

sync.Map is not strongly typed and would require type casting.

Rakshith-R

Thanks !

Rakshith-R · 2025-09-12T07:32:04Z

@Mergifyio rebase

This patch introduces a caching mechanism based on the shared informers for ControllerPublish secrets. Signed-off-by: Niraj Yadav <[email protected]>

mergify · 2025-09-12T07:32:34Z

rebase

✅ Branch has been successfully rebased

iPraveenParihar

Thanks!

iPraveenParihar · 2025-09-12T08:23:41Z

/test ci/centos/mini-e2e/k8s-1.33

iPraveenParihar · 2025-09-12T14:08:27Z

@Mergifyio rebase

mergify · 2025-09-12T14:08:32Z

rebase

☑️ Nothing to do, the required conditions are not met

Details

any of:
- #commits > 1 [📌 rebase requirement]
- #commits-behind > 0 [📌 rebase requirement]
- -linear-history [📌 rebase requirement]
-closed [📌 rebase requirement]
-conflict [📌 rebase requirement]
queue-position = -1 [📌 rebase requirement]

iPraveenParihar · 2025-09-12T14:08:52Z

@Mergifyio queue

mergify · 2025-09-12T14:09:11Z

queue

🛑 The pull request has been removed from the queue `default`

Details

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details about the failure.

mergify · 2025-09-12T14:09:15Z

This pull request has been removed from the queue for the following reason: checks failed.

The merge conditions cannot be satisfied due to failing checks:

🛑 multi-arch-build

You may have to fix your CI before adding the pull request to the queue again.
If you update this pull request, to fix the CI, it will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue instead, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

iPraveenParihar · 2025-09-12T16:49:48Z

@Mergifyio queue

mergify · 2025-09-12T16:50:08Z

queue

🛑 The pull request has been removed from the queue `default`

Details

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details about the failure.

ceph-csi-bot · 2025-09-12T16:50:17Z

/test ci/centos/k8s-e2e-external-storage/1.33

ceph-csi-bot · 2025-09-12T16:50:17Z

/test ci/centos/k8s-e2e-external-storage/1.32

ceph-csi-bot · 2025-09-12T16:50:18Z

/test ci/centos/upgrade-tests-cephfs

ceph-csi-bot · 2025-09-12T16:50:18Z

/test ci/centos/k8s-e2e-external-storage/1.31

ceph-csi-bot · 2025-09-12T16:50:18Z

/test ci/centos/mini-e2e-helm/k8s-1.32

ceph-csi-bot · 2025-09-12T16:50:18Z

/test ci/centos/mini-e2e-helm/k8s-1.33

ceph-csi-bot · 2025-09-12T16:50:18Z

/test ci/centos/upgrade-tests-rbd

ceph-csi-bot · 2025-09-12T16:50:19Z

/test ci/centos/mini-e2e-helm/k8s-1.31

ceph-csi-bot · 2025-09-12T16:50:19Z

/test ci/centos/mini-e2e/k8s-1.32

ceph-csi-bot · 2025-09-12T16:50:19Z

/test ci/centos/mini-e2e/k8s-1.33

ceph-csi-bot · 2025-09-12T16:50:19Z

/test ci/centos/mini-e2e/k8s-1.31

iPraveenParihar · 2025-09-12T16:50:32Z

https://github.com/Mergifyio queue

mergify · 2025-09-12T16:50:42Z

queue

🛑 The pull request has been removed from the queue `default`

Details

The merge conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details about the failure.

iPraveenParihar · 2025-09-15T05:45:39Z

@Mergifyio refresh

mergify · 2025-09-15T05:45:46Z

refresh

✅ Pull request refreshed

This patch adds watch and list RBACs to clusterroles for CephFS and RBD nodeplugin. This is to comply with the enhancement at: ceph/ceph-csi#5497 Signed-off-by: Niraj Yadav <[email protected]>

This patch adds watch and list RBACs to clusterroles for CephFS and RBD nodeplugin. This is to comply with the enhancement at: ceph/ceph-csi#5497 Signed-off-by: Niraj Yadav <[email protected]> (cherry picked from commit 0308496)

Rakshith-R reviewed Sep 2, 2025

View reviewed changes

Comment thread internal/util/k8s/secrets.go Outdated

black-dragon74 force-pushed the cache-ctrl-pub-secret branch from 88e216a to 6521610 Compare September 3, 2025 07:03

Rakshith-R reviewed Sep 3, 2025

View reviewed changes

Comment thread internal/util/k8s/secrets.go Outdated

black-dragon74 force-pushed the cache-ctrl-pub-secret branch from 6521610 to 0c1960f Compare September 3, 2025 14:33

black-dragon74 marked this pull request as ready for review September 3, 2025 14:33

Rakshith-R requested a review from nixpanic September 3, 2025 14:54

black-dragon74 force-pushed the cache-ctrl-pub-secret branch 2 times, most recently from c9aa5ae to c1d7017 Compare September 4, 2025 09:43

nixpanic reviewed Sep 4, 2025

View reviewed changes

black-dragon74 force-pushed the cache-ctrl-pub-secret branch from c1d7017 to 915b8b7 Compare September 4, 2025 12:01

nixpanic reviewed Sep 4, 2025

View reviewed changes

Comment thread internal/util/k8s/secrets.go Outdated

black-dragon74 force-pushed the cache-ctrl-pub-secret branch from 915b8b7 to 6d7b981 Compare September 5, 2025 08:28

Rakshith-R requested a review from nixpanic September 8, 2025 07:36

nixpanic reviewed Sep 9, 2025

View reviewed changes

black-dragon74 force-pushed the cache-ctrl-pub-secret branch from 6d7b981 to f8cb6b4 Compare September 9, 2025 11:00

black-dragon74 requested review from Rakshith-R and nixpanic September 9, 2025 11:01

nixpanic reviewed Sep 9, 2025

View reviewed changes

black-dragon74 force-pushed the cache-ctrl-pub-secret branch from f8cb6b4 to d874ea9 Compare September 11, 2025 13:53

Rakshith-R approved these changes Sep 12, 2025

View reviewed changes

Rakshith-R requested a review from iPraveenParihar September 12, 2025 07:31

util: cache the controller publish secret

05f8a41

This patch introduces a caching mechanism based on the shared informers for ControllerPublish secrets. Signed-off-by: Niraj Yadav <[email protected]>

ceph-csi-bot force-pushed the cache-ctrl-pub-secret branch from d874ea9 to 05f8a41 Compare September 12, 2025 07:32

iPraveenParihar approved these changes Sep 12, 2025

View reviewed changes

iPraveenParihar added the ok-to-test Label to trigger E2E tests label Sep 12, 2025

ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Sep 12, 2025

mergify Bot merged commit 8c80d56 into ceph:devel Sep 15, 2025
37 of 38 checks passed

This was referenced Sep 26, 2025

deploy: Add watch permission for Secrets to CephFS ctrlplugin ceph/ceph-csi-operator#324

Merged

DFBUGS-4259: [release-4.20] deploy: Add watch permission for Secrets to CephFS ctrlplugin red-hat-storage/ceph-csi-operator#149

Merged

black-dragon74 mentioned this pull request Nov 26, 2025

Add list and watch RBACs for secrets to nodeplugin clusterroles ceph/ceph-csi-operator#363

Merged

Conversation

black-dragon74 commented Aug 13, 2025

Describe what this PR does

Is there anything that requires special attention

Uh oh!

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic commented Sep 4, 2025

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

black-dragon74 Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

Rakshith-R commented Sep 12, 2025

Uh oh!

mergify Bot commented Sep 12, 2025

✅ Branch has been successfully rebased

Uh oh!

iPraveenParihar left a comment

Choose a reason for hiding this comment

Uh oh!

iPraveenParihar commented Sep 12, 2025

Uh oh!

iPraveenParihar commented Sep 12, 2025

Uh oh!

mergify Bot commented Sep 12, 2025

☑️ Nothing to do, the required conditions are not met

Uh oh!

iPraveenParihar commented Sep 12, 2025

Uh oh!

mergify Bot commented Sep 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 The pull request has been removed from the queue default

Uh oh!

mergify Bot commented Sep 12, 2025

Uh oh!

iPraveenParihar commented Sep 12, 2025

Uh oh!

mergify Bot commented Sep 12, 2025

🛑 The pull request has been removed from the queue default

Uh oh!

black-dragon74 Sep 11, 2025 •

edited

Loading

mergify Bot commented Sep 12, 2025 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

🛑 The pull request has been removed from the queue `default`

🛑 The pull request has been removed from the queue `default`