fix: Remove namespaceSelector from webhook definition #879

akrejcir · 2024-02-09T09:05:33Z

What this PR does / why we need it:
The ValidatingWebhookConfiguration created by OLM can have namespaceSelector field set. We don't want it, because the webhook checks that there is only one SSP resource in the cluster.

Which issue(s) this PR fixes:
Jira: https://issues.redhat.com/browse/CNV-36636

Release note:

None

openshift-ci · 2024-02-09T09:05:36Z

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

akrejcir · 2024-02-09T09:12:47Z

/cc @0xFelix @jcanocan

0xFelix · 2024-02-12T08:50:51Z

webhooks/ssp_webhook.go

@@ -53,6 +66,65 @@ func Setup(mgr ctrl.Manager) error {
 		Complete()
 }

+// The OLM limits the webhook scope to the namespaces that are defined in the OperatorGroup


Is that an issue for core KubeVirt too? How do we avoid the creation of multiple KubeVirt CRs when deployed via HCO? Does it use the same approach?

It is not. SSP ValidatingWebhookConfiguration is created by OLM. The webhook for Kubevirt resource is created by virt-operator and it only checks deletion, not create or update.

webhooks/ssp_webhook.go

tests/webhook_test.go

akrejcir · 2024-02-15T14:01:57Z

I've changed the logic completely. It uses a controller now.
@0xFelix @jcanocan can please you take another look?

tests/webhook_controller_test.go

0xFelix · 2024-02-19T08:36:34Z

controllers/webhook_controller.go

+// that watches ValidatingWebhookConfiguration created by OLM,
+// and removes any namespaceSelector defined in it.
+//
+// The OLM limits the webhook scope to the namespaces that are defined in the OperatorGroup


Btw. is this a bug than should be opened on OLM?

It is not an OLM bug. Our downstream CSV manifest has intentionally set installModes field to OwnNamespace and SingleNamespace. We want to limit the scope of our operators.

SSP webhook is special compared to regular webhooks, that it should also prevent the creation of SSP CRs in other namespaces, so it needs to be triggered if these objects are created in different namespace.

I understand, but isn't that a valid use case the we should be able to map in OLM?

I don't know. It looks like a design choice that should be discussed with OLM team.

Would you be able to investigate it? Do other repos in the kubevirt org have the same issue (e.g. HCO)?

HCO has the same issue. My previous revision of this PR was copied from: https://github.com/kubevirt/hyperconverged-cluster-operator/blob/7ac82e83ad975bf369f7ad1425936047ae892eba/pkg/webhooks/setup.go#L71

Kubevirt does not have a webhook that checks Kubevirt resource, and I'm not sure about CDI.

controllers/webhook_controller.go

0xFelix · 2024-02-19T08:39:37Z

controllers/wehook_test.go

+	It("should not remove namespaceSelector from non SSP webhook", func() {
+		webhookConfig.Webhooks[0].Rules[0].APIGroups = []string{"non-ssp-api-group"}
+
+		Expect(fakeClient.Create(context.Background(), webhookConfig)).To(Succeed())
+
+		_, err := testController.Reconcile(context.Background(), testRequest)
+		Expect(err).ToNot(HaveOccurred())
+
+		updatedConfig := &admissionv1.ValidatingWebhookConfiguration{}
+		Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(webhookConfig), updatedConfig)).To(Succeed())
+
+		Expect(updatedConfig).To(Equal(webhookConfig))
+	})
+
+	It("should not remove namespaceSelector from webhook without labels", func() {
+		webhookConfig.Labels = nil
+
+		Expect(fakeClient.Create(context.Background(), webhookConfig)).To(Succeed())
+
+		_, err := testController.Reconcile(context.Background(), testRequest)
+		Expect(err).ToNot(HaveOccurred())
+
+		updatedConfig := &admissionv1.ValidatingWebhookConfiguration{}
+		Expect(fakeClient.Get(context.Background(), client.ObjectKeyFromObject(webhookConfig), updatedConfig)).To(Succeed())
+
+		Expect(updatedConfig).To(Equal(webhookConfig))
+	})


Maye put these two into a table?

I'd prefer not to. It is only 2 tests. If there were more similar test cases, then I would do it.

The ValidatingWebhookConfiguration created by OLM can have namespaceSelector field set. We don't want that, because the webhook checks that there is only one SSP resource in the cluster. This commit adds a controller that watches ValidatingWebhookConfigurations and removes the namespaceSelector, if the object has a specific label. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

sonarcloud · 2024-02-19T12:30:46Z

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

akrejcir · 2024-02-19T14:22:05Z

/retest

akrejcir · 2024-02-20T09:24:36Z

/retest

0xFelix

/approve

0xFelix · 2024-02-20T15:00:45Z

controllers/webhook_controller.go

