Introduce webhook to prevent more than 1 KIP resource in a single namespace #115

dkwon17 · 2023-11-28T16:51:14Z

Related issue: https://issues.redhat.com/browse/CRW-4980

This PR creates a validation webhook ensures that only one KIP CR can be created in a given namespace:

https://github.com/che-incubator/kubernetes-image-puller-operator/assets/83611742/09e20dad-6ec0-4a07-b326-ce2e40f26a39.

This PR also generates deployment manifests under deploy/deployment:

└── deploy/deployment/
    ├── kubernetes/
    │   ├── objects/
    │   └── combined.yaml
    └── openshift/
        ├── objects/
        └── combined.yaml

where manifests under kubernetes is meant for installation on a kubernetes cluster (requires cert-manager), and manifests under openshift is meant for openshift cluster (requires service-ca)

This is generated automatically when running make deploy.

How to test this PR

Build the operator images (optional)
I've already built it here:

quay.io/dkwon17/kubernetes-image-puller-operator:validation

The image can be built by running this command:

make docker-build IMG=quay.io/<username>/kubernetes-image-puller-operator:<tag>

In a new cluster, deploy the operator:

make deploy IMG=quay.io/dkwon17/kubernetes-image-puller-operator:validation

In the default namespace, try to create two KubernetesImagePuller resources in the same namespace. The webhook should prevent that from happening with an error message:

Screenshot

Testing the bundle build

With this PR, building a bundle image and deploying with operator-sdk run bundle works as expected:

Set env vars.

Note: you must have your own kubernetes-image-puller-operator and kubernetes-image-puller-operator-bundle repositories available under your quay account.

$ export IMAGE_TAG_BASE=quay.io/<username>/kubernetes-image-puller-operator
$ export VERSION=1.1.0

Build and push the operator image:

$ make docker-build docker-push

Build and push the bundle image:

$ rm -rf bundle/manifests
$ make bundle bundle-build bundle-push

Install the bundle on a cluster with OLM:

$ ./bin/operator-sdk run bundle $BUNDLE_IMG

The Kubernetes Image Puller operator should install successfully, and the webhook functionality described in the How to test this PR should work as well.

Uninstall the operator

./bin/operator-sdk cleanup kubernetes-imagepuller-operator

Signed-off-by: David Kwon <[email protected]>

operator-sdk create webhook --group che --version v1alpha1 --kind KubernetesImagePuller --defaulting --programmatic-validation --force Writing kustomize manifests for you to edit... Writing scaffold for you to edit... api/v1alpha1/kubernetesimagepuller_webhook.go $ operator-sdk version operator-sdk version: "v1.9.2", commit: "319e77f2fff57c6c862bddd64ecf9c562f2d2161", kubernetes version: "1.20.2", go version: "go1.16.6", GOOS: "linux", GOARCH: "amd64" Signed-off-by: David Kwon <[email protected]>

Signed-off-by: David Kwon <[email protected]>

github-actions · 2023-11-28T16:51:27Z

Click here to review and test in web IDE:

github-actions · 2023-11-29T01:33:23Z

Click here to review and test in web IDE:

.github/workflows/pr-check.yml

…bhook, remove mutating webhook Signed-off-by: David Kwon <[email protected]>

Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-11-29T02:25:08Z

Click here to review and test in web IDE:

dkwon17 · 2023-11-29T02:27:50Z

Makefile

@@ -169,7 +169,8 @@ bundle: generate manifests download-kustomize download-operator-sdk ## Generate

 	CSV_PATH=$$($(MAKE) csv-path)
 	yq -riY '.metadata.annotations.containerImage = "'$(IMG)'"' $${CSV_PATH}
-	yq -riY '.spec.install.spec.deployments[0].spec.template.spec.containers[1].image = "'$(IMG)'"' $${CSV_PATH}
+	# Update container image for container 'kuebrnetes-image-puller-operator' in the list of deployments
+	yq -riY '.spec.install.spec.deployments[0].spec.template.spec.containers[] |= (select(.name == "kubernetes-image-puller-operator") .image |= "'$(IMG)'")' $${CSV_PATH}


