feat: sanitization of resources for matching

[csviri]

No description provided.

[csviri]

@metacosm @shawkins this is how we could fill data that is making mess now with SSA matching ( preprocess desired ). Pls let me know what do you think, we can discuss if we want to go this way.
Also make some checks on known issues

Also this is something that we would be able to turn off by feature flag.

[shawkins]

I'd still advocate for something like that update filter which will check to see if old and new are the same to help prevent reconciliation loops - with logging it could also help to capture additional cases that would need to be addressed by something like this pr.

Another consideration is making the use of SSA type based - in particular for Secrets. Upstream they certainly didn't feel there was much need to use SSA for every kind.

[metacosm]

Design-wise, this shouldn't be a static call, but rather a per-type instance so that we use polymorphism instead of having a big if-else cascade based on type with a default no-op being used in most cases.

[csviri]

I would not complicate that for now, until this is small, we can refactor when we have more of it.

[csviri]

I'd still advocate for something like that update filter which will check to see if old and new are the same to help prevent reconciliation loops - with logging it could also help to capture additional cases that would need to be addressed by something like this pr.

yes, this is also something that we should consider, but also these are not necessary mutual exclusive, or? I mean the reconciliation will be triggered by other sources and then matching will fail.

Another consideration is making the use of SSA type based - in particular for Secrets. Upstream they certainly didn't feel there was much need to use SSA for every kind.

This is a good question/point, how should we approach that case. If we don't really advise to do SSA for certain resource type. Should be this something pre-configured? or just documented?

[shawkins]

yes, this is also something that we should consider, but also these are not necessary mutual exclusive, or? I mean the reconciliation will be triggered by other sources and then matching will fail.

I'm not saying they are exclusive - just want to make sure it's included in comprehensively addressing the problems.

There is the problem behavior of performing a SSA when one is not needed. There are two possible consequences:

• nothing happens on the server side, which the kubernetes folks say should have about the same performance as a get call. So avoiding this is good, but it will really only cause users problem in situations where there's a lot of api server load.
• a new revision is created (that's the behavior that's been noted with at least Secrets, Ingress). This is dangerous as it can cause the operator sdk to simply keep reconciling.

[metacosm]

To me, all this is sounding like SSA is not really ready for mainstream usage. It's too fraught with subtleties and edge cases that can get you in trouble.

[csviri]

I'm not saying they are exclusive - just want to make sure it's included in comprehensively addressing the problems.

Yes, I think there is a separate PR for that, I agree with that part, only we have to take a look there to automatically add such filter and put it behind a feature flag.

[shawkins]

This is a good question/point, how should we approach that case. If we don't really advise to do SSA for certain resource type. Should be this something pre-configured? or just documented?

I'd opt for pre-configured in the case of Secrets rather than defaulting to an error condition if stringData is used. Out of an abundance of caution, ConfigMaps should be treated similarly.

To me, all this is sounding like SSA is not really ready for mainstream usage. It's too fraught with subtleties and edge cases that can get you in trouble.

Right and the feedback upstream is just to not use it when it's not working as expected.

An alternative is to change SSA to opt-in per DR rather than opt-out.

[csviri]

I'd opt for pre-configured in the case of Secrets rather than defaulting to an error condition if stringData is used. Out of an abundance of caution, ConfigMaps should be treated similarly.

I think those are two different things again. I agree that we could say that Secrets and config maps by default should not use SSA. But if we introduce a feature flag, someone could turn it on and still use stringData, than we can still throw the exception, but if some does not use the string data is should work as expected.

In other words I prefer such documentation as a code, if it does not bring too much complexity.

[shawkins]

I think those are two different things again.

Not entirely. In practical terms, what is the advantage of using SSA with a Secret? I'd argue that it's very minimal.

[csviri]

I think those are two different things again.

Not entirely. In practical terms, what is the advantage of using SSA with a Secret? I'd argue that it's very minimal.

I agree, although I can imaging that multiple controllers fill some values in a secret or config map, independently.

So basically I would say have it there both, so don't use ssa by default for secret and configmap, but if somebody turns it on, we can still throw the exception if it uses stringdata.

or do you think this is an overkill approach?
I see dependent resources is the high level approach where, the framework handles such common mistakes instead of user, so helps them also understand the issues, and by default works as it is intended. (Although what is intended might be sometimes bury in k8s)

[shawkins]

I agree, although I can imaging that multiple controllers fill some values in a secret or config map, independently.

They can, but the existing merging strategies are pretty straight-forward for doing that.

[csviri]

I agree, although I can imaging that multiple controllers fill some values in a secret or config map, independently.

They can, but the existing merging strategies are pretty straight-forward for doing that.

So what are the alternatives, shouldn't we allow to use CM/Secret with SSA?

Or just set it by default non-SSA, and that is it?

[shawkins]

Or just set it by default non-SSA, and that is it?

I'm fine with the operator framework being opinionated - whatever is likely to be the best / simplest solution should be the default. For Secrets that would be to not use SSA. That way noone will be surprised when migrating from an earlier JOSDK version that they need to change their logic away from using stringData or have to turn off SSA.

[csviri]

@metacosm @shawkins I added additional config that will opt out by default Secret and ConfigMap. The additional checks still there. So for now taking the "cover every aspect" approach. Thus check everything that we can, since this does not increase complexity that much. Most of these checks are trivial to read and understand.

[shawkins]

LGTM, just one nit. Do you want me to refine the #2028 into something that will automatically detect/prevent reconciliation loops with SSA?