Add Validation and Mutating Webhook support #15

raukadah · 2024-11-26T13:56:34Z

This pull-request adds following things based on webhooks doc:

Validation and Mutating webhooks for watcher, WatcherAPI, WatcherDecisionengine, WatcherApplier
Add run-with-webhook make target for local development with webhook
Fixed webhooks error while during docker-build
go.mod changes to start a webserver to serve the endpoints for the validating and mutating webhooks.

Jira: https://issues.redhat.com/browse/OSPRH-11933

openshift-ci · 2024-11-26T13:56:37Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

softwarefactory-project-zuul · 2024-11-26T14:36:55Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ba9786543d1b477ab2bbe1da7cd6334f

❌ openstack-meta-content-provider FAILURE in 8m 58s
⚠️ watcher-operator-validation SKIPPED Skipped due to failed job openstack-meta-content-provider

SeanMooney · 2024-11-26T18:26:20Z

we should also add the maketarget for running with a webhook and the hacking scripts
you can see examples here

https://github.com/openstack-k8s-operators/placement-operator/tree/main/hack
https://github.com/openstack-k8s-operators/placement-operator/blob/main/Makefile#L349-L362

raukadah · 2024-11-27T05:50:58Z

we should also add the maketarget for running with a webhook and the hacking scripts you can see examples here

https://github.com/openstack-k8s-operators/placement-operator/tree/main/hack https://github.com/openstack-k8s-operators/placement-operator/blob/main/Makefile#L349-L362

Thank you @SeanMooney for sending the links, Updated the pr with run-with-webhook target.

softwarefactory-project-zuul · 2024-11-27T05:59:25Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/1e5d782b2e0845dea8118600fef8dab3

❌ openstack-meta-content-provider FAILURE in 8m 23s
⚠️ watcher-operator-validation SKIPPED Skipped due to failed job openstack-meta-content-provider

softwarefactory-project-zuul · 2024-11-27T06:29:48Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/3adf6a259ce1402dbde43ea02d20c073

❌ openstack-meta-content-provider FAILURE in 8m 59s
⚠️ watcher-operator-validation SKIPPED Skipped due to failed job openstack-meta-content-provider

softwarefactory-project-zuul · 2024-11-27T06:40:56Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ccb0a0d778f146ebb67c6e6e3092b324

❌ openstack-meta-content-provider FAILURE in 8m 32s
⚠️ watcher-operator-validation SKIPPED Skipped due to failed job openstack-meta-content-provider

softwarefactory-project-zuul · 2024-11-27T10:37:23Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e5a842175ea24bafa7fc7da68678c56a

✔️ openstack-meta-content-provider SUCCESS in 1h 24m 37s
❌ watcher-operator-validation FAILURE in 1h 14m 09s

raukadah · 2024-11-27T11:33:52Z

Moving to draft till we make EDPM job passing!

raukadah · 2024-11-27T11:44:06Z

/test functional

raukadah · 2024-11-27T13:31:17Z

/test functional
/test precommit-check

softwarefactory-project-zuul · 2024-11-27T16:05:25Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/8c703bd1c7ba4ac4a6976478ed9ac941

✔️ openstack-meta-content-provider SUCCESS in 2h 12m 29s
❌ watcher-operator-validation FAILURE in 1h 20m 34s

raukadah · 2024-11-28T06:26:16Z

Current failure: https://logserver.rdoproject.org/15/15/b62f9b9fb09a97a2a68a111dfb4a1e7ab96f27ba/github-check/watcher-operator-validation/0a57ce1/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack-operators/all_resources.log

pod/watcher-operator-controller-manager-5cc4b6f59d-nzbx8              1/2     CrashLoopBackOff   7 (10s ago)      11m

From pod log: https://logserver.rdoproject.org/15/15/b62f9b9fb09a97a2a68a111dfb4a1e7ab96f27ba/github-check/watcher-operator-validation/0a57ce1/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack-operators/pods/watcher-operator-controller-manager-5cc4b6f59d-nzbx8/logs/manager.log

