Adds a webhook to prevent regular users from creating a KubernetesImagePuller resource #493

dkwon17 · 2023-11-02T18:28:53Z

This PR is for: https://redhat-internal.slack.com/archives/CHK0J6HT6/p1696522004380079

This PR is based on the same webhook, but for CheCluster: #377

In this screenshot, a regular user sandboxdev-1 is unable to create a KubernetesImagePuller resource:

This PR was tested by following these steps and running make dev-deploy-latest and make dev-deploy-e2e-member-local.

Paired with codeready-toolchain/toolchain-e2e#840

openshift-ci · 2023-11-02T18:29:06Z

Hi @dkwon17. Thanks for your PR.

I'm waiting for a codeready-toolchain member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

alexeykazakov

Ideally we would like to have Che specific webhooks decoupled from the sandbox operators. That includes both CheCluster validator and the KubernetesImagePuller webhooks. And we can deploy them along with the Che operator through our CD tools.

Is there any specific reason for keeping them here?

dkwon17 · 2023-11-02T20:40:08Z

I could be wrong but this seemed to be the easiest way to have the webhook, since there was already a similar webhook (checluster webhook) in this repository

And we can deploy them along with the Che operator through our CD tools.

I need to investigate if this works, but do you mean have the CD tool create a ValidatingWebhookConfiguration object with:

...
      clientConfig:
        service:
          name: member-operator-webhook
          namespace: ${NAMESPACE}
          path: "/validate-users-create-admin-resources"
          port: 443
      rules:
        - operations: ["CREATE"]
          apiGroups: ["che.eclipse.org"]
          apiVersions: ["v1alpha1"]
          resources: ["kubernetesimagepullers"]
          scope: "Namespaced"
        - operations: ["CREATE"]
          apiGroups: ["eclipse.che"]
          apiVersions: ["v2"]
          resources: ["checlusters"]
          scope: Namespaced

And implement a new /validate-users-create-admin-resources endpoint for the member operator webhook server?

I'm still learning how webhooks work and are implemented, but AFAIK this will decouple Che/KubernetesImagePuller specific webhooks from this repo, and can still keep the same functionality

alexeykazakov · 2023-11-02T21:30:28Z

Webhook is basically a service running in a namespace plus a webhook configuration (ValidatingWebhookConfiguration CR) which defines that webhook and points to the corresponding service.

So you would need to:

move your webhook source code to a different repo owned/controlled by the Che team. For example a simple go app which listens to an endpoint & port. See an example of such app here:

member-operator/cmd/webhook/main.go

Line 107 in a10d84e

mux.HandleFunc("/validate-users-checlusters", checlusterValidator.HandleValidate)
build an image for that webhook
Create a manifest for the Deployment (which runs a container for the image you built above), Service and ValidatingWebhookConfiguration.
Then add that manifest as an OpenShift Template to our sre repo next to the DevSpaces operator manifests so it's deployed

codecov · 2023-11-02T21:33:26Z

Codecov Report

Merging #493 (b756673) into master (1992c36) will increase coverage by 0.06%.
The diff coverage is 50.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #493      +/-   ##
==========================================
+ Coverage   82.39%   82.46%   +0.06%     
==========================================
  Files          29       29              
  Lines        3193     3188       -5     
==========================================
- Hits         2631     2629       -2     
+ Misses        426      424       -2     
+ Partials      136      135       -1

Files	Coverage Δ
...lidatingwebhook/validate_k8simagepuller_request.go	`30.95% <50.00%> (ø)`

deploy/webhook/member-operator-webhook.yaml

pkg/webhook/validatingwebhook/validate_k8simagepuller_request.go

alexeykazakov · 2023-11-09T18:27:28Z

/ok-to-test

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

alexeykazakov · 2023-11-10T01:28:41Z

And could you please do not rebase, squash commits or push with force in PRs? Just keep adding new commits. It would make PR review easier since we could review new commits only instead of reviewing everything from scratch every time.

dkwon17 · 2023-11-10T13:04:45Z

/retest

dkwon17 · 2023-11-10T14:19:36Z

For the failing e2e test:

Test result

