Bug 1826446: (fix) Admission Webhook names must be unique #1489

awgreene · 2020-04-28T22:17:07Z

Problem:
OLM currently creates all ValidatingWebhookConfigurations and
MutatingWebhookConfigurations using the name provided in the
WebhookDescription. This causes an issue when an operator that defines
an admission webhook in the CSV is installed in multiple namespaces,
causes the two operators to fight over the admission webhook.

Solution:
Generate a unique name for each webhook using the GenerateName field in
the Object Metadata.

openshift-ci-robot · 2020-04-28T22:18:54Z

@awgreene: This pull request references Bugzilla bug 1826446, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1826446: (fix) Admission Webhook names must be unique

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.

awgreene · 2020-04-29T02:59:15Z

/retest

harishsurf

Looks good. I had some review comments for the webhooks test. For ginkgo conversion, you could refer to these slides. There is also a cheatsheet

test/e2e/webhook_e2e_test.go

awgreene · 2020-04-29T15:12:10Z

Looks good. I had some review comments for the webhooks test. For ginkgo conversion, you could refer to these slides. There is also a cheatsheet
@harishsurf I knew this was coming! Already working on it!

dinhxuanvu

Looking good.

pkg/controller/operators/olm/apiservices.go

pkg/controller/install/webhook.go

pkg/controller/operators/olm/operator.go

awgreene · 2020-04-30T13:54:43Z

@dinhxuanvu I've addressed your comment, could you please review?

awgreene · 2020-04-30T15:12:53Z

/retest

could not wait for build: the build root failed after 2m11s with reason FetchSourceFailed: Failed to fetch the input source.

awgreene · 2020-04-30T16:23:00Z

/retest

release "release-latest" failed: the pod ci-op-7482wfws/release-latest failed after 18s (failed containers: release): ContainerFailed one or more containers exited
Container release exited with code 1, reason Error

pkg/controller/install/webhook.go

awgreene · 2020-05-01T18:36:20Z

/hold
Working on Ginkgo e2e tests.

exdx · 2020-05-01T18:37:28Z

/lgtm
/retest

harishsurf

Thanks Alex, for taking the time to convert the tests to ginkgo. I have some comments. Please ping me if you have any questions.

test/e2e/webhook_e2e_test.go

pkg/controller/install/webhook.go

harishsurf · 2020-05-04T19:14:34Z

/lgtm for ginkgo tests

awgreene · 2020-05-05T16:15:00Z

/test e2e-gcp

ERROR: (gcloud.compute.instance-groups.list-instances) could not parse resource []

awgreene · 2020-05-05T19:23:45Z

/retest

pkg/controller/install/webhook.go

ecordell · 2020-05-05T20:37:43Z

/lgtm

njhale

Thanks for fixing this!

Blocking: I had one primary concern about reusing the Name field.

Non-blocking: In general I think we should focus on ways to deduplicate our code. For example, the methods for validating and mutating webhooks here are nearly identical; one possible way to simplify might be to use a dynamic client and add an additional prototype layer -- WebhookConfig -- and pass them to a single function.

njhale · 2020-05-05T21:23:59Z

pkg/controller/install/webhook.go

 			return err
 		}
 	} else {
-		return err
-	}
+		for _, webhook := range existingWebhooks.Items {


nit: we can avoid the surrounding else statement by returning at the end of the if block above.

njhale · 2020-05-05T21:27:18Z

pkg/controller/install/webhook.go

+	webhookLabels := ownerutil.OwnerLabel(i.owner, i.owner.GetObjectKind().GroupVersionKind().Kind)
+	webhookLabels[WebhookDescKey] = desc.Name
+	webhookSelector := labels.SelectorFromSet(webhookLabels).String()


nit: this might make a nice helper

njhale · 2020-05-05T21:37:27Z

pkg/controller/install/webhook.go

+	return nil
+}
+
+const WebhookDescKey = "webhookDescriptionName"


Note: eventually, we should update OLM's use of labels to follow the k8s label conventions.

In this case:

Key names should be all lowercase, with words separated by dashes instead of camelCase
For instance, prefer foo.kubernetes.io/foo-bar over foo.kubernetes.io/fooBar, prefer desired-replicas over DesiredReplicas

njhale · 2020-05-05T21:38:08Z

