Add validation webhook deployment by jsafrane · Pull Request #62 · openshift/cluster-csi-snapshot-controller-operator

jsafrane · 2020-11-09T14:27:16Z

Add deployment of upstream validation webhook as a separate Controller that handles webhook Deployment.

Move OperatorClient to a standalone package/
Since there are two controllers running in parallel now, make sure that whichever controller finishes the first sync, it sets also Unknown condition of the other controller. Otherwise the whole operator could be Available: true too early.

ValidatingWebhookConfiguration is deployed by CVO, which is technically wrong - it will overwrite caBundle injected by another operator. There is https://github.com/openshift/library-go/pull/836/files in progress that should fix this issue.

WIP:

~~get downstream image and update related-images~~ We have the images now.

@openshift/storage

openshift-ci-robot · 2020-11-09T14:27:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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

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

jsafrane · 2020-11-09T17:07:27Z

/retest

jsafrane · 2020-12-02T12:00:06Z

/retest

jsafrane · 2020-12-02T13:46:01Z

/hold
we need ART images built, should be tomorrow.

bertinatto · 2020-12-02T14:12:13Z

manifests/09_webhook_config.yaml

+  labels:
+    app: csi-snapshot-webhook
+  annotations:
+    service.beta.openshift.io/inject-cabundle: "true"


Don't we need to specify the self-managed-high-availability profile? Same question for the Service manifest

bertinatto · 2020-12-02T14:21:06Z

pkg/operator/webhookdeployment/webhook.go

+
+	queue workqueue.RateLimitingInterface
+
+	stopCh <-chan struct{}


Not necessary since we're using library-go

bertinatto · 2020-12-02T14:22:29Z

pkg/operator/webhookdeployment/webhook.go

+	opSpec, opStatus, _, err := c.client.GetOperatorState()
+	if err != nil {
+		return err
+	}


IMO it's a good practice to check for NotFound errors here (even though we don't check in the other controller)

bertinatto · 2020-12-02T14:40:41Z

pkg/operator/webhookdeployment/webhook.go

+	return factory.New().WithSync(c.sync).WithSyncDegradedOnError(client).WithInformers(
+		client.Informer(),
+		deployInformer.Informer(),
+	).ToController(WebhookControllerName, eventRecorder)


It could be helpful to add a suffix to this recorder in order to distinguish this controller (eventrecorder.WithComponentSuffix(...)

bertinatto · 2020-12-02T14:42:58Z

pkg/operator/webhookdeployment/webhook.go

+		return err
+	}
+	lastGeneration := resourcemerge.ExpectedDeploymentGeneration(deployment, opStatus.Generations)
+	deployment, _, err = resourceapply.ApplyDeployment(c.kubeClient.AppsV1(), c.eventRecorder, deployment, lastGeneration)


