Do not allow extra fields when validating CRDs

[bobcatfish]

Expected Behavior

If I typo a field in an optional field in a CRD (i.e. provide an extra field), the validation webhook (or worst case, runtime check) should tell me the CRD is invalid.

Actual Behavior

"Extra" fields are ignored, for example in the demo pipeline, there was a bug where one of the instances of inputSourceBindings was not renamed to resources:

https://github.com/knative/build-pipeline/blob/c9341e7f0e5dae53a4c5b1638401b4d65401f1b3/examples/pipeline.yaml#L24-L44

This was ignored (and everything still worked b/c at that time the Pipeline executed in the order it was declared in).

Steps to Reproduce the Problem

1. Add an extra invalid field to a CRD, for example you can modify the build-push.yaml example spec to have the extra field helloIDoNotBelong: foo:

apiVersion: pipeline.knative.dev/v1alpha1
kind: Task
metadata:
  name: build-push
  namespace: default
spec:
  helloIDoNotBelong: foo
  inputs:
...

2. Apply the CRD:

kubectl apply -f examples/build-push.yaml

3. Notice that it applies just fine and the extra field is ignored!

[hoegaarden]

For this to be fixed we'd need pruning to be enabled in kubernetes -- it is not implemented yet.
This needs to be implemented in the APIServer, when our code is invoked we do not have access to those additional fields.

[bobcatfish]

Thanks for the links @hoegaarden !!

Question about pruning @hoegaarden - if we prune away the extra fields, does that imply we're ignoring them? I think we definitely want to tell the user they provided some incorrect fields?

In the meantime, maybe we could create our own hacky sort of solution, e.g. inspect the fields somehow before or after deserializing? Maybe we can make openapi validation help us?

[dgageot]

@bobcatfish I have a prototype that uses the content of the kubectl.kubernetes.io/last-applied-configuration annotation.

This is basically the json form of the resource that the kubctl cli puts here.
We search this json for extra fields.

wdyt?

[bobcatfish]

(whoops @dgageot sorry this took so long to reply to!)

That's an interesting idea - in that case, would we only realize there were extra fields after the resource is created? I'd be interested in seeing the prototype!

[dgageot]

@bobcatfish see #422

[dlorenc]

@jonjohnsonjr mentioned we might want to leave these here to make schema upgrades easier. Is that still the case?

[jonjohnsonjr]

It doesn't make schema upgrades easier, but it does make downgrades possible.

If you don't allow unknown fields and you ever plan to add a new field, your users won't be able to downgrade build-pipeline, because the new field will be unrecognized by the old build-pipeline.

You can get around this by staging new fields for 1 release (but disallowing them) and only allowing n-1 downgrades, but it's a bit delicate.