pkg/controller/install/webhook.go

 			return err
 		}
 	} else {
-		return err
+		for _, webhook := range existingWebhooks.Items {


nit: we can avoid the else here too.

pkg/controller/install/webhook.go

njhale · 2020-05-06T13:20:38Z

/hold

Awaiting operator-framework/api#34 and a new api version tag.

Problem: OLM currently creates all ValidatingWebhookConfigurations and MutatingWebhookConfigurations using the name provided in the WebhookDescription. This causes an issue when an operator that defines an admission webhook in the CSV is installed in multiple namespaces, causes the two operators to fight over the admission webhook. Solution: Generate a unique name for each webhook using the GenerateName field in the Object Metadata.

njhale · 2020-05-07T01:49:05Z

/retest

njhale · 2020-05-07T02:02:40Z

I think this looks good.

Note: I think the e2e tests need some style tweaks, but we can follow up later.

/hold cancel
/approve

openshift-ci-robot · 2020-05-07T02:02:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, njhale

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

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

dinhxuanvu · 2020-05-07T02:13:30Z

/lgtm

openshift-ci-robot · 2020-05-07T03:37:55Z

@awgreene: All pull requests linked via external trackers have merged: operator-framework/operator-lifecycle-manager#1489. Bugzilla bug 1826446 has been moved to the MODIFIED state.

In response to this:

Bug 1826446: (fix) Admission Webhook names must be unique

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.

openshift-ci-robot requested review from exdx and gallettilance April 28, 2020 22:17

awgreene changed the title ~~(fix) Admission Webhook names must be unique~~ Bug 1826446: (fix) Admission Webhook names must be unique Apr 28, 2020

openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Apr 28, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 28, 2020

harishsurf reviewed Apr 29, 2020

View reviewed changes

test/e2e/webhook_e2e_test.go Outdated Show resolved Hide resolved

test/e2e/webhook_e2e_test.go Outdated Show resolved Hide resolved

test/e2e/webhook_e2e_test.go Outdated Show resolved Hide resolved

test/e2e/webhook_e2e_test.go Outdated Show resolved Hide resolved

dinhxuanvu requested changes Apr 29, 2020

View reviewed changes

awgreene force-pushed the webhook-unique-names branch 2 times, most recently from f4e2ea5 to 531372d Compare April 30, 2020 13:43

exdx reviewed May 1, 2020

View reviewed changes

pkg/controller/install/webhook.go Outdated Show resolved Hide resolved

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

openshift-ci-robot assigned exdx May 1, 2020

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

awgreene force-pushed the webhook-unique-names branch 2 times, most recently from 2b5cc29 to e396f4a Compare May 4, 2020 17:47

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 4, 2020

harishsurf reviewed May 4, 2020

View reviewed changes

benluddy reviewed May 4, 2020

View reviewed changes

pkg/controller/install/webhook.go Outdated Show resolved Hide resolved

awgreene force-pushed the webhook-unique-names branch from e396f4a to d447c11 Compare May 4, 2020 19:05

awgreene force-pushed the webhook-unique-names branch 2 times, most recently from ad4bd9e to 470403c Compare May 5, 2020 01:51

awgreene force-pushed the webhook-unique-names branch from 49aad40 to 9a9ba6a Compare May 5, 2020 14:38

awgreene force-pushed the webhook-unique-names branch from 9a9ba6a to 5466275 Compare May 5, 2020 16:56

ecordell reviewed May 5, 2020

View reviewed changes

pkg/controller/install/webhook.go Outdated Show resolved Hide resolved

openshift-ci-robot assigned ecordell May 5, 2020

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

njhale reviewed May 5, 2020

View reviewed changes

awgreene mentioned this pull request May 5, 2020

Update webhook description operator-framework/api#34

Merged

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

awgreene force-pushed the webhook-unique-names branch from 5466275 to d816cb4 Compare May 6, 2020 22:38

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 6, 2020

awgreene force-pushed the webhook-unique-names branch 2 times, most recently from 2461433 to e2b30f9 Compare May 6, 2020 22:49

awgreene mentioned this pull request May 6, 2020

Bump OLM version to 0.15.0 #1503

Merged

5 tasks

awgreene force-pushed the webhook-unique-names branch from e2b30f9 to 61b9d60 Compare May 6, 2020 23:12

awgreene force-pushed the webhook-unique-names branch from 61b9d60 to c6819bb Compare May 6, 2020 23:30

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

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

openshift-ci-robot assigned dinhxuanvu May 7, 2020

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

openshift-merge-robot merged commit 6544650 into operator-framework:master May 7, 2020

Bug 1826446: (fix) Admission Webhook names must be unique #1489

Bug 1826446: (fix) Admission Webhook names must be unique #1489

Uh oh!

Conversation

awgreene commented Apr 28, 2020

Uh oh!

openshift-ci-robot commented Apr 28, 2020

Uh oh!

awgreene commented Apr 29, 2020

Uh oh!

harishsurf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awgreene commented Apr 29, 2020

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

awgreene commented Apr 30, 2020

Uh oh!

awgreene commented Apr 30, 2020

Uh oh!

awgreene commented Apr 30, 2020

Uh oh!

Uh oh!

awgreene commented May 1, 2020

Uh oh!

exdx commented May 1, 2020

Uh oh!

harishsurf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

harishsurf commented May 4, 2020

Uh oh!

awgreene commented May 5, 2020

Uh oh!

awgreene commented May 5, 2020

Uh oh!

Uh oh!

ecordell commented May 5, 2020

Uh oh!

njhale left a comment

Choose a reason for hiding this comment

Uh oh!

njhale May 5, 2020

Choose a reason for hiding this comment

Uh oh!

njhale May 5, 2020

Choose a reason for hiding this comment

Uh oh!

njhale May 5, 2020

Choose a reason for hiding this comment

Uh oh!

njhale May 5, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

njhale commented May 6, 2020

Uh oh!

njhale commented May 7, 2020

Uh oh!

njhale commented May 7, 2020