[multi-container] toggling the default enable-multi-container from true to false will break things

[dprotaso]

What version of Knative?

enable-multi-container flag was introduced in this PR #7167

Expected Behavior

Operators who set enable-multi-container default to true shouldn't be able to set it back to false or even delete the entry in the config map

Actual Behavior

There are no restrictions to how enable-multi-container can be set.

Thus if users create multi-container revisions and then the operator sets the default to false reconciliation of those resources will start to constantly fail.

Steps to Reproduce the Problem

[markusthoemmes]

This might be somewhat non-trivial as our current validation logic for ConfigMaps only gets the state of the update itself, not the "before" state, thus (I think?) it cannot decide whether you're actually toggling or not. As such, this might require significant changes to how we validate ConfigMaps in our webhook. Does that sound right @dprotaso?

[dprotaso]

Yeah I was thinking about that as well

[markusthoemmes]

We might be lucky because the configmap.Constructors type is map[string]interface{} and the functions are called by reflection already. Maybe we can make it so that it can take both unary and 2-arity functions and depending on which it gets, it passes the correct parameters in?

We could then create a ValidateABC function for the features configmap that we pass into the webhook here

serving/cmd/webhook/main.go

Line 154 in 5ce203e

configmap.Constructors{

. That function takes both the old and the new CM, does validation and maybe returns the new CM or an error.

Does that sound just about feasible? I'd take this issue to mentor a new contributor in :).

[vagababov]

I don't see this working though. I can just delete the CM. Or you're going to block deletion of CM as well?

[skonto]

@markusthoemmes regarding update If I read correctly this new enhancement by K8s (immutable configmaps) might be useful so configuration could be split to immutable and mutable parts. @vagababov It will probably cover deletion too.

[dprotaso]

@markusthoemmes regarding update If I read correctly this new enhancement by K8s (immutable configmaps) might be useful so configuration could be split to immutable and mutable parts. @vagababov It will probably cover deletion too.

Our current min K8s version is 1.16 so it'll be sometime before we can adopt that feature

[vagababov]

Also we don't want CMs to be immutable. But operators might, i.e. mark the CMs as immutable in the operator deployment.

[dprotaso]

I also realize this issue applies to other features - ie. enabling and disabling fieldref.

Maybe the solution for now is operator awareness - maybe via comments in the features configmap

[skonto]

Our current min K8s version is 1.16 so it'll be sometime before we can adopt that feature.

True it will take a few months, but it seems it will go to Beta.

Also we don't want CMs to be immutable. But operators might.

If there are settings not meant to be changed once set, then they can be put in a special immutable configmap. This way it would be clear what can or cannot be changed at runtime IMHO.
An alternative would be to keep critical config replicated at the Knative CR object (eg. in status) or at the operator CR object (if operators are used) since configuration is part of the state of your system anyway. This way you know your past state and you could reject new updates. Of course CR objects can be updated so you dont get much protection but if they are deleted it means your system needs to be reconciled anyway.

[markusthoemmes]

Seems to me as adding a cautionary warning to the configmap setting is sufficient for now?

[vagababov]

Warning is fine.

---

If there are settings not meant to be changed once set...

Not necessarily true, settings might be set by mistake. I might be sure that disabling the setting is going to be safe, etc.

[skonto]

@markusthoemmes afaik the setting has been removed from the defaults.yaml file in #8315, is there some other place to add the warning (or do you mean emit a log msg)?

[markusthoemmes]

features.yaml has it now I believe.

[markusthoemmes]

/assign