When running make bundle on the dogfooding cluster, the order of the containers are different. This change makes it so that we don't assume that the kubernetes-image-puller-operator container is defined at index 1

Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-11-29T15:12:39Z

Click here to review and test in web IDE:

amisevsk

Generally LGTM, though I haven't been able to test yet.

api/v1alpha1/kubernetesimagepuller_webhook.go

amisevsk · 2023-11-29T22:27:12Z

config/default/webhookcainjection_patch.yaml

+metadata:
+  name: validating-webhook-configuration
+  annotations:
+    cert-manager.io/inject-ca-from: $(CERTIFICATE_NAMESPACE)/$(CERTIFICATE_NAME)


I haven't had a chance to test this, but speaking from (distant) experience in setting up similar for DWO -- have we verified this works on both Kubernetes and OpenShift? I recall some differences in how certs for webhooks are managed on OpenShift vs. Kubernetes. On Kubernetes we can use cert-manager but on OpenShift we need (IIRC) the service-ca.

@dkwon17
We have different configuration for Kubernetes and OpenShift.
Please have a look at [1]. As far as I remember I was inspired from DWO configuration.

[1] https://github.com/eclipse-che/che-operator/tree/a4e19ead9d324dce09f5ca400a5cb7eff3abd253/config/openshift/patches

.github/workflows/pr-check.yml

tolusha · 2023-11-30T12:33:43Z

config/certmanager/certificate.yaml

+apiVersion: cert-manager.io/v1
+kind: Issuer
+metadata:
+  name: selfsigned-issuer


All resources names must be unique, like kubernetes-image-puller-selfsigned-issuer

I believe it should be prefixed by kustomize on generation

Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-11-30T21:50:42Z

Click here to review and test in web IDE:

- Create gen-deployment makefile goal. Generates deploy folder for kubernetes and openshift. - Use name-prefix to prefix resources - Move operator service account from rbac directory to manager directory Signed-off-by: David Kwon <[email protected]>

- Create deploy.mk to avoid checking for kubectl or oc when make test is ran within to dockerfile - Create prefix overlay to prevent prefixing csv for bundle build Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-12-05T20:30:06Z

Click here to review and test in web IDE:

dkwon17 · 2023-12-05T21:00:22Z

Makefile

+		for file in temp??; do
+			name_kind=$$(yq -r '"\(.metadata.name).\(.kind)"' "$${file}")
+			mv "$${file}" "$${OBJECTS_DIR}/$${name_kind}.yaml"
+		done


This is the approach taken by che-operator:
https://github.com/eclipse-che/che-operator/blob/d99f8923f8a7649295ce97e9c0f3650730823278/Makefile#L170-L191

dkwon17 · 2023-12-05T21:03:13Z

config/kubernetes/kustomization.yaml

+#   Red Hat, Inc. - initial API and implementation
+#
+
+namePrefix: k8s-image-puller-operator-


I've set the prefix to k8s-image-puller-operator- instead of kubernetes-image-puller-operator- because the longer prefix led to resource names that were too long (IIRC, it was one of the rolebindings)

I stuck with kubernetes-image-puller-operator- to be consistent with the kubernetes-image-puller pod names

dkwon17 · 2023-12-05T21:04:32Z

config/default/manager_config_patch.yaml

@@ -13,7 +13,7 @@
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: kubernetes-image-puller-operator
+  name: manager


Because of the new kustomize namePrefix (example) and resource renames, I plan to indicate in the KIP operator release notes what resource names have been changed

Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-12-05T21:39:06Z

Click here to review and test in web IDE:

Signed-off-by: David Kwon <[email protected]>

github-actions · 2023-12-06T19:22:44Z

Click here to review and test in web IDE:

dkwon17 · 2023-12-06T19:26:38Z

Thanks for the reviews @amisevsk @tolusha, I've updated this PR.

The main update is that there is a new goal in the makefile gen-deployment that generates manifests for the kubernetes and openshift installation, similar to how che-operator and devworkspace-operator functions.

