[th/validating-webhook] add validating admission webhook to check DpuOperatorConfig

[thom311]

• use operator-sdk create webhook to add the scaffolding for a validating admission webhook
• implement checks for DpuOperatorConfig
  • enforce name to be dpu-operator-config
  • check mode for valid values

https://issues.redhat.com/browse/IIC-317

Missing: run the webhook in the kind cluster. As such, the tests don't cover this (though, there is a unit test that checks the validation code a bit). Maybe make test also need to run the admission webhook (if possible).

[openshift-ci]

Hi @thom311. Thanks for your PR.

I'm waiting for a openshift 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-sigs/prow repository.

[wizhaoredhat]

This looks good! Very clean for implementing the webhook. The issues you had before, how did you solve them? What were the issues such that we know what can go wrong.

[thom311]

This looks good! Very clean for implementing the webhook. The issues you had before, how did you solve them? What were the issues such that we know what can go wrong.

The main problem is that I didn't understand how to correctly make TLS certificates for the webhook work.

One approach is to uncomment the [CERTMANAGER] parts. That took me a while to understand how to get that to work. In particular, we must install the cert-manager operator, which is available in Openshift but not enabled by default. That is a problem for using it in dpu-operator.

Compared to sriov-network-operator, that seems implemented mostly by hand (not using operator-sdk?). So what is done there does not directly translate and spread out over many commits. It was not easy to understand or follow. It supports both cert-manager and service.beta.openshift.io.

For this branch, only service.beta.openshift.io is implemented. That seems sufficient to me, it also seems to work on Microshift.

[wizhaoredhat]

/approve

[bn222]

can you fix having to manually do go get github.com/onsi/ginkgo/[email protected]

[thom311]

the upgrade is done to avoid some failure in make test. Is there a problem, or what's the use of staying with an old version (and a different version from the toplevel go.mod)?

[thom311]

without (cd api/ && go get github.com/onsi/ginkgo/[email protected]), a subsequent make test gives a warning:

$ make test
GOFLAGS='' /data/src/dpu-operator/bin/controller-gen rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
for d in . dpu-api api tools ; do \
        if [ "$d" = . ] ; then \
                (cd $d && go mod vendor) || exit $? ; \
        fi ; \
        (cd $d && go mod tidy) || exit $? ; \
done
GOFLAGS='' /data/src/dpu-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
test -s /data/src/dpu-operator/bin/setup-envtest || GOBIN=/data/src/dpu-operator/bin GOFLAGS='' go install sigs.k8s.io/controller-runtime/tools/[email protected]
test -s /data/src/dpu-operator/bin/ginkgo && /data/src/dpu-operator/bin/ginkgo version | grep -q v2.20.2 || \
GOBIN=/data/src/dpu-operator/bin GOFLAGS='' go install github.com/onsi/ginkgo/v2/[email protected]
FAST_TEST=false KUBEBUILDER_ASSETS="/data/src/dpu-operator/bin/k8s/1.27.1-linux-amd64" /data/src/dpu-operator/bin/ginkgo --repeat 4  ./... -coverprofile cover.out
Ginkgo detected a version mismatch between the Ginkgo CLI and the version of Ginkgo imported by your packages:
  Ginkgo CLI Version:
    2.20.2
  Mismatched package versions found:
    2.14.0 used by v1

  Ginkgo will continue to attempt to run but you may see errors (including flag
  parsing errors) and should either update your go.mod or your version of the
  Ginkgo CLI to match.

  To install the matching version of the CLI run
    go install github.com/onsi/ginkgo/v2/ginkgo
  from a path that contains a go.mod file.  Alternatively you can use
    go run github.com/onsi/ginkgo/v2/ginkgo
  from a path that contains a go.mod file to invoke the matching version of the
  Ginkgo CLI.

  If you are attempting to test multiple packages that each have a different
  version of the Ginkgo library with a single Ginkgo CLI that is currently
  unsupported.
  
[1732290406] Webhook Suite - 3/3 specs ••• SUCCESS! 8.971230559s PASS
[1732290406] Cnihelper Suite - 1/1 specs • SUCCESS! 984.902µs PASS
...

[bn222]

make gingko

[thom311]

do you mean, to add a make target for calling go get ginko?

[thom311]

I think that is not correct.

[bn222]

what's wrong with that?

