OCPCLOUD-913 - Tests for webhook configurations provisioning by MAO #176

Danil-Grigorev · 2020-07-17T11:54:58Z

Currently, we are managing webhooks from MAO, as CVO is not expected to handle resources with changing spec by another operator at the upgrade time. The implementation is covered with unit tests, but we need to ensure cluster-actuator-pkg contains e2e tests for such implementation too.

This PR is satisfying this requirement.

Danil-Grigorev · 2020-07-17T13:41:15Z

/retest

JoelSpeed

Looks good, not sure about a couple of the tests, left some minor suggestions otherwise :)

JoelSpeed · 2020-07-17T14:06:33Z

pkg/framework/webhooks.go

+		if err := c.Delete(context.TODO(), webhookConfiguraiton); err != nil {
+			klog.Errorf("error querying api for ValidatingWebhookConfiguration object %q: %v, retrying...", webhookConfiguraiton.Name, err)
+			return false, nil
+		}
+		return true, nil


What about trying to delete a resource that doesn't exist? Should we account for that?

JoelSpeed · 2020-07-17T14:06:38Z

pkg/framework/webhooks.go

+	return wait.PollImmediate(RetryShort, WaitShort, func() (bool, error) {
+		if err := c.Delete(context.TODO(), webhookConfiguraiton); err != nil {
+			klog.Errorf("error querying api for MutatingWebhookConfiguration object %q: %v, retrying...", webhookConfiguraiton.Name, err)
+			return false, nil
+		}
+		return true, nil


What about trying to delete a resource that doesn't exist? Should we account for that?

JoelSpeed · 2020-07-17T14:09:40Z

pkg/framework/webhooks.go

+// IsMutatingWebhookConfigurationMatched expects a matching MutatingWebhookConfiguration to be present in the cluster
+func IsMutatingWebhookConfigurationMatched(c client.Client) bool {


Maybe this should be called Synced rather than Matched, since we are checking if MAO has synced it to what we expect right?

JoelSpeed · 2020-07-17T14:09:44Z

pkg/framework/webhooks.go

+}
+
+// IsValidatingWebhookConfigurationMatched expects a matching MutatingWebhookConfiguration to be present in the cluster
+func IsValidatingWebhookConfigurationMatched(c client.Client) bool {


Maybe this should be called Synced rather than Matched, since we are checking if MAO has synced it to what we expect right?

JoelSpeed · 2020-07-17T14:10:16Z

pkg/operators/machine-api-operator.go

+	It("reconcile webhook configuration", func() {
+		client, err := framework.LoadClient()
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(framework.IsMutatingWebhookConfigurationMatched(client)).To(BeTrue())
+	})
+
+	It("reconcile validating webhook configuration", func() {
+		client, err := framework.LoadClient()
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(framework.IsValidatingWebhookConfigurationMatched(client)).To(BeTrue())
+	})


One of these specifies the type, the other doesn't, should we make them both specify?

JoelSpeed · 2020-07-17T14:15:07Z

pkg/operators/machine-api-operator.go

+
+		Expect(framework.UpdateMutatingWebhookConfiguration(client, toUpdate)).To(Succeed())
+
+		Expect(framework.IsMutatingWebhookConfigurationMatched(client)).To(BeTrue())


Surely after updating it can never match the default again, because there will be no CA Bundle in there?

JoelSpeed · 2020-07-17T14:15:14Z

pkg/operators/machine-api-operator.go

+
+		Expect(framework.UpdateValidatingWebhookConfiguration(client, toUpdate)).To(Succeed())
+
+		Expect(framework.IsValidatingWebhookConfigurationMatched(client)).To(BeTrue())


Surely after updating it can never match the default again, because there will be no CA Bundle in there?

It's the place where DeepDerivative would still fit, as it will ignore differences with empty field on the default resource. So it will still match :)

Danil-Grigorev · 2020-07-20T10:17:03Z

/retest

Danil-Grigorev · 2020-07-20T12:03:26Z

/retest

JoelSpeed

Ok, LGTM, added a couple of suggestions for adding a comment, WDYT?

JoelSpeed · 2020-07-21T11:36:24Z

pkg/framework/webhooks.go

+			return false, nil
+		}
+
+		equal := equality.Semantic.DeepDerivative(DefaultMutatingWebhookConfiguration.Webhooks, existing.Webhooks)


Might be worth a note that this will should the CABundle field that has been set by the injector and that's why we are using DeepDerivative

JoelSpeed · 2020-07-21T11:36:27Z

pkg/framework/webhooks.go

+			return false, nil
+		}
+
+		equal := equality.Semantic.DeepDerivative(DefaultValidatingWebhookConfiguration.Webhooks, existing.Webhooks)


Might be worth a note that this will should the CABundle field that has been set by the injector and that's why we are using DeepDerivative

Danil-Grigorev · 2020-07-22T08:36:59Z

@JoelSpeed I added a small comment on the DeepDerivative usage.

JoelSpeed · 2020-07-22T10:07:30Z

/approve

Thanks!

openshift-ci-robot · 2020-07-22T10:07:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

Danil-Grigorev · 2020-07-22T13:43:15Z

/retest

enxebre · 2020-07-23T12:07:31Z

pkg/framework/webhooks.go

+
+// UpdateMutatingWebhookConfiguration updates the specified mutating webhook configuration
+func UpdateMutatingWebhookConfiguration(c client.Client, updated *admissionregistrationv1.MutatingWebhookConfiguration) error {
+	return wait.PollImmediate(RetryShort, WaitMedium, func() (bool, error) {


why WaitMedium here?
this is GetMutatingWebhookConfiguration and so nesting wait.PollImmediate calls.

I think the only purpose of using wait.PollImmediate in this library funcs is to protect and don't fail the test on a transient network issue? to that end we could may be just wait a few seconds? wdyt?

Sounds good to me. Webhooks are ready to react as soon as they are updated in the cluster. I could see potential to use apply method from library-ho, but short wait should be enough in any case.

@enxebre I decreased both wait periods to be at 1 min at max, in case of the client throttling appears, and 3 min for waiting on webhook configuration to be synced, PTAL

enxebre · 2020-07-23T13:30:34Z

thanks, not a blocker for this PR but my point is that I think even WaitShort is too much here. This are not eventually consistent functions. This are atomic actions, the only reason to wait.poll here, if any, is protection against network transient issues. For that a shorter e.g 5s wait period would be reasonable. WDYT?
/lgtm

openshift-bot · 2020-07-23T13:41:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-23T13:55:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-23T14:47:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-23T16:05:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-23T17:48:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-23T19:35:20Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T12:02:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T13:20:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T13:33:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T13:59:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T14:38:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T14:51:22Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T15:56:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T17:01:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T17:14:26Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T18:58:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-24T20:16:16Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Danil-Grigorev · 2020-07-24T20:28:32Z

/hold until #181 passes

Danil-Grigorev · 2020-07-25T14:37:22Z

/hold cancel
/retest

openshift-bot · 2020-07-25T16:11:18Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T16:50:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T17:03:19Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T17:42:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T19:13:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T20:18:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T21:23:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T21:36:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T22:15:14Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T22:28:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T23:07:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-25T23:20:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-07-26T00:24:33Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-operator	`52fcced`	link	`/test e2e-gcp-operator`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

openshift-bot · 2020-07-26T00:25:15Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Revendor MAO with webhooks implementation

f6310f0

openshift-ci-robot requested review from JoelSpeed and elmiko July 17, 2020 11:55

Danil-Grigorev changed the title ~~Webhooks mao e2e~~ OCPCLOUD-913 - Tests for webhook configurations provisioning by MAO Jul 17, 2020

JoelSpeed reviewed Jul 17, 2020

View reviewed changes

Danil-Grigorev force-pushed the webhooks-mao-e2e branch from f537152 to a1fe11c Compare July 20, 2020 08:40

Danil-Grigorev requested a review from JoelSpeed July 20, 2020 10:04

JoelSpeed reviewed Jul 21, 2020

View reviewed changes

Extend testing framework with MAO webhooks

d6d46d9

Danil-Grigorev force-pushed the webhooks-mao-e2e branch from a1fe11c to 343c357 Compare July 22, 2020 08:34

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2020

enxebre reviewed Jul 23, 2020

View reviewed changes

Add tests for webhook resources provisioned by MAO

52fcced

Danil-Grigorev force-pushed the webhooks-mao-e2e branch from 343c357 to 52fcced Compare July 23, 2020 12:22

openshift-ci-robot assigned enxebre Jul 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020

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

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

openshift-merge-robot merged commit d98f9a7 into openshift:master Jul 26, 2020

		// IsMutatingWebhookConfigurationMatched expects a matching MutatingWebhookConfiguration to be present in the cluster
		func IsMutatingWebhookConfigurationMatched(c client.Client) bool {


		Expect(framework.UpdateMutatingWebhookConfiguration(client, toUpdate)).To(Succeed())

		Expect(framework.IsMutatingWebhookConfigurationMatched(client)).To(BeTrue())


		Expect(framework.UpdateValidatingWebhookConfiguration(client, toUpdate)).To(Succeed())

		Expect(framework.IsValidatingWebhookConfigurationMatched(client)).To(BeTrue())

OCPCLOUD-913 - Tests for webhook configurations provisioning by MAO #176

OCPCLOUD-913 - Tests for webhook configurations provisioning by MAO #176

Uh oh!

Conversation

Danil-Grigorev commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danil-Grigorev commented Jul 17, 2020

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Jul 20, 2020

Uh oh!

Danil-Grigorev commented Jul 20, 2020

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Jul 22, 2020

Uh oh!

JoelSpeed commented Jul 22, 2020

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

Danil-Grigorev commented Jul 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enxebre commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 23, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Uh oh!

openshift-bot commented Jul 24, 2020

Danil-Grigorev commented Jul 17, 2020 •

edited

Loading

openshift-ci-robot commented Jul 26, 2020 •

edited

Loading