The make deploy command will generate the manifests and apply them based on the platform:

kubernetes-image-puller-operator/build/make/deploy.mk

Lines 23 to 24 in 57b2278

    
           deploy: manifests download-kustomize kustomize-operator-image gen-deployment ## Deploy controller to the K8s cluster specified in ~/.kube/config.	 
        
           	$(K8S_CLI) apply -f $(DEPLOYMENT_DIR)/$(PLATFORM)/combined.yaml

Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-12-06T21:08:03Z

Click here to review and test in web IDE:

tolusha · 2023-12-07T14:23:37Z

Makefile


 manifests: download-controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
-	$(CONTROLLER_GEN) crd:crdVersions=v1 rbac:roleName=manager-role webhook paths="./..." output:crd:artifacts:config=config/crd/bases
+	$(CONTROLLER_GEN) crd:crdVersions=v1 rbac:roleName=manager-role paths="./..." output:crd:artifacts:config=config/crd/bases


if you remove webhook parameter, then you don't need annotation anymore // +kubebuilder:webhook:path ...

tolusha · 2023-12-07T14:40:10Z

@dkwon17
How is OLM bundle is generated?
Could we check operator update?

dkwon17 · 2023-12-07T15:58:44Z

@tolusha I've generated the bundle image and tried it out with Testing the bundle build steps I've added to the PR description: #115 (comment)

Basically to build and deploy on a cluster, I've done:

export IMG=quay.io/dkwon17/kubernetes-image-puller-operator:1.1.0
export BUNDLE_IMG=quay.io/dkwon17/kubernetes-image-puller-operator-bundle:1.1.0
make docker-build docker-push 
make bundle bundle-build bundle-push
./bin/operator-sdk run bundle $BUNDLE_IMG

Could we check operator update?

What is the best way I can check that?

Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-12-07T21:43:51Z

Click here to review and test in web IDE:

tolusha · 2023-12-08T09:10:59Z

It is hard to review all resources and spot possible issues. So, in order to be sure everything works correctly, we have to:

Ensure that README.md is up to date, it is possible to deploy KIP on both Kubernetes and OpenShift clusters
Release works
Try to update KIP on OpenShift cluster to a newer version.

dkwon17 · 2023-12-12T21:27:18Z

I have tested this PR with Kubernetes and OpenShift, as well as the operator upgrade

First, I created the operator image:

export IMAGE_TAG_BASE=quay.io/dkwon17/kubernetes-image-puller-operator
export VERSION=1.1.0
make docker-build docker-push

Test make deploy on minikube

Run:

kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.13.3/cert-manager.yaml
make deploy IMG=quay.io/dkwon17/kubernetes-image-puller-operator:1.1.0

(screenshots have a different image tag, it is the same image as dkwon17/kubernetes-image-puller-operator:1.1.0)

After creating 1 KIP CR in `kubernetes-image-puller-operator` namespace

After trying to create a second KIP CR in `kubernetes-image-puller-operator` namespace:

Test make deploy on openshift

Run:

make deploy IMG=quay.io/dkwon17/kubernetes-image-puller-operator:1.1.0

After creating 1 KIP CR in `kubernetes-image-puller-operator` namespace

After trying to create a second KIP CR in `kubernetes-image-puller-operator` namespace:

dkwon17 · 2023-12-12T21:28:35Z

Test and deploy using OLM

Run:

rm -rf bundle/manifests
make bundle
bundle/test-bundle.sh

KIP operator on the operators tab:

KIP pods after creating a KIP CR in a namespace:

Webhook when trying to create a second KIP CR in the same namespace:

The message appears twice, but this happens only once. Subsequent attempts only result in 1 message. I don't get this problem when trying to create the KIP CR using kubectl, maybe it's an issue with the OpenShift console?

dkwon17 · 2023-12-12T21:29:59Z

Test with release-olm-bundle

Run

./make-release.sh $VERSION --release-olm-bundle

The catalog image is pushed: quay.io/dkwon17/kubernetes-image-puller-operator-catalog:stable

