Add validating admission webhook to Helm chart by nojnhuh · Pull Request #75 · kubernetes-sigs/dra-example-driver

nojnhuh · 2025-01-17T20:21:37Z

This change adds a validating admission webhook that validates the
driver-specific opaque parameters that can be specified in
ResourceClaims and ResourceClaimTemplates.

Fixes #49

nojnhuh · 2025-01-17T20:21:49Z

.github/workflows/tests.yaml

+    - name: Test
+      run: go test -v -race ./...


I also made this particular change in #70, but added that again here to run the new tests added by this PR in CI.

nojnhuh · 2025-01-17T20:21:58Z

README.md

 local-path-storage   local-path-provisioner-7dbf974f64-9jmc7                            1/1     Running   0          1m
 ```

+When the validating admission webhook is enabled (as it is by default), cert-manager and its CRDs must be installed.


Instead of taking on a cert-manager dependency, I experimented with Helm's builtin in genSignedCert and adjacent template functions. The main functional caveat with those is that every helm upgrade will generate a new cert, but if the Deployment doesn't change, then a new Pod will not be created to pick up to that new cert, leading to connection issues until a new Pod is created. That might be acceptable for what this example driver will be used for, but probably isn't a pattern we want to establish for "real" drivers.

Webhook doesn't seem to be vital for a DRA driver to function. Why not keep the webhook disabled in values, and instruct to enable the webhook if needed, given that it has a dependency and that dependency is not part of the chart?

Can do. Disabled it by default and reworded this to match.

nojnhuh · 2025-01-17T20:22:12Z

cmd/dra-example-webhook/main.go

+limitations under the License.
+*/
+
+package main


For context, this webhook implementation is largely based on the agnhost one.

nojnhuh · 2025-01-17T20:22:34Z

cmd/dra-example-webhook/main.go

+		gpuConfig, ok := decodedConfig.(*configapi.GpuConfig)
+		if !ok {
+			errs = append(errs, fmt.Errorf("expected v1alpha1.GpuConfig at %s but got: %T", fieldPath, decodedConfig))
+			continue
+		}
+		err = gpuConfig.Validate()
+		if err != nil {
+			errs = append(errs, fmt.Errorf("object at %s is invalid: %w", fieldPath, err))
+		}


Since this is about the extent of the driver-specific logic, I wonder if we could eventually make most of this a library in https://github.com/kubernetes/dynamic-resource-allocation where driver authors would only have to provide this custom type, a validation function, and an invocation from a main package. Getting some mileage on this implementation though seems wise before making it too easy :)

nojnhuh · 2025-01-17T20:22:51Z

deployments/container/Dockerfile

 LABEL description="See summary"

 COPY --from=build /artifacts/dra-example-kubeletplugin /usr/bin/dra-example-kubeletplugin
+COPY --from=build /artifacts/dra-example-webhook       /usr/bin/dra-example-webhook


I know it would be generally more desirable to have separate container images for the kubelet plugin and the webhook, but I wasn't 100% sure that's desired here based on the above make cmds invocation building all of the packages under ./cmd. It certainly does simplify some things in the Helm chart if there's only one image in play.

nojnhuh · 2025-01-17T20:23:06Z

deployments/helm/dra-example-driver/templates/_helpers.tpl

+{{- define "dra-example-driver.webhookServiceAccountName" -}}
+{{- $name := printf "%s-webhook-service-account" (include "dra-example-driver.fullname" .) }}
+{{- if .Values.webhook.serviceAccount.create }}
+{{- default $name .Values.webhook.serviceAccount.name }}
+{{- else }}
+{{- default "default-webhook" .Values.webhook.serviceAccount.name }}
+{{- end }}
+{{- end }}


I opted to create a separate ServiceAccount for the webhook since the webhook doesn't require any RBAC permissions. The Helm chart already seemed to be assuming that there will be only one ServiceAccount though, so the values structure is a little clunky where the kubelet plugin's ServiceAccount is rooted at serviceAccount where the webhook's is rooted at webhook.serviceAccount. Happy to make that more consistent how if that's desired.

nojnhuh · 2025-01-17T20:23:20Z

deployments/helm/dra-example-driver/templates/kubeletplugin.yaml

  namespace: {{ include "dra-example-driver.namespace" . }}
  labels:
    {{- include "dra-example-driver.labels" . | nindent 4 }}
+    app.kubernetes.io/component: kubeletplugin


This label is used to distinguish the kubeletplugin from the webhook so selectors don't get confused.

nojnhuh · 2025-01-17T20:23:47Z

cmd/dra-example-kubeletplugin/cdi.go


 const (
-	cdiVendor = "k8s." + DriverName
+	cdiVendor = "k8s." + consts.DriverName


I'm not generally a fan of consts packages but that seemed at least better than duplicating the variable. Open to suggestions for a better name or a place for this variable where it can be used by two different main packages. This change is in a separate commit from the rest of the webhook changes in case that makes this easier to review at all, but please let me know if I should squash.

Why not, for what it's worth, I keep a version in a package, it can be overriden during build-time so we don't have to change the version manually - commit hash, for example, but the release version too.

pohly · 2025-02-19T09:00:51Z

/milestone v1.33

@byako: can you review this? Do we need to find someone else?

byako

LGTM, couple of NITs

byako · 2025-03-03T12:32:47Z

cmd/dra-example-kubeletplugin/cdi.go


 const (
-	cdiVendor = "k8s." + DriverName
+	cdiVendor = "k8s." + consts.DriverName


Why not, for what it's worth, I keep a version in a package, it can be overriden during build-time so we don't have to change the version manually - commit hash, for example, but the release version too.

byako · 2025-03-03T12:36:03Z

cmd/dra-example-webhook/main.go

+	// verify the content type is accurate
+	contentType := r.Header.Get("Content-Type")
+	if contentType != "application/json" {
+		msg := fmt.Sprintf("contentType=%s, expect application/json", contentType)


Suggested change

msg := fmt.Sprintf("contentType=%s, expect application/json", contentType)

msg := fmt.Sprintf("contentType=%s, expected application/json", contentType)

byako · 2025-03-03T12:36:57Z

cmd/dra-example-webhook/main.go

+		return
+	}
+
+	klog.V(2).Info(fmt.Sprintf("handling request: %s", body))


Suggested change

klog.V(2).Info(fmt.Sprintf("handling request: %s", body))

klog.V(2).Infof("handling request: %s", body)

byako · 2025-03-03T12:38:02Z

cmd/dra-example-webhook/main.go

+	responseAdmissionReview.Response = admit(*requestedAdmissionReview)
+	responseAdmissionReview.Response.UID = requestedAdmissionReview.Request.UID
+
+	klog.V(2).Info(fmt.Sprintf("sending response: %v", responseAdmissionReview))


Suggested change

klog.V(2).Info(fmt.Sprintf("sending response: %v", responseAdmissionReview))

klog.V(2).Infof("sending response: %v", responseAdmissionReview)

byako · 2025-03-03T12:39:23Z

cmd/dra-example-webhook/main.go

+		deviceConfigs = claimTemplate.Spec.Spec.Devices.Config
+		specPath = "spec.spec"
+	default:
+		msg := fmt.Sprintf("expect resource to be %s or %s, got %s", resourceClaimResource, resourceClaimTemplateResource, ar.Request.Resource)


Suggested change

msg := fmt.Sprintf("expect resource to be %s or %s, got %s", resourceClaimResource, resourceClaimTemplateResource, ar.Request.Resource)

msg := fmt.Sprintf("expected resource to be %s or %s, got %s", resourceClaimResource, resourceClaimTemplateResource, ar.Request.Resource)

k8s-ci-robot · 2025-03-03T12:45:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: byako, nojnhuh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [byako]

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

This change adds a validating admission webhook that validates the driver-specific opaque parameters that can be specified in ResourceClaims and ResourceClaimTemplates.

nojnhuh · 2025-03-03T16:58:55Z

LGTM, couple of NITs

@byako I addressed those comments. Thanks!

byako · 2025-03-04T13:00:19Z

/lgtm

This is a straight copy of this PR kubernetes-sigs/dra-example-driver#75, with minimal changes to make it work in this repo. Signed-off-by: Kevin Klues <kklues@nvidia.com>

move DriverName const to importable package

0faf3f8

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 17, 2025

k8s-ci-robot requested review from klueska and pohly January 17, 2025 20:21

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 17, 2025

nojnhuh commented Jan 17, 2025

View reviewed changes

byako approved these changes Mar 3, 2025

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2025

Add validating admission webhook to Helm chart

162b104

This change adds a validating admission webhook that validates the driver-specific opaque parameters that can be specified in ResourceClaims and ResourceClaimTemplates.

nojnhuh force-pushed the webhook branch from 1b54dec to 162b104 Compare March 3, 2025 16:58

k8s-ci-robot assigned byako Mar 4, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2025

k8s-ci-robot merged commit cfa165d into kubernetes-sigs:main Mar 4, 2025
6 checks passed

johnbelamaric moved this from 👀 In review to ✅ Done in Dynamic Resource Allocation Mar 4, 2025

klueska mentioned this pull request Aug 12, 2025

Add validating admission webhook to verify opaque configs NVIDIA/k8s-dra-driver-gpu#461

Merged

nojnhuh mentioned this pull request Oct 22, 2025

DRA: Provide helper for implementing webhooks for opaque config kubernetes/kubernetes#134792

Closed

	msg := fmt.Sprintf("contentType=%s, expect application/json", contentType)
	msg := fmt.Sprintf("contentType=%s, expected application/json", contentType)

	klog.V(2).Info(fmt.Sprintf("handling request: %s", body))
	klog.V(2).Infof("handling request: %s", body)

	klog.V(2).Info(fmt.Sprintf("sending response: %v", responseAdmissionReview))
	klog.V(2).Infof("sending response: %v", responseAdmissionReview)

	msg := fmt.Sprintf("expect resource to be %s or %s, got %s", resourceClaimResource, resourceClaimTemplateResource, ar.Request.Resource)
	msg := fmt.Sprintf("expected resource to be %s or %s, got %s", resourceClaimResource, resourceClaimTemplateResource, ar.Request.Resource)

Conversation

nojnhuh commented Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly commented Feb 19, 2025

Uh oh!

byako left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 3, 2025

Uh oh!

nojnhuh commented Mar 3, 2025

Uh oh!

byako commented Mar 4, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nojnhuh commented Jan 17, 2025 •

edited

Loading