Uh oh!
There was an error while loading. Please reload this page.
Pass label selector predicate to the Secret Watch and handler - #443
Conversation
| var preds []predicate.Predicate | ||
| if r.selectorPredicate != nil { | ||
| preds = append(preds, r.selectorPredicate) | ||
| var secretPreds []predicate.TypedPredicate[*corev1.Secret] |
There was a problem hiding this comment.
I get that you are harmonizing with the existing code, but do we need to use a slice at all here (for either preds or secretPreds)?
There was a problem hiding this comment.
Ah, got it. It's to handle the nil case...
| obj, | ||
| handler.OnlyControllerOwner(), | ||
| ), | ||
| secretPreds..., |
There was a problem hiding this comment.
could we get away with fewer changes if we used client.Object in the watch, like e.g.:
if err := c.Watch(
source.Kind(
mgr.GetCache(),
client.Object(secret),
handler.TypedEnqueueRequestForOwner[client.Object](
mgr.GetScheme(),
mgr.GetRESTMapper(),
obj,
handler.OnlyControllerOwner(),
),
preds...,
),
); err != nil {
return err
}
then we don't need to duplicate the predicate slice
There was a problem hiding this comment.
You are right, this is better
| }) | ||
| }) | ||
| }) | ||
| When("label selector set", func() { |
perdasilva
left a comment
There was a problem hiding this comment.
lgtm - just added a small suggestion that could obviate the need to refactor the option because of the requirement for a TypedPredicate. I don't know if there could be side-effects though.
Head branch was pushed to by a user without write access
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #443 +/- ##
==========================================
- Coverage 85.06% 78.49% -6.58%
==========================================
Files 19 31 +12 Lines 1346 2520 +1174 ==========================================
+ Hits 1145 1978 +833 - Misses 125 452 +327 - Partials 76 90 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| if r.selectorPredicate != nil { | ||
| preds = append(preds, r.selectorPredicate) | ||
| if r.labelSelector.MatchLabels != nil { |
There was a problem hiding this comment.
Hm, so AFAICT WithSelector accepts a selector with match expressions (and no match labels), but then this check causes these expressions to be ignored? 🤔
This would be an unwelcome surprise for the user, no?
There was a problem hiding this comment.
That's a good catch!
I think it depends on how operator handles selectors. If operator uses built-in ParseToLabelSelector from k8s like in stackrox then this line will not be ignored and behaviour should be correct. But if some operator which uses this plugin creates selector manually with providing MatchExpressions only then there will be still a bug for multiple version for the same operator.
I also tested fir PR locally and my setup had such selector generation and it worked as expected:
...
selector := "app=" + appLabelValue
labelSelector, err := metav1.ParseToLabelSelector(selector)
...
There was a problem hiding this comment.
Well, it depends on what you pass to ParseToLabelSelector. If you use any other operator than = or == then AFAICT MatchLabels will be empty and MatchExpressions will not.
I think the best way would be to change the if to do the emptiness check differently...
What
Updated the
setupWatchesmethod to passpredicateiflabelSelectoris set to the reconciler.Why
If run two reconcilers with the same GVK but different labelSelectors then both reconcilers reconcile each other RCs ignoring configured LabelSelectors. This could be considered as not expected behavior if running two or more reconcilers with the same GVK but different labelSelectors is covered scenario. This PR fixes this issue.
Example logs of two reconcilers with the same GVK but different label selector.
There is only one CR with label
app=nginx.Reconciler1 has
app=nginxlabel selectorReconciler2 has
app=foolabel selectorReconciler1:
Reconciler2:
Manual testing
This small setup was used for running locally two reconcilers:
Running locally with the fix shows that only one reconciler reconcile the CR and another
Reconciler1:
Reconciler2: