allow unknown metadata fields by dprotaso · Pull Request #2249 · knative/pkg

dprotaso · 2021-09-01T15:00:58Z

dprotaso · 2021-09-01T15:02:16Z

/assign @julz @markusthoemmes @shinigambit
/hold

I'm going to test downstream

markusthoemmes

I love this 🥳 . Had something similar in mind but didn't dare doing the parsing shenanigans. Great stuff!

markusthoemmes · 2021-09-01T15:09:56Z

+			if v == "metadata" && level == 1 {
+				start = dec.InputOffset()
+				x := struct{}{}
+				if err = dec.Decode(&x); err != nil {


Is this guaranteed to move the decoder to the end of the entire metadata object? Even if there's sub objects like annotations for example? Is that worth an extra test?

Is this guaranteed to move the decoder to the end of the entire metadata object?

Yeah - it's decoding the metadata value into the empty struct.

Even if there's sub objects like annotations for example? Is that worth an extra test?

It's easy enough to add so we can catch regressions if someone changes the implementation

dprotaso · 2021-09-01T15:43:08Z

I love this 🥳 . Had something similar in mind but didn't dare doing the parsing shenanigans. Great stuff!

The biggest gain over the naive implementation is because we bail out early after we have the metadata byte offsets.

this is for catching any regressions

codecov · 2021-09-01T16:06:54Z

Codecov Report

Merging #2249 (801d2af) into main (e957ee5) will increase coverage by 0.13%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##             main    #2249      +/-   ##
==========================================
+ Coverage   63.92%   64.05%   +0.13%     
==========================================
  Files         220      221       +1     
  Lines        9550     9596      +46     
==========================================
+ Hits         6105     6147      +42     
- Misses       3172     3174       +2     
- Partials      273      275       +2

Impacted Files	Coverage Δ
webhook/resourcesemantics/defaulting/defaulting.go	`79.62% <75.00%> (-0.50%)`	⬇️
...k/resourcesemantics/validation/validation_admit.go	`92.94% <75.00%> (-0.32%)`	⬇️
webhook/json/decode.go	`96.29% <96.29%> (ø)`
test/gcs/mock/mock.go	`90.32% <0.00%> (-1.08%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82f3a9f...801d2af. Read the comment docs.

markusthoemmes · 2021-09-01T16:11:28Z

+
+	for {
+		t, err = dec.Token()
+		if err == io.EOF { //nolint


can we not do the errors.Is thing? (I guess this is asking for that?)

You could but it's not necessary - https://pkg.go.dev/encoding/json#Decoder.Token states it will return io.EOF

dprotaso · 2021-09-01T16:25:16Z

Downstream serving passes
/hold cancel

markusthoemmes

/lgtm
/approve

knative-prow-robot · 2021-09-02T14:23:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, markusthoemmes

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~webhook/OWNERS~~ [dprotaso,markusthoemmes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dprotaso · 2021-09-02T14:36:55Z

/cherry-pick release-0.25
/cherry-pick release-0.24
/cherry-pick release-0.23
/cherry-pick release-0.22

knative-prow-robot · 2021-09-02T14:37:43Z

@dprotaso: new pull request created: #2254

Details

In response to this:

/cherry-pick release-0.25
/cherry-pick release-0.24
/cherry-pick release-0.23
/cherry-pick release-0.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dprotaso · 2021-09-02T14:39:46Z

@chizhg do we have the latest cherry pick plugin? It should support multiple invocations

kubernetes/test-infra#21714

dprotaso · 2021-09-02T14:39:56Z

/cherry-pick release-0.24

dprotaso · 2021-09-02T14:40:00Z

/cherry-pick release-0.23

dprotaso · 2021-09-02T14:40:03Z

/cherry-pick release-0.22

knative-prow-robot · 2021-09-02T14:40:33Z

@dprotaso: new pull request created: #2255

Details

In response to this:

/cherry-pick release-0.24

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2021-09-02T14:40:37Z

@dprotaso: new pull request created: #2256

Details

In response to this:

/cherry-pick release-0.23

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2021-09-02T14:40:41Z

@dprotaso: new pull request created: #2257

Details

In response to this:

/cherry-pick release-0.22

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Need to pull in knative/pkg#2249 for k8s 1.22 support

* Bump to cert-manager 1.5.3 1.2.0 no longer works with k8s 1.22 * Bump to latest eventing patch releases Need to pull in knative/pkg#2249 for k8s 1.22 support

Validation webhook was failing with `json: unknown field "subresource"` on decoding the admission request. It's because of the new "subresource" field in `metadata.managedFields`[^0]. Folks in Knative fixed the issue[^1] with creating a custom json decoder that ignores unknown fields *only* in metadata. [^0]: kubernetes/apimachinery@5c420e5 [^1]: knative/pkg#2249

* K8SPXC-885: Fix validation webhook on k8s 1.22 Validation webhook was failing with `json: unknown field "subresource"` on decoding the admission request. It's because of the new "subresource" field in `metadata.managedFields`[^0]. Folks in Knative fixed the issue[^1] with creating a custom json decoder that ignores unknown fields *only* in metadata. [^0]: kubernetes/apimachinery@5c420e5 [^1]: knative/pkg#2249 * address review comments Co-authored-by: Mykola Marzhan <mykola.marzhan@percona.com> Co-authored-by: Mykola Marzhan <mykola.marzhan@percona.com>

allow unknown metadata fields

18a3eca

google-cla Bot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 1, 2021

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 1, 2021

knative-prow-robot assigned julz Sep 1, 2021

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2021

knative-prow-robot assigned markusthoemmes and leetcope Sep 1, 2021

markusthoemmes reviewed Sep 1, 2021

View reviewed changes

dprotaso mentioned this pull request Sep 1, 2021

[wip] test metadata fix knative/serving#11896

Closed

dprotaso added 2 commits September 1, 2021 11:53

fix lint warnings

9a9b736

include tests for nested structures in the metadata value

801d2af

this is for catching any regressions

markusthoemmes reviewed Sep 1, 2021

View reviewed changes

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2021

markusthoemmes approved these changes Sep 2, 2021

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 2, 2021

knative-prow-robot merged commit 0482448 into knative:main Sep 2, 2021

dprotaso deleted the allow-unknown-metadata-fields branch September 2, 2021 14:36

knative-prow-robot mentioned this pull request Sep 2, 2021

[release-0.25] allow unknown metadata fields #2254

Merged

knative-prow-robot mentioned this pull request Sep 2, 2021

[release-0.24] allow unknown metadata fields #2255

Merged

knative-prow-robot mentioned this pull request Sep 2, 2021

[release-0.23] allow unknown metadata fields #2256

Merged

knative-prow-robot mentioned this pull request Sep 2, 2021

[release-0.22] allow unknown metadata fields #2257

Merged

benmoss added a commit to benmoss/eventing-rabbitmq that referenced this pull request Sep 17, 2021

Bump to latest eventing patch releases

b89bfbf

Need to pull in knative/pkg#2249 for k8s 1.22 support

benmoss mentioned this pull request Sep 17, 2021

Bump eventing and cert-manager knative-extensions/eventing-rabbitmq#422

Merged

benmoss added a commit to benmoss/eventing-rabbitmq that referenced this pull request Sep 20, 2021

Bump to latest eventing patch releases

df70916

Need to pull in knative/pkg#2249 for k8s 1.22 support

devguyio mentioned this pull request Sep 23, 2021

[SRVKS-790] Patch subresource to unblock webhooks on 4.9 (#308) (#311) openshift-knative/eventing-kafka#354

Closed

egegunes mentioned this pull request Oct 15, 2021

K8SPXC-885: Fix validation webhook on k8s 1.22 percona/percona-xtradb-cluster-operator#1021

Merged

psschwei mentioned this pull request Oct 18, 2021

Set minikube Kubernetes version to 1.21.5 knative-extensions/kn-plugin-quickstart#130

Closed

Conversation

dprotaso commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dprotaso commented Sep 1, 2021

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

markusthoemmes Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

dprotaso Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markusthoemmes Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

dprotaso Sep 1, 2021

Choose a reason for hiding this comment

Uh oh!

dprotaso commented Sep 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Sep 2, 2021

Uh oh!

dprotaso commented Sep 2, 2021

Uh oh!

knative-prow-robot commented Sep 2, 2021

Uh oh!

dprotaso commented Sep 2, 2021

Uh oh!

dprotaso commented Sep 2, 2021

Uh oh!

dprotaso commented Sep 2, 2021

Uh oh!

dprotaso commented Sep 2, 2021

Uh oh!

knative-prow-robot commented Sep 2, 2021

Uh oh!

knative-prow-robot commented Sep 2, 2021

Uh oh!

knative-prow-robot commented Sep 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dprotaso commented Sep 1, 2021 •

edited

Loading

dprotaso commented Sep 1, 2021 •

edited

Loading

codecov Bot commented Sep 1, 2021 •

edited

Loading

dprotaso commented Sep 1, 2021 •

edited

Loading