2024-11-27T15:54:00Z	INFO	controller-runtime.metrics	Shutting down metrics server with timeout of 1 minute
2024-11-27T15:54:00Z	INFO	Wait completed, proceeding to shutdown the manager
I1127 15:54:00.093441       1 leaderelection.go:250] attempting to acquire leader lease openstack-operators/5049980f.openstack.org...
2024-11-27T15:54:00Z	ERROR	setup	problem running manager	{"error": "open /tmp/k8s-webhook-server/serving-certs/tls.crt: no such file or directory"}
main.main
	/remote-source/main.go:179
runtime.main
	/usr/lib/golang/src/runtime/proc.go:267

amoralej · 2024-11-28T08:08:51Z

A high level question. Checking at others operators, i see two different patterns:

Some operators, as barbican and cinder, create webhooks only for the top-level CRD (cinder or babican, i.e.)
Others (nova) creates independent webhooks for all the CRDs.

This PR is implementing (2), but, given that the user should be using only the top-level Watcher, I'd be inclined to go with (1) for simplicity.

What do you think?

I'll do a more detailed review today.

amoralej

Bug found in patch_webhook_configurations.yaml while testing locally

amoralej · 2024-11-29T12:39:09Z

hack/run_with_local_webhook.sh

+    - CREATE
+    - UPDATE
+    resources:
+    - watcher


This should be watchers

amoralej · 2024-11-29T12:39:32Z

hack/run_with_local_webhook.sh

+    - CREATE
+    - UPDATE
+    resources:
+    - watcher


amoralej · 2024-11-29T17:52:19Z

/approve

openshift-ci · 2024-11-29T17:52:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: amoralej

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

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

abays · 2024-12-02T13:33:41Z

main.go

+		if err = (&watcherv1beta1.WatcherDecisionEngine{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "WatcherDecisionEngine")
+			os.Exit(1)
+		}


Should add a line like this [1] after this line.

