Repository: knative/pkg
Pull request #2252 (2021-09-02 02:24:40)

Avoid double-resyncs without leader election.
tl;dr Without leader election enabled, we've been suffering from double resyncs, this fixes that.

Essentially when the informers are started, prior to starting the controllers, they fill the controller's workqueue with ~the world.  Once we start the controllers, the leader election code calls `Promote` on a `Bucket` so that it starts processing them as their leader, and then is requeues every key that falls into that `Bucket`.

When leader election is disabled, we use the `UniversalBucket`, which always owns everything.  This means that the first pass through the workqueue was already hitting every key, so re-enqueuing every key results in superfluous processing of those keys.

This change makes the `LeaderAwareFuncs` elide the call to `PromoteFunc` when the `enq` function passed is `nil`, and makes the `unopposedElector` have a `nil` `enq` field when we explicitly pass it the `UniversalBucket` because leader election is disabled.

/kind cleanup

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release note in the block
below. Include the string "action required" if additional action is required of
users switching to the new release, for example in case of a breaking change.

Write as if you are speaking to users, not other Knative contributors. If this
change has no user-visible impact, no release-note is needed.
-->
```release-note

```

**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [knative/docs]: <issue or pr link>
- [Feature Track]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2252#" title="Author self-approved">mattmoor</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

- ~~[OWNERS](https://github.com/knative/pkg/blob/main/OWNERS)~~ [mattmoor]

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: `TestMetricsExport/OpenCensus` in the actions leg that failed
dprotaso: Can we add some unit tests for the `leaderelection` and `reconciler` package changes?

Patch:
diff --git a/leaderelection/context.go b/leaderelection/context.go
index d30ecd462..4758ccba5 100644
--- a/leaderelection/context.go
+++ b/leaderelection/context.go
@@ -94,7 +94,11 @@ func BuildElector(ctx context.Context, la reconciler.LeaderAware, queueName stri
 	return &unopposedElector{
 		la:  la,
 		bkt: reconciler.UniversalBucket(),
-		enq: enq,
+		// The UniversalBucket owns everything, so there is never a need to
+		// enqueue things (no possible state change).  We pass nil here to
+		// avoid filling the queue for an extra resynce at startup along
+		// this path.
+		enq: nil,
 	}, nil
 }
 
diff --git a/reconciler/leader.go b/reconciler/leader.go
index 6fece8526..ed7254c46 100644
--- a/reconciler/leader.go
+++ b/reconciler/leader.go
@@ -97,7 +97,7 @@ func (laf *LeaderAwareFuncs) Promote(b Bucket, enq func(Bucket, types.Namespaced
 		laf.buckets[b.Name()] = b
 	}()
 
-	if promote := laf.PromoteFunc; promote != nil {
+	if promote := laf.PromoteFunc; promote != nil && enq != nil {
 		return promote(b, enq)
 	}
 	return nil
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/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 db805b4a5..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)
 		}
 	}


Test patch:
diff --git a/controller/controller_test.go b/controller/controller_test.go
index 2e9fb45b3..6ce2e3dd0 100644
--- a/controller/controller_test.go
+++ b/controller/controller_test.go
@@ -941,15 +941,15 @@ func (cr *countingLeaderAwareReconciler) Reconcile(ctx context.Context, key stri
 }
 
 func TestStartAndShutdownWithLeaderAwareNoElection(t *testing.T) {
-	promoted := make(chan struct{})
 	r := &countingLeaderAwareReconciler{
 		LeaderAwareFuncs: reconciler.LeaderAwareFuncs{
 			PromoteFunc: func(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {
-				close(promoted)
+				t.Error("Promote should not be called when no leader election is enabled.")
 				return nil
 			},
 		},
 	}
+
 	impl := NewContext(context.TODO(), r, ControllerOptions{
 		Logger:        TestLogger(t),
 		WorkQueueName: "Testing",
@@ -968,13 +968,10 @@ func TestStartAndShutdownWithLeaderAwareNoElection(t *testing.T) {
 	})
 
 	select {
-	case <-promoted:
-		// We expect to be promoted immediately, since there is no
-		// ElectorBuilder attached to the context.
 	case <-doneCh:
 		t.Fatal("StartAll finished early.")
 	case <-time.After(10 * time.Second):
-		t.Error("Timed out waiting for StartAll.")
+		// Give it some time to run.
 	}
 
 	cancel()
diff --git a/leaderelection/context_test.go b/leaderelection/context_test.go
index 271bab215..1732e9557 100644
--- a/leaderelection/context_test.go
+++ b/leaderelection/context_test.go
@@ -323,3 +323,35 @@ func TestWithStatefulSetBuilder(t *testing.T) {
 		t.Fatal("Timed out waiting for promotion.")
 	}
 }
+
+func TestWithUnopposedElector(t *testing.T) {
+	laf := &reconciler.LeaderAwareFuncs{
+		PromoteFunc: func(bkt reconciler.Bucket, enq func(reconciler.Bucket, types.NamespacedName)) error {
+			t.Error("Unexpected call to PromoteFunc with unopposedElector")
+			return nil
+		},
+	}
+
+	ctx := context.Background()
+	if HasLeaderElection(ctx) {
+		t.Error("HasLeaderElection() = true, wanted false")
+	}
+
+	le, err := BuildElector(ctx, laf, "name", func(reconciler.Bucket, types.NamespacedName) {
+		t.Error("Unexpected call to enqueue function.")
+	})
+	if err != nil {
+		t.Fatal("BuildElector() =", err)
+	}
+
+	if _, ok := le.(*unopposedElector); !ok {
+		t.Fatalf("BuildElector() = %T, wanted an unopposedElector", le)
+	}
+
+	ctx, cancel := context.WithCancel(context.Background())
+	t.Cleanup(cancel)
+	go le.Run(ctx)
+
+	// Wait to see if PromoteFunc is called with nil or our enq function.
+	<-time.After(time.Second)
+}
diff --git a/reconciler/leader_test.go b/reconciler/leader_test.go
index 34d6eefe5..82f32ce1a 100644
--- a/reconciler/leader_test.go
+++ b/reconciler/leader_test.go
@@ -42,6 +42,11 @@ func TestLeaderAwareFuncs(t *testing.T) {
 	}
 
 	laf.PromoteFunc = func(bkt Bucket, gotFunc func(Bucket, types.NamespacedName)) error {
+		if gotFunc == nil {
+			t.Error("PromoteFunc should not be called with nil gotFunc")
+			return nil
+		}
+
 		gotFunc(bkt, wantKey)
 		if !called {
 			t.Error("gotFunc didn't call wantFunc!")
@@ -79,4 +84,7 @@ func TestLeaderAwareFuncs(t *testing.T) {
 	if laf.IsLeaderFor(wantKey) {
 		t.Error("IsLeaderFor() = true, wanted false")
 	}
+
+	// We should elide calls with a nil enq
+	laf.Promote(wantBkt, nil)
 }
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