Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CRS associated secret updates will not trigger CRS Reconcile #10557

Open
Levi080513 opened this issue May 6, 2024 · 8 comments · May be fixed by #10633
Open

CRS associated secret updates will not trigger CRS Reconcile #10557

Levi080513 opened this issue May 6, 2024 · 8 comments · May be fixed by #10633
Assignees
@sbueringer
Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.

Comments

@Levi080513
Copy link
Contributor

What steps did you take and what happened?

  1. Update the secret associated with CRS.
  2. CRS Controller will not trigger reconcile.

What did you expect to happen?

CRS Controller triggers Reconcile immediately.

Cluster API version

1.6.4

Kubernetes version

1.25.15

Anything else you would like to add?

I guess the problem may lie here:

cluster-api/main.go

Lines 306 to 317 in 6d400b5

Cache: cache.Options{
DefaultNamespaces: watchNamespaces,
SyncPeriod: &syncPeriod,
ByObject: map[client.Object]cache.ByObject{
// Note: Only Secrets with the cluster name label are cached.
// The default client of the manager won't use the cache for secrets at all (see Client.Cache.DisableFor).
// The cached secrets will only be used by the secretCachingClient we create below.
&corev1.Secret{}: {
Label: clusterSecretCacheSelector,
},
},
},

The secret CR cache filtered cluster.x-k8s.io/cluster-name label, This configuration is passed through the method manager.New => cluster.New => cache.New => cache.newCache => internal.NewInformers and finally takes effect the Informers object for secret CR.
When the ClusterResourceSetReconciler.SetupWithManager function is executed, we observe the secret CR.
And when the controller starts, it will execute the function manager.Cache.GetInformer => delegatingByGVKCache.GetInformer => informerCache.GetInformer => Informers.Get => Informers.addInformerToMap => Informers.newInformer Gets the informer object. Since the filtering configuration takes effect on the secret CR Informers Object, the final generated Informers Object will filter out all object events without the cluster.x-k8s.io/cluster-name" label.

https://github.com/kubernetes-sigs/controller-runtime/blob/dca5e8b2b00a1ab10a617f6c4a6fd0d40f9a974e/pkg/cache/internal/informers.go#L296-L354

And i test when the secret configured cluster.x-k8s.io/cluster-name label, if the secret updated, CRS Controller triggers Reconcile immediately.

Label(s) to be applied

/kind bug
One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels.
@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 6, 2024
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

CAPI contributors will take a look as soon as possible, apply one of the triage/* labels and provide further guidance.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@sbueringer
Copy link
Member

Hm. I would have assumed that PartialObjectMeta is handled separately

@Levi080513
Copy link
Contributor Author

Hm. I would have assumed that PartialObjectMeta is handled separately

But the selector filter is effective. Is there something wrong with my understanding?
https://github.com/kubernetes-sigs/controller-runtime/blob/dca5e8b2b00a1ab10a617f6c4a6fd0d40f9a974e/pkg/cache/internal/informers.go#L308-L324

@sbueringer
Copy link
Member

No probably not. I just meant that was my initial assumption. I'll take a closer look soon

/assign

@sbueringer
Copy link
Member

(@chrischdi Ah just remembered that we talked about this issue yesterday, let me know if you want to take it over)

@fabriziopandini fabriziopandini added the priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. label May 8, 2024
@jessehu
Copy link
Contributor

jessehu commented May 14, 2024

when the secret configured cluster.x-k8s.io/cluster-name label, if the secret updated, CRS Controller triggers Reconcile immediately.

Since CRS uses the cluster-name label selector, would it be reasonable to require the secrets used by CRS to have the same cluster-name label?

  clusterSelector:
    matchLabels:
      cluster.x-k8s.io/cluster-name:

@sbueringer
Copy link
Member

It would work as a workaround, but seems not ideal because crs applies to multiple clusters

@chrischdi chrischdi linked a pull request May 16, 2024 that will close this issue
Open
@chrischdi
Copy link
Member

I created a PR which fixes this. To note: there had been other options too:

current state:

  • in main.go: we configure the LabelSelector which configures the cache
    • this LabelSelector gets propagated to the informer's list/watch functions and is to filter the on server-side (APIServer)
    • this filters out all secrets which don't match the LabelSelector (which was done to optimize resource usage and because we don't want to have all secrets of the management cluster in our cache)
    • this config (LabelSelector) gets applied to the structured, unstructured and partialObjectMeta watches of for the type, so also to the WatchesMetadata for Secrets in the controller for ClusterResourceSets.

Fix:

  1. (as implemented in 🐛 crs: use separate cache for partial metadata watches on secrets to include all secrets #10633): Use a custom cache when doing WatchesMetadata for secrets in the controller
  • by that we don't inherit the above described LabelSelector. This is okay because we only watch for PartialObjectMetadata here anyway.
  1. Drop the LabelSelector and use a predicate to filter instead + transform function to drop information
  • this is not optimal because dropping the label selector would get effective for all Watches on Secrets (not only the one used for ClusterResourceSets).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbueringer sbueringer

Labels
kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.
Projects
None yet
Milestone
No milestone
6 participants
@jessehu @chrischdi @sbueringer @fabriziopandini @k8s-ci-robot @Levi080513