`generate`: correctly set CSV webhookDefinition deployment names #3761

estroz · 2020-08-19T19:18:33Z

Description of the change:

internal/generate: set webhook deployment name by searching for deployments selected by webhook services

Motivation for the change: an element of a CSV's spec.webhookDefinitions's deployment name was being inferred from the webhook config's service reference name, which often does not match the deployment's name. Instead, we can look up what service a webhook is using, then use the service's label selector to look up the deployment via labels on the pods for which the service will proxy.

/cc @varshaprasad96

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

varshaprasad96 · 2020-08-24T15:09:31Z

We need to not mention the volume mount point of server certs/ ports of web server in csv (though this is optional). However, there one another issue with the naming of certificates because of which #3762 and this are blocked.
Created an issue with controller runtime: kubernetes-sigs/controller-runtime#1135

hasbro17 · 2020-09-14T21:54:33Z

Does this need a CHANGELOG framgent?

hasbro17 · 2020-09-14T22:03:52Z

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

 	csv.Spec.WebhookDefinitions = webhookDescriptions
 }

+var defaultAdmissionReviewVersions = []string{"v1beta1"}


This goes to v1 webhooks once we bump to controller-tools v0.4.0 right? Or rather plugins v3 I think.

This has to do with what version of admission reviews the webhook server serves, which is still v1beta1 on controller-runtime's master.

Ah okay that's not linked to wehooks v1 then, although I imagine we'll want to bump that to v1 as well soon.

hasbro17 · 2020-09-14T22:36:48Z

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

+		sel := dep.Spec.Template.GetLabels()
+		// A null label selector matches no objects.
+		if sel == nil {
+			continue
+		}
+		// An empty label selector matches all objects.
+		if len(sel) == 0 {
+			depName = dep.GetName()
+			break
+		}


Are you looking at the deployment's selector which is spec.Selector or the pod labels here?

If it's just the pod labels the var should be renamed to reflect that: sel := dep.Spec.Template.GetLabels() to podLabels := dep.Spec.Template.GetLabels()
Also the comments saying label selector.

Plus what does An empty label selector matches all objects mean here?
The label selector can't be empty. It's a required field. But even if it is, what does that signify here.

Originally I was using the dep's selector, but that proved unnecessary (and much more difficult) because the webhook service already has a selector. Therefore the variable name and empty selector check are irrelevant, will change/remove.

Additionally the label comparison needs to check if service.Spec.Selector is a subset of dep.Spec.Template.GetLabels(), since the service provides the label constraint here.

That makes more sense, thanks for clarifying.

hasbro17

LGTM

varshaprasad96 · 2020-09-15T03:08:55Z

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

+	if len(description.AdmissionReviewVersions) == 0 {
+		description.AdmissionReviewVersions = defaultAdmissionReviewVersions
+	}
+


Can we have a check here to see if description.SideEffects is null, if so set it to SideEffectClassNone? Because having it null causes installplan to fail.

I’ll do this in a follow-up. Ideally I wouldn’t be modifying admission review versions in this PR either since they’re unrelated to the deployment name fix, so I’ll break that out too.

