Repository: knative/pkg
Pull request #2279 (2021-09-07 22:19:01)

Enable using `duckv1.WithPod` with our webhook infra.
Today you can't use `duckv1.WithPod` to author webhooks because it doesn't implement the `Validate` or `SetDefaults` methods, and that makes sense since they are (by definition) a generic encapsulation of a number of types.

However, if we could infuse `ctx` with appropriate callbacks for `Validate` and `SetDefaults` then folks can use our types, but infuse `ctx` with callbacks that perform the appropriate validation.

What I have in mind is something along these lines:

```go
return defaulting.NewAdmissionController(ctx,
	// Name of the resource webhook.
	"foo.bar.dev",

	// The path on which to serve the webhook.
	"/defaulting",

	// The resources to default.
	map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
		appsv1.SchemeGroupVersion.WithKind("Deployment"):  &duckv1.WithPod{},
		appsv1.SchemeGroupVersion.WithKind("ReplicaSet"):  &duckv1.WithPod{},
		appsv1.SchemeGroupVersion.WithKind("StatefulSet"): &duckv1.WithPod{},
		appsv1.SchemeGroupVersion.WithKind("DaemonSet"):   &duckv1.WithPod{},
		batchv1.SchemeGroupVersion.WithKind("Job"):        &duckv1.WithPod{},
	},

	// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
	func(ctx context.Context) context.Context{
		return duckv1.WithPodSpecDefaulter(ctx, myFancyLogic)
	},

	// Whether to disallow unknown fields.
	false,
)
```

It is roughly equivalent for validation.

/kind enhancement


I was looking at this through the lens of @dlorenc `cosigned` webhook, which currently has to futz around with a mix of concrete types and dynamic clients to achieve a similar result.  I'm hoping that with this we can just write some logic against `duckv1.WithPod` that captures all of these types.

**Release Note**

```release-note

```

**Docs**

```docs

```


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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/2279#" 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:

- ~~[apis/duck/OWNERS](https://github.com/knative/pkg/blob/main/apis/duck/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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
> Merging [#2279](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (07a9d7e) into [main](https://codecov.io/gh/knative/pkg/commit/73d4fe679a2e60ad93f157c005780309fd5cf21e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) (73d4fe6) will **increase** coverage by `0.06%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/pkg/pull/2279/graphs/tree.svg?width=650&height=150&src=pr&token=rlqpDnfxbO&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

```diff
@@            Coverage Diff             @@
##             main    #2279      +/-   ##
==========================================
+ Coverage   64.18%   64.25%   +0.06%     
==========================================
  Files         221      223       +2     
  Lines        9578     9593      +15     
==========================================
+ Hits         6148     6164      +16     
+ Misses       3152     3151       -1     
  Partials      278      278              
```


| [Impacted Files](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [apis/duck/v1/podspec\_types.go](https://codecov.io/gh/knative/pkg/pull/2279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL3BvZHNwZWNfdHlwZXMuZ28=) | `100.00% <ø> (ø)` | |
| [apis/duck/v1/podspec\_defaults.go](https://codecov.io/gh/knative/pkg/pull/2279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL3BvZHNwZWNfZGVmYXVsdHMuZ28=) | `100.00% <100.00%> (ø)` | |
| [apis/duck/v1/podspec\_validation.go](https://codecov.io/gh/knative/pkg/pull/2279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-YXBpcy9kdWNrL3YxL3BvZHNwZWNfdmFsaWRhdGlvbi5nbw==) | `100.00% <100.00%> (ø)` | |
| [controller/controller.go](https://codecov.io/gh/knative/pkg/pull/2279/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-Y29udHJvbGxlci9jb250cm9sbGVyLmdv) | `86.49% <0.00%> (+0.36%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Last update [73d4fe6...07a9d7e](https://codecov.io/gh/knative/pkg/pull/2279?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

n3wscott: /lgtm cancel
n3wscott: /lgtm

Patch:
diff --git a/apis/duck/v1/podspec_defaults.go b/apis/duck/v1/podspec_defaults.go
new file mode 100644
index 000000000..9fc3f3ceb
--- /dev/null
+++ b/apis/duck/v1/podspec_defaults.go
@@ -0,0 +1,47 @@
+/*
+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 v1
+
+import (
+	"context"
+)
+
+// PodSpecDefaulter is a callback to validate a PodSpecable.
+type PodSpecDefaulter func(context.Context, *WithPod)
+
+// SetDefaults implements apis.Defaultable
+func (wp *WithPod) SetDefaults(ctx context.Context) {
+	if psd := GetPodSpecDefaulter(ctx); psd != nil {
+		psd(ctx, wp)
+	}
+}
+
+// psvKey is used for associating a PodSpecDefaulter with a context.Context
+type psdKey struct{}
+
+func WithPodSpecDefaulter(ctx context.Context, psd PodSpecDefaulter) context.Context {
+	return context.WithValue(ctx, psdKey{}, psd)
+}
+
+// GetPodSpecDefaulter extracts the PodSpecDefaulter from the context.
+func GetPodSpecDefaulter(ctx context.Context) PodSpecDefaulter {
+	untyped := ctx.Value(psdKey{})
+	if untyped == nil {
+		return nil
+	}
+	return untyped.(PodSpecDefaulter)
+}
diff --git a/apis/duck/v1/podspec_types.go b/apis/duck/v1/podspec_types.go
index a8d172416..b822a4847 100644
--- a/apis/duck/v1/podspec_types.go
+++ b/apis/duck/v1/podspec_types.go
@@ -45,6 +45,9 @@ type WithPod struct {
 	Spec WithPodSpec `json:"spec,omitempty"`
 }
 
+var _ apis.Validatable = (*WithPod)(nil)
+var _ apis.Defaultable = (*WithPod)(nil)
+
 // WithPodSpec is the shell around the PodSpecable within WithPod.
 type WithPodSpec struct {
 	Template PodSpecable `json:"template,omitempty"`
@@ -57,13 +60,13 @@ var (
 )
 
 // GetFullType implements duck.Implementable
-func (*PodSpecable) GetFullType() ducktypes.Populatable {
+func (wp *PodSpecable) GetFullType() ducktypes.Populatable {
 	return &WithPod{}
 }
 
 // Populate implements duck.Populatable
-func (t *WithPod) Populate() {
-	t.Spec.Template = PodSpecable{
+func (wp *WithPod) Populate() {
+	wp.Spec.Template = PodSpecable{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{
 				"foo": "bar",
@@ -79,7 +82,7 @@ func (t *WithPod) Populate() {
 }
 
 // GetListType implements apis.Listable
-func (*WithPod) GetListType() runtime.Object {
+func (wp *WithPod) GetListType() runtime.Object {
 	return &WithPodList{}
 }
 
diff --git a/apis/duck/v1/podspec_validation.go b/apis/duck/v1/podspec_validation.go
new file mode 100644
index 000000000..3f8f28b6c
--- /dev/null
+++ b/apis/duck/v1/podspec_validation.go
@@ -0,0 +1,50 @@
+/*
+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 v1
+
+import (
+	"context"
+
+	"knative.dev/pkg/apis"
+)
+
+// PodSpecValidator is a callback to validate a PodSpecable.
+type PodSpecValidator func(context.Context, *WithPod) *apis.FieldError
+
+// Validate implements apis.Validatable
+func (wp *WithPod) Validate(ctx context.Context) *apis.FieldError {
+	if psv := GetPodSpecValidator(ctx); psv != nil {
+		return psv(ctx, wp)
+	}
+	return nil
+}
+
+// psvKey is used for associating a PodSpecValidator with a context.Context
+type psvKey struct{}
+
+func WithPodSpecValidator(ctx context.Context, psv PodSpecValidator) context.Context {
+	return context.WithValue(ctx, psvKey{}, psv)
+}
+
+// GetPodSpecValidator extracts the PodSpecValidator from the context.
+func GetPodSpecValidator(ctx context.Context) PodSpecValidator {
+	untyped := ctx.Value(psvKey{})
+	if untyped == nil {
+		return nil
+	}
+	return untyped.(PodSpecValidator)
+}


Test patch:
diff --git a/apis/duck/v1/podspec_defaults_test.go b/apis/duck/v1/podspec_defaults_test.go
new file mode 100644
index 000000000..ed1b378af
--- /dev/null
+++ b/apis/duck/v1/podspec_defaults_test.go
@@ -0,0 +1,99 @@
+/*
+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 v1
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestPodSpecDefaulting(t *testing.T) {
+	p := WithPod{
+		Spec: WithPodSpec{
+			Template: PodSpecable{
+				Spec: corev1.PodSpec{
+					Containers: []corev1.Container{{
+						Name:  "blah",
+						Image: "busybox",
+					}},
+				},
+			},
+		},
+	}
+
+	tests := []struct {
+		name string
+		with func(context.Context) context.Context
+		want *WithPod
+	}{{
+		name: "no check",
+		with: func(ctx context.Context) context.Context {
+			return ctx
+		},
+		want: p.DeepCopy(),
+	}, {
+		name: "no change",
+		with: func(ctx context.Context) context.Context {
+			return WithPodSpecDefaulter(ctx, func(ctx context.Context, wp *WithPod) {
+			})
+		},
+		want: p.DeepCopy(),
+	}, {
+		name: "no busybox",
+		with: func(ctx context.Context) context.Context {
+			return WithPodSpecDefaulter(ctx, func(ctx context.Context, wp *WithPod) {
+				for i, c := range wp.Spec.Template.Spec.InitContainers {
+					if !strings.Contains(c.Image, "@") {
+						wp.Spec.Template.Spec.InitContainers[i].Image = c.Image + "@sha256:deadbeef"
+					}
+				}
+				for i, c := range wp.Spec.Template.Spec.Containers {
+					if !strings.Contains(c.Image, "@") {
+						wp.Spec.Template.Spec.Containers[i].Image = c.Image + "@sha256:deadbeef"
+					}
+				}
+			})
+		},
+		want: &WithPod{
+			Spec: WithPodSpec{
+				Template: PodSpecable{
+					Spec: corev1.PodSpec{
+						Containers: []corev1.Container{{
+							Name:  "blah",
+							Image: "busybox@sha256:deadbeef",
+						}},
+					},
+				},
+			},
+		},
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ctx := test.with(context.Background())
+			got := p.DeepCopy()
+			got.SetDefaults(ctx)
+			if !cmp.Equal(test.want, got) {
+				t.Errorf("SetDefaults (-want, +got) = %s", cmp.Diff(test.want, got))
+			}
+		})
+	}
+}
diff --git a/apis/duck/v1/podspec_validation_test.go b/apis/duck/v1/podspec_validation_test.go
new file mode 100644
index 000000000..933f6efee
--- /dev/null
+++ b/apis/duck/v1/podspec_validation_test.go
@@ -0,0 +1,87 @@
+/*
+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 v1
+
+import (
+	"context"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"knative.dev/pkg/apis"
+)
+
+func TestPodSpecValidation(t *testing.T) {
+	tests := []struct {
+		name string
+		with func(context.Context) context.Context
+		want *apis.FieldError
+	}{{
+		name: "no check",
+		with: func(ctx context.Context) context.Context {
+			return ctx
+		},
+		want: nil,
+	}, {
+		name: "no error",
+		with: func(ctx context.Context) context.Context {
+			return WithPodSpecValidator(ctx, func(ctx context.Context, wp *WithPod) *apis.FieldError {
+				return nil
+			})
+		},
+		want: nil,
+	}, {
+		name: "no busybox",
+		with: func(ctx context.Context) context.Context {
+			return WithPodSpecValidator(ctx, func(ctx context.Context, wp *WithPod) *apis.FieldError {
+				for i, c := range wp.Spec.Template.Spec.InitContainers {
+					if c.Image == "busybox" {
+						return apis.ErrInvalidValue(c.Image, "image").ViaFieldIndex("spec.template.spec.initContainers", i)
+					}
+				}
+				for i, c := range wp.Spec.Template.Spec.Containers {
+					if c.Image == "busybox" {
+						return apis.ErrInvalidValue(c.Image, "image").ViaFieldIndex("spec.template.spec.containers", i)
+					}
+				}
+				return nil
+			})
+		},
+		want: apis.ErrInvalidValue("busybox", "spec.template.spec.containers[0].image"),
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			p := WithPod{
+				Spec: WithPodSpec{
+					Template: PodSpecable{
+						Spec: corev1.PodSpec{
+							Containers: []corev1.Container{{
+								Name:  "blah",
+								Image: "busybox",
+							}},
+						},
+					},
+				},
+			}
+			ctx := test.with(context.Background())
+			got := p.Validate(ctx)
+			if test.want.Error() != got.Error() {
+				t.Errorf("Validate() = %v, wanted %v", got, test.want)
+			}
+		})
+	}
+}