=== CONT  TestSetupMigration
    member.go:2316: checking ValidatingWebhookConfiguration 'member-operator-validating-webhook'
    member.go:2345: 
        	Error Trace:	/tmp/toolchain-e2e/testsupport/wait/member.go:2345
        	            				/tmp/toolchain-e2e/testsupport/wait/member.go:2155
        	            				/tmp/toolchain-e2e/testsupport/init.go:127
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/tmp/toolchain-e2e/testsupport/init.go:45
        	            				/tmp/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Not equal: 
        	            	expected: "users.kubernetesimagepullers.webhook.sandbox"
        	            	actual  : "users.checlusters.webhook.sandbox"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-users.kubernetesimagepullers.webhook.sandbox
        	            	+users.checlusters.webhook.sandbox
        	Test:       	TestSetupMigration
    member.go:2355: 
        	Error Trace:	/tmp/toolchain-e2e/testsupport/wait/member.go:2355
        	            				/tmp/toolchain-e2e/testsupport/wait/member.go:2155
        	            				/tmp/toolchain-e2e/testsupport/init.go:127
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/tmp/toolchain-e2e/testsupport/init.go:45
        	            				/tmp/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Not equal: 
        	            	expected: "/validate-users-kubernetesimagepullers"
        	            	actual  : "/validate-users-checlusters"
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-/validate-users-kubernetesimagepullers
        	            	+/validate-users-checlusters
        	Test:       	TestSetupMigration
    member.go:2361: 
        	Error Trace:	/tmp/toolchain-e2e/testsupport/wait/member.go:2361
        	            				/tmp/toolchain-e2e/testsupport/wait/member.go:2155
        	            				/tmp/toolchain-e2e/testsupport/init.go:127
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/tmp/toolchain-e2e/testsupport/init.go:45
        	            				/tmp/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Not equal: 
        	            	expected: []string{"che.eclipse.org"}
        	            	actual  : []string{"org.eclipse.che"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	 ([]string) (len=1) {
        	            	- (string) (len=15) "che.eclipse.org"
        	            	+ (string) (len=15) "org.eclipse.che"
        	            	 }
        	Test:       	TestSetupMigration
    member.go:2362: 
        	Error Trace:	/tmp/toolchain-e2e/testsupport/wait/member.go:2362
        	            				/tmp/toolchain-e2e/testsupport/wait/member.go:2155
        	            				/tmp/toolchain-e2e/testsupport/init.go:127
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/tmp/toolchain-e2e/testsupport/init.go:45
        	            				/tmp/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Not equal: 
        	            	expected: []string{"v1alpha1"}
        	            	actual  : []string{"v2"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	 ([]string) (len=1) {
        	            	- (string) (len=8) "v1alpha1"
        	            	+ (string) (len=2) "v2"
        	            	 }
        	Test:       	TestSetupMigration
    member.go:2363: 
        	Error Trace:	/tmp/toolchain-e2e/testsupport/wait/member.go:2363
        	            				/tmp/toolchain-e2e/testsupport/wait/member.go:2155
        	            				/tmp/toolchain-e2e/testsupport/init.go:127
        	            				/usr/local/go/src/sync/once.go:74
        	            				/usr/local/go/src/sync/once.go:65
        	            				/tmp/toolchain-e2e/testsupport/init.go:45
        	            				/tmp/toolchain-e2e/test/migration/setup/setup_migration_test.go:12
        	Error:      	Not equal: 
        	            	expected: []string{"kubernetesimagepullers"}
        	            	actual  : []string{"checlusters"}
        	            	
        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1,3 +1,3 @@
        	            	 ([]string) (len=1) {
        	            	- (string) (len=22) "kubernetesimagepullers"
        	            	+ (string) (len=11) "checlusters"
        	            	 }
        	Test:       	TestSetupMigration

Any idea where the checluster webhook is coming from? I have the updated tests here: https://github.com/dkwon17/toolchain-e2e/tree/kip_webhook

alexeykazakov · 2023-11-10T18:16:34Z

@dkwon17 it's a known problem with our migration e2e tests for webhook.
In our e2e tests we do the following:

install the previous (currently running in prod) version of the operators
run same tests
update the operators (so they now run the newer version from the actual PR)
run all the tests

So we can make sure that the operator upgrade from the current version to the newest one doesn't break anything.
However in steps 1 and 3 we wait for the all the deployments (operator controllers, webhooks, etc) to be be in expected state which includes webhook configuration verification: https://github.com/codeready-toolchain/toolchain-e2e/blob/f30d23daadd3888816b8363be893c68067a36566/testsupport/wait/member.go#L2315
But you are changing the configuration. So the configuration in steps 1 and 3 are different.
Previously we had to disable webhook verification tests temporally to work it around:
See an example of such PR here: codeready-toolchain/toolchain-e2e#809
And then after merging that PR we re-enabled the tests: codeready-toolchain/toolchain-e2e#822

There is another PR to address this problem: codeready-toolchain/toolchain-e2e#836 so we don't have to disable the tests anymore.

cc: @rajivnathan

dkwon17 · 2023-11-13T15:22:02Z

I see, thank you for clarification, I've made the e2e tests PR here, with the validation tests temporarily disabled: codeready-toolchain/toolchain-e2e#840

openshift-ci · 2023-11-14T00:00:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexeykazakov, dkwon17

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

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

sonarqubecloud · 2023-11-14T20:12:53Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
4.1% Duplication

dkwon17 · 2023-11-14T20:35:43Z

/retest

alexeykazakov · 2023-11-14T23:55:39Z

/retest

dkwon17 requested review from MatousJobanek, xcoulon, alexeykazakov, rajivnathan, ranakan19, sbryzak and mfrancisc as code owners November 2, 2023 18:28

openshift-ci bot added the needs-ok-to-test label Nov 2, 2023

dkwon17 marked this pull request as draft November 2, 2023 18:29

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

dkwon17 force-pushed the kip_webhook branch from 6bd5dc9 to 2d161bf Compare November 2, 2023 18:37

dkwon17 marked this pull request as ready for review November 2, 2023 18:37

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

alexeykazakov reviewed Nov 2, 2023

View reviewed changes

alexeykazakov closed this Nov 2, 2023

alexeykazakov reopened this Nov 2, 2023

dkwon17 force-pushed the kip_webhook branch from 2d161bf to 8a5e17a Compare November 3, 2023 15:57

dkwon17 marked this pull request as draft November 6, 2023 16:19

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

dkwon17 force-pushed the kip_webhook branch from 2f5cd77 to 3095ff5 Compare November 9, 2023 15:24

dkwon17 marked this pull request as ready for review November 9, 2023 15:25

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

openshift-ci bot requested a review from alexeykazakov November 9, 2023 15:25

alexeykazakov reviewed Nov 9, 2023

View reviewed changes

deploy/webhook/member-operator-webhook.yaml Outdated Show resolved Hide resolved

pkg/webhook/validatingwebhook/validate_k8simagepuller_request.go Show resolved Hide resolved

openshift-ci bot added the ok-to-test label Nov 9, 2023

openshift-ci bot removed the needs-ok-to-test label Nov 9, 2023

Prevent regular users from creating KIP resource, delete CheCluster

0f17536

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

dkwon17 force-pushed the kip_webhook branch from fc15f75 to 0f17536 Compare November 9, 2023 23:07

Merge branch 'master' into kip_webhook

1a99b68

dkwon17 mentioned this pull request Nov 13, 2023

E2e tests for K8sImagePuller validating webhook codeready-toolchain/toolchain-e2e#840

Merged

Merge branch 'master' into kip_webhook

bc0f913

alexeykazakov approved these changes Nov 14, 2023

View reviewed changes

openshift-ci bot added the approved label Nov 14, 2023

dkwon17 and others added 2 commits November 14, 2023 11:47

Merge branch 'master' into kip_webhook

643cdf6

Merge branch 'master' into kip_webhook

b756673

alexeykazakov merged commit 082aed8 into codeready-toolchain:master Nov 15, 2023
11 of 13 checks passed

dkwon17 mentioned this pull request Jan 18, 2024

Remove kubernetesimagepuller webhook #526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds a webhook to prevent regular users from creating a KubernetesImagePuller resource #493

Adds a webhook to prevent regular users from creating a KubernetesImagePuller resource #493

dkwon17 commented Nov 2, 2023 •

edited by alexeykazakov

Loading

openshift-ci bot commented Nov 2, 2023

alexeykazakov left a comment

dkwon17 commented Nov 2, 2023

alexeykazakov commented Nov 2, 2023

codecov bot commented Nov 2, 2023 •

edited

Loading

alexeykazakov commented Nov 9, 2023

alexeykazakov commented Nov 10, 2023

dkwon17 commented Nov 10, 2023

dkwon17 commented Nov 10, 2023 •

edited

Loading

alexeykazakov commented Nov 10, 2023 •

edited

Loading

dkwon17 commented Nov 13, 2023

openshift-ci bot commented Nov 14, 2023

sonarqubecloud bot commented Nov 14, 2023

dkwon17 commented Nov 14, 2023

alexeykazakov commented Nov 14, 2023

Adds a webhook to prevent regular users from creating a KubernetesImagePuller resource #493

Adds a webhook to prevent regular users from creating a KubernetesImagePuller resource #493

Conversation

dkwon17 commented Nov 2, 2023 • edited by alexeykazakov Loading

openshift-ci bot commented Nov 2, 2023

alexeykazakov left a comment

Choose a reason for hiding this comment

dkwon17 commented Nov 2, 2023

alexeykazakov commented Nov 2, 2023

codecov bot commented Nov 2, 2023 • edited Loading

Codecov Report

alexeykazakov commented Nov 9, 2023

alexeykazakov commented Nov 10, 2023

dkwon17 commented Nov 10, 2023

dkwon17 commented Nov 10, 2023 • edited Loading

alexeykazakov commented Nov 10, 2023 • edited Loading

dkwon17 commented Nov 13, 2023

openshift-ci bot commented Nov 14, 2023

sonarqubecloud bot commented Nov 14, 2023

dkwon17 commented Nov 14, 2023

alexeykazakov commented Nov 14, 2023

dkwon17 commented Nov 2, 2023 •

edited by alexeykazakov

Loading

codecov bot commented Nov 2, 2023 •

edited

Loading

dkwon17 commented Nov 10, 2023 •

edited

Loading

alexeykazakov commented Nov 10, 2023 •

edited

Loading