dkwon17 · 2023-12-12T21:32:20Z

Test upgrading KIP operator v1.0.6 to KIP operator v1.1.0

Install KIP operator v1.0.6 from OpeartorHub
Create catalog source:

apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: kubernetes-imagepuller-operator
  namespace: openshift-marketplace
spec:
  image: quay.io/dkwon17/kubernetes-image-puller-operator-catalog:stable
  sourceType: grpc
  updateStrategy:
    registryPoll:
      interval: 15m

Update the KIP operator subscription to reference the new catalog source
Wait until upgrade completes:

Screenshots

Deployment updated:

The following old components are updated as well

Old components

with the exception of the metrics-reader cluster role. This role remains in the cluster, despite not being defined in bundle/manifests anymore.

New components

dkwon17 · 2023-12-12T21:33:41Z

Is there something else I should check @tolusha ?

I will add a new commit updating the readme for:

resource name changes
cert-manager / service-ca requirement for webhook

tolusha · 2023-12-13T10:36:11Z

I run make bundle to generate a new bundle to see the differences.

There are two files with identical content, must be only one:

kubernetes-image-puller-operator-metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml
metrics-reader_rbac.authorization.k8s.io_v1_clusterrole.yaml

There are two files with identical content, must be only one:

controller-manager-metrics-service_v1_service.yaml
kubernetes-image-puller-operator-manager-metrics-service_v1_service.yaml

The changes below are not critical if old resources are removed after upgrade:
3. SA name changed kubernetes-image-puller-operator -> kubernetes-image-puller-operator-sa
4. Deployment name changes kubernetes-image-puller-operator -> kubernetes-image-puller-operator-manager

…m for webhook, update test-bundle.sh Signed-off-by: dkwon17 <[email protected]>

github-actions · 2023-12-13T14:37:39Z

Click here to review and test in web IDE:

dkwon17 · 2023-12-13T16:17:49Z

Yes, I should mention, before make bundle, I run rm -rf ./bundle/manifests to remove the old manifests

dkwon17 · 2023-12-13T22:51:27Z

@ibuziuk and I discussed releaesing KIP operator version 1.1.0 which contains the webhook introduced by this PR.

@tolusha does it make more sense to update the readme and bundle/ folder in a separate PR?

tolusha · 2023-12-14T14:43:14Z

bundle folder is supposed to be updated on release, so yes, it has to be a different PR

dkwon17 · 2023-12-18T14:08:14Z

Thank you @tolusha @amisevsk merging soon

dkwon17 added 5 commits November 22, 2023 15:15

Rename domain to eclipse.org, to match line 14

f41782f

Signed-off-by: David Kwon <[email protected]>

Fix package name

6221a6a

Signed-off-by: David Kwon <[email protected]>

Uncomment webhook config in kustomizaztion.yaml

c4f499d

Signed-off-by: David Kwon <[email protected]>

Rename deployment and container name for webhook patch

7c4d144

Signed-off-by: David Kwon <[email protected]>

dkwon17 requested a review from ibuziuk November 29, 2023 02:20

dkwon17 marked this pull request as ready for review November 29, 2023 02:20

dkwon17 requested a review from tolusha November 29, 2023 02:20

dkwon17 commented Nov 29, 2023

View reviewed changes

.github/workflows/pr-check.yml Outdated Show resolved Hide resolved

dkwon17 added 2 commits November 29, 2023 02:24

Run make manifests to create webook manifest, implement validation we…

7e6d6f7

…bhook, remove mutating webhook Signed-off-by: David Kwon <[email protected]>

Update makefile to update correct container image

4c4d59a

Signed-off-by: dkwon17 <[email protected]>

dkwon17 force-pushed the validation-webhook-restrict-cr branch from 4c49489 to 4c4d59a Compare November 29, 2023 02:24

dkwon17 commented Nov 29, 2023

View reviewed changes

Update github workflows to set up kubebuilder, fix failing test

6a94760

Signed-off-by: dkwon17 <[email protected]>

