Repository: knative/pkg
Pull request #2255 (2021-09-02 14:40:32)

[release-0.24] allow unknown metadata fields
This is an automated cherry-pick of #2249

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2255#" title="Author self-approved">knative-prow-robot</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mattmoor** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @mattmoor` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/release-0.24/webhook/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mattmoor"]} -->
knative-prow-robot: @knative-prow-robot: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | c34ffe06a0ecdac02295784a746e86f42d66d74c | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2255/pull-knative-pkg-unit-tests/1433439709740142592) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/2255). [Your PR dashboard](https://gubernator.knative.dev/pr/knative-prow-robot).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @knative-prow-robot: The following tests **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | c34ffe06a0ecdac02295784a746e86f42d66d74c | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2255/pull-knative-pkg-unit-tests/1433439709740142592) | `/test pull-knative-pkg-unit-tests`
pull-knative-pkg-build-tests | c34ffe06a0ecdac02295784a746e86f42d66d74c | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/2255/pull-knative-pkg-build-tests/1433439709706588160) | `/test pull-knative-pkg-build-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/2255). [Your PR dashboard](https://gubernator.knative.dev/pr/knative-prow-robot).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/webhook/configmaps/configmaps.go b/webhook/configmaps/configmaps.go
index 4d7ea5bfd..5a4c4d888 100644
--- a/webhook/configmaps/configmaps.go
+++ b/webhook/configmaps/configmaps.go
@@ -17,7 +17,6 @@ limitations under the License.
 package configmaps
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -192,8 +191,7 @@ func (ac *reconciler) validate(ctx context.Context, req *admissionv1.AdmissionRe
 
 	var newObj corev1.ConfigMap
 	if len(newBytes) != 0 {
-		newDecoder := json.NewDecoder(bytes.NewBuffer(newBytes))
-		if err := newDecoder.Decode(&newObj); err != nil {
+		if err := json.Unmarshal(newBytes, &newObj); err != nil {
 			return fmt.Errorf("cannot decode incoming new object: %w", err)
 		}
 	}
diff --git a/webhook/json/decode.go b/webhook/json/decode.go
new file mode 100644
index 000000000..9e206a10b
--- /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 { //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 {
+					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/psbinding/psbinding.go b/webhook/psbinding/psbinding.go
index ef16a9d83..9fa34d2c0 100644
--- a/webhook/psbinding/psbinding.go
+++ b/webhook/psbinding/psbinding.go
@@ -17,7 +17,6 @@ limitations under the License.
 package psbinding
 
 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
@@ -178,8 +177,7 @@ func (ac *Reconciler) Admit(ctx context.Context, request *admissionv1.AdmissionR
 	}
 
 	orig := &duckv1.WithPod{}
-	decoder := json.NewDecoder(bytes.NewBuffer(request.Object.Raw))
-	if err := decoder.Decode(&orig); err != nil {
+	if err := json.Unmarshal(request.Object.Raw, orig); err != nil {
 		return webhook.MakeErrorStatus("unable to decode object: %v", err)
 	}
 
diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go
index a5bcc4137..504dcaa06 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/validation/validation_admit.go b/webhook/resourcesemantics/validation/validation_admit.go
index f2d7ad9bf..0544d88a6 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)
 		}
 	}
@@ -201,8 +194,7 @@ func (ac *reconciler) callback(ctx context.Context, req *admissionv1.AdmissionRe
 	if c, ok := ac.callbacks[gvk]; ok {
 		if _, supported := c.supportedVerbs[req.Operation]; supported {
 			unstruct := &unstructured.Unstructured{}
-			newDecoder := json.NewDecoder(bytes.NewBuffer(toDecode))
-			if err := newDecoder.Decode(&unstruct); err != nil {
+			if err := json.Unmarshal(toDecode, unstruct); err != nil {
 				return fmt.Errorf("cannot decode incoming new object: %w", err)
 			}
 


Test patch:
diff --git a/webhook/json/decode_test.go b/webhook/json/decode_test.go
new file mode 100644
index 000000000..85006d759
--- /dev/null
+++ b/webhook/json/decode_test.go
@@ -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")
+}
diff --git a/webhook/resourcesemantics/defaulting/defaulting_test.go b/webhook/resourcesemantics/defaulting/defaulting_test.go
index a30dc4105..dd5230762 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_test.go b/webhook/resourcesemantics/validation/validation_admit_test.go
index 009d7d494..43d049b12 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