Refinements to SSA matching by shawkins · Pull Request #2028 · operator-framework/java-operator-sdk

shawkins · 2023-08-23T17:57:19Z

This highlights the state observed with kubernetes/kubernetes#118519

Note that the desired Secret uses stringData and the actual state after SSA references stringData as managed, but the resource is actually using data instead.

The matcher will currently throw an NPE when this occurs, so there's a minor change to allow it to continue - but it still of course sees this as a difference.

We know this occurs with Ingress as well - an empty path was being used in Keycloak because that is recommended in the openshift documentation when using the ImplementationSpecific pathType. If it's left in the spec, it gets pruned and that causes the matching logic to think there has been a change. However it does not seem necessary to use an empty path and so we removed that.

Some thoughts on how to handle this:

It is likely an api server bug, but it doesn't look like it will be resolved any time soon.
This situation, or anything involving pruning / modification of the apiServer state vs what's in the managed fields is problematic, and it won't be understood by the generic matcher either.
You could consider some type specific canonicalization logic (Secret conversion of stringData to data for example) - but automatically handling empty strings will likely be more problematic.
recommend or automatically install something like the MetadataAwareOnUpdateFilter when SSA is being used. Even if the update is issued this will filter out the update event because there will be no real changes in the new vs the old cache state. Of course this logic is extremely close to what's in the generic update matcher (just that status would be meaningful also) - so there is potential for reuse.

csviri · 2023-08-25T08:48:24Z

The matcher will currently throw an NPE when this occurs, so there's a minor change to allow it to continue - but it still of course sees this as a difference.

Not sure if there is a good solution for this, except to handle this as a specific, case, and provide an implementation accordingly.

csviri · 2023-09-06T10:36:18Z

I think this needs a rebase too :)

shawkins · 2023-09-13T12:40:46Z

@csviri In double checking the statefulset and ingress issues that were highlighted previous in kubernetes issues, they do seem to be resolved with newer kubernetes versions wrt creating additional resource versions (they will still have match issues though).

I've refined this pr to do a couple of things:

localizes sanitation to ssa matcher. If possible it doesn't seem like the josdk should be explicitly setting fields in the spec that aren't part of the users desired state.
further ensures values won't be logged
will detect if a non-meaningful revision has been created

csviri

Looks good to me, thank you!
except the missing tests. Could you add at least some unit test for the filter.

