Add webhook validation for empty database and rabbitmq #33

raukadah · 2025-01-08T09:29:53Z

databaseInstance and rabbitMqClusterName are required fields. If an user specify databaseInstance and rabbitMqClusterName field as empty string. The webhook should fail it saying as there cannot be empty.

This pr adds the validations for the same.

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

Depends-On: openstack-k8s-operators/ci-framework#2658

openshift-ci · 2025-01-08T09:29:57Z

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

abays · 2025-01-08T17:41:35Z

Another option is to make these fields pointers and then use the kubebuilder required validation for these. Though I also see some places where databaseInstance and rabbitMqClusterName use kubebuilder validation without using a pointer string [1][2], which seems like it wouldn't work. That is because the defaulting webhooks will set the field to the empty string, and then the kubebuilder validation will be "tricked" into thinking the user explicitly set a value in this manner -- this is a classic problem. Using a pointer prevents this because the zero-value of nil remains intact throughout the webhook processing and thus will properly be treated as a empty value during subsequent kubebuilder validation.

[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L60-L64
[2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L177-L181

raukadah · 2025-01-09T15:25:44Z

Another option is to make these fields pointers and then use the kubebuilder required validation for these. Though I also see some places where databaseInstance and rabbitMqClusterName use kubebuilder validation without using a pointer string [1][2], which seems like it wouldn't work. That is because the defaulting webhooks will set the field to the empty string, and then the kubebuilder validation will be "tricked" into thinking the user explicitly set a value in this manner -- this is a classic problem. Using a pointer prevents this because the zero-value of nil remains intact throughout the webhook processing and thus will properly be treated as a empty value during subsequent kubebuilder validation.

[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L60-L64 [2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L177-L181

Thank you for the suggestion @abays , I have added them in existing patch. But while running tests, I am hitting following error:

chandankumar@raukadahlaptop:~/projects/watcher-operator$ make test
test -f go.work || GOTOOLCHAIN=go1.21.0 go work init
go work use .
go work use ./api
go work sync
test -s /home/chandankumar/projects/watcher-operator/bin/controller-gen && /home/chandankumar/projects/watcher-operator/bin/controller-gen --version | grep -q v0.14.0 || \
GOBIN=/home/chandankumar/projects/watcher-operator/bin go install sigs.k8s.io/controller-tools/cmd/[email protected]
/home/chandankumar/projects/watcher-operator/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases && \
 	rm -f api/bases/* && cp -a config/crd/bases api/
/home/chandankumar/projects/watcher-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
# github.com/openstack-k8s-operators/watcher-operator/tests/functional
vet: tests/functional/watcher_controller_test.go:199:6: cannot use GetWatcher(watcherTest.Instance).Spec.DatabaseInstance (variable of type *string) as string value in argument to mariadb.CreateDBService
make: *** [Makefile:127: vet] Error 1

I might have missed something in this change. Need your suggestion here, thank you!

api/v1beta1/common_types.go

abays · 2025-01-13T12:27:22Z

Another option is to make these fields pointers and then use the kubebuilder required validation for these. Though I also see some places where databaseInstance and rabbitMqClusterName use kubebuilder validation without using a pointer string [1][2], which seems like it wouldn't work. That is because the defaulting webhooks will set the field to the empty string, and then the kubebuilder validation will be "tricked" into thinking the user explicitly set a value in this manner -- this is a classic problem. Using a pointer prevents this because the zero-value of nil remains intact throughout the webhook processing and thus will properly be treated as a empty value during subsequent kubebuilder validation.
[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L60-L64 [2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L177-L181

Thank you for the suggestion @abays , I have added them in existing patch. But while running tests, I am hitting following error:
chandankumar@raukadahlaptop:~/projects/watcher-operator$ make test
test -f go.work || GOTOOLCHAIN=go1.21.0 go work init
go work use .
go work use ./api
go work sync
test -s /home/chandankumar/projects/watcher-operator/bin/controller-gen && /home/chandankumar/projects/watcher-operator/bin/controller-gen --version | grep -q v0.14.0 || \
GOBIN=/home/chandankumar/projects/watcher-operator/bin go install sigs.k8s.io/controller-tools/cmd/[email protected]
/home/chandankumar/projects/watcher-operator/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases && \
 	rm -f api/bases/* && cp -a config/crd/bases api/
/home/chandankumar/projects/watcher-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
# github.com/openstack-k8s-operators/watcher-operator/tests/functional
vet: tests/functional/watcher_controller_test.go:199:6: cannot use GetWatcher(watcherTest.Instance).Spec.DatabaseInstance (variable of type *string) as string value in argument to mariadb.CreateDBService
make: *** [Makefile:127: vet] Error 1
I might have missed something in this change. Need your suggestion here, thank you!

If you're going to use the string pointers, then you have to dereference the pointers when you use them in the code. For instance [1] [2].

That * in front of the pointer is what is called "dereferencing". It means you want to use the actual value of the pointer, not the pointer itself. The idea with pointers is that they are memory locations instead of direct values. So if you want to use the value that the memory location points to, you have to "dereference" it

[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L96
[2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/controllers/keystoneapi_controller.go#L1082

raukadah · 2025-01-15T06:28:41Z

Another option is to make these fields pointers and then use the kubebuilder required validation for these. Though I also see some places where databaseInstance and rabbitMqClusterName use kubebuilder validation without using a pointer string [1][2], which seems like it wouldn't work. That is because the defaulting webhooks will set the field to the empty string, and then the kubebuilder validation will be "tricked" into thinking the user explicitly set a value in this manner -- this is a classic problem. Using a pointer prevents this because the zero-value of nil remains intact throughout the webhook processing and thus will properly be treated as a empty value during subsequent kubebuilder validation.
[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L60-L64 [2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L177-L181

Thank you for the suggestion @abays , I have added them in existing patch. But while running tests, I am hitting following error:
chandankumar@raukadahlaptop:~/projects/watcher-operator$ make test
test -f go.work || GOTOOLCHAIN=go1.21.0 go work init
go work use .
go work use ./api
go work sync
test -s /home/chandankumar/projects/watcher-operator/bin/controller-gen && /home/chandankumar/projects/watcher-operator/bin/controller-gen --version | grep -q v0.14.0 || \
GOBIN=/home/chandankumar/projects/watcher-operator/bin go install sigs.k8s.io/controller-tools/cmd/[email protected]
/home/chandankumar/projects/watcher-operator/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases && \
 	rm -f api/bases/* && cp -a config/crd/bases api/
/home/chandankumar/projects/watcher-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
# github.com/openstack-k8s-operators/watcher-operator/tests/functional
vet: tests/functional/watcher_controller_test.go:199:6: cannot use GetWatcher(watcherTest.Instance).Spec.DatabaseInstance (variable of type *string) as string value in argument to mariadb.CreateDBService
make: *** [Makefile:127: vet] Error 1
I might have missed something in this change. Need your suggestion here, thank you!
If you're going to use the string pointers, then you have to dereference the pointers when you use them in the code. For instance [1] [2].

That * in front of the pointer is what is called "dereferencing". It means you want to use the actual value of the pointer, not the pointer itself. The idea with pointers is that they are memory locations instead of direct values. So if you want to use the value that the memory location points to, you have to "dereference" it

[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L96 [2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/controllers/keystoneapi_controller.go#L1082

Thank you @abays , Now it is fixed :-)

tests/functional/base_test.go

softwarefactory-project-zuul · 2025-01-16T07:44: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/799b775a8b0845f491e57c92d190ab1c

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 1h 31m 41s
✔️ watcher-operator-validation SUCCESS in 1h 21m 14s
❌ watcher-operator-kuttl RETRY_LIMIT in 30m 27s

raukadah · 2025-01-16T08:26:01Z

https://logserver.rdoproject.org/33/33/90c6e85a3dced5f9e1581515b0b81512410569c7/github-check/watcher-operator-kuttl/07df1b7/job-output.txt

TASK [install kuttl test_suite dependencies chdir={{ ansible_user_dir }}/{{ operator_basedir }}, _raw_params=make kuttl-test-prep] ***
2025-01-16 01:44:32.802688 | controller | Thursday 16 January 2025  01:43:59 -0500 (0:00:00.471)       0:10:13.908 ******
2025-01-16 01:44:32.802701 | controller | FAILED - RETRYING: [controller]: install kuttl test_suite dependencies (3 retries left).
2025-01-16 01:44:32.884299 | controller | FAILED - RETRYING: [controller]: install kuttl test_suite dependencies (2 retries left).
2025-01-16 01:44:32.884439 | controller | FAILED - RETRYING: [controller]: install kuttl test_suite dependencies (1 retries left).
2025-01-16 01:44:32.884458 | controller | fatal: [controller]: FAILED! => changed=true
2025-01-16 01:44:32.884472 | controller |   attempts: 3
2025-01-16 01:44:32.884486 | controller |   cmd:
2025-01-16 01:44:32.884500 | controller |   - make
2025-01-16 01:44:32.884514 | controller |   - kuttl-test-prep
2025-01-16 01:44:32.884528 | controller |   delta: '0:00:00.481700'
2025-01-16 01:44:32.884542 | controller |   end: '2025-01-16 01:44:32.780119'
2025-01-16 01:44:32.884555 | controller |   msg: non-zero return code
2025-01-16 01:44:32.884569 | controller |   rc: 2
2025-01-16 01:44:32.884582 | controller |   start: '2025-01-16 01:44:32.298419'
2025-01-16 01:44:32.884596 | controller |   stderr: |-
2025-01-16 01:44:32.884609 | controller |     error: resource mapping not found for name: "openstack" namespace: "watcher-kuttl-default" from "tests/kuttl/test-suites/default/deps/": no matches for kind "OpenStackControlPlane" in version "core.openstack.org/v1beta1"
2025-01-16 01:44:32.884624 | controller |     ensure CRDs are installed first
2025-01-16 01:44:32.884637 | controller |     make: *** [Makefile:406: kuttl-test-prep] Error 1
2025-01-16 01:44:32.884651 | controller |   stderr_lines: <omitted>
2025-01-16 01:44:32.884669 | controller |   stdout: |-
2025-01-16 01:44:32.884683 | controller |     oc apply -k tests/kuttl/test-suites/default/deps/ --timeout=120s
2025-01-16 01:44:32.884697 | controller |     namespace/watcher-kuttl-default unchanged
2025-01-16 01:44:32.884710 | controller |     secret/osp-secret unchanged
2025-01-16 01:44:32.884724 | controller |   stdout_lines: <omitted>

recheck

raukadah · 2025-01-16T08:37:27Z

recheck

softwarefactory-project-zuul · 2025-01-16T09:30:04Z

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/464569b495934f318e49c5f8cd05ea4e

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 51m 05s
❌ watcher-operator-validation FAILURE in 40m 10s
❌ watcher-operator-kuttl RETRY_LIMIT in 29m 14s

cescgina · 2025-01-16T14:52:34Z

recheck

amoralej · 2025-01-16T14:57:30Z

Another option is to make these fields pointers and then use the kubebuilder required validation for these. Though I also see some places where databaseInstance and rabbitMqClusterName use kubebuilder validation without using a pointer string [1][2], which seems like it wouldn't work. That is because the defaulting webhooks will set the field to the empty string, and then the kubebuilder validation will be "tricked" into thinking the user explicitly set a value in this manner -- this is a classic problem. Using a pointer prevents this because the zero-value of nil remains intact throughout the webhook processing and thus will properly be treated as a empty value during subsequent kubebuilder validation.

[1] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L60-L64 [2] https://github.com/openstack-k8s-operators/keystone-operator/blob/16c3ed8e549f8591dd17c94fcd165620c8a5444f/api/v1beta1/keystoneapi_types.go#L177-L181

Thanks for this information! I had no idea about that ...

softwarefactory-project-zuul · 2025-01-16T16:52:34Z

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/86468220f573427d8dbdba6ad8e54766

✔️ noop SUCCESS in 0s
✔️ openstack-meta-content-provider SUCCESS in 1h 52m 49s
✔️ watcher-operator-validation SUCCESS in 1h 23m 06s
❌ watcher-operator-kuttl RETRY_LIMIT in 56m 58s

cescgina · 2025-01-16T17:02:09Z

recheck

softwarefactory-project-zuul · 2025-01-16T18:59:11Z

This change depends on a change that failed to merge.

Change openstack-k8s-operators/ci-framework#2658 is needed.

cescgina · 2025-01-16T19:00:13Z

recheck

amoralej · 2025-01-17T13:00:19Z

requires rebase, that other than that lgtm

databaseInstance and rabbitMqClusterName are required fields. If an user specify databaseInstance and rabbitMqClusterName field as empty string. The webhook should fail it saying as there cannot be empty. This pr adds the validations for the same. Jira: https://issues.redhat.com/browse/OSPRH-11933 Signed-off-by: Chandan Kumar (raukadah) <[email protected]>

amoralej · 2025-01-17T14:13:59Z

/approve

openshift-ci · 2025-01-17T14:14:05Z

[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

openshift-ci bot added the do-not-merge/work-in-progress label Jan 8, 2025

raukadah force-pushed the required_webhook_validation branch from d7009d7 to aa71a31 Compare January 9, 2025 15:24

marios reviewed Jan 13, 2025

View reviewed changes

api/v1beta1/common_types.go Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase label Jan 13, 2025

raukadah force-pushed the required_webhook_validation branch from aa71a31 to 2fe1ea4 Compare January 15, 2025 06:27

openshift-merge-robot removed the needs-rebase label Jan 15, 2025

raukadah marked this pull request as ready for review January 15, 2025 07:28

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 15, 2025

openshift-ci bot requested review from marios and rlandy January 15, 2025 07:28

raukadah force-pushed the required_webhook_validation branch from 2fe1ea4 to b205f1e Compare January 15, 2025 07:55

raukadah commented Jan 15, 2025

View reviewed changes

tests/functional/base_test.go Outdated Show resolved Hide resolved

raukadah force-pushed the required_webhook_validation branch 2 times, most recently from 7f00222 to 90c6e85 Compare January 16, 2025 06:11

raukadah mentioned this pull request Jan 16, 2025

Integrate WatcherAPI creation and update on the main Watcher reconcile #38

Merged

openshift-merge-robot added the needs-rebase label Jan 17, 2025

raukadah force-pushed the required_webhook_validation branch from 90c6e85 to afae6cf Compare January 17, 2025 13:03

openshift-merge-robot removed the needs-rebase label Jan 17, 2025

openshift-ci bot added the approved label Jan 17, 2025

cescgina approved these changes Jan 17, 2025

View reviewed changes

openshift-ci bot assigned cescgina Jan 17, 2025

openshift-ci bot added the lgtm label Jan 17, 2025

openshift-merge-bot bot merged commit c3c5bca into openstack-k8s-operators:main Jan 17, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add webhook validation for empty database and rabbitmq #33

Add webhook validation for empty database and rabbitmq #33

raukadah commented Jan 8, 2025 •

edited by cescgina

Loading

openshift-ci bot commented Jan 8, 2025

abays commented Jan 8, 2025 •

edited

Loading

raukadah commented Jan 9, 2025

abays commented Jan 13, 2025

raukadah commented Jan 15, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

raukadah commented Jan 16, 2025

raukadah commented Jan 16, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

cescgina commented Jan 16, 2025

amoralej commented Jan 16, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

cescgina commented Jan 16, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

cescgina commented Jan 16, 2025

amoralej commented Jan 17, 2025

amoralej commented Jan 17, 2025

openshift-ci bot commented Jan 17, 2025

Add webhook validation for empty database and rabbitmq #33

Add webhook validation for empty database and rabbitmq #33

Conversation

raukadah commented Jan 8, 2025 • edited by cescgina Loading

openshift-ci bot commented Jan 8, 2025

abays commented Jan 8, 2025 • edited Loading

raukadah commented Jan 9, 2025

abays commented Jan 13, 2025

raukadah commented Jan 15, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

raukadah commented Jan 16, 2025

raukadah commented Jan 16, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

cescgina commented Jan 16, 2025

amoralej commented Jan 16, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

cescgina commented Jan 16, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

cescgina commented Jan 16, 2025

amoralej commented Jan 17, 2025

amoralej commented Jan 17, 2025

openshift-ci bot commented Jan 17, 2025

raukadah commented Jan 8, 2025 •

edited by cescgina

Loading

abays commented Jan 8, 2025 •

edited

Loading