Enable webhooks in envtest #3209

Boutheina-Dab · 2020-06-18T12:45:19Z

What this PR does / why we need it:
This PR is to enable webhooks in envtest

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged): Fixes #2788

k8s-ci-robot · 2020-06-18T12:45:27Z

Welcome @Boutheina-Dab!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-06-18T12:45:27Z

Hi @Boutheina-Dab. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fabriziopandini · 2020-06-18T13:40:45Z

/ok-to-test

detiber · 2020-06-18T15:38:20Z

test/helpers/envtest.go

+								Rule: admissionv1beta1.Rule{
+									APIGroups:   []string{"apps"},
+									APIVersions: []string{"v1"},
+									Resources:   []string{"deployments"},


Is this just a placeholder for testing?

Is the goal to make this parameterized in a way to deploy the webhooks for the CRD resources we are generating? If so, would it be possible to leverage the generated yaml for this somehow? I'd like to see if we can avoid having to replicate content manually for tests where it could get out of sync with the annotations/generated content.

It's to enable webhooks in envtest.
This PR is to fix the issue #2788

@Boutheina-Dab my apologies for not being a bit more clear, I was referencing the hardcoding of apps.v1.deployments here along with the various other properties of the webhooks (side effects, path, mutating vs non-mutating, etc)

@detiber no problem. I got it. Where can I find the generated yaml file of the CRD resources? Sorry, but I'm still new in the codebase of capi.

If I may add my two cents to this discussion,
If our goal is to add webhooks for the core types, I think that we should try to use the webhook yaml configuration from

/config/webhook

boostrap/kubeadm/config/webhook

controlplane/kubeadm/config/webhook

Instead of hardcoding a copy of the webhook definition here, so there is no effort for keeping things in sync.

From a quick look at the controller runtime code, this can be achieved by setting WebhookInstallOptions.DirectoryPaths instead of WebhookInstallOptions. ValidatingWebhooks/MutatingWebhooks (even do it is not clear to me if calling kustomize is on us or on controller runtime)

thanks @fabriziopandini Actually, yes, I was working on that this morning, and I tried to set DirectoryPaths: []string{filepath.Join("config", "webhooks")}, (instead of WebhookInstallOptions. ValidatingWebhooks/MutatingWebhooks). I am testing it. But, didn't include yet:

boostrap/kubeadm/config/webhook

controlplane/kubeadm/config/webhook
(As long as I understand, only manifest yaml file is needed to install webhook, do we need to call kustomization?)

That's a good question.
AFAIK, kustomization.yaml defines some patches for the webhooks, like e.g. webhookcainjection_patch.yaml, but TBH, I'm not sure this is required for envtest.
@vincepri ^^

In the meantime, you can experiment on using the manifest alone

It should work using those directly, although it might be worthwhile in the future to run kustomize in memory but let's tackle that when controller-runtime has support for conversion webhooks as well

Boutheina-Dab · 2020-06-19T16:23:19Z

@Boutheina-Dab: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-test 61b82f8 link /test pull-cluster-api-test
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

/retest

fabriziopandini · 2020-06-22T10:34:59Z

test/helpers/envtest.go

@@ -114,6 +121,27 @@ func NewTestEnvironment() *TestEnvironment {
 	}
 }

