⚠ (:warning:, major) Webhook support in envtest #787

alenkacz · 2020-02-01T13:46:25Z

Ability to test webhooks with envtest

lots of code is reused from PoC done in #645

Fixes #563

Co-authored-by: Solly Ross sollyross@google.com

alenkacz · 2020-02-01T13:47:56Z

/assign DirectXMan12 gerred

/hold

@DirectXMan12 I want to add you as co-author, what mail should I use? :-)

alenkacz · 2020-02-01T14:54:20Z

/retest

alenkacz · 2020-02-01T15:21:58Z

strange, the test failure seems like it should be unrelated but it fails consistently so I'll check it out...

EDIT: Oh funny, that test started failing because I changed this --admission-control=AlwaysAdmit to --enable-admission-plugins=ValidatingAdmissionWebhook and the namespace really does not exist

alenkacz · 2020-02-03T10:41:08Z

pkg/cache/cache_test.go

@@ -96,6 +96,14 @@ func CacheTest(createCacheFunc func(config *rest.Config, opts cache.Options) (ca
 			Expect(cfg).NotTo(BeNil())

 			By("creating three pods")
+			cl, err := client.New(cfg, client.Options{})


this is because of #787 (comment)

pkg/envtest/server.go

DirectXMan12

left a few comments. generally looks good

pkg/envtest/envtest_test.go

pkg/envtest/server.go

pkg/envtest/webhook.go

pkg/envtest/envtest_suite_test.go

vincepri · 2020-02-03T16:55:40Z

pkg/envtest/webhook.go

+	"k8s.io/apimachinery/pkg/util/wait"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	admissionreg "k8s.io/api/admissionregistration/v1beta1"


1.16 adds webhook/v1, should we update to that version to avoid the same issue we had with CRDs?

what was that issue? I was talking with joe about proper support of v1 webhooks, I'll be creating an issue for that. But I assume this is something more subtle... :-)

I was more suggesting we start with webhooks/v1 instead of v1beta1. If we don't, we might have to add support later in the future.

We had the same issue for envtest CRD, and to be backward compatible we have to handle both versions, which makes the whole code a little tricky. #752

oh I see, yeah that makes sense to me if @DirectXMan12 is OK with that...

i agree with this. if there's an interface here too, can we introduce a helper that uses this interface? if not....ok then.

basically I want to abstract the version from the object as much as possible and generally select the client's storage version (as we would with CRDs).

generally 👍 on latest version selection from our level of abstraction, and binding CR release versions to certain core GVK versions.

I think probably the safest method here is some sort of helper that can deal with v1 and v1beta1 webhooks like we do with CRDs

since we're prob stuck with v1beta1 for at least a bit longer

(basically should be able to copy the code from the CRD handling logic, more-or-less)

pkg/envtest/webhook.go

alenkacz · 2020-02-04T09:56:27Z

@vincepri @DirectXMan12 should be ready for re-review

alenkacz · 2020-02-04T15:59:44Z

/hold cancel

alenkacz · 2020-02-04T16:00:57Z

/hold cancel

pkg/envtest/server.go

pkg/envtest/webhook.go

alenkacz · 2020-02-28T12:15:44Z

/retest

mengqiy

A few nits. Other places LGTM

pkg/envtest/envtest_suite_test.go

pkg/envtest/webhook.go

pkg/envtest/webhook_test.go

pkg/envtest/webhook.go

alenkacz · 2020-02-29T18:55:12Z

@mengqiy thanks for the review, you can do another pass :)

DirectXMan12

let's fix the namespace plugin backwards compat issue, then we can merge this

mengqiy · 2020-03-04T21:07:10Z

/retest

alenkacz · 2020-03-05T12:33:21Z

pkg/internal/testing/integration/internal/apiserver.go

@@ -9,7 +9,8 @@ var APIServerDefaultArgs = []string{
 	"--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}",
 	"--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}",
 	"--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}",
-	"--admission-control=AlwaysAdmit",
+	"--enable-admission-plugins=MutatingAdmissionWebhook,ValidatingAdmissionWebhook",
+	"--disable-admission-plugins=NamespaceLifecycle,LimitRanger,ServiceAccount,TaintNodesByCondition,Priority,DefaultTolerationSeconds,DefaultStorageClass,StorageObjectInUseProtection,PersistentVolumeClaimResize,RuntimeClass,ResourceQuota",


this is to be backward compatible

alenkacz · 2020-03-05T16:33:42Z

/retest

Co-authored-by: Solly Ross <sollyross@google.com>

mengqiy · 2020-03-05T19:11:26Z

@alenkacz Don't forget to create the followup PR with the breaking change after this PR merges.
/lgtm
/approve

k8s-ci-robot · 2020-03-05T19:14:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alenkacz, mengqiy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mengqiy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 1, 2020

k8s-ci-robot requested review from mengqiy and vincepri February 1, 2020 13:46

k8s-ci-robot assigned DirectXMan12 Feb 1, 2020

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2020

k8s-ci-robot assigned gerred Feb 1, 2020

alenkacz force-pushed the av/envtest-webhook branch from a0dce79 to 62590e8 Compare February 1, 2020 13:53

alenkacz force-pushed the av/envtest-webhook branch 2 times, most recently from 44265a2 to c0c55b0 Compare February 3, 2020 10:40

alenkacz commented Feb 3, 2020

View reviewed changes

pkg/envtest/server.go Outdated Show resolved Hide resolved

alenkacz force-pushed the av/envtest-webhook branch from c0c55b0 to e4f7c03 Compare February 3, 2020 10:45

DirectXMan12 suggested changes Feb 3, 2020

View reviewed changes

pkg/envtest/envtest_test.go Show resolved Hide resolved

pkg/envtest/server.go Outdated Show resolved Hide resolved

pkg/envtest/webhook.go Outdated Show resolved Hide resolved

pkg/envtest/envtest_suite_test.go Show resolved Hide resolved

vincepri reviewed Feb 3, 2020

View reviewed changes

alenkacz force-pushed the av/envtest-webhook branch from e4f7c03 to d0bf5bb Compare February 4, 2020 09:54

alenkacz force-pushed the av/envtest-webhook branch 2 times, most recently from 58e638f to 0a2957d Compare February 4, 2020 10:38

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2020

gerred reviewed Feb 5, 2020

View reviewed changes

pkg/envtest/server.go Show resolved Hide resolved

gerred reviewed Feb 5, 2020

View reviewed changes

pkg/envtest/webhook.go Outdated Show resolved Hide resolved

DirectXMan12 mentioned this pull request Feb 5, 2020

[wip] ⚠️ Secure Serving & Webhook Support in envtest #645

Closed

alenkacz force-pushed the av/envtest-webhook branch from 0a2957d to 1522566 Compare February 7, 2020 10:29

alenkacz force-pushed the av/envtest-webhook branch from 987f485 to 898d49b Compare February 28, 2020 10:17

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2020

alenkacz force-pushed the av/envtest-webhook branch from 898d49b to fca2a6d Compare February 28, 2020 10:47

alenkacz changed the title ~~WIP: ⚠ (:warning:, major) Webhook support in envtest~~ ⚠ (:warning:, major) Webhook support in envtest Feb 28, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 28, 2020

alenkacz force-pushed the av/envtest-webhook branch 3 times, most recently from 99e4cac to 007c9a8 Compare February 28, 2020 11:02

mengqiy reviewed Feb 28, 2020

View reviewed changes

alenkacz force-pushed the av/envtest-webhook branch from 007c9a8 to 79ba043 Compare February 29, 2020 18:54

alenkacz force-pushed the av/envtest-webhook branch from 79ba043 to f1ae851 Compare March 4, 2020 09:18

DirectXMan12 suggested changes Mar 4, 2020

View reviewed changes

alenkacz force-pushed the av/envtest-webhook branch 2 times, most recently from 88c1091 to 88442a4 Compare March 5, 2020 12:32

alenkacz commented Mar 5, 2020

View reviewed changes

alenkacz force-pushed the av/envtest-webhook branch 2 times, most recently from 92e6872 to 606079a Compare March 5, 2020 16:07

Webhook support in envtest

528cd19

Co-authored-by: Solly Ross <sollyross@google.com>

alenkacz force-pushed the av/envtest-webhook branch from 606079a to 528cd19 Compare March 5, 2020 17:20

k8s-ci-robot assigned mengqiy Mar 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 5, 2020

k8s-ci-robot merged commit bfc9827 into kubernetes-sigs:master Mar 5, 2020

mengqiy mentioned this pull request Apr 25, 2020

Come up with webhook developer workflow to test it locally kubernetes-sigs/kubebuilder#400

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠ (:warning:, major) Webhook support in envtest #787

⚠ (:warning:, major) Webhook support in envtest #787

alenkacz commented Feb 1, 2020 •

edited

Loading

alenkacz commented Feb 1, 2020

alenkacz commented Feb 1, 2020

alenkacz commented Feb 1, 2020 •

edited

Loading

alenkacz Feb 3, 2020

DirectXMan12 left a comment

vincepri Feb 3, 2020

alenkacz Feb 4, 2020

vincepri Feb 4, 2020

alenkacz Feb 4, 2020

gerred Feb 5, 2020

DirectXMan12 Feb 20, 2020

DirectXMan12 Feb 20, 2020

DirectXMan12 Feb 20, 2020

alenkacz commented Feb 4, 2020

alenkacz commented Feb 4, 2020 •

edited

Loading

alenkacz commented Feb 4, 2020

alenkacz commented Feb 28, 2020

mengqiy left a comment

alenkacz commented Feb 29, 2020

DirectXMan12 left a comment

mengqiy commented Mar 4, 2020

alenkacz Mar 5, 2020

alenkacz commented Mar 5, 2020

mengqiy commented Mar 5, 2020

k8s-ci-robot commented Mar 5, 2020

⚠ (:warning:, major) Webhook support in envtest #787

⚠ (:warning:, major) Webhook support in envtest #787

Conversation

alenkacz commented Feb 1, 2020 • edited Loading

alenkacz commented Feb 1, 2020

alenkacz commented Feb 1, 2020

alenkacz commented Feb 1, 2020 • edited Loading

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alenkacz commented Feb 4, 2020

alenkacz commented Feb 4, 2020 • edited Loading

alenkacz commented Feb 4, 2020

alenkacz commented Feb 28, 2020

mengqiy left a comment

Choose a reason for hiding this comment

alenkacz commented Feb 29, 2020

DirectXMan12 left a comment

Choose a reason for hiding this comment

mengqiy commented Mar 4, 2020

Choose a reason for hiding this comment

alenkacz commented Mar 5, 2020

mengqiy commented Mar 5, 2020

k8s-ci-robot commented Mar 5, 2020

alenkacz commented Feb 1, 2020 •

edited

Loading

alenkacz commented Feb 1, 2020 •

edited

Loading

alenkacz commented Feb 4, 2020 •

edited

Loading