(We can think also some additional integration tests and feature flags, but that can be a separate PR

csviri · 2023-09-14T08:54:34Z

+ * <p>
+ * Several of these circumstances seem to have been addressed by ~1.25+
+ */
+public class MetadataAwareOnUpdateFilter<T extends HasMetadata> implements OnUpdateFilter<T> {


Actually would be nice to have an IT for this. That shows the problem, and it is solved by this.

I've retested the issue seen with Secrets, Ingress, and StatefulSets on 1.26.3 - they no longer exhibit the behavior of creating an additional version. The upstream statefulset issue mentions a fix in 1.25. So this is really a fix to ensure there's no regressions or to support older Kube versions with SSA. Alternatively we could just document this rather than introducing this class.

Secrets and Ingress (with empty strings) though will still have matching issues, but this is a lesser problem as we're already excluding secrets from SSA and without the additional revision there won't be the loop behavior.

Ahh, in that case, yes, it might be just enough to document it IMO.

But I guess the move of the sanitization still makes sense.

Ok, narrowed this PR to not include the detection of meaningless additional revisions.

thx, fine to merge this from my point

metacosm · 2023-09-14T12:59:16Z

-  }
-
  protected boolean useSSA(Context<P> context) {
+    if (this instanceof ResourceUpdaterMatcher) {


Could we add a comment there as to why we deactivate SSA in that situation, please?

Tbh not sure that ResourceUpdateMatcher abstraction makes that much anymore. But we can discuss in an other PR

metacosm · 2023-09-14T14:09:54Z

@shawkins you need to sign your commits off with -s (or equivalent) now
A quick way to do so for all your commits is to do a git rebase --signoff HEAD~N and then force push.

shawkins · 2023-09-14T14:15:29Z

@shawkins you need to sign your commits off with -s (or equivalent) now

Yes, I've seen that. For some reason that box won't stay checked by default in eclipse.

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins · 2023-09-21T13:28:29Z

@metacosm @csviri rebased

csviri · 2023-09-22T07:25:00Z

after rebase, we can merge also this imo

shawkins · 2023-09-22T11:07:31Z

after rebase, we can merge also this imo

Again? Is next being rebased against main frequently? Ah, this was due to the other pr being merged, not a general rebase. I'll update things.

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steve Hawkins <shawkins@redhat.com>

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steven Hawkins <shawkins@redhat.com>

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2023

shawkins mentioned this pull request Aug 24, 2023

NullPointerException in SSABasedGenericKubernetesResourceMatcher #2032

Closed

csviri reviewed Aug 25, 2023

View reviewed changes

Comment thread ...io/javaoperatorsdk/operator/processing/dependent/kubernetes/MetadataAwareOnUpdateFilter.java Resolved

csviri force-pushed the next branch from 5280d26 to 02f843c Compare August 29, 2023 08:25

csviri mentioned this pull request Aug 29, 2023

Issues with SSA Matching and Approach to Handle Them #2038

Closed

csviri force-pushed the next branch 4 times, most recently from 2bdd8f9 to e9c2639 Compare September 4, 2023 08:40

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2023

csviri force-pushed the next branch from 39bc7af to a3e2c24 Compare September 11, 2023 08:27

shawkins mentioned this pull request Sep 11, 2023

feat: sanitization of resources for matching #2042

Merged

csviri force-pushed the next branch from a3e2c24 to 4ec9a19 Compare September 12, 2023 06:51

shawkins force-pushed the iss2000 branch from 0d8433b to 0d3c484 Compare September 13, 2023 12:38

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2023

shawkins changed the title ~~shows what is happening with secrets that use stringdata and ssa~~ generally prevent reconciliation loops with ssa Sep 13, 2023

shawkins marked this pull request as ready for review September 13, 2023 12:38

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2023

openshift-ci Bot requested review from adam-sandor and csviri September 13, 2023 12:38

shawkins force-pushed the iss2000 branch from 0d3c484 to 528410e Compare September 13, 2023 12:39

shawkins force-pushed the iss2000 branch from 528410e to 2c23172 Compare September 13, 2023 13:33

csviri requested changes Sep 14, 2023

View reviewed changes

csviri reviewed Sep 14, 2023

View reviewed changes

Comment thread ...io/javaoperatorsdk/operator/processing/dependent/kubernetes/MetadataAwareOnUpdateFilter.java Resolved

csviri reviewed Sep 14, 2023

View reviewed changes

Comment thread ...io/javaoperatorsdk/operator/processing/dependent/kubernetes/MetadataAwareOnUpdateFilter.java Resolved

metacosm reviewed Sep 14, 2023

View reviewed changes

Comment thread ...io/javaoperatorsdk/operator/processing/dependent/kubernetes/MetadataAwareOnUpdateFilter.java Resolved

csviri reviewed Sep 14, 2023

View reviewed changes

shawkins force-pushed the iss2000 branch from 49712fc to 271dae3 Compare September 14, 2023 11:46

csviri requested a review from metacosm September 14, 2023 12:29

metacosm requested changes Sep 14, 2023

View reviewed changes

shawkins requested a review from metacosm September 14, 2023 13:22

metacosm reviewed Sep 14, 2023

View reviewed changes

Comment thread ...io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java Resolved

shawkins force-pushed the iss2000 branch from d376e56 to ecbc145 Compare September 14, 2023 14:18

shawkins requested review from csviri and metacosm September 14, 2023 14:18

csviri approved these changes Sep 14, 2023

View reviewed changes

metacosm approved these changes Sep 14, 2023

View reviewed changes

metacosm force-pushed the next branch from e38e279 to 5047ca0 Compare September 15, 2023 18:04

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2023

csviri force-pushed the next branch 2 times, most recently from 6d0fb7f to 5766e89 Compare September 18, 2023 13:55

various refinements to ssa logic

37ee351

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins force-pushed the iss2000 branch from ecbc145 to 37ee351 Compare September 21, 2023 13:27

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2023

csviri approved these changes Sep 21, 2023

View reviewed changes

Merge branch 'next' into iss2000

a2fa5da

csviri merged commit 2c50a83 into operator-framework:next Sep 22, 2023

csviri pushed a commit that referenced this pull request Oct 3, 2023

various refinements to ssa logic (#2028)

b5c829a

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steve Hawkins <shawkins@redhat.com>

shawkins added a commit that referenced this pull request Oct 4, 2023

various refinements to ssa logic (#2028)

c225f71

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steven Hawkins <shawkins@redhat.com>

csviri pushed a commit that referenced this pull request Oct 4, 2023

various refinements to ssa logic (#2028)

d45328f

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steven Hawkins <shawkins@redhat.com>

csviri pushed a commit that referenced this pull request Oct 18, 2023

various refinements to ssa logic (#2028)

ed7e13d

- localizes sanitation to ssa matcher - further ensures values won't be logged Signed-off-by: Steven Hawkins <shawkins@redhat.com>

Conversation

shawkins commented Aug 23, 2023

Uh oh!

csviri commented Aug 25, 2023

Uh oh!

Uh oh!

csviri commented Sep 6, 2023

Uh oh!

shawkins commented Sep 13, 2023

Uh oh!

csviri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacosm commented Sep 14, 2023

Uh oh!

Uh oh!

shawkins commented Sep 14, 2023

Uh oh!

shawkins commented Sep 21, 2023

Uh oh!

csviri commented Sep 22, 2023

Uh oh!

shawkins commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shawkins commented Sep 22, 2023 •

edited

Loading