amisevsk reviewed Nov 29, 2023

View reviewed changes

tolusha reviewed Nov 30, 2023

View reviewed changes

Remove action, specify SHELL for make test

6f70e83

Signed-off-by: dkwon17 <[email protected]>

dkwon17 added 2 commits December 5, 2023 20:06

Fix broken docker image build due to missing kubectl & oc cli

d79b648

- Create deploy.mk to avoid checking for kubectl or oc when make test is ran within to dockerfile - Create prefix overlay to prevent prefixing csv for bundle build Signed-off-by: dkwon17 <[email protected]>

dkwon17 commented Dec 5, 2023

View reviewed changes

Small update

eee2310

Signed-off-by: dkwon17 <[email protected]>

Small fixes to makefile

57b2278

Signed-off-by: David Kwon <[email protected]>

dkwon17 mentioned this pull request Dec 6, 2023

Update try-in-web-ide GH action to latest 1.x.x version #117

Merged

Edit makefile prereq for gen-deployment goal

0bf4b2a

Signed-off-by: dkwon17 <[email protected]>

tolusha reviewed Dec 7, 2023

View reviewed changes

Fix make bundle command

8c51f28

Signed-off-by: dkwon17 <[email protected]>

dkwon17 marked this pull request as draft December 7, 2023 21:52

dkwon17 marked this pull request as ready for review December 12, 2023 21:32

Change service name to match service name automatically created by ol…

fc79d3d

…m for webhook, update test-bundle.sh Signed-off-by: dkwon17 <[email protected]>

tolusha approved these changes Dec 14, 2023

View reviewed changes

amisevsk approved these changes Dec 15, 2023

View reviewed changes

dkwon17 merged commit 6cc03a0 into che-incubator:main Dec 18, 2023
5 checks passed

dkwon17 added the made-with-che Changes were made using Che label Dec 19, 2023

Introduce webhook to prevent more than 1 KIP resource in a single namespace #115

Introduce webhook to prevent more than 1 KIP resource in a single namespace #115

Conversation

dkwon17 commented Nov 28, 2023 • edited Loading

How to test this PR

Testing the bundle build

github-actions bot commented Nov 28, 2023

github-actions bot commented Nov 29, 2023

github-actions bot commented Nov 29, 2023

dkwon17 Nov 29, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2023

amisevsk left a comment

Choose a reason for hiding this comment

amisevsk Nov 29, 2023

Choose a reason for hiding this comment

tolusha Nov 30, 2023

Choose a reason for hiding this comment

tolusha Nov 30, 2023

Choose a reason for hiding this comment

amisevsk Nov 30, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 30, 2023

github-actions bot commented Dec 5, 2023

dkwon17 Dec 5, 2023

Choose a reason for hiding this comment

dkwon17 Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

dkwon17 Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2023

github-actions bot commented Dec 6, 2023

dkwon17 commented Dec 6, 2023 • edited Loading

github-actions bot commented Dec 6, 2023

tolusha Dec 7, 2023

Choose a reason for hiding this comment

tolusha commented Dec 7, 2023

dkwon17 commented Dec 7, 2023

github-actions bot commented Dec 7, 2023

tolusha commented Dec 8, 2023

dkwon17 commented Dec 12, 2023

dkwon17 commented Dec 12, 2023

dkwon17 commented Dec 12, 2023

dkwon17 commented Dec 12, 2023

dkwon17 commented Dec 12, 2023

tolusha commented Dec 13, 2023

github-actions bot commented Dec 13, 2023

dkwon17 commented Dec 13, 2023 • edited Loading

dkwon17 commented Dec 13, 2023

tolusha commented Dec 14, 2023

dkwon17 commented Dec 18, 2023

dkwon17 commented Nov 28, 2023 •

edited

Loading

dkwon17 Dec 5, 2023 •

edited

Loading

dkwon17 Dec 5, 2023 •

edited

Loading

dkwon17 commented Dec 6, 2023 •

edited

Loading

dkwon17 commented Dec 13, 2023 •

edited

Loading