From d9092de2f23127abd631e09a8c168cf9fa82c827 Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 1 Sep 2021 10:58:43 -0400 Subject: [PATCH 1/3] allow unknown metadata fields --- webhook/json/decode.go | 131 +++++++++++ webhook/json/decode_test.go | 205 ++++++++++++++++++ .../defaulting/defaulting.go | 17 +- .../defaulting/defaulting_test.go | 29 +++ .../validation/validation_admit.go | 17 +- .../validation/validation_admit_test.go | 29 +++ 6 files changed, 404 insertions(+), 24 deletions(-) create mode 100644 webhook/json/decode.go create mode 100644 webhook/json/decode_test.go diff --git a/webhook/json/decode.go b/webhook/json/decode.go new file mode 100644 index 0000000000..a3dd179d57 --- /dev/null +++ b/webhook/json/decode.go @@ -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 { + break + } + if err != nil { + return + } + + switch v := t.(type) { + case json.Delim: + if v == '{' { + level += 1 + } else if v == '}' { + level -= 1 + } + case string: + if v == "metadata" && level == 1 { + start = dec.InputOffset() + x := struct{}{} + if err = dec.Decode(&x); err != nil { + return -1, -1, err + } + end = dec.InputOffset() + + // we exit early to stop processing the rest of the object + return + } + } + } + return -1, -1, nil +} diff --git a/webhook/json/decode_test.go b/webhook/json/decode_test.go new file mode 100644 index 0000000000..a5e26cb381 --- /dev/null +++ b/webhook/json/decode_test.go @@ -0,0 +1,205 @@ +/* +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: "mutliple 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: "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") +} diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go index a5bcc4137d..504dcaa066 100644 --- a/webhook/resourcesemantics/defaulting/defaulting.go +++ b/webhook/resourcesemantics/defaulting/defaulting.go @@ -17,9 +17,7 @@ limitations under the License. package defaulting import ( - "bytes" "context" - "encoding/json" "errors" "fmt" "sort" @@ -47,6 +45,7 @@ import ( "knative.dev/pkg/system" "knative.dev/pkg/webhook" certresources "knative.dev/pkg/webhook/certificates/resources" + "knative.dev/pkg/webhook/json" "knative.dev/pkg/webhook/resourcesemantics" ) @@ -241,21 +240,15 @@ func (ac *reconciler) mutate(ctx context.Context, req *admissionv1.AdmissionRequ if len(newBytes) != 0 { newObj = handler.DeepCopyObject().(resourcesemantics.GenericCRD) - newDecoder := json.NewDecoder(bytes.NewBuffer(newBytes)) - if ac.disallowUnknownFields { - newDecoder.DisallowUnknownFields() - } - if err := newDecoder.Decode(&newObj); err != nil { + err := json.Decode(newBytes, newObj, ac.disallowUnknownFields) + if err != nil { return nil, fmt.Errorf("cannot decode incoming new object: %w", err) } } if len(oldBytes) != 0 { oldObj = handler.DeepCopyObject().(resourcesemantics.GenericCRD) - oldDecoder := json.NewDecoder(bytes.NewBuffer(oldBytes)) - if ac.disallowUnknownFields { - oldDecoder.DisallowUnknownFields() - } - if err := oldDecoder.Decode(&oldObj); err != nil { + err := json.Decode(oldBytes, oldObj, ac.disallowUnknownFields) + if err != nil { return nil, fmt.Errorf("cannot decode incoming old object: %w", err) } } diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go index a30dc4105a..dd52307621 100644 --- a/webhook/resourcesemantics/defaulting/defaulting_test.go +++ b/webhook/resourcesemantics/defaulting/defaulting_test.go @@ -187,6 +187,35 @@ func TestUnknownFieldFails(t *testing.T) { `mutation failed: cannot decode incoming new object: json: unknown field "foo"`) } +func TestUnknownMetadataFieldSucceeds(t *testing.T) { + _, ac := newNonRunningTestResourceAdmissionController(t) + req := &admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Kind: metav1.GroupVersionKind{ + Group: "pkg.knative.dev", + Version: "v1alpha1", + Kind: "Resource", + }, + } + + marshaled, err := json.Marshal(map[string]interface{}{ + "apiVersion": "pkg.knative.dev/v1alpha1", + "kind": "Resource", + "metadata": map[string]string{ + "unknown": "property", + }, + "spec": map[string]string{ + "fieldWithValidation": "magic value", + }, + }) + if err != nil { + t.Fatal("Failed to marshal resource:", err) + } + req.Object.Raw = marshaled + + ExpectAllowed(t, ac.Admit(TestContextWithLogger(t), req)) +} + func TestAdmitCreates(t *testing.T) { tests := []struct { name string diff --git a/webhook/resourcesemantics/validation/validation_admit.go b/webhook/resourcesemantics/validation/validation_admit.go index f2d7ad9bf5..067cdb9f58 100644 --- a/webhook/resourcesemantics/validation/validation_admit.go +++ b/webhook/resourcesemantics/validation/validation_admit.go @@ -17,9 +17,7 @@ limitations under the License. package validation import ( - "bytes" "context" - "encoding/json" "errors" "fmt" @@ -31,6 +29,7 @@ import ( kubeclient "knative.dev/pkg/client/injection/kube/client" "knative.dev/pkg/logging" "knative.dev/pkg/webhook" + "knative.dev/pkg/webhook/json" "knative.dev/pkg/webhook/resourcesemantics" ) @@ -110,11 +109,8 @@ func (ac *reconciler) decodeRequestAndPrepareContext( var newObj resourcesemantics.GenericCRD if len(newBytes) != 0 { newObj = handler.DeepCopyObject().(resourcesemantics.GenericCRD) - newDecoder := json.NewDecoder(bytes.NewBuffer(newBytes)) - if ac.disallowUnknownFields { - newDecoder.DisallowUnknownFields() - } - if err := newDecoder.Decode(&newObj); err != nil { + err := json.Decode(newBytes, newObj, ac.disallowUnknownFields) + if err != nil { return ctx, nil, fmt.Errorf("cannot decode incoming new object: %w", err) } } @@ -122,11 +118,8 @@ func (ac *reconciler) decodeRequestAndPrepareContext( var oldObj resourcesemantics.GenericCRD if len(oldBytes) != 0 { oldObj = handler.DeepCopyObject().(resourcesemantics.GenericCRD) - oldDecoder := json.NewDecoder(bytes.NewBuffer(oldBytes)) - if ac.disallowUnknownFields { - oldDecoder.DisallowUnknownFields() - } - if err := oldDecoder.Decode(&oldObj); err != nil { + err := json.Decode(oldBytes, oldObj, ac.disallowUnknownFields) + if err != nil { return ctx, nil, fmt.Errorf("cannot decode incoming old object: %w", err) } } diff --git a/webhook/resourcesemantics/validation/validation_admit_test.go b/webhook/resourcesemantics/validation/validation_admit_test.go index 009d7d4942..43d049b126 100644 --- a/webhook/resourcesemantics/validation/validation_admit_test.go +++ b/webhook/resourcesemantics/validation/validation_admit_test.go @@ -213,6 +213,35 @@ func TestUnknownFieldFails(t *testing.T) { `decoding request failed: cannot decode incoming new object: json: unknown field "foo"`) } +func TestUnknownMetadataFieldSucceeds(t *testing.T) { + _, ac := newNonRunningTestResourceAdmissionController(t) + req := &admissionv1.AdmissionRequest{ + Operation: admissionv1.Create, + Kind: metav1.GroupVersionKind{ + Group: "pkg.knative.dev", + Version: "v1alpha1", + Kind: "Resource", + }, + } + + marshaled, err := json.Marshal(map[string]interface{}{ + "apiVersion": "pkg.knative.dev/v1alpha1", + "kind": "Resource", + "metadata": map[string]string{ + "unknown": "property", + }, + "spec": map[string]string{ + "fieldWithValidation": "magic value", + }, + }) + if err != nil { + t.Fatal("Failed to marshal resource:", err) + } + req.Object.Raw = marshaled + + ExpectAllowed(t, ac.Admit(TestContextWithLogger(t), req)) +} + func TestAdmitCreates(t *testing.T) { tests := []struct { name string From b2c6b8d63d6ea0b0f51d4e8d72cd29bc06d5c94d Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 1 Sep 2021 11:53:13 -0400 Subject: [PATCH 2/3] fix lint warnings --- webhook/json/decode.go | 6 +++--- webhook/json/decode_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/webhook/json/decode.go b/webhook/json/decode.go index a3dd179d57..9e206a10ba 100644 --- a/webhook/json/decode.go +++ b/webhook/json/decode.go @@ -99,7 +99,7 @@ func findMetadataOffsets(bites []byte) (start, end int64, err error) { for { t, err = dec.Token() - if err == io.EOF { + if err == io.EOF { //nolint break } if err != nil { @@ -109,9 +109,9 @@ func findMetadataOffsets(bites []byte) (start, end int64, err error) { switch v := t.(type) { case json.Delim: if v == '{' { - level += 1 + level++ } else if v == '}' { - level -= 1 + level-- } case string: if v == "metadata" && level == 1 { diff --git a/webhook/json/decode_test.go b/webhook/json/decode_test.go index a5e26cb381..81e2201b03 100644 --- a/webhook/json/decode_test.go +++ b/webhook/json/decode_test.go @@ -85,7 +85,7 @@ func TestDecode_KnownFields(t *testing.T) { }`, wantErr: true, }, { - name: "mutliple metadata keys in the JSON", + name: "multiple metadata keys in the JSON", input: `{ "metadata":{"name":"some-name", "namespace":"some-namespace", "bomba":"boom"}, "spec":{"metadata":"value"} From c34ffe06a0ecdac02295784a746e86f42d66d74c Mon Sep 17 00:00:00 2001 From: dprotaso Date: Wed, 1 Sep 2021 11:55:48 -0400 Subject: [PATCH 3/3] include tests for nested structures in the metadata value this is for catching any regressions --- webhook/json/decode_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/webhook/json/decode_test.go b/webhook/json/decode_test.go index 81e2201b03..85006d7599 100644 --- a/webhook/json/decode_test.go +++ b/webhook/json/decode_test.go @@ -97,6 +97,32 @@ func TestDecode_KnownFields(t *testing.T) { }, 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