[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/80d688a88c19a7cc99ffae4f227924a7810db107/main.go#L161

this is somethign i never got around to optimising in the nova operator but
again i would like to clean this up into a for loop over like we do with

NewReconcilers

https://github.com/openstack-k8s-operators/watcher-operator/blob/main/controllers/watcher_common.go#L56-L79

we have one function that map of reconsiler name to and instance of the object

and then a second function that struct and loops over each setting them up and checking for an error.

https://github.com/openstack-k8s-operators/watcher-operator/blob/main/controllers/watcher_common.go#L81-L91

we should try and avoid having long if chains like this were we can by transforming it into a data-driven flow.

that is escpially true when it all or nothing (if any fails we abort)
and when it needs to be done in two places, main.go and suite_tests.go

Can I refactor these function as a follow up pr?

sure lets move forward with this one as is and follow up

Below is the command used to generate the same. ``` operator-sdk create webhook --group watcher --version v1beta1 --kind Watcher --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Below is the command used to generate webhook: ``` operator-sdk create webhook --group watcher --version v1beta1 --kind WatcherAPI --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Below is the command used to generate the same: ``` operator-sdk create webhook --group watcher --version v1beta1 --kind WatcherDecisionEngine --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Below is the command used to generate the same: ``` operator-sdk create webhook --group watcher --version v1beta1 --kind WatcherApplier --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

It will help to run with webhook. Signed-off-by: Chandan Kumar <[email protected]>

SeanMooney · 2024-12-02T14:49:20Z

api/v1beta1/watcher_webhook.go

+// log is for logging in this package.
+var watcherlog = logf.Log.WithName("watcher-resource")
+
+func (r *Watcher) SetupWebhookWithManager(mgr ctrl.Manager) error {


nit: this is not really part of the API module

its a method that should be defined on the reconcile base class

https://github.com/openstack-k8s-operators/watcher-operator/blob/main/controllers/watcher_common.go#L81-L92

or factored out into a common class like this

https://github.com/openstack-k8s-operators/nova-operator/blob/main/api/v1beta1/common_webhook.go

there is actually still some technical debt in the nova operator to meve this from being defined on each webhook to webhook to a common subclass so we could proceed with this as is i just want to call out the fac that this will largely be identical for every webhooks so we should try and duplicate this in the futrure

Thank you @SeanMooney , I have move this method into common_webhook.go

SeanMooney · 2024-12-02T14:50:45Z

api/v1beta1/watcher_webhook.go

+		Complete()
+}
+
+// TODO(user): EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!


you should proably remove theses TODO comments

SeanMooney · 2024-12-02T14:53:24Z

main.go

+		if err = (&watcherv1beta1.WatcherDecisionEngine{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "WatcherDecisionEngine")
+			os.Exit(1)
+		}


this is somethign i never got around to optimising in the nova operator but
again i would like to clean this up into a for loop over like we do with

NewReconcilers

https://github.com/openstack-k8s-operators/watcher-operator/blob/main/controllers/watcher_common.go#L56-L79

we have one function that map of reconsiler name to and instance of the object

and then a second function that struct and loops over each setting them up and checking for an error.

https://github.com/openstack-k8s-operators/watcher-operator/blob/main/controllers/watcher_common.go#L81-L91

we should try and avoid having long if chains like this were we can by transforming it into a data-driven flow.

that is escpially true when it all or nothing (if any fails we abort)
and when it needs to be done in two places, main.go and suite_tests.go

tests/functional/suite_test.go

raukadah · 2024-12-04T06:12:04Z

Moving it to Draft, working on existing comment!

softwarefactory-project-zuul · 2024-12-04T09:51:31Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e2220f3d861c4f5da974c84100f44222

✔️ noop SUCCESS in 0s
❌ openstack-meta-content-provider FAILURE in 7m 50s
⚠️ watcher-operator-validation SKIPPED Skipped due to failed job openstack-meta-content-provider

It also fixes: - Fix admission.Validator value in variable declaration error - Fix functional tests and pre-commit - Fix tls.crt no such file or directory - Use METRICS_PORT to 33080 and HEALTH_PORT to 33081 Signed-off-by: Chandan Kumar <[email protected]>

SeanMooney · 2024-12-06T14:09:44Z

main.go

+		if err = (&watcherv1beta1.WatcherDecisionEngine{}).SetupWebhookWithManager(mgr); err != nil {
+			setupLog.Error(err, "unable to create webhook", "webhook", "WatcherDecisionEngine")
+			os.Exit(1)
+		}


sure lets move forward with this one as is and follow up

done

openshift-ci bot added the do-not-merge/work-in-progress label Nov 26, 2024

raukadah force-pushed the webhook branch from 6ed5a8e to 9cd7ca4 Compare November 27, 2024 06:19

raukadah force-pushed the webhook branch from 9cd7ca4 to e3f3927 Compare November 27, 2024 06:31

raukadah force-pushed the webhook branch from e3f3927 to efacf51 Compare November 27, 2024 06:46

raukadah marked this pull request as ready for review November 27, 2024 09:27

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 27, 2024

openshift-ci bot requested review from marios and SeanMooney November 27, 2024 09:27

raukadah requested review from amoralej and abays November 27, 2024 09:27

raukadah changed the title ~~Webhook~~ Add Validation and Mutating Webhook support Nov 27, 2024

raukadah marked this pull request as draft November 27, 2024 11:33

openshift-ci bot added the do-not-merge/work-in-progress label Nov 27, 2024

raukadah force-pushed the webhook branch 3 times, most recently from 2ed29ef to b62f9b9 Compare November 27, 2024 13:30

amoralej previously requested changes Nov 29, 2024

View reviewed changes

raukadah force-pushed the webhook branch from b7e21be to 3880490 Compare November 29, 2024 15:01

openshift-ci bot added the approved label Nov 29, 2024

abays reviewed Dec 2, 2024

View reviewed changes

raukadah added 5 commits December 2, 2024 19:25

Generate webhook for kind Watcher

6fae535

Below is the command used to generate the same. ``` operator-sdk create webhook --group watcher --version v1beta1 --kind Watcher --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Generate Webhook for kind WatcherAPI

725fbde

Below is the command used to generate webhook: ``` operator-sdk create webhook --group watcher --version v1beta1 --kind WatcherAPI --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Generate webhook for WatcherDecisionEngine

945b82e

Below is the command used to generate the same: ``` operator-sdk create webhook --group watcher --version v1beta1 --kind WatcherDecisionEngine --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Generate webhook for WatcherApplier

d93b406

Below is the command used to generate the same: ``` operator-sdk create webhook --group watcher --version v1beta1 --kind WatcherApplier --programmatic-validation --defaulting ``` Signed-off-by: Chandan Kumar <[email protected]>

Added run-with-webhook make target

9174b90

It will help to run with webhook. Signed-off-by: Chandan Kumar <[email protected]>

raukadah force-pushed the webhook branch from 3880490 to 160c7dd Compare December 2, 2024 13:55

SeanMooney requested changes Dec 2, 2024

View reviewed changes

openshift-ci bot assigned SeanMooney Dec 2, 2024

SeanMooney requested a review from amoralej December 2, 2024 15:02

raukadah force-pushed the webhook branch from 160c7dd to 31c1d35 Compare December 4, 2024 05:53

raukadah marked this pull request as draft December 4, 2024 06:11

openshift-ci bot added the do-not-merge/work-in-progress label Dec 4, 2024

raukadah force-pushed the webhook branch from 31c1d35 to 2b556e5 Compare December 4, 2024 09:14

raukadah force-pushed the webhook branch from 2b556e5 to 9e88d17 Compare December 6, 2024 06:33

raukadah marked this pull request as ready for review December 6, 2024 10:40

openshift-ci bot removed the do-not-merge/work-in-progress label Dec 6, 2024

openshift-ci bot requested a review from cescgina December 6, 2024 10:40

SeanMooney approved these changes Dec 6, 2024

View reviewed changes

openshift-ci bot added the lgtm label Dec 6, 2024

openshift-merge-bot bot merged commit b552b28 into openstack-k8s-operators:main Dec 6, 2024
6 checks passed

Add Validation and Mutating Webhook support #15

Add Validation and Mutating Webhook support #15

Conversation

raukadah commented Nov 26, 2024 • edited Loading

openshift-ci bot commented Nov 26, 2024

softwarefactory-project-zuul bot commented Nov 26, 2024

SeanMooney commented Nov 26, 2024

raukadah commented Nov 27, 2024

softwarefactory-project-zuul bot commented Nov 27, 2024

softwarefactory-project-zuul bot commented Nov 27, 2024

softwarefactory-project-zuul bot commented Nov 27, 2024

softwarefactory-project-zuul bot commented Nov 27, 2024

raukadah commented Nov 27, 2024

raukadah commented Nov 27, 2024

raukadah commented Nov 27, 2024

softwarefactory-project-zuul bot commented Nov 27, 2024

raukadah commented Nov 28, 2024

amoralej commented Nov 28, 2024

amoralej left a comment

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

Choose a reason for hiding this comment

amoralej commented Nov 29, 2024

openshift-ci bot commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raukadah Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raukadah Dec 4, 2024 • 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

Choose a reason for hiding this comment

raukadah commented Dec 4, 2024

softwarefactory-project-zuul bot commented Dec 4, 2024

Choose a reason for hiding this comment

raukadah commented Nov 26, 2024 •

edited

Loading

raukadah Dec 4, 2024 •

edited

Loading

raukadah Dec 4, 2024 •

edited

Loading