-
Notifications
You must be signed in to change notification settings - Fork 342
allow unknown metadata fields #2249
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,131 @@ | ||
| /* | ||
| Copyright 2021 The Knative Authors | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package json | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "io" | ||
| ) | ||
|
|
||
| var ( | ||
| emptyMeta = []byte(`:{}`) | ||
| metaPrefix = []byte(`{"metadata"`) | ||
| metaSuffix = []byte(`}`) | ||
| ) | ||
|
|
||
| var ( | ||
| // Unmarshal is an alias for json.Unmarshal | ||
| Unmarshal = json.Unmarshal | ||
|
|
||
| //Marshal is an alias for json.Marshal | ||
| Marshal = json.Marshal | ||
| ) | ||
|
|
||
| // Decode will parse the json byte array to the target object. When | ||
| // unknown fields are _not_ allowed we still accept unknown | ||
| // fields in the Object's metadata | ||
| // | ||
| // See https://github.com/knative/serving/issues/11448 for details | ||
| func Decode(bites []byte, target interface{}, disallowUnknownFields bool) error { | ||
| if !disallowUnknownFields { | ||
| return json.Unmarshal(bites, target) | ||
| } | ||
|
|
||
| // If we don't allow unknown fields we skip validating fields in the metadata | ||
| // block since that is opaque to us and validated by the API server | ||
| start, end, err := findMetadataOffsets(bites) | ||
| if err != nil { | ||
| return err | ||
| } else if start == -1 || end == -1 { | ||
| // If for some reason the json does not have metadata continue with normal parsing | ||
| dec := json.NewDecoder(bytes.NewReader(bites)) | ||
| dec.DisallowUnknownFields() | ||
| return dec.Decode(target) | ||
| } | ||
|
|
||
| before := bites[:start] | ||
| metadata := bites[start:end] | ||
| after := bites[end:] | ||
|
|
||
| // Parse everything but skip metadata | ||
| dec := json.NewDecoder(io.MultiReader( | ||
| bytes.NewReader(before), | ||
| bytes.NewReader(emptyMeta), | ||
| bytes.NewReader(after), | ||
| )) | ||
|
|
||
| dec.DisallowUnknownFields() | ||
| if err := dec.Decode(target); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Now we parse just the metadata | ||
| dec = json.NewDecoder(io.MultiReader( | ||
| bytes.NewReader(metaPrefix), | ||
| bytes.NewReader(metadata), | ||
| bytes.NewReader(metaSuffix), | ||
| )) | ||
|
|
||
| if err := dec.Decode(target); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func findMetadataOffsets(bites []byte) (start, end int64, err error) { | ||
| start, end = -1, -1 | ||
| level := 0 | ||
|
|
||
| var ( | ||
| dec = json.NewDecoder(bytes.NewReader(bites)) | ||
| t json.Token | ||
| ) | ||
|
|
||
| for { | ||
| t, err = dec.Token() | ||
| if err == io.EOF { //nolint | ||
| break | ||
| } | ||
| if err != nil { | ||
| return | ||
| } | ||
|
|
||
| switch v := t.(type) { | ||
| case json.Delim: | ||
| if v == '{' { | ||
| level++ | ||
| } else if v == '}' { | ||
| level-- | ||
| } | ||
| case string: | ||
| if v == "metadata" && level == 1 { | ||
| start = dec.InputOffset() | ||
| x := struct{}{} | ||
| if err = dec.Decode(&x); err != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this guaranteed to move the decoder to the end of the entire metadata object? Even if there's sub objects like
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah - it's decoding the metadata value into the empty struct.
It's easy enough to add so we can catch regressions if someone changes the implementation |
||
| return -1, -1, err | ||
| } | ||
| end = dec.InputOffset() | ||
|
|
||
| // we exit early to stop processing the rest of the object | ||
| return | ||
| } | ||
| } | ||
| } | ||
| return -1, -1, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,231 @@ | ||
| /* | ||
| Copyright 2021 The Knative Authors | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package json | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "context" | ||
| "errors" | ||
| "testing" | ||
|
|
||
| "github.com/google/go-cmp/cmp" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "knative.dev/pkg/apis" | ||
| "knative.dev/pkg/webhook/resourcesemantics" | ||
| ) | ||
|
|
||
| func TestDecode_KnownFields(t *testing.T) { | ||
| // We ignore type meta since inline only works with k8s yaml parsers | ||
| cases := []struct { | ||
| name string | ||
| input string | ||
|
|
||
| want fixture | ||
| wantErr bool | ||
| }{{ | ||
| name: "no metadata", | ||
| input: `{}`, | ||
| want: fixture{}, | ||
| }, { | ||
| name: "known fields", | ||
| input: `{ "metadata":{"name":"some-name", "namespace":"some-namespace"} }`, | ||
| want: fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| }, | ||
| }, | ||
| }, { | ||
| name: "with spec", | ||
| input: `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace"}, | ||
| "spec":{"key":"value"} | ||
| }`, | ||
| want: fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| }, | ||
| Spec: map[string]string{"key": "value"}, | ||
| }, | ||
| }, { | ||
| name: "unknown metadata field", | ||
| input: `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace", "bomba":"boom"}, | ||
| "spec":{"key":"value"} | ||
| }`, | ||
| want: fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| }, | ||
| Spec: map[string]string{"key": "value"}, | ||
| }, | ||
| }, { | ||
| name: "unknown field top level", | ||
| input: `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace"}, | ||
| "bomba": "boom", | ||
| "spec":{"key":"value"} | ||
| }`, | ||
| wantErr: true, | ||
| }, { | ||
| name: "multiple metadata keys in the JSON", | ||
| input: `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace", "bomba":"boom"}, | ||
| "spec":{"metadata":"value"} | ||
| }`, | ||
| want: fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| }, | ||
| Spec: map[string]string{"metadata": "value"}, | ||
| }, | ||
| }, { | ||
| name: "nested object in metadata", | ||
| input: `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace", "labels":{"key":"value"}} | ||
| }`, | ||
| want: fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| Labels: map[string]string{ | ||
| "key": "value", | ||
| }, | ||
| }, | ||
| }, | ||
| }, { | ||
| name: "nested array in metadata", | ||
| input: `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace", "finalizers":["first","second"]} | ||
| }`, | ||
| want: fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| Finalizers: []string{"first", "second"}, | ||
| }, | ||
| }, | ||
| }, { | ||
| name: "bad input", | ||
| // note use two characters so our decoder fails on the second token lookup | ||
| input: "{{", | ||
| wantErr: true, | ||
| }} | ||
|
|
||
| for _, tc := range cases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| got := &fixture{} | ||
| err := Decode([]byte(tc.input), got, true) | ||
|
|
||
| if tc.wantErr && err == nil { | ||
| t.Fatal("DecodeTo() expected an error") | ||
| } else if !tc.wantErr && err != nil { | ||
| t.Fatal("unexpected error", err) | ||
| } | ||
|
|
||
| // Don't bother checking against the fixture if | ||
| // we expected an error | ||
| if tc.wantErr { | ||
| return | ||
| } | ||
|
|
||
| if diff := cmp.Diff(&tc.want, got); diff != "" { | ||
| t.Error("unexpected diff", diff) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestDecode_AllowUnknownFields(t *testing.T) { | ||
| input := `{ | ||
| "metadata":{"name":"some-name", "namespace":"some-namespace"}, | ||
| "bomba": "boom", | ||
| "spec": {"some":"value"} | ||
| }` | ||
|
|
||
| got := &fixture{} | ||
| want := &fixture{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: "some-name", | ||
| Namespace: "some-namespace", | ||
| }, | ||
| Spec: map[string]string{"some": "value"}, | ||
| } | ||
|
|
||
| err := Decode([]byte(input), got, false) | ||
| if err != nil { | ||
| t.Fatal("unexpected error", err) | ||
| } | ||
|
|
||
| if diff := cmp.Diff(want, got); diff != "" { | ||
| t.Error("unexpected diff", diff) | ||
| } | ||
| } | ||
|
|
||
| // Note: this test is paired with the failingFixture and knows | ||
| // that the implementation of Decode parses the json in two passes | ||
| // the first being with an empty metadata '{}' - the second being the real | ||
| // input | ||
| func TestDecode_UnmarshalMetadataFailed(t *testing.T) { | ||
| input := `{ "metadata":{"name":"some-name", "namespace":"some-namespace"} }` | ||
| err := Decode([]byte(input), &failingFixture{}, true) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| } | ||
|
|
||
| type fixture struct { | ||
| // Our decoder doesn't support `inline` that's a sig.k8s.io/yaml feature | ||
| // So we skip parsing this property | ||
| metav1.TypeMeta `json:"-"` | ||
| metav1.ObjectMeta `json:"metadata"` | ||
| Spec map[string]string `json:"spec"` | ||
| } | ||
|
|
||
| var _ resourcesemantics.GenericCRD = (*fixture)(nil) | ||
|
|
||
| func (f *fixture) DeepCopyObject() runtime.Object { | ||
| panic("not implemented") | ||
| } | ||
|
|
||
| func (f *fixture) SetDefaults(context.Context) { | ||
| panic("not implemented") | ||
| } | ||
|
|
||
| func (f *fixture) Validate(context.Context) *apis.FieldError { | ||
| panic("not implemented") | ||
| } | ||
|
|
||
| type failingFixture struct { | ||
| fixture | ||
| Meta failingMeta `json:"metadata"` | ||
| } | ||
|
|
||
| type failingMeta struct { | ||
| metav1.ObjectMeta | ||
| } | ||
|
|
||
| func (f *failingMeta) UnmarshalJSON(bites []byte) error { | ||
| if bytes.Equal([]byte("{}"), bites) { | ||
| return nil | ||
| } | ||
| return errors.New("bomba") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we not do the
errors.Isthing? (I guess this is asking for that?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could but it's not necessary - https://pkg.go.dev/encoding/json#Decoder.Token states it will return
io.EOF