+	if changed := updateWebhookConfiguration(webhookConfig); !changed {
+		return reconcile.Result{}, nil
+	}


nit

Suggested change

if changed := updateWebhookConfiguration(webhookConfig); !changed {

return reconcile.Result{}, nil

}

if !updateWebhookConfiguration(webhookConfig) {

return reconcile.Result{}, nil

}

This is really small. I don't want to retrigger CI because of it.

0xFelix · 2024-02-20T15:02:41Z

controllers/webhook_controller.go

+// that watches ValidatingWebhookConfiguration created by OLM,
+// and removes any namespaceSelector defined in it.
+//
+// The OLM limits the webhook scope to the namespaces that are defined in the OperatorGroup


Would you be able to investigate it? Do other repos in the kubevirt org have the same issue (e.g. HCO)?

kubevirt-bot · 2024-02-20T15:02:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 0xFelix

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~~ [0xFelix]

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

akrejcir · 2024-02-21T12:56:44Z

@jcanocan can you please take another look?

jcanocan · 2024-02-21T14:49:58Z

/lgtm

kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Feb 9, 2024

kubevirt-bot requested review from ksimon1 and opokornyy February 9, 2024 09:06

akrejcir force-pushed the webhook-namespace-selector branch 2 times, most recently from d1f74be to abaf479 Compare February 9, 2024 09:12

akrejcir marked this pull request as ready for review February 9, 2024 09:12

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 9, 2024

kubevirt-bot requested review from 0xFelix, jcanocan and lyarwood February 9, 2024 09:12

akrejcir force-pushed the webhook-namespace-selector branch from abaf479 to 7f26a60 Compare February 9, 2024 12:35

0xFelix reviewed Feb 12, 2024

View reviewed changes

akrejcir force-pushed the webhook-namespace-selector branch from 7f26a60 to ca40f1f Compare February 15, 2024 14:01

jcanocan reviewed Feb 15, 2024

View reviewed changes

tests/webhook_controller_test.go Show resolved Hide resolved

akrejcir force-pushed the webhook-namespace-selector branch from ca40f1f to 9098e78 Compare February 16, 2024 09:33

0xFelix reviewed Feb 19, 2024

View reviewed changes

akrejcir force-pushed the webhook-namespace-selector branch 2 times, most recently from 3878186 to c30f13a Compare February 19, 2024 10:47

akrejcir force-pushed the webhook-namespace-selector branch from c30f13a to 948eeaa Compare February 19, 2024 12:30

0xFelix reviewed Feb 20, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2024

kubevirt-bot assigned jcanocan Feb 21, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 21, 2024

kubevirt-bot merged commit 2ca6001 into kubevirt:main Feb 21, 2024
12 checks passed

akrejcir deleted the webhook-namespace-selector branch February 21, 2024 14:53

This was referenced May 2, 2024

[release-v0.19] fix: Remove namespaceSelector from webhook definition #978

Merged

[release-v0.18] fix: Remove namespaceSelector from webhook definition #979

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove namespaceSelector from webhook definition #879

fix: Remove namespaceSelector from webhook definition #879

akrejcir commented Feb 9, 2024

openshift-ci bot commented Feb 9, 2024

akrejcir commented Feb 9, 2024

0xFelix Feb 12, 2024

akrejcir Feb 12, 2024

akrejcir commented Feb 15, 2024 •

edited

0xFelix Feb 19, 2024

akrejcir Feb 19, 2024 •

edited

0xFelix Feb 19, 2024

akrejcir Feb 19, 2024

0xFelix Feb 20, 2024

akrejcir Feb 20, 2024

0xFelix Feb 19, 2024

akrejcir Feb 19, 2024

sonarcloud bot commented Feb 19, 2024

akrejcir commented Feb 19, 2024

akrejcir commented Feb 20, 2024

0xFelix left a comment

0xFelix Feb 20, 2024

akrejcir Feb 21, 2024

0xFelix Feb 20, 2024

kubevirt-bot commented Feb 20, 2024

akrejcir commented Feb 21, 2024

jcanocan commented Feb 21, 2024

fix: Remove namespaceSelector from webhook definition #879

fix: Remove namespaceSelector from webhook definition #879

Conversation

akrejcir commented Feb 9, 2024

openshift-ci bot commented Feb 9, 2024

akrejcir commented Feb 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akrejcir commented Feb 15, 2024 • edited

Choose a reason for hiding this comment

akrejcir Feb 19, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 19, 2024

Quality Gate passed

akrejcir commented Feb 19, 2024

akrejcir commented Feb 20, 2024

0xFelix 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

kubevirt-bot commented Feb 20, 2024

akrejcir commented Feb 21, 2024

jcanocan commented Feb 21, 2024

akrejcir commented Feb 15, 2024 •

edited

akrejcir Feb 19, 2024 •

edited