"breaking change" by expanding allowed values

[Tim Hockin]

Hi all,

Context: https://github.com/kubernetes/kubernetes/pull/74526/files

We certainly consider making validation more restrictive to be a
breaking change. Strictly speaking, we should also consider expanding
validation to be a breaking change -- there could be validators or
tools out there that embed our documented validation rules.

How do we want to handle these changes? In the above PR we have a
relatively simple and straight-forward expansion -- allowing 0 where
it used to not be allowed. Should we block such changes and require a
new field? Or do we accept that this sort of breaking change is
allowed? If so, do we have boundaries for such expansion?

Looking forward to a lively and pedantic discussion.

Tim

[Brian Grant]

Using a new field isn't possible. The same information can't be represented in 2 different fields, and also minReplicas defaults to 1.

This technically can't be solved without a new resource type, as we did when introducing ReplicaSet to supersede ReplicationController, but that would cause significant user pain.

As much as I dislike case-by-case decisions on issues like this, I think we should allow the change. Zero replicas is valid for workload controllers.

I think this change is unlikely to break clients, but it's worth some outreach to ask. We're doing similarly with "kubectl convert" and "kubectl get --export".

Some day, I still would like to expose validation information via OpenAPI, which would enable clients to not hardcode such constraints.

--
You received this message because you are subscribed to the Google Groups "kubernetes-api-reviewers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-api-rev...@googlegroups.com.
To post to this group, send email to kubernetes-a...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-api-reviewers/CAO_RewZS6f39PZvgRoyBAEmx%2BormjEMXXN-iYX-pyAhzVs1OVg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

[Daniel Smith]

I agree with Brian, it looks like the validation rules were not even documented previously, it's very hard for me to get pedantically excited about this breaking someone. To make that happen I think you'll need to come up with an example that's meant for programmatic consumption, e.g. something in status.

On Wed, Apr 17, 2019 at 8:32 AM 'Brian Grant' via kubernetes-api-reviewers <kubernetes-a...@googlegroups.com> wrote:

Using a new field isn't possible. The same information can't be represented in 2 different fields, and also minReplicas defaults to 1.

This technically can't be solved without a new resource type, as we did when introducing ReplicaSet to supersede ReplicationController, but that would cause significant user pain.

As much as I dislike case-by-case decisions on issues like this, I think we should allow the change. Zero replicas is valid for workload controllers.

I think this change is unlikely to break clients, but it's worth some outreach to ask. We're doing similarly with "kubectl convert" and "kubectl get --export".

Some day, I still would like to expose validation information via OpenAPI, which would enable clients to not hardcode such constraints.

It's likely to happen for CRDs first. It won't be a panacea, e.g. clients don't and won't ever syncronously get the discovery doc, validate, and write their changes--there will be caching and delays, etc. But at least they should be able to fix without recompiling.

 

On Tue, Apr 16, 2019 at 4:09 PM 'Tim Hockin' via kubernetes-api-reviewers <kubernetes-a...@googlegroups.com> wrote:

Hi all,

Context: https://github.com/kubernetes/kubernetes/pull/74526/files

We certainly consider making validation more restrictive to be a
breaking change.  Strictly speaking, we should also consider expanding
validation to be a breaking change -- there could be validators or
tools out there that embed our documented validation rules.

How do we want to handle these changes?  In the above PR we have a
relatively simple and straight-forward expansion -- allowing 0 where
it used to not be allowed.  Should we block such changes and require a
new field?  Or do we accept that this sort of breaking change is
allowed?  If so, do we have boundaries for such expansion?

Looking forward to a lively and pedantic discussion.

Tim

--
You received this message because you are subscribed to the Google Groups "kubernetes-api-reviewers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-api-rev...@googlegroups.com.
To post to this group, send email to kubernetes-a...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-api-reviewers/CAO_RewZS6f39PZvgRoyBAEmx%2BormjEMXXN-iYX-pyAhzVs1OVg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "kubernetes-api-reviewers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-api-rev...@googlegroups.com.
To post to this group, send email to kubernetes-a...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-api-reviewers/CAKCBhs4ApjE16k4uJM5EXKnM_JB1B8DFeFGPXD%2Bgpk9jTe9J1Q%40mail.gmail.com.

[Jordan Liggitt]

Agree a second field doesn't really help (and actually makes things worse if it changes the semantic meaning of a field existing clients look at and think they understand).

