CNV-34094: Add validating webhook #3132

nunnatsa · 2023-10-25T10:23:17Z

What this PR does / why we need it:
Add validating webhook for HostedClusters and NodePools, to check if kubevirt external infra cluster is with sufficient CNV and K8s versions.

This is already checked on the operator, but adding this logic to the webhook will prevent the creating of resources in insufficient environment, in advance. Fixing this error is not an easy task and require upgrading the infra cluster, or the CNV installation, and we prefer to fail fast in this case.

Which issue(s) this PR fixes
Fixes #CNV-34094

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci-robot · 2023-10-25T10:23:21Z

@nunnatsa: This pull request references CNV-34094 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

What this PR does / why we need it:
Add validating webhook for HostedClusters and NodePools, to check if kubevirt external infra cluster is with sufficient CNV and K8s versions.

This is already checked on the operator, but adding this logic to the webhook will prevent the creating of resources in insufficient environment, in advance. Fixing this error is not an easy task and require upgrading the infra cluster, or the CNV installation, and we prefer to fail fast in this case.

Which issue(s) this PR fixes
Fixes #CNV-34094

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

openshift-ci-robot · 2023-10-25T10:23:27Z

@nunnatsa: This pull request references CNV-34094 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

What this PR does / why we need it:
Add validating webhook for HostedClusters and NodePools, to check if kubevirt external infra cluster is with sufficient CNV and K8s versions.

This is already checked on the operator, but adding this logic to the webhook will prevent the creating of resources in insufficient environment, in advance. Fixing this error is not an easy task and require upgrading the infra cluster, or the CNV installation, and we prefer to fail fast in this case.

Which issue(s) this PR fixes
Fixes #CNV-34094

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

nunnatsa · 2023-10-25T12:08:37Z

/test e2e-aws

nunnatsa · 2023-10-25T18:02:18Z

/retest

nunnatsa · 2023-10-26T05:54:24Z

/test e2e-aws

nunnatsa · 2023-10-26T09:36:22Z

/retest

davidvossel

looks great! i made some minor comments. I also want to get Alberto's thoughts on removing the code that deletes the validation webhook. We obviously can't delete the validation webhook or else your logic here wouldn't work, but i want to make sure we account for whatever he was needing that deletion logic for. it's possible we might need to still delete the webhook but only if we are not explicitly having it enabled

davidvossel · 2023-10-26T19:17:51Z

Makefile

@@ -242,6 +242,7 @@ ci-install-hypershift-private:
 		--oidc-storage-provider-s3-credentials=/etc/hypershift-pool-aws-credentials/credentials \
 		--oidc-storage-provider-s3-bucket-name=hypershift-ci-oidc \
 		--oidc-storage-provider-s3-region=us-east-1 \
+		--enable-validating-webhook \


we don't want to add the validating webhook here. This is used for CI jobs not related to kubevirt or self managed

davidvossel · 2023-10-26T19:20:13Z

cmd/install/assets/hypershift_operator.go

+	hcPath := "/validate-hypershift-openshift-io-v1beta1-hostedcluster"
+	npPath := "/validate-hypershift-openshift-io-v1beta1-nodepool"
+	sideEffects := admissionregistrationv1.SideEffectClassNone
+	timeout := int32(10)


The mutating webhook uses a 15 second timeout. it would be good to be consistent

davidvossel · 2023-10-26T19:55:31Z

hypershift-operator/main.go

-	// Since we dropped the validation webhook server we need to ensure this resource doesn't exist
-	// otherwise it will intercept kas requests and fail.
-	// TODO (alberto): dropped in 4.14.
-	validatingWebhookConfiguration := &admissionregistrationv1.ValidatingWebhookConfiguration{
-		TypeMeta: metav1.TypeMeta{
-			Kind:       "ValidatingWebhookConfiguration",
-			APIVersion: admissionregistrationv1.SchemeGroupVersion.String(),
-		},
-		ObjectMeta: metav1.ObjectMeta{
-			Namespace: opts.Namespace,
-			Name:      hyperv1.GroupVersion.Group,
-		},
-	}
-	if err := mgr.GetClient().Delete(ctx, validatingWebhookConfiguration); err != nil {
-		if !apierrors.IsNotFound(err) {
-			return err
-		}
-	}
-


@enxebre will we cause any issues by removing this code that deletes the validation webhook?

yes, we might have some envs running on versions where going through this is still necessary. So this would need to gate on if webhookNotEnabled kind of thing.
Also in addition please consider using ignore fail policy.

Thanks @enxebre.

I'm not sure how to handle this. The flag is only known to the cli, and this is where the webhook is created. Main is running inside the cluster, knowing nothing about the installation and its configuration, AFAIK.

The only thing I can think of, is to use different webhook name for the new one. I don't like this idea and I find it confusing and error prompt. Also, not sure it's possible. The current name is hyperv1.GroupVersion.Group

Do you have any other idea?

we can explore either move the logic out to "install" or propagate the input into the HO options

davidvossel · 2023-10-26T20:54:47Z

hypershift-operator/controllers/hostedcluster/hostedcluster_webhook.go

