Skip to content

Commit

Permalink
DSC, DSCI: add validating webhook (#711)
Browse files Browse the repository at this point in the history 
* webhook: add initial skeleton

Originally it was generated with

```operator-sdk create webhook --group datasciencecluster --version v1 --kind DataScienceCluster  --programmatic-validation```

but webhook.Validator interface (like described in the kubebuilder
book[1]) does not work well for the purpose of the webhook due to
needs to access openshift cluster (client.Client) to check existing
instances of DSC.

So, direct implementation of Handler was done inspired by [2] and
odh-notebooks implementation [3].

Move it from api package closer to controllers as in [3] as well
since it's not DataScienceCluster or DSCInitialization extention
anymore. Amend webhook_suite_test.go's path to configs accordingly.

Fix linter issues in webhook_suite_test.go:
- disable ssl check;
- move to package webhook_test

certmanager files removed too due to usage of OpenShift service
serving certificates[4] (see also
service.beta.openshift.io/inject-cabundle annotation in
config/webhook/kustomization.yaml).

Add webhook generation to `make manifests` target so
webhook/manifests.yaml is generated with it.

Since DSCI creation now requires webhook it should be delayed after
manager started. Move it to a closure and add it to the manager for
run with Add() API. It requires explicit declaration of the
interface variable otherwise complains about type mismatch for the
function literal.

[1] https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation
[2] https://book-v1.book.kubebuilder.io/beyond_basics/sample_webhook.html
[3] https://github.com/opendatahub-io/kubeflow/blob/v1.7-branch/components/odh-notebook-controller/controllers/notebook_webhook.go
[4] https://docs.openshift.com/container-platform/4.9/security/certificates/service-serving-certificate.html

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* webhook: implement one instance enforcing

The webhook is written with the idea to handle both Create and
Update requests (configured in config/webhook/manifests.yaml), but
at the moment only duplication check on Create is implemented.

Implements the logic which is done now on reconcile time [1] (same
for DSCI).

It checks for 0 instances, not 1, since when the webhook is running
the object has not been created yet. Means if it's 1 then it handles
request to create a second one.

It could be probably possible to use generics but does not make a
lot of sense for such a simple case.

Closes: #693

[1] https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/controllers/datasciencecluster/datasciencecluster_controller.go#L98

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: add tests to check duplication blocking

Add both envtest and e2e tests of a second DataScienceCluster
instance creation blocking.

envtest's one is a part of webhook test suite.

e2e:

Add `name` parameter to setupDSCInstance() function to reuse it.

Use require.Error() as the assertion, shorter and more straight
logic than implementing it in the test itself.

Add e2e test to check DSCInitialization similar way.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

* tests: e2e: refactor duplication tests in more abstract way

Factor out common code using Unstructured/List objects.

Change structure to remind more prepare/action/assert.

Use "require" features when appropriate.

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

---------

Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

chore(webhook): (#870)

- add testcase on DSCI
- remove kubebuilder marker not needed
- remove checks on instance number in existing controllers
- re-generate bundle
- we do not act on update but we keep it on webhook for now

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
  • Loading branch information
@ykaliuta @zdtsw
ykaliuta authored and zdtsw committed Feb 23, 2024
1 parent 8c2a81a commit 9992e2a
Show file tree
Hide file tree
Showing 24 changed files with 670 additions and 180 deletions.
4 changes: 1 addition & 3 deletions Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -138,9 +138,7 @@ endef

.PHONY: manifests
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
# TODO: enable below when we do webhook
# $(CONTROLLER_GEN) rbac:roleName=controller-manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
$(CONTROLLER_GEN) rbac:roleName=controller-manager-role crd:ignoreUnexportedFields=true paths="./..." output:crd:artifacts:config=config/crd/bases
$(CONTROLLER_GEN) rbac:roleName=controller-manager-role crd:ignoreUnexportedFields=true webhook paths="./..." output:crd:artifacts:config=config/crd/bases
$(call fetch-external-crds,github.com/openshift/api,route/v1)
$(call fetch-external-crds,github.com/openshift/api,user/v1)

Expand Down
6 changes: 6 additions & 0 deletions PROJECT
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ resources:
kind: DSCInitialization
path: github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1
version: v1
webhooks:
validation: true
webhookVersion: v1
- api:
crdVersion: v1
namespaced: false
Expand All @@ -30,4 +33,7 @@ resources:
kind: DataScienceCluster
path: github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1
version: v1
webhooks:
validation: true
webhookVersion: v1
version: "3"
36 changes: 18 additions & 18 deletions bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -105,12 +105,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -149,12 +149,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -195,12 +195,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -298,12 +298,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -342,12 +342,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -385,12 +385,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -428,12 +428,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down Expand Up @@ -471,12 +471,12 @@ spec:
description: 'sourcePath is the subpath within contextDir
where kustomize builds start. Examples include
any sub-folder or path: `base`, `overlays/dev`,
`default`, `odh` etc'
`default`, `odh` etc.'
type: string
uri:
default: ""
description: uri is the URI point to a git repo
with tag/branch. e.g https://github.com/org/repo/tarball/<tag/branch>
with tag/branch. e.g. https://github.com/org/repo/tarball/<tag/branch>
type: string
type: object
type: array
Expand Down
2 changes: 2 additions & 0 deletions bundle/manifests/dscinitialization.opendatahub.io_dscinitializations.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ spec:
field.
properties:
customCABundle:
default: ""
description: A custom CA bundle that will be available for all components
in the Data Science Cluster(DSC). This bundle will be stored
in odh-trusted-ca-bundle ConfigMap .data.odh-ca-bundle.crt .
Expand All @@ -146,6 +147,7 @@ spec:
pattern: ^(Managed|Unmanaged|Force|Removed)$
type: string
required:
- customCABundle
- managementState
type: object
required:
Expand Down
24 changes: 24 additions & 0 deletions bundle/manifests/redhat-ods-operator-webhook-service_v1_service.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
apiVersion: v1
kind: Service
metadata:
annotations:
service.beta.openshift.io/inject-cabundle: "true"
service.beta.openshift.io/serving-cert-secret-name: redhat-ods-operator-controller-webhook-cert
creationTimestamp: null
labels:
app.kubernetes.io/component: webhook
app.kubernetes.io/created-by: rhods-operator
app.kubernetes.io/instance: webhook-service
app.kubernetes.io/managed-by: kustomize
app.kubernetes.io/name: service
app.kubernetes.io/part-of: rhods-operator
name: redhat-ods-operator-webhook-service
spec:
ports:
- port: 443
protocol: TCP
targetPort: 9443
selector:
control-plane: controller-manager
status:
loadBalancer: {}
Loading

0 comments on commit 9992e2a

Please sign in to comment.