[thom311]

there is no make target make gingko in dpu-operator main branch.

[bn222]

https://github.com/openshift/dpu-operator/blob/main/Makefile#L342

[thom311]

ok, that exists...

But how is that helping with the problem at hand?

make test already depends on make ginkgo, so that was already called. The warning I pasted seems rather clear: it's not supported to have different versions of the ginkgo library.

I don't understand what you are suggesting, or what you think the problem is. What means "can you fix having to manually do go get github.com/onsi/ginkgo/[email protected]"?

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thom311, wizhaoredhat

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 [wizhaoredhat]

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

[thom311]

/test all

[openshift-ci]

@thom311: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/test all

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-sigs/prow repository.

[vrindle]

/ok-to-test

[bn222]

Why doesn't it run? You commit message seems to say that there is more work needed here.

[thom311]

I reworded the commit message and this code comment.

As commit message says, that I was unable to make it work. The commit message says, that work is not currently planned, but could be done in the future (if somebody figures out how to make it work).

Probably it should be something like the following (which doesn't work):

From 1cce9259b8db7c250ae422f004313ca424f914a8 Mon Sep 17 00:00:00 2001
From: Thomas Haller <[email protected]>
Date: Mon, 25 Nov 2024 22:30:22 +0100
Subject: [PATCH 1/2] testutils: fix global variable for TestEnv

We will actually need this global variable, that gets initialized
during BeforeSuite(). Add it as "testutils.TestEnv".
---
 internal/controller/suite_test.go  | 2 --
 internal/testutils/test_cluster.go | 6 ++++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go
index 41ef6740831f..1a1e1b87d90e 100644
--- a/internal/controller/suite_test.go
+++ b/internal/controller/suite_test.go
@@ -23,7 +23,6 @@ import (
 	. "github.com/onsi/gomega"
 
 	"k8s.io/client-go/rest"
-	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	//+kubebuilder:scaffold:imports
@@ -33,7 +32,6 @@ import (
 // http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
 
 var cfg *rest.Config
-var testEnv *envtest.Environment
 
 func TestControllers(t *testing.T) {
 	RegisterFailHandler(Fail)
diff --git a/internal/testutils/test_cluster.go b/internal/testutils/test_cluster.go
index 84b7e6dd746a..5739b121d232 100644
--- a/internal/testutils/test_cluster.go
+++ b/internal/testutils/test_cluster.go
@@ -37,6 +37,8 @@ var (
 	setupLog                  = ctrl.Log.WithName("setup")
 )
 
+var TestEnv *envtest.Environment
+
 func relativeToAbs(path string) string {
 	_, file, _, _ := runtime.Caller(0)
 	file, err := filepath.Abs(file)
@@ -48,7 +50,7 @@ func bootstrapTestEnv(restConfig *rest.Config) {
 	var err error
 	trueVal := true
 	By("bootstrapping test environment")
-	testEnv := &envtest.Environment{
+	TestEnv = &envtest.Environment{
 		CRDDirectoryPaths: []string{
 			relativeToAbs("../../config/crd/bases"),
 			relativeToAbs("../../test/crd"),
@@ -58,7 +60,7 @@ func bootstrapTestEnv(restConfig *rest.Config) {
 		Config:                restConfig,
 	}
 	By("starting the test env")
-	cfg, err := testEnv.Start()
+	cfg, err := TestEnv.Start()
 	Expect(err).NotTo(HaveOccurred())
 	Expect(cfg).NotTo(BeNil())
 }

base-commit: cccb01b501f633015d7e48d1afb2fb66c60b66cd
-- 
2.47.1


From 62afdecc264dccbf2a3fc8836c4ee1940d20d89a Mon Sep 17 00:00:00 2001
From: Thomas Haller <[email protected]>
Date: Mon, 2 Dec 2024 17:53:26 +0100
Subject: [PATCH 2/2] controller/test: enable controller-test for new webhook

This doesn't actually work.
---
 .../dpuoperatorconfig_controller_test.go      | 25 ++++++++++++++++++-
 internal/testutils/test_cluster.go            |  3 +++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/internal/controller/dpuoperatorconfig_controller_test.go b/internal/controller/dpuoperatorconfig_controller_test.go
index 6c3a834899f4..497fc3c1b714 100755
--- a/internal/controller/dpuoperatorconfig_controller_test.go
+++ b/internal/controller/dpuoperatorconfig_controller_test.go
@@ -2,8 +2,12 @@ package controller
 
 import (
 	"context"
+	"crypto/tls"
+	"fmt"
+	"net"
 	"os"
 	"sync"
+	"time"
 
 	netattdefv1 "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1"
 
@@ -120,16 +124,24 @@ func startDPUControllerManager(ctx context.Context, client *rest.Config, wg *syn
 	utilruntime.Must(configv1.AddToScheme(scheme))
 	utilruntime.Must(netattdefv1.AddToScheme(scheme))
 
+	webhookInstallOptions := &testutils.TestEnv.WebhookInstallOptions
 	mgr, err := ctrl.NewManager(client, ctrl.Options{
 		Scheme: scheme,
 		Metrics: server.Options{
 			BindAddress: ":18001",
 		},
-		WebhookServer:    webhook.NewServer(webhook.Options{Port: 9443}),
+		WebhookServer: webhook.NewServer(webhook.Options{
+			Host:    webhookInstallOptions.LocalServingHost,
+			Port:    webhookInstallOptions.LocalServingPort,
+			CertDir: webhookInstallOptions.LocalServingCertDir,
+		}),
 		LeaderElectionID: "1e46962d.openshift.io",
 	})
 	Expect(err).NotTo(HaveOccurred())
 
+	err = (&configv1.DpuOperatorConfig{}).SetupWebhookWithManager(mgr)
+	Expect(err).NotTo(HaveOccurred())
+
 	b := NewDpuOperatorConfigReconciler(mgr.GetClient(), mgr.GetScheme(), "mock-image", plugin.CreateVspImagesMap(false, setupLog))
 	err = b.SetupWithManager(mgr)
 	Expect(err).NotTo(HaveOccurred())
@@ -143,6 +155,17 @@ func startDPUControllerManager(ctx context.Context, client *rest.Config, wg *syn
 	}()
 	<-mgr.Elected()
 
+	// wait for the webhook server to get ready
+	dialer := &net.Dialer{Timeout: time.Second}
+	addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
+	Eventually(func() error {
+		conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
+		if err != nil {
+			return err
+		}
+		return conn.Close()
+	}).Should(Succeed())
+
 	return mgr
 }
 
diff --git a/internal/testutils/test_cluster.go b/internal/testutils/test_cluster.go
index 5739b121d232..eea46b5e9db3 100644
--- a/internal/testutils/test_cluster.go
+++ b/internal/testutils/test_cluster.go
@@ -58,6 +58,9 @@ func bootstrapTestEnv(restConfig *rest.Config) {
 		ErrorIfCRDPathMissing: true,
 		UseExistingCluster:    &trueVal,
 		Config:                restConfig,
+		WebhookInstallOptions: envtest.WebhookInstallOptions{
+			Paths: []string{relativeToAbs("../../config/webhook")},
+		},
 	}
 	By("starting the test env")
 	cfg, err := TestEnv.Start()
-- 
2.47.1

[thom311]

@bn222 my point is: I tried for a long time (and failed) to add the validating webhook to dpuoperatorconfig_controller_test.go.

If you think that is necessary, then I'd need some pointers (possibly building on the patch ^^).

But it shouldn't be necessary, because:

• there is a working unit test that has the validating webhook running. This one does test the webhook.
• a validating webhook can only reject a CR. Having that in dpuoperatorconfig_controller_test.go only makes a difference, when trying to configure invalid CRs. Which we can do on the other unit test.
• extending the tests can be done later at any time.

[bn222]

cancelled ci because we need to get vsp as lib in first

[bn222]

@vrindle you're in an excellent position to review this in detail since you've approached this differently.

[bn222]

/unhold

[openshift-ci]

@thom311: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/make-vendor-check	afae1b7	link	true	/test make-vendor-check
ci/prow/make-e2e-test	afae1b7	link	true	/test make-e2e-test

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

[bn222]

If we can't get this working in kind, we need at least e2e test. Can you add tests against a cda cluster?

Definitely remove the piece of code that doesn't make sense:

https://github.com/openshift/dpu-operator/pull/213/files#diff-24489aa02e68225d41d99794e841fe30ffb62033024bd3dd2e66fa9cc364035eR242-R244