You should use syncContext.Recorder() to get the right event recover (and also get rid of the one specified in csiSnapshotWebhookController

bertinatto · 2020-12-02T14:46:54Z

pkg/operator/webhookdeployment/webhook.go

+		deploymentAvailable.Status = operatorapi.ConditionTrue
+	} else {
+		deploymentAvailable.Status = operatorapi.ConditionFalse
+		deploymentAvailable.Reason = "WaitDeployment"


In library-go the reason was set to Deploying: bertinatto/library-go@fa1bce1

Do we want to keep the Reason consistent with CSI driver operators?

Used Deploying everywhere

bertinatto · 2020-12-02T14:47:54Z

pkg/operator/webhookdeployment/webhook.go

+	}
+	if deployment.Status.ObservedGeneration != deployment.Generation {
+		deploymentProgressing.Status = operatorapi.ConditionTrue
+		deploymentProgressing.Reason = "NewGeneration"


Same question about the Reason

Used Deploying everywhere

bertinatto · 2020-12-02T15:03:57Z

pkg/operator/webhookdeployment/webhook.go

+			if deployment.Status.UpdatedReplicas == *deployment.Spec.Replicas {
+				deploymentProgressing.Status = operatorapi.ConditionFalse
+				// All replicas were updated, set the version
+				c.versionGetter.SetVersion(webhookVersionName, c.operandVersion)


In theory Available could still be false here. In that case, setting the operand version here would be incorrect.

Also, we should only report operator version only after all operands have been rolled out:

https://github.com/openshift/api/blob/master/config/v1/types_cluster_operator.go#L45

However, currently we report the operator version in the other controller, which has no idea if the webhook operand has been rolled out yet.

How can we solve this? Another controller only to set the operator version? 🤔

It sets its own version, webhookVersionName - versions is an array. So there are two distinct version, which can should converge to the same value. Honestly, I don't know what CVO does with this array :-)

Used Deploying everywhere

I mean, I'm concerned about 2 things:

Setting webhookVersionName should be OK here, but I think we need to make sure we do so only when Progressing=false and Available=true. Currently it's setting when Progressing=false, which is too soon.

Since we're introducing a new operand (the webhook), we need to change this part of the code to only set the operator when both operands (webhook and the snapshot-controller) have rolled out (currently it only accounts for the snapshot-controller):

https://github.com/openshift/cluster-csi-snapshot-controller-operator/blob/master/pkg/operator/sync.go#L170

I completely removed the version, I need to think about it more.

bertinatto · 2020-12-02T15:16:10Z

pkg/operator/webhookdeployment/webhook.go

+	}
+
+	updateGenerationFn := func(newStatus *operatorapi.OperatorStatus) error {
+		if deployment != nil {


Would it have panicked above if deployment were nil?

It would :-)
Removed the check... What could possibly go wrong, right? :-D

bertinatto · 2020-12-02T15:17:59Z

pkg/operator/webhookdeployment/webhook.go

+	); err != nil {
+		return err
+	}
+	return nil


nit: just return err

bertinatto · 2020-12-02T17:50:04Z

assets/webhook_deployment.yaml

+      tolerations:
+      - key: "node-role.kubernetes.io/master"
+        operator: Exists
+        effect: NoSchedule


CriticalAddonsOnly toleration?

Actually the other operand deployment has a few other tolerations. I suppose it makes sense they are consistent?

cluster-csi-snapshot-controller-operator/assets/csi_controller_deployment.yaml

Lines 34 to 44 in 530c59e

- key: "node.kubernetes.io/unreachable"

operator: "Exists"

effect: "NoExecute"

tolerationSeconds: 120

- key: "node.kubernetes.io/not-ready"

operator: "Exists"

effect: "NoExecute"

tolerationSeconds: 120

- key: node-role.kubernetes.io/master

operator: Exists

effect: "NoSchedule"

Copied tolerations from the controller

bertinatto · 2020-12-02T18:08:50Z

assets/webhook_deployment.yaml

+        resources:
+          requests:
+            cpu: 10m
+      serviceAccountName: csi-snapshot-controller-operator


Is this what we want (csi-snapshot-controller-operator)?

The webhook does not talk to Kubernetes API at all. Should it have a separate service account?

Should this webhook have a SA at all?

The multus webhook doesn't need one: https://github.com/openshift/multus-cni/blob/master/deployment/webhook/deployment.yaml

Removed the service account, it will get the default one from the namespace.

jsafrane · 2020-12-03T08:29:59Z

/retest

jsafrane · 2020-12-03T13:44:03Z

/hold cancel
Brew says it was built for rhaos-4.7-rhel-8-candidate.

bertinatto · 2020-12-03T14:41:07Z

pkg/operator/webhookdeployment/webhook_test.go

+func TestSync(t *testing.T) {
+	const replica0 = 0
+	const replica1 = 1
+	const replica2 = 2


I think you're not using these consts

bertinatto

/lgtm

bertinatto · 2020-12-03T17:56:42Z

/retest

bertinatto

/hold

To check the secret issue.

bertinatto · 2020-12-03T18:46:02Z

assets/webhook_deployment.yaml

+      volumes:
+      - name: certs
+        secret:
+          secretName: csi-snapshot-webhook-secret


Where is this being created?

From service.beta.openshift.io/inject-cabundle annotation,

Don't use the controller image for webhook Deployment.

bertinatto · 2020-12-03T19:54:05Z

/hold cancel

jsafrane · 2020-12-03T21:06:18Z

/test e2e-aws

jsafrane · 2020-12-03T21:07:46Z

The webhook pod fails to start:

panic: listen tcp :443: bind: permission denied

Use port 8443 to listen for the webhook.

jsafrane · 2020-12-04T09:00:22Z

It was installed! https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-csi-snapshot-controller-operator/62/pull-ci-openshift-cluster-csi-snapshot-controller-operator-master-e2e-aws/1334612973666177024

And the webhook runs and admits snapshots: https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_cluster-csi-snapshot-controller-operator/62/pull-ci-openshift-cluster-csi-snapshot-controller-operator-master-e2e-aws/1334612973666177024/artifacts/e2e-aws/gather-extra/pods/openshift-cluster-storage-operator_csi-snapshot-webhook-75f64c6865-snt8b_webhook.log

(which reminds me to check the log levels, this is too noisy)
/retest

bertinatto

/lgtm

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 Nov 9, 2020

openshift-ci-robot requested review from bertinatto and gnufied November 9, 2020 14:27

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

Move OperatorClient to a separate directory

8ce110c

jsafrane force-pushed the add-webhook branch 5 times, most recently from 2b3e04a to 58d787f Compare November 30, 2020 14:10

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

jsafrane force-pushed the add-webhook branch from 58d787f to 530c59e Compare November 30, 2020 19:59

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

jsafrane changed the title ~~WIP: Add validation webhook deployment~~ \Add validation webhook deployment Dec 2, 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 Dec 2, 2020

jsafrane changed the title ~~\Add validation webhook deployment~~ Add validation webhook deployment Dec 2, 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 Dec 2, 2020

bertinatto reviewed Dec 2, 2020

View reviewed changes

jsafrane force-pushed the add-webhook branch from d289b9f to 61d86b3 Compare December 2, 2020 20:40

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

jsafrane force-pushed the add-webhook branch 2 times, most recently from f28293e to d5f6409 Compare December 3, 2020 14:13

bertinatto reviewed Dec 3, 2020

View reviewed changes

Add controller for CSi snapshot webhook Deployment

b2b96e2

jsafrane force-pushed the add-webhook branch from d5f6409 to b2b96e2 Compare December 3, 2020 15:25

bertinatto reviewed Dec 3, 2020

View reviewed changes

openshift-ci-robot assigned bertinatto Dec 3, 2020

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

bertinatto reviewed Dec 3, 2020

View reviewed changes

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed lgtm Indicates that a PR is ready to be merged. labels Dec 3, 2020

Fix webhook Deployment image

d4f16ce

Don't use the controller image for webhook Deployment.

jsafrane force-pushed the add-webhook branch from 26f8879 to d4f16ce Compare December 3, 2020 19:29

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

Use non-privileged port

35eb335

Use port 8443 to listen for the webhook.

jsafrane force-pushed the add-webhook branch from 20409a3 to 35eb335 Compare December 3, 2020 21:37

bertinatto reviewed Dec 4, 2020

View reviewed changes

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

openshift-merge-robot merged commit 332a311 into openshift:master Dec 4, 2020

	- key: "node.kubernetes.io/unreachable"
	operator: "Exists"
	effect: "NoExecute"
	tolerationSeconds: 120
	- key: "node.kubernetes.io/not-ready"
	operator: "Exists"
	effect: "NoExecute"
	tolerationSeconds: 120
	- key: node-role.kubernetes.io/master
	operator: Exists
	effect: "NoSchedule"

Comments

Conversation

jsafrane commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 9, 2020

Uh oh!

jsafrane commented Nov 9, 2020

Uh oh!

jsafrane commented Dec 2, 2020

Uh oh!

jsafrane commented Dec 2, 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!

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!

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!

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!

bertinatto Dec 2, 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!

Choose a reason for hiding this comment

Uh oh!

jsafrane commented Dec 3, 2020

Uh oh!

jsafrane commented Dec 3, 2020

jsafrane commented Nov 9, 2020 •

edited

Loading

bertinatto Dec 2, 2020 •

edited

Loading

jsafrane commented Dec 4, 2020 •

edited

Loading