+func initializeWebhookInEnvironment() {
+	namespacedScopeV1Beta1 := admissionv1beta1.NamespacedScope


Tests are failing because those variables are declared but not used.
(In case you don't already know) you can use make lint or make lint-full (a little bit slower) to check these types of problems locally before pushing new changes

Boutheina-Dab · 2020-06-22T15:51:23Z

@Boutheina-Dab: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-test 61b82f8 link /test pull-cluster-api-test
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

/reset

vincepri · 2020-08-20T15:47:04Z

Makefile

+	-killall -9 etcd
+	-killall -9 kube-apiserver


Any reason we need these killall directives? Shouldn't envtest shutdown at the end of each test?

I think so. envtest should shutdown at the end of each test. In fact "make clean-envtest" does not find any process to kill ..

Let's remove it if that's the case? wdyt?

test/helpers/envtest.go

vincepri · 2020-08-20T15:58:36Z

test/helpers/envtest.go

+
+	klog.V(2).Infof("Waiting for webhook port %d to be open prior to running tests", port)
+	timeout := time.Second
+	notOpen := true


Seems this is unused?

WaitForWebhooks() function is used in suites_test here: https://github.com/Boutheina-Dab/cluster-api/blob/432f4986cf09388ccb46f6212bf43af74c32d3e0/controllers/suite_test.go#L118

test/helpers/envtest.go

vincepri · 2020-08-21T14:10:55Z

api/v1alpha3/machinehealthcheck_webhook_test.go

+func Init() {
+	minNodeStartupTimeout = metav1.Duration{Duration: 1 * time.Millisecond}
+}


Suggested change

func Init() {

minNodeStartupTimeout = metav1.Duration{Duration: 1 * time.Millisecond}

}

func init() {

minNodeStartupTimeout = metav1.Duration{Duration: 1 * time.Millisecond}

}

@vincepri init() function does not set minNodeStartupTimeout to 1ms (see test output) for envtest.
That's why I was adding the func SetminNodeStartupTimeoutForTest() in the envtest, which seems like it fixes the issue

You're right, the init function only works when the api/v1alpha3 package is tested, what about something like this?

diff --git a/api/v1alpha3/machinehealthcheck_webhook.go b/api/v1alpha3/machinehealthcheck_webhook.go index 67b99bfba..d951cccdd 100644 --- a/api/v1alpha3/machinehealthcheck_webhook.go +++ b/api/v1alpha3/machinehealthcheck_webhook.go @@ -18,6 +18,7 @@ package v1alpha3 import ( "fmt" + "sync" "time" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -36,10 +37,23 @@ var ( // 10 minutes should allow the instance to start and the node to join the // cluster on most providers. defaultNodeStartupTimeout = metav1.Duration{Duration: 10 * time.Minute} + // Minimum time allowed for a node to start up - minNodeStartupTimeout = metav1.Duration{Duration: 30 * time.Second} + minNodeStartupTimeout = metav1.Duration{Duration: 30 * time.Second} + minNodeStartupTimeoutOnce sync.Once ) +// SetMinNodeStartupTimeout allows users to optionally set a custom timeout +// for the validation webhook. +// +// This function is mostly used within envtest (integration tests), and should +// never be used in a production environment. +func SetMinNodeStartupTimeout(d metav1.Duration) { + minNodeStartupTimeoutOnce.Do(func() { + minNodeStartupTimeout = d + }) +} + func (m *MachineHealthCheck) SetupWebhookWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). For(m).

It gives this: Exptected failure, but got no error. here: https://github.com/Boutheina-Dab/cluster-api/blob/45a4b6eaa83cc3f81872d1a959da3440b6fe6d65/api/v1alpha3/machinehealthcheck_webhook_test.go#L198

api/v1alpha3/machinehealthcheck_webhook.go

test/helpers/envtest.go

randomvariable · 2020-08-24T16:57:15Z

api/v1alpha3/machinehealthcheck_webhook.go

+	minNodeStartupTimeoutOnce.Do(func() {
+		minNodeStartupTimeout = d
+	})


@vincepri, this code doesn't work (i.e. causes MHC tests to fail) unless GODEBUG=asyncpreemptoff=1 is used.

Works fine if it's not wrapped in the sync.once construct.

This is with Go 1.15

Is this related to the CI failures? We can avoid using sync.Once, and do a plain unsafe set. We can fix it later using the atomic package (I don't think this would be a problem though).

Can you provide more information on why it'd make tests fail? To my understanding, we're still using Go 1.13.x in CI

Yeah, it's causing the CI failures (at commit 25686716d). Have no idea why it's making the tests fail, it's just if I remove it it works again.

randomvariable · 2020-08-25T15:07:58Z

Need to address linter statements

test/helpers/envtest.go:277:2: S1000: should use for range instead of for { select {} } (gosimple)
	for {
	^
test/helpers/envtest.go:278:3: S1037: should use time.Sleep instead of elaborate way of sleeping (gosimple)
		select {
		^

CecileRobertMichon · 2020-08-25T19:10:11Z

@Boutheina-Dab please make sure to squash the commits before this merges

randomvariable · 2020-08-26T10:28:49Z

Can you rewrite the commit message "commit after rebase with master", and then can lgtm.

randomvariable · 2020-08-26T16:03:40Z

/lgtm

vincepri

/approve

Great work @Boutheina-Dab ! Thank you for working on this 🎉

k8s-ci-robot · 2020-08-26T20:23:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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~~ [vincepri]

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

Boutheina-Dab · 2020-08-27T07:47:23Z

/approve

Great work @Boutheina-Dab ! Thank you for working on this 🎉

Thanks all for your review and comments! Thanks @randomvariable for your precious help :)

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 18, 2020

k8s-ci-robot requested review from justinsb and ncdc June 18, 2020 12:45

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 18, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 18, 2020

detiber reviewed Jun 18, 2020

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 19, 2020

fabriziopandini reviewed Jun 22, 2020

View reviewed changes

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 22, 2020

Boutheina-Dab force-pushed the capi-webhooks-envtest branch from cbf8cf7 to 6f00f92 Compare June 23, 2020 11:43

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 1, 2020

vincepri reviewed Aug 20, 2020

View reviewed changes

Boutheina-Dab force-pushed the capi-webhooks-envtest branch from 3d7eb4e to a36fb79 Compare August 21, 2020 12:58

vincepri reviewed Aug 21, 2020

View reviewed changes

api/v1alpha3/machinehealthcheck_webhook.go Outdated Show resolved Hide resolved

vincepri reviewed Aug 21, 2020

View reviewed changes

test/helpers/envtest.go Outdated Show resolved Hide resolved

Boutheina-Dab force-pushed the capi-webhooks-envtest branch from 1777053 to 426f75f Compare August 24, 2020 15:09

vincepri reviewed Aug 24, 2020

View reviewed changes

test/helpers/envtest.go Outdated Show resolved Hide resolved

randomvariable reviewed Aug 24, 2020

View reviewed changes

ncdc changed the title ~~Capi webhooks envtest~~ Enable webhooks in envtest Aug 25, 2020

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

Boutheina-Dab force-pushed the capi-webhooks-envtest branch from 28059e9 to 434b8f1 Compare August 26, 2020 09:21

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

Boutheina-Dab force-pushed the capi-webhooks-envtest branch from 434b8f1 to 7623ecf Compare August 26, 2020 10:51

Boutheina-Dab mentioned this pull request Aug 26, 2020

🌱 mhc: Ensure log output when returning errors to controller runtime #3531

Merged

Boutheina-Dab added 2 commits August 26, 2020 17:53

Add selector to CRS test

dfdca63

enable webhooks in envtest

9637747

Boutheina-Dab force-pushed the capi-webhooks-envtest branch from 7623ecf to 9637747 Compare August 26, 2020 16:02

k8s-ci-robot assigned randomvariable Aug 26, 2020

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

vincepri approved these changes Aug 26, 2020

View reviewed changes

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

k8s-ci-robot merged commit 9b29fa5 into kubernetes-sigs:master Aug 26, 2020

randomvariable mentioned this pull request Aug 27, 2020

Resolve unit test flake in MHC #3539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable webhooks in envtest #3209

Enable webhooks in envtest #3209

Boutheina-Dab commented Jun 18, 2020 •

edited by ncdc

Loading

k8s-ci-robot commented Jun 18, 2020

k8s-ci-robot commented Jun 18, 2020

fabriziopandini commented Jun 18, 2020

detiber Jun 18, 2020

Boutheina-Dab Jun 18, 2020

detiber Jun 18, 2020

Boutheina-Dab Jun 19, 2020

fabriziopandini Jun 19, 2020 •

edited

Loading

Boutheina-Dab Jun 19, 2020

fabriziopandini Jun 19, 2020

vincepri Jun 19, 2020

Boutheina-Dab commented Jun 19, 2020 •

edited

Loading

fabriziopandini Jun 22, 2020

Boutheina-Dab commented Jun 22, 2020

vincepri Aug 20, 2020

Boutheina-Dab Aug 20, 2020

vincepri Aug 21, 2020

vincepri Aug 20, 2020

Boutheina-Dab Aug 21, 2020

vincepri Aug 21, 2020

Boutheina-Dab Aug 21, 2020 •

edited

Loading

vincepri Aug 21, 2020

Boutheina-Dab Aug 21, 2020

randomvariable Aug 24, 2020 •

edited

Loading

vincepri Aug 24, 2020 •

edited

Loading

randomvariable Aug 24, 2020 •

edited

Loading

randomvariable commented Aug 25, 2020

CecileRobertMichon commented Aug 25, 2020

randomvariable commented Aug 26, 2020

randomvariable commented Aug 26, 2020

vincepri left a comment

k8s-ci-robot commented Aug 26, 2020

Boutheina-Dab commented Aug 27, 2020

Enable webhooks in envtest #3209

Enable webhooks in envtest #3209

Conversation

Boutheina-Dab commented Jun 18, 2020 • edited by ncdc Loading

k8s-ci-robot commented Jun 18, 2020

k8s-ci-robot commented Jun 18, 2020

fabriziopandini commented Jun 18, 2020

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

fabriziopandini Jun 19, 2020 • edited Loading

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

Boutheina-Dab commented Jun 19, 2020 • edited Loading

Choose a reason for hiding this comment

Boutheina-Dab commented Jun 22, 2020

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

Boutheina-Dab Aug 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomvariable Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

vincepri Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

randomvariable Aug 24, 2020 • edited Loading

Choose a reason for hiding this comment

randomvariable commented Aug 25, 2020

CecileRobertMichon commented Aug 25, 2020

randomvariable commented Aug 26, 2020

randomvariable commented Aug 26, 2020

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 26, 2020

Boutheina-Dab commented Aug 27, 2020

Boutheina-Dab commented Jun 18, 2020 •

edited by ncdc

Loading

fabriziopandini Jun 19, 2020 •

edited

Loading

Boutheina-Dab commented Jun 19, 2020 •

edited

Loading

Boutheina-Dab Aug 21, 2020 •

edited

Loading

randomvariable Aug 24, 2020 •

edited

Loading

vincepri Aug 24, 2020 •

edited

Loading

randomvariable Aug 24, 2020 •

edited

Loading