+
+	if _, isTimeout := ctx.Deadline(); !isTimeout {
+		var cancel context.CancelFunc
+		ctx, cancel = context.WithTimeout(ctx, time.Second*3)


i'd feel better if we extended this to a 5 second timeout... it likely doesn't matter, but i'm more on the conservative side when it comes to timing out operations.

I'll make it 10, to be closer to the webhook T/O (15 sec)

davidvossel · 2023-10-26T20:57:00Z

kubevirtexternalinfra/externalinfra.go

+	eg.Go(func() error {
+		var err error
+		cnvVersion, err = cl.GetInfraKubevirtVersion(egCtx)
+		return err
+	})
+
+	eg.Go(func() error {
+		var err error
+		k8sVersion, err = cl.GetInfraK8sVersion()
+		return err
+	})
+
+	err := eg.Wait()
+	if err != nil {
+		return err
+	}


that's kind of neat. I haven't seen this usage of errgroups before. looks good

https://medium.com/@yardenlaif/go-sync-or-go-home-errgroup-f91a0ee72d3f

Add validating webhook for HostedClusters and NodePools, to check if kubevirt external infra cluster is with sufficient CNV and K8s versions. This is already checked on the operator, but adding this logic to the webhook will prevent the creating of resources in insufficient environment, in advance. Fixing this error is not an easy task and require upgrading the infra cluster, or the CNV installation, and we prefer to fail fast in this case. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

nunnatsa · 2023-10-30T13:32:30Z

/rest e2e-aws

nunnatsa · 2023-10-30T13:44:48Z

/test e2e-aws

davidvossel

/lgtm
/approve

openshift-ci · 2023-11-01T20:50:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, nunnatsa

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

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

openshift-ci-robot · 2023-11-01T22:33:45Z

/retest-required

Remaining retests: 0 against base HEAD 2462a4f and 2 for PR HEAD d95a109 in total

nunnatsa · 2023-11-02T04:02:50Z

/retest

openshift-ci · 2023-11-02T05:49:26Z

@nunnatsa: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 25, 2023

openshift-ci bot added the do-not-merge/needs-area label Oct 25, 2023

openshift-ci bot requested review from hasueki and sjenning October 25, 2023 10:23

nunnatsa force-pushed the validate-kv-infra branch from 5e10041 to ab7b9ea Compare October 25, 2023 10:46

nunnatsa force-pushed the validate-kv-infra branch from ab7b9ea to ad53696 Compare October 26, 2023 11:19

davidvossel reviewed Oct 26, 2023

View reviewed changes

nunnatsa force-pushed the validate-kv-infra branch from ad53696 to d6c8587 Compare October 28, 2023 09:45

nunnatsa force-pushed the validate-kv-infra branch from d6c8587 to d95a109 Compare October 30, 2023 12:00

davidvossel reviewed Nov 1, 2023

View reviewed changes

openshift-ci bot assigned davidvossel Nov 1, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 1, 2023

openshift-ci bot merged commit f6bcddb into openshift:main Nov 2, 2023
12 checks passed

nunnatsa deleted the validate-kv-infra branch November 2, 2023 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNV-34094: Add validating webhook #3132

CNV-34094: Add validating webhook #3132

nunnatsa commented Oct 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Oct 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Oct 25, 2023 •

edited by openshift-ci bot

nunnatsa commented Oct 25, 2023

nunnatsa commented Oct 25, 2023

nunnatsa commented Oct 26, 2023

nunnatsa commented Oct 26, 2023

davidvossel left a comment

davidvossel Oct 26, 2023

davidvossel Oct 26, 2023

davidvossel Oct 26, 2023

enxebre Oct 30, 2023

nunnatsa Oct 30, 2023 •

edited

enxebre Oct 30, 2023

davidvossel Oct 26, 2023

nunnatsa Oct 28, 2023

davidvossel Oct 26, 2023

nunnatsa Oct 28, 2023

nunnatsa commented Oct 30, 2023

nunnatsa commented Oct 30, 2023

davidvossel left a comment

openshift-ci bot commented Nov 1, 2023

openshift-ci-robot commented Nov 1, 2023

nunnatsa commented Nov 2, 2023

openshift-ci bot commented Nov 2, 2023

CNV-34094: Add validating webhook #3132

CNV-34094: Add validating webhook #3132

Conversation

nunnatsa commented Oct 25, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Oct 25, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Oct 25, 2023 • edited by openshift-ci bot

nunnatsa commented Oct 25, 2023

nunnatsa commented Oct 25, 2023

nunnatsa commented Oct 26, 2023

nunnatsa commented Oct 26, 2023

davidvossel 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

Choose a reason for hiding this comment

nunnatsa Oct 30, 2023 • 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

nunnatsa commented Oct 30, 2023

nunnatsa commented Oct 30, 2023

davidvossel left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 1, 2023

openshift-ci-robot commented Nov 1, 2023

nunnatsa commented Nov 2, 2023

openshift-ci bot commented Nov 2, 2023

nunnatsa commented Oct 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Oct 25, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Oct 25, 2023 •

edited by openshift-ci bot

nunnatsa Oct 30, 2023 •

edited