varshaprasad96 · 2020-09-15T03:23:33Z

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

 	if serviceRef := webhook.ClientConfig.Service; serviceRef != nil {
 		if serviceRef.Port != nil {
 			description.ContainerPort = *serviceRef.Port
 		}


Also, if serviceRef.Port == nil and description.ContainerPort is not set, can we default it to 9443 which is used in case of cert-manager. It is a required field which needs to be defined in WebhookDescription. Since 443 (which olm defaults to) is restricted to root user, I suppose setting it to 9443 is a better option?

Will do in a follow-up.

varshaprasad96 · 2020-09-15T03:25:31Z

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

+	if len(description.AdmissionReviewVersions) == 0 {
+		description.AdmissionReviewVersions = defaultAdmissionReviewVersions
+	}
+


The same sideEffects and containerPort as in validating WH needs to be set here too.

varshaprasad96 · 2020-09-15T03:31:36Z

internal/generate/clusterserviceversion/clusterserviceversion_updaters.go

+// findMatchingDeploymentAndServiceForWebhook matches a Service to a webhook's client config (if it uses a service)
+// then matches that Service to a Deployment by comparing label selectors (if the Service uses label selectors).
+// The names of both Service and Deployment are returned if found.
+func findMatchingDeploymentAndServiceForWebhook(c *collector.Manifests, wcc admissionregv1.WebhookClientConfig) (depName, serviceName string) {


I am not sure if this is in scope of this PR. But currently, if using Webhooks we need not specify volumeMounts and webhook server port in CSV. Could we have a check for that, and accordingly not append ContainerVolumeMounts in Deployment.

I think this will need discussion before changing. We can’t remove all volume mounts and/or ports since these will probably contain other values set by users; in general the generator should avoid modifying declarative manifests like that, which risks changing what they declare.

Instead we need to properly patch the deployment via kustomize before passing it to the generator. @joelanford had a thought or two on this.

…yments selected by webhook services

joelanford · 2020-09-17T16:48:41Z

/cherry-pick v1.0.x

openshift-cherrypick-robot · 2020-09-17T16:48:50Z

@joelanford: new pull request created: #3904

In response to this:

/cherry-pick v1.0.x

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 a review from varshaprasad96 August 19, 2020 19:18

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 19, 2020

estroz mentioned this pull request Aug 19, 2020

[WIP] generate: support conversion webhooks #3762

Closed

2 tasks

estroz changed the title ~~[WIP] generate: correctly set CSV webhookDefinition deployment names~~ generate: correctly set CSV webhookDefinition deployment names Sep 14, 2020

openshift-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 Sep 14, 2020

hasbro17 reviewed Sep 14, 2020

View reviewed changes

estroz force-pushed the bugfix/set-webhook-deployment-name branch 2 times, most recently from 4965a8f to b44edf5 Compare September 14, 2020 23:49

hasbro17 approved these changes Sep 15, 2020

View reviewed changes

varshaprasad96 reviewed Sep 15, 2020

View reviewed changes

estroz force-pushed the bugfix/set-webhook-deployment-name branch from b44edf5 to f1b533a Compare September 15, 2020 16:40

internal/generate: set webhook deployment name by searching for deplo…

71e21aa

…yments selected by webhook services

estroz force-pushed the bugfix/set-webhook-deployment-name branch from f1b533a to 71e21aa Compare September 15, 2020 16:54

estroz merged commit f3a767b into operator-framework:master Sep 17, 2020

estroz deleted the bugfix/set-webhook-deployment-name branch September 17, 2020 16:43

openshift-cherrypick-robot mentioned this pull request Sep 17, 2020

[v1.0.x] generate: correctly set CSV webhookDefinition deployment names #3904

Merged

varshaprasad96 mentioned this pull request Sep 17, 2020

generate: default CSV admissionReviewVersions and sideEffects #3903

Merged

2 tasks

varshaprasad96 mentioned this pull request Nov 8, 2020

Error while running conversion webhooks with OLM #4206

Closed

generate: correctly set CSV webhookDefinition deployment names #3761

generate: correctly set CSV webhookDefinition deployment names #3761

Uh oh!

Conversation

estroz commented Aug 19, 2020

Uh oh!

varshaprasad96 commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasbro17 commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

hasbro17 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!

varshaprasad96 Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

varshaprasad96 Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford commented Sep 17, 2020

Uh oh!

openshift-cherrypick-robot commented Sep 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

`generate`: correctly set CSV webhookDefinition deployment names #3761

`generate`: correctly set CSV webhookDefinition deployment names #3761

varshaprasad96 commented Aug 24, 2020 •

edited

Loading

hasbro17 commented Sep 14, 2020 •

edited

Loading

varshaprasad96 Sep 15, 2020 •

edited

Loading

varshaprasad96 Sep 15, 2020 •

edited

Loading