Admission webhoooks #1436

awgreene · 2020-04-08T20:49:48Z

This PR introduces OLM support for validating and mutating admission
webhooks by updating the ClusterServiceVersion CRD to include a
WebhooksDefinitions array. In the current iteration, OLM supports
cycling the CA Certs required for webhooks.

awgreene · 2020-04-08T20:51:18Z

pkg/api/wrappers/deployment_install_client.go

@@ -45,6 +47,14 @@ func NewInstallStrategyDeploymentClient(opClient operatorclient.ClientInterface,
 	}
 }

+func (c *InstallStrategyDeploymentClientForNamespace) GetOpClient() operatorclient.ClientInterface {


Should be cleaned up in refactor.

awgreene · 2020-04-08T20:51:23Z

pkg/api/wrappers/deployment_install_client.go

+	return c.opClient
+}
+
+func (c *InstallStrategyDeploymentClientForNamespace) GetOpLister() operatorlister.OperatorLister {


Should be cleaned up in refactor.

pkg/controller/operators/olm/operator.go

pkg/controller/operators/olm/apiservices.go

pkg/controller/install/apiservice.go

pkg/api/apis/operators/clusterserviceversion_types.go

estroz

Two suggestions

pkg/controller/install/webhook.go

pkg/controller/operators/olm/operator.go

jmazzitelli · 2020-04-10T19:17:30Z

How will this be exposed to Ansible operators?

awgreene · 2020-04-11T17:14:48Z

/retest

awgreene · 2020-04-11T19:30:30Z

/retest

awgreene · 2020-04-12T15:15:38Z

/retest

The following tests failed but ran locally

creation with dependencies [It]
image update [It]

awgreene · 2020-04-13T03:17:04Z

/retest

ecordell · 2020-04-13T14:10:24Z

How will this be exposed to Ansible operators?

That's probably a question for operator-sdk. This is for packaging / installing an admission webhook with your operator, and doesn't have any opinions about how the webhook is implemented. @jmazzitelli

harishsurf · 2020-04-13T23:04:11Z

pkg/api/apis/operators/clusterserviceversion_types.go

@@ -163,6 +246,7 @@ type ClusterServiceVersionSpec struct {
 	Maturity                  string
 	CustomResourceDefinitions CustomResourceDefinitions
 	APIServiceDefinitions     APIServiceDefinitions
+	WebhookDefinitions        []WebhookDescription


typo: the slice should be []WebhookDefinitions ?

I wanted to keep it similar to APIServiceDefinitions, I'm not opposed to changing the variable (API) name though.

pkg/api/apis/operators/v1alpha1/clusterserviceversion.go

harishsurf · 2020-04-13T23:21:43Z

pkg/api/apis/operators/v1alpha1/clusterserviceversion_types.go

@@ -191,6 +276,8 @@ type ClusterServiceVersionSpec struct {
 	CustomResourceDefinitions CustomResourceDefinitions `json:"customresourcedefinitions,omitempty"`
 	APIServiceDefinitions     APIServiceDefinitions     `json:"apiservicedefinitions,omitempty"`
 	// +listType=set
+	WebhookDefinitions []WebhookDescription `json:"webhookdefinitions,omitempty"`


typo: the slice should be []WebhookDefinitions ?

See this comment

ecordell

This looks really good, and I think you've set it up well for the planned refactoring to these parts.

I left a few comments for small things, the only thing that I'm concerned about addressing is the 0.14.2 release (should be my first comment in this review)

deploy/ocp/manifests/latest

pkg/controller/install/caresources.go

ecordell · 2020-04-14T00:10:22Z

pkg/controller/install/deployment.go

+		previousStrategy:       previousStrategy,
+		templateAnnotations:    templateAnnotations,
+		initializers:           initializers,
+		apiServiceDescriptions: apiDescs,


refactor suggestion: is there a way we can model the apidescriptions/webhookdescriptions as just more initializer functions?

pkg/controller/install/webhook.go

ecordell · 2020-04-14T00:14:03Z

pkg/controller/operators/olm/operator.go

@@ -1366,6 +1367,14 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			return
 		}

+		// Check if Webhooks have valid rules
+		for _, desc := range out.Spec.WebhookDefinitions {
+			if syncError = install.ValidWebhookRules(desc.Rules); syncError != nil {


future nit: we should tie these validations to the the validation library used in opm

pkg/controller/operators/olm/operator_test.go

test/e2e/csv_e2e_test.go

awgreene · 2020-04-15T22:25:52Z

/test e2e-gcp

awgreene · 2020-04-15T23:11:12Z

/retest

awgreene · 2020-04-16T00:30:38Z

/retest

awgreene · 2020-04-16T02:20:55Z

/retest

awgreene · 2020-04-16T03:58:35Z

/retest

openshift-bot · 2020-04-16T05:02:28Z

/retest

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

openshift-bot · 2020-04-16T05:41:22Z

/retest

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

openshift-bot · 2020-04-16T08:43:24Z

/retest

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

awgreene · 2020-04-16T13:30:50Z

/retest

awgreene · 2020-04-16T14:16:48Z

Closed and reopened to kick off tests after another PR was merged.

awgreene · 2020-04-16T15:47:18Z

/retest

awgreene · 2020-04-16T16:40:54Z

/retest

awgreene · 2020-04-16T17:47:10Z

/retest

awgreene · 2020-04-16T18:25:13Z

/retest

This commit introduces a change that moves the APIService install logic to the installer.

This commit introduces OLM support for validating and mutating admission webhooks by updating the ClusterServiceVersion CRD to include a WebhooksDefinitions array. In the current iteration, OLM supports cycling the CA Certs required for webhooks.

openshift-ci-robot · 2020-04-16T19:01:53Z

New changes are detected. LGTM label has been removed.

awgreene · 2020-04-16T19:31:40Z

/retest

awgreene · 2020-04-16T20:43:39Z

/retest

awgreene · 2020-04-16T21:05:35Z

/retest

openshift-ci-robot · 2020-04-16T23:24:31Z

@awgreene: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp	`468d1f6`	link	`/test e2e-gcp`
ci/prow/e2e-gcp-upgrade	`468d1f6`	link	`/test e2e-gcp-upgrade`

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.

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.

awgreene · 2020-04-17T00:28:19Z

Passing e2e-aws, e2e-gcp is broken right now - manually merging as a result

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 Apr 8, 2020

openshift-ci-robot requested review from anik120 and Bowenislandsong April 8, 2020 20:50

awgreene commented Apr 8, 2020

View reviewed changes

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

awgreene commented Apr 8, 2020

View reviewed changes

pkg/controller/operators/olm/apiservices.go Outdated Show resolved Hide resolved

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

awgreene commented Apr 8, 2020

View reviewed changes

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

ecordell reviewed Apr 8, 2020

View reviewed changes

pkg/api/apis/operators/clusterserviceversion_types.go Outdated Show resolved Hide resolved

awgreene force-pushed the admission-webhoooks branch 2 times, most recently from 77bc8a4 to 2c60954 Compare April 8, 2020 22:17

estroz reviewed Apr 8, 2020

View reviewed changes

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

pkg/controller/operators/olm/operator.go Outdated Show resolved Hide resolved

awgreene force-pushed the admission-webhoooks branch 4 times, most recently from ec8d5e3 to 2c029d5 Compare April 9, 2020 01:00

ecordell mentioned this pull request Apr 10, 2020

WIP: Webhook Admissions #1102

Closed

5 tasks

awgreene force-pushed the admission-webhoooks branch from 2c029d5 to 46ce701 Compare April 13, 2020 15:55

awgreene changed the title ~~WIP: Admission webhoooks~~ Admission webhoooks Apr 13, 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 Apr 13, 2020

awgreene force-pushed the admission-webhoooks branch from e77a2bf to 8838e17 Compare April 13, 2020 23:21

harishsurf reviewed Apr 13, 2020

View reviewed changes

ecordell reviewed Apr 14, 2020

View reviewed changes

estroz mentioned this pull request Apr 14, 2020

generate csv: support validating and mutating webhooks operator-framework/operator-sdk#2729

Merged

awgreene closed this Apr 16, 2020

awgreene reopened this Apr 16, 2020

awgreene added 2 commits April 16, 2020 15:01

Move APIService to installer

bde5fe7

This commit introduces a change that moves the APIService install logic to the installer.

OLM Support for Admission Webhooks

468d1f6

This commit introduces OLM support for validating and mutating admission webhooks by updating the ClusterServiceVersion CRD to include a WebhooksDefinitions array. In the current iteration, OLM supports cycling the CA Certs required for webhooks.

awgreene force-pushed the admission-webhoooks branch from 2ca6a24 to 468d1f6 Compare April 16, 2020 19:01

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

awgreene merged commit 3854141 into operator-framework:master Apr 17, 2020

awgreene mentioned this pull request Apr 25, 2020

Adding certificate docs openshift/openshift-docs#18254

Merged

aharbis mentioned this pull request Apr 28, 2020

How to retrieve ClusterRole in operator pod installed via OLM operator-framework/operator-sdk#2925

Closed

awgreene mentioned this pull request May 19, 2020

Add Admission Webhook Document operator-framework/olm-docs#17

Merged

Admission webhoooks #1436

Admission webhoooks #1436

Conversation

awgreene commented Apr 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz left a comment

Choose a reason for hiding this comment

jmazzitelli commented Apr 10, 2020

awgreene commented Apr 11, 2020

awgreene commented Apr 11, 2020

awgreene commented Apr 12, 2020

awgreene commented Apr 13, 2020

ecordell commented Apr 13, 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

ecordell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awgreene commented Apr 15, 2020

awgreene commented Apr 15, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

openshift-bot commented Apr 16, 2020

openshift-bot commented Apr 16, 2020

openshift-bot commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

openshift-ci-robot commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

awgreene commented Apr 16, 2020

openshift-ci-robot commented Apr 16, 2020

awgreene commented Apr 17, 2020

awgreene commented Apr 8, 2020 •

edited

Loading

ecordell commented Apr 13, 2020 •

edited

Loading