Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

consider allowing unknown fields in the webhooks #11065

Closed
dprotaso opened this issue Mar 29, 2021 · 3 comments
Closed

consider allowing unknown fields in the webhooks #11065

dprotaso opened this issue Mar 29, 2021 · 3 comments

Comments

@dprotaso
Copy link
Member

dprotaso commented Mar 29, 2021

Our upgrade/downgrade test is very good at catching inclusion of new fields

ie. from #11038 (comment)_

"Failed to migrate: unable to patch resource serving-tests/pizzaplanet-post-upgrade-service-00001 (gvr: serving.knative.dev/v1, Resource=revisions) - admission webhook "webhook.serving.knative.dev" denied the request: mutation failed: cannot decode incoming new object: json: unknown field "actualReplicas""

To have downgrade tests passing I've see two approaches:

  1. Work around this by adding fields in two releases. This approach poses challenges if we'd like to introduce a new field with a non-zero default value.
  2. Relax this constraint and accept the world. This removes safe guards and user validation
@dprotaso dprotaso changed the title consider toggling on unknown fields in the webhook parser consider allowing unknown fields in the webhook parser Mar 29, 2021
@dprotaso dprotaso changed the title consider allowing unknown fields in the webhook parser consider allowing unknown fields in the webhooks Mar 29, 2021
@dprotaso
Copy link
Member Author

dprotaso commented Mar 29, 2021

A third more extreme option may be to take an official stance and not support downgrading - and remove the test. (probably a non-starter)

@dprotaso
Copy link
Member Author

I'm leaning towards option #1 keep the status quo - I think defaulting could be done after the fact

@dprotaso
Copy link
Member Author

dprotaso commented Apr 1, 2021

Gonna to close this off - as I opened it to prompt a discussion but that didn't happen and I think the status quo is still the right approach.

@dprotaso dprotaso closed this as completed Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant