feat(webhook): add validating webhook skeleton#5678
feat(webhook): add validating webhook skeleton#5678Rajneesh180 wants to merge 3 commits intofluid-cloudnative:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request lays the foundational groundwork for integrating validating admission webhooks into the Fluid project. It establishes a basic webhook handler capable of performing initial validation checks on incoming Kubernetes resources, specifically ensuring that all objects possess a 'metadata.name'. This is a crucial first step towards implementing comprehensive validation for Fluid's Custom Resource Definitions, enhancing data integrity and operational stability. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Hi @Rajneesh180. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a skeleton for a validating admission webhook. The implementation is straightforward, but it can be improved by better leveraging the controller-runtime framework. Specifically, the handler should use the injected decoder for object deserialization instead of manual JSON parsing. This change simplifies the code and makes it more robust. I've also suggested an update to the unit test to accommodate this change.
| ) | ||
|
|
||
| func TestValidatingHandler_Basic(t *testing.T) { | ||
| h := NewValidatingHandler() |
There was a problem hiding this comment.
Given the suggested change to use the decoder in validating_handler.go, this test will fail with a nil pointer dereference because the decoder is not initialized. You should create and inject a decoder into the handler to ensure the test can run successfully and validate the handler's logic.
| h := NewValidatingHandler() | |
| h := NewValidatingHandler() | |
| decoder := admission.NewDecoder(runtime.NewScheme()) | |
| err := h.InjectDecoder(decoder) | |
| assert.NoError(t, err) |
| var obj map[string]interface{} | ||
| if err := json.Unmarshal(req.Object.Raw, &obj); err != nil { | ||
| return admission.Errored(http.StatusBadRequest, err) | ||
| } | ||
|
|
||
| metadata, ok := obj["metadata"].(map[string]interface{}) | ||
| if !ok { | ||
| return admission.Denied("metadata.name is required") | ||
| } | ||
| name, ok := metadata["name"].(string) | ||
| if !ok || name == "" { | ||
| return admission.Denied("metadata.name is required") | ||
| } |
There was a problem hiding this comment.
The Handle function manually unmarshals the JSON payload into a map[string]interface{}. A more idiomatic and robust approach is to use the injected decoder to unmarshal the request object into an unstructured object. This leverages the controller-runtime framework's capabilities, simplifies the code, and makes it less error-prone.
By using unstructured.Unstructured, you can easily access metadata fields like the object name with obj.GetName().
You will also need to adjust your imports:
- "encoding/json"
+ "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" obj := &unstructured.Unstructured{}
if err := h.decoder.Decode(req, obj); err != nil {
return admission.Errored(http.StatusBadRequest, err)
}
if obj.GetName() == "" {
return admission.Denied("metadata.name is required")
}
Rajneesh180
force-pushed
the
feat/validation-webhook-skeleton
branch
from
362e705 to
31f75a0
Compare
There was a problem hiding this comment.
Pull request overview
Adds an initial validating admission webhook handler for Fluid resources, intended as the starting point for validating Fluid CRDs.
Changes:
- Introduces a new validating webhook handler skeleton with basic generic validation and Dataset-specific validation logic.
- Adds unit tests for the validating handler behavior.
- Registers the validating handler map in the webhook handler registry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| pkg/webhook/handler/validating/webhook.go | Adds validating webhook handler map and setup wrapper. |
| pkg/webhook/handler/validating/validating_handler.go | Implements validating admission handler with Dataset-typed and generic JSON fallback validation. |
| pkg/webhook/handler/validating/validating_handler_test.go | Adds unit tests covering generic and Dataset validation paths. |
| pkg/webhook/handler/register.go | Enables registering validating handlers during init. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| // Require either Mounts or Runtimes to be present | ||
| if len(ds.Spec.Mounts) == 0 && len(ds.Spec.Runtimes) == 0 { | ||
| return admission.Denied("dataset.spec must contain at least one mount or runtime") | ||
| } |
There was a problem hiding this comment.
This denies Datasets when both spec.mounts and spec.runtimes are empty, but DatasetSpec.Mounts is explicitly documented as optional and may be empty for some runtimes (see api/v1alpha1/dataset_types.go). As-is, this webhook would reject some Datasets that the API types/CRD allow; please align the validation rule with the API contract (or scope the rule to only the cases where mounts/runtimes are required).
| // Require either Mounts or Runtimes to be present | |
| if len(ds.Spec.Mounts) == 0 && len(ds.Spec.Runtimes) == 0 { | |
| return admission.Denied("dataset.spec must contain at least one mount or runtime") | |
| } |
|
|
||
| // Fallback generic validation: ensure object contains metadata.name | ||
| var obj map[string]interface{} | ||
| if err := json.Unmarshal(req.Object.Raw, &obj); err != nil { |
There was a problem hiding this comment.
req.Object.Raw can be empty (e.g., DELETE admission requests commonly use OldObject). In that case this code returns a 400 error, which can unintentionally block deletes if the webhook is configured for them. Consider explicitly handling req.Operation == Delete and/or using req.OldObject when Object.Raw is empty.
| if err := json.Unmarshal(req.Object.Raw, &obj); err != nil { | |
| raw := req.Object.Raw | |
| if len(raw) == 0 { | |
| raw = req.OldObject.Raw | |
| } | |
| if len(raw) == 0 { | |
| // No object payload available to validate; do not block the request | |
| return admission.Allowed("no object to validate") | |
| } | |
| if err := json.Unmarshal(raw, &obj); err != nil { |
| ds := &v1alpha1.Dataset{} | ||
| raw, _ := json.Marshal(ds) | ||
| req := admission.Request{ | ||
| AdmissionRequest: admissionv1.AdmissionRequest{ | ||
| Object: runtime.RawExtension{Raw: raw}, |
There was a problem hiding this comment.
These admission payloads are built by marshaling a v1alpha1.Dataset{} without setting apiVersion/kind (TypeMeta) or metadata.name. admission.Decoder.Decode relies on GVK in the raw object, so decoding may fail and the handler will fall back to the generic JSON path, making the test non-representative. Set TypeMeta/ObjectMeta (or craft raw JSON including them) so the test deterministically exercises the typed Dataset validation branch.
| ds.Spec.Mounts = []v1alpha1.Mount{{MountPoint: "abc"}} | ||
| raw, _ = json.Marshal(ds) | ||
| req.AdmissionRequest.Object.Raw = raw | ||
| resp = h.Handle(context.Background(), req) | ||
| assert.False(t, resp.Allowed) |
There was a problem hiding this comment.
The current webhook only rejects empty mountPoint, so this test case expecting denial for mountPoint: "abc" will fail. Either enforce the intended constraint in the webhook (e.g., align with the CRD’s MinLength=5 on MountPoint) or adjust/remove this test case.
|
|
||
| // Dataset missing mounts and runtimes -> denied | ||
| ds := &v1alpha1.Dataset{} | ||
| raw, _ := json.Marshal(ds) |
There was a problem hiding this comment.
json.Marshal errors are ignored here. Please check the returned error and fail the test if it’s non-nil to avoid hiding unexpected issues.
| // Dataset with invalid mount (too short) -> denied | ||
| ds = &v1alpha1.Dataset{} | ||
| ds.Spec.Mounts = []v1alpha1.Mount{{MountPoint: "abc"}} | ||
| raw, _ = json.Marshal(ds) |
There was a problem hiding this comment.
json.Marshal errors are ignored here. Please check the returned error and fail the test if it’s non-nil to avoid hiding unexpected issues.
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/webhook/admission" | ||
| ) | ||
|
|
There was a problem hiding this comment.
This new webhook entrypoint is missing a +kubebuilder:webhook marker (like pkg/webhook/handler/mutating/webhook.go) so controller-gen won’t generate a ValidatingWebhookConfiguration for it. Add the marker with the intended path/resources/verbs (e.g., Dataset CRDs) so the webhook can actually be installed.
| //+kubebuilder:webhook:path=/validate,mutating=false,sideEffects=None,failurePolicy=Fail,groups=*,resources=*,verbs=create;update;delete,versions=*,name=fluid-validating-webhook.fluid.io,admissionReviewVersions=v1 |
| package validating | ||
|
|
||
| import ( | ||
| "github.com/fluid-cloudnative/fluid/pkg/common" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" |
There was a problem hiding this comment.
Repository files under pkg/webhook/handler typically include the Apache 2.0 license header (e.g. pkg/webhook/handler/register.go, pkg/webhook/handler/mutating/webhook.go). Consider adding the standard header to this new source file for consistency and licensing compliance.
| // Try to decode into a known type (Dataset) when possible | ||
| var ds v1alpha1.Dataset | ||
| if h.decoder != nil { | ||
| if err := h.decoder.Decode(req, &ds); err == nil { |
There was a problem hiding this comment.
The metadata.name requirement is only enforced in the JSON fallback path. When Dataset decoding succeeds, the handler can allow a Dataset with an empty metadata.name (or only generateName) without running the name check, which conflicts with the PR description. Consider validating name in the typed branch (or doing the generic metadata validation first).
| if err := h.decoder.Decode(req, &ds); err == nil { | |
| if err := h.decoder.Decode(req, &ds); err == nil { | |
| // Ensure metadata.name is present in the typed Dataset path | |
| if ds.Name == "" { | |
| return admission.Denied("metadata.name is required") | |
| } |
| "testing" | ||
|
|
||
| "encoding/json" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| admissionv1 "k8s.io/api/admission/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/webhook/admission" | ||
| v1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" |
There was a problem hiding this comment.
Import grouping/order isn’t gofmt’d (e.g., encoding/json is separated from other stdlib imports). Please run gofmt/goimports so formatting checks don’t fail in CI.
| "testing" | |
| "encoding/json" | |
| "github.com/stretchr/testify/assert" | |
| admissionv1 "k8s.io/api/admission/v1" | |
| "k8s.io/apimachinery/pkg/runtime" | |
| "sigs.k8s.io/controller-runtime/pkg/webhook/admission" | |
| v1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" | |
| "encoding/json" | |
| "testing" | |
| v1alpha1 "github.com/fluid-cloudnative/fluid/api/v1alpha1" | |
| "github.com/stretchr/testify/assert" | |
| admissionv1 "k8s.io/api/admission/v1" | |
| "k8s.io/apimachinery/pkg/runtime" | |
| "sigs.k8s.io/controller-runtime/pkg/webhook/admission" |
…t validation, table-driven tests - Add Apache 2.0 license headers to all files - Add kubebuilder:webhook marker for config generation - Remove incorrect mounts+runtimes required check (Mounts is optional) - Remove raw JSON fallback path in favor of decoder-only flow - Handle DELETE requests with empty Object.Raw gracefully - Validate individual mount entries and runtime entries - Rewrite tests as table-driven with proper TypeMeta/ObjectMeta
|
Updated with a round of fixes based on the review feedback:
Could someone add |
|
Adds a basic validating admission webhook handler skeleton with a simple validation check (metadata.name required) and unit tests. This is the first step toward a full validating webhook for Fluid CRDs.