Expanding to allow 0 where it wasn't previously allowed isn't concerning to me. The types of value expansion that require more thought are ones where the new values could reasonably lead to unsafe use (e.g. envvar character expansion to include ':' or configmap key expansion to include '/')

Practically, whenever we want to loosen validation, we need to do so over two releases so that the API server in various scenarios (like HA API clusters) doesn't choke on persisted data (described here).

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-api-reviewers/CAB_J3bbhc-g%2B4uF5J1iRWHSB%3DruuHuFic7kRFqbNz4AVZJBgPg%40mail.gmail.com.

[Brian Grant]

On Wed, Apr 17, 2019 at 8:57 AM Jordan Liggitt <lig...@google.com> wrote:

Agree a second field doesn't really help (and actually makes things worse if it changes the semantic meaning of a field existing clients look at and think they understand).

Expanding to allow 0 where it wasn't previously allowed isn't concerning to me. The types of value expansion that require more thought are ones where the new values could reasonably lead to unsafe use (e.g. envvar character expansion to include ':' or configmap key expansion to include '/')

Practically, whenever we want to loosen validation, we need to do so over two releases so that the API server in various scenarios (like HA API clusters) doesn't choke on persisted data (described here).

Excellent point. We definitely need to document that.

Another thing to consider eventually would be to render resource instances failing validation on read from storage inert, in the sense discussed here:

https://github.com/kubernetes/kubernetes/issues/170#issuecomment-73318883 

https://github.com/kubernetes/kubernetes/issues/3585#issuecomment-70348967

That could potentially improve availability and time to repair/recovery in the event that a breaking change slipped through.

[Clayton Coleman]

So the rules for widening validation are:

1. you shouldn't do it UNLESS

2. you can make the case that widening it fits the use case and passes some arbitrary bar set by value / utility trade off AND

3. you can convince people that it won't break most clients AND

4. it doesn't have security implications

?

To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-api-reviewers/CAKCBhs5whCmdK9rfnPRMUKYRVpcHwnq%3DgN%3D%3DYgq5HUd2O%3DPMJg%40mail.gmail.com.

[Brian Grant]

On Wed, Apr 17, 2019 at 9:50 AM Clayton Coleman <ccol...@redhat.com> wrote:

So the rules for widening validation are:

1. you shouldn't do it UNLESS

Yes. :-( :-( :-(

I'm not sure I have a better answer. Sometimes we paint ourselves into a corner.

[Clayton Coleman]

I'm not terribly opposed, just trying to tease apart the bits we could come up with heuristics on.  I totally agree perfect enemy of the good.

I know user interfaces read and display HPA objects.  What are the impacts to those?  Would a dialog that shows 1-X or unset break dangerously if it was 0-X?

[Tim Hockin]

...and if you meet these criteria, you must stage the rollout over 2 releases

[Tim Hockin]

Thanks all, I will translate this into the PR

[Tim Hockin]

Any volunteers to crystallize this into conventions docs?

[Jordan Liggitt]

I'll open a PR

[Brian Grant]

By "1-X" did you mean minus or "to"?

Probably the latter?

Authoring tools that validate minReplicas might not allow 0 until they get updated.

One risk could be for interfaces displaying the values in resources created by arbitrary means. Even a simple if statement to check whether "replicas" should be singular or plural could barf if not assuming zero. I'd hope such tools would reuse the same code as for controllers, in which case they'd have to support zero already. Or just special-case the singular case only.

[Clayton Coleman]

On Apr 17, 2019, at 11:57 PM, Brian Grant <brian...@google.com> wrote:

By "1-X" did you mean minus or "to"?

Probably the latter?

The range

Authoring tools that validate minReplicas might not allow 0 until they get updated.

Is there the possibility of introducing a security hole or crash in those user interfaces because they might be doing divide by replicas if set (always non-nil)?

Early autoscaler UIs had to do crazy things in order to show effective values - I wouldn’t be surprised if people have math calculations based on replicas and assumed we wouldn’t change the range (or more likely, didn’t think about it).

[Tim Hockin]

While we're on this topic, maybe we can craft stronger guidance around
validations and how to use them, such that in the future we have more
wiggle room for changes such as this. E.g. validating user input is
not the same as validating data read from the apiserver. I'm just
hand-waving -- I have put no thought into this. It just seems like
this problem should be avoidable by carfully crafted loopholes. Or
not..