From f2ffab4c3cfeb3de11ad6b6d5e2ef1e7a610f42a Mon Sep 17 00:00:00 2001 From: Tuomas Katila Date: Tue, 1 Sep 2026 14:54:49 +0300 Subject: [PATCH] recovery: add the GPURecoveryPlan webhook Add the defaulting and validating admission webhooks for GPURecoveryPlan. The defaulter generates an "app-XXXXXXXX" ID for every spec.approvals entry that does not carry one, and fills in xpuSmi.pullPolicy. defaultResetType is deliberately left alone: neither accepted value can be inferred from the cluster, and a wrong guess is silent - the recovery Job runs a reset the platform cannot perform and exits 0 - so the validator rejects the omission instead. The validator checks the PCI ID formats, restricts defaultResetType to the two platform resets, and rejects approval entries that can never match (both or neither of eventId/selector, duplicate IDs, persistent without a selector, node selectors that are not valid label sets). Two checks are load-bearing rather than cosmetic: spec.drain.namespacesToSkip entries that are not DNS labels would silently protect nothing, and spec.firmware.file is interpolated into the shell command a privileged reflash Job runs, so it is restricted to a character allow-list with no path components. On update, spec.firmware is immutable while a reflash event is in-progress or blocked - both are approved and about to flash. It stays editable in the missing-firmware and waiting-approval states, where the operator is asking the admin to correct that very field. Signed-off-by: Tuomas Katila --- PROJECT | 4 + api/v1alpha1/gpurecoveryplan_webhook.go | 347 ++++++++++ api/v1alpha1/gpurecoveryplan_webhook_test.go | 694 +++++++++++++++++++ api/v1alpha1/webhook_suite_test.go | 3 + api/v1alpha1/zz_generated.deepcopy.go | 30 + cmd/main.go | 4 + config/webhook/manifests.yaml | 40 ++ 7 files changed, 1122 insertions(+) create mode 100644 api/v1alpha1/gpurecoveryplan_webhook.go create mode 100644 api/v1alpha1/gpurecoveryplan_webhook_test.go diff --git a/PROJECT b/PROJECT index e45c2d8..c614922 100644 --- a/PROJECT +++ b/PROJECT @@ -38,4 +38,8 @@ resources: kind: GPURecoveryPlan path: github.com/intel/gpu-base-operator/api/v1alpha1 version: v1alpha1 + webhooks: + defaulting: true + validation: true + webhookVersion: v1 version: "3" diff --git a/api/v1alpha1/gpurecoveryplan_webhook.go b/api/v1alpha1/gpurecoveryplan_webhook.go new file mode 100644 index 0000000..b8b46c7 --- /dev/null +++ b/api/v1alpha1/gpurecoveryplan_webhook.go @@ -0,0 +1,347 @@ +/* +Copyright 2026 Intel Corporation. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + "path/filepath" + "reflect" + "regexp" + "strings" + + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/validation" + ctrl "sigs.k8s.io/controller-runtime" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + "github.com/distribution/reference" +) + +const ( + approvalPrefix = "app-" +) + +// nolint:unused +// log is for logging in this package. +var gpurecoveryplanlog = logf.Log.WithName("gpurecoveryplan-resource") + +// SetupGPURecoveryPlanWebhookWithManager registers the webhook for GPURecoveryPlan in the manager. +func SetupGPURecoveryPlanWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr, &GPURecoveryPlan{}). + WithValidator(&GPURecoveryPlanCustomValidator{}). + WithDefaulter(&GPURecoveryPlanCustomDefaulter{}). + Complete() +} + +// +kubebuilder:webhook:path=/mutate-intel-com-v1alpha1-gpurecoveryplan,mutating=true,failurePolicy=fail,sideEffects=None,groups=intel.com,resources=gpurecoveryplans,verbs=create;update,versions=v1alpha1,name=mgpurecoveryplan-v1alpha1.kb.io,admissionReviewVersions=v1 + +// GPURecoveryPlanCustomDefaulter struct is responsible for setting default values on the custom resource of the +// Kind GPURecoveryPlan when those are created or updated. +type GPURecoveryPlanCustomDefaulter struct{} + +var _ admission.Defaulter[*GPURecoveryPlan] = &GPURecoveryPlanCustomDefaulter{} + +// Default implements webhook.CustomDefaulter so a webhook will be registered for the Kind GPURecoveryPlan. +// It generates IDs for any spec.approvals entries that are missing one. +func (d *GPURecoveryPlanCustomDefaulter) Default(_ context.Context, plan *GPURecoveryPlan) error { + if plan == nil { + return fmt.Errorf("expected a GPURecoveryPlan object but got nil") + } + + gpurecoveryplanlog.Info("Defaulting for GPURecoveryPlan", "name", plan.GetName()) + + presentIDs := make(map[string]bool) + + for i := range plan.Spec.Approvals { + if plan.Spec.Approvals[i].ID != "" { + presentIDs[plan.Spec.Approvals[i].ID] = true + } + } + + for i := range plan.Spec.Approvals { + if plan.Spec.Approvals[i].ID == "" { + id, err := generateApprovalID(presentIDs) + if err != nil { + return fmt.Errorf("failed to generate approval ID: %w", err) + } + + plan.Spec.Approvals[i].ID = id + } + } + + if plan.Spec.XpuSmi.PullPolicy == "" { + plan.Spec.XpuSmi.PullPolicy = "IfNotPresent" + } + + return nil +} + +// generateApprovalID returns a random ID in the form "app-XXXXXXXX" (8 random hex chars). +func generateApprovalID(presentIDs map[string]bool) (string, error) { + // Try up to 10 times to generate a unique ID. + for i := 0; i < 10; i++ { + b := make([]byte, 4) + if _, err := rand.Read(b); err != nil { + return "", err + } + + // Create an ID and check if it already exists in the presentIDs map. + // If it does, generate a new one. + id := approvalPrefix + hex.EncodeToString(b) + if presentIDs[id] { + continue + } + + presentIDs[id] = true + + return id, nil + } + + return "", fmt.Errorf("failed to generate a unique approval ID after 10 attempts") +} + +// +kubebuilder:webhook:path=/validate-intel-com-v1alpha1-gpurecoveryplan,mutating=false,failurePolicy=fail,sideEffects=None,groups=intel.com,resources=gpurecoveryplans,verbs=create;update,versions=v1alpha1,name=vgpurecoveryplan-v1alpha1.kb.io,admissionReviewVersions=v1 + +// GPURecoveryPlanCustomValidator struct is responsible for validating the GPURecoveryPlan resource +// when it is created, updated, or deleted. +type GPURecoveryPlanCustomValidator struct{} + +var _ admission.Validator[*GPURecoveryPlan] = &GPURecoveryPlanCustomValidator{} + +var pciIDPattern = regexp.MustCompile(`^0x[0-9a-fA-F]{4}$`) + +// firmwareFileNamePattern is the allow-list for spec.firmware.file. Deliberately an +// allow-list and not a deny-list of shell metacharacters: the name ends up in a command line +// inside a privileged root container, where anything unanticipated is worse than a rejected CR. +var firmwareFileNamePattern = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) + +// ValidateCreate implements webhook.CustomValidator so a webhook will be registered for the type GPURecoveryPlan. +func (v *GPURecoveryPlanCustomValidator) ValidateCreate(_ context.Context, plan *GPURecoveryPlan) (admission.Warnings, error) { + if plan == nil { + return nil, fmt.Errorf("expected a GPURecoveryPlan object but got nil") + } + + gpurecoveryplanlog.Info("Validation for GPURecoveryPlan upon creation", "name", plan.GetName()) + + return nil, validateRecoveryPlanSpec(&plan.Spec) +} + +// ValidateUpdate implements webhook.CustomValidator so a webhook will be registered for the type GPURecoveryPlan. +// It also prevents firmware changes while any reflash event is in-progress. +func (v *GPURecoveryPlanCustomValidator) ValidateUpdate(_ context.Context, oldPlan, newPlan *GPURecoveryPlan) (admission.Warnings, error) { + if oldPlan == nil || newPlan == nil { + return nil, fmt.Errorf("expected GPURecoveryPlan objects but got nil") + } + + gpurecoveryplanlog.Info("Validation for GPURecoveryPlan upon update", "name", newPlan.GetName()) + + if err := validateRecoveryPlanSpec(&newPlan.Spec); err != nil { + return nil, err + } + + if firmwareUpdateActive(oldPlan) && !reflect.DeepEqual(oldPlan.Spec.Firmware, newPlan.Spec.Firmware) { + return nil, fmt.Errorf( + "spec.firmware is immutable while a reflash event is in-progress or blocked") + } + + return nil, nil +} + +// ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type GPURecoveryPlan. +func (v *GPURecoveryPlanCustomValidator) ValidateDelete(_ context.Context, plan *GPURecoveryPlan) (admission.Warnings, error) { + if plan == nil { + return nil, fmt.Errorf("expected a GPURecoveryPlan object but got nil") + } + + gpurecoveryplanlog.Info("Validation for GPURecoveryPlan upon deletion", "name", plan.GetName()) + + return nil, nil +} + +// validateRecoveryPlanSpec validates the spec fields of a GPURecoveryPlan. +func validateRecoveryPlanSpec(spec *GPURecoveryPlanSpec) error { + if spec.DeviceID == "" { + return fmt.Errorf("spec.deviceId is required") + } + + if !pciIDPattern.MatchString(spec.DeviceID) { + return fmt.Errorf("spec.deviceId %q must match pattern 0x[0-9a-fA-F]{4}", spec.DeviceID) + } + + // Mandatory, and restricted to the two platform-selected resets. + if spec.DefaultResetType == "" { + return fmt.Errorf("spec.defaultResetType is required: %q where the PCIe slots support hot-plug, %q otherwise", + RecoveryTypeSlot, RecoveryTypeAMC) + } + + if spec.DefaultResetType != RecoveryTypeSlot && spec.DefaultResetType != RecoveryTypeAMC { + return fmt.Errorf("spec.defaultResetType %q is not a platform reset; use %q or %q, and "+ + "spec.approvals[].override to run %q on a single event", + spec.DefaultResetType, RecoveryTypeSlot, RecoveryTypeAMC, RecoveryTypeSBR) + } + + if spec.SubDeviceID != "" && !pciIDPattern.MatchString(spec.SubDeviceID) { + return fmt.Errorf("spec.subDeviceId %q must match pattern 0x[0-9a-fA-F]{4}", spec.SubDeviceID) + } + + if spec.SubVendorID != "" && !pciIDPattern.MatchString(spec.SubVendorID) { + return fmt.Errorf("spec.subVendorId %q must match pattern 0x[0-9a-fA-F]{4}", spec.SubVendorID) + } + + if err := validateApprovals(spec.Approvals); err != nil { + return err + } + + if err := validateDrain(&spec.Drain); err != nil { + return err + } + + if spec.XpuSmi.Image != "" { + if _, err := reference.ParseAnyReference(spec.XpuSmi.Image); err != nil { + return fmt.Errorf("spec.xpuSmi.image %q is not a valid image reference: %w", spec.XpuSmi.Image, err) + } + } + + if spec.Firmware != nil { + if err := validateFirmware(spec.Firmware); err != nil { + return err + } + } + + return nil +} + +// validateApprovals checks that each approval entry is internally consistent. +func validateApprovals(approvals []RecoveryApproval) error { + seenIDs := make(map[string]bool) + + for i, a := range approvals { + if a.ID != "" { + if seenIDs[a.ID] { + return fmt.Errorf("spec.approvals[%d]: duplicate approval ID %q", i, a.ID) + } + + seenIDs[a.ID] = true + } + + hasEventID := a.EventID != "" + hasSelector := a.Selector != nil + + if hasEventID && hasSelector { + return fmt.Errorf("spec.approvals[%d]: eventId and selector are mutually exclusive", i) + } + + if !hasEventID && !hasSelector { + return fmt.Errorf("spec.approvals[%d]: one of eventId or selector must be set", i) + } + + if a.Persistent && !hasSelector { + return fmt.Errorf("spec.approvals[%d]: persistent=true is only valid with a selector", i) + } + + // Reject label selectors that can never match a real Node. Node label matching + // uses labels.SelectorFromSet, which does not validate its input, so an invalid + // key would silently match nothing and the approval would appear to be ignored. + if hasSelector && len(a.Selector.NodeSelector) > 0 { + if _, err := labels.ValidatedSelectorFromSet(labels.Set(a.Selector.NodeSelector)); err != nil { + return fmt.Errorf("spec.approvals[%d].selector.nodeSelector is not a valid label selector: %w", i, err) + } + } + } + + return nil +} + +// validateDrain checks the pre-reset drain configuration. +func validateDrain(drain *DrainSpec) error { + seen := make(map[string]bool, len(drain.NamespacesToSkip)) + + for i, ns := range drain.NamespacesToSkip { + if errs := validation.IsDNS1123Label(ns); len(errs) > 0 { + return fmt.Errorf("spec.drain.namespacesToSkip[%d] %q is not a valid namespace name: %s", + i, ns, strings.Join(errs, "; ")) + } + + if seen[ns] { + return fmt.Errorf("spec.drain.namespacesToSkip[%d]: duplicate namespace %q", i, ns) + } + + seen[ns] = true + } + + return nil +} + +// validateFirmware validates the reflash firmware spec. +func validateFirmware(fw *FirmwareSpec) error { + if fw.Source.ContainerSource == nil && fw.Source.VolumeSource == nil { + return fmt.Errorf("spec.firmware.source: at least one of containerSource or volumeSource must be set") + } + + if fw.Source.ContainerSource != nil { + if fw.Source.ContainerSource.Name == "" { + return fmt.Errorf("spec.firmware.source.containerSource.name must not be empty") + } + + if _, err := reference.ParseAnyReference(fw.Source.ContainerSource.Name); err != nil { + return fmt.Errorf("spec.firmware.source.containerSource.name %q is not a valid image reference: %w", fw.Source.ContainerSource.Name, err) + } + } + + if fw.Source.VolumeSource != nil && fw.Source.VolumeSource.Name == "" { + return fmt.Errorf("spec.firmware.source.volumeSource.name must not be empty") + } + + // The filename is interpolated into the shell command the reflash Job runs, so both checks + // below are load-bearing rather than cosmetic: a path component would let the flash read + // outside the firmware mount, and the character allow-list keeps shell metacharacters out of + // a command running privileged as root. + if fw.File == "" { + return fmt.Errorf("spec.firmware.file must not be empty") + } + + if filepath.Base(fw.File) != fw.File { + return fmt.Errorf("spec.firmware.file %q must not contain path components", fw.File) + } + + if !firmwareFileNamePattern.MatchString(fw.File) { + return fmt.Errorf("spec.firmware.file %q contains invalid characters", fw.File) + } + + return nil +} + +// firmwareUpdateActive returns true when any reflash-type event is blocking +// changes to spec.firmware. +func firmwareUpdateActive(plan *GPURecoveryPlan) bool { + for _, evt := range plan.Status.Events { + if !evt.RecoveryType.IsReflash() { + continue + } + + switch evt.State { + case RecoveryEventStateInProgress, RecoveryEventStateBlocked: + return true + } + } + + return false +} diff --git a/api/v1alpha1/gpurecoveryplan_webhook_test.go b/api/v1alpha1/gpurecoveryplan_webhook_test.go new file mode 100644 index 0000000..0bee830 --- /dev/null +++ b/api/v1alpha1/gpurecoveryplan_webhook_test.go @@ -0,0 +1,694 @@ +/* +Copyright 2026 Intel Corporation. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +// validPlan returns a minimal valid GPURecoveryPlan for use in tests. Minimal includes +// defaultResetType: the platform's reset mechanism cannot be inferred, so the field is mandatory. +func validPlan() *GPURecoveryPlan { + return &GPURecoveryPlan{ + Spec: GPURecoveryPlanSpec{ + DeviceID: "0x1234", + DefaultResetType: RecoveryTypeSlot, + }, + } +} + +var _ = Describe("GPURecoveryPlan Webhook", func() { + var ( + obj *GPURecoveryPlan + oldObj *GPURecoveryPlan + validator GPURecoveryPlanCustomValidator + defaulter GPURecoveryPlanCustomDefaulter + ) + + BeforeEach(func() { + obj = validPlan() + oldObj = validPlan() + validator = GPURecoveryPlanCustomValidator{} + defaulter = GPURecoveryPlanCustomDefaulter{} + Expect(validator).NotTo(BeNil()) + Expect(defaulter).NotTo(BeNil()) + }) + + // ── Defaulter ──────────────────────────────────────────────────────────────── + + Context("Defaulting Webhook", func() { + It("should generate IDs for approvals that have none", func() { + obj.Spec.Approvals = []RecoveryApproval{ + {EventID: "evt-aabb"}, + {EventID: "evt-ccdd"}, + } + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + + Expect(obj.Spec.Approvals[0].ID).To(MatchRegexp(`^app-[0-9a-f]{8}$`)) + Expect(obj.Spec.Approvals[1].ID).To(MatchRegexp(`^app-[0-9a-f]{8}$`)) + }) + + It("should not overwrite an existing approval ID", func() { + obj.Spec.Approvals = []RecoveryApproval{ + {ID: "app-12345678", EventID: "evt-aabb"}, + } + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + Expect(obj.Spec.Approvals[0].ID).To(Equal("app-12345678")) + }) + + It("should leave an empty approvals list unchanged", func() { + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + Expect(obj.Spec.Approvals).To(BeEmpty()) + }) + + // defaultResetType is deliberately not defaulted: neither accepted value is safe to + // assume, and a wrong guess is silent — the Job runs a reset the platform cannot perform + // and exits 0. The validator rejects the omission instead. + It("should not invent a defaultResetType", func() { + obj.Spec.DefaultResetType = "" + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + Expect(obj.Spec.DefaultResetType).To(BeEmpty()) + }) + + It("should not overwrite an explicit defaultResetType", func() { + obj.Spec.DefaultResetType = RecoveryTypeAMC + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + Expect(obj.Spec.DefaultResetType).To(Equal(RecoveryTypeAMC)) + }) + + It("should generate distinct IDs across multiple approvals", func() { + obj.Spec.Approvals = make([]RecoveryApproval, 10) + for i := range obj.Spec.Approvals { + obj.Spec.Approvals[i] = RecoveryApproval{ + Selector: &ApprovalSelector{RecoveryType: RecoveryTypeSBR}, + } + } + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + + seen := make(map[string]bool) + for _, a := range obj.Spec.Approvals { + Expect(a.ID).To(MatchRegexp(`^app-[0-9a-f]{8}$`)) + Expect(seen[a.ID]).To(BeFalse(), "duplicate ID generated: %s", a.ID) + seen[a.ID] = true + } + }) + + // The CRD defaults spec.xpuSmi to {pullPolicy: IfNotPresent}, which covers the field being + // absent; this covers spec.xpuSmi being written with only an image in it, and the case + // where webhooks are the only defaulting in play. + It("should default xpuSmi.pullPolicy", func() { + obj.Spec.XpuSmi = XpuSmiSpec{Image: "registry/xpu-smi:latest"} + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + Expect(obj.Spec.XpuSmi.PullPolicy).To(Equal("IfNotPresent")) + }) + + It("should not overwrite an explicit xpuSmi.pullPolicy", func() { + obj.Spec.XpuSmi.PullPolicy = "Always" + + Expect(defaulter.Default(ctx, obj)).To(Succeed()) + Expect(obj.Spec.XpuSmi.PullPolicy).To(Equal("Always")) + }) + }) + + // ── Validator – ValidateCreate ──────────────────────────────────────────────── + + Context("ValidateCreate", func() { + It("should accept a minimal valid plan", func() { + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject a missing deviceId", func() { + obj.Spec.DeviceID = "" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("deviceId")) + }) + + It("should reject an invalid deviceId format", func() { + obj.Spec.DeviceID = "1234" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("deviceId")) + }) + + // The error has to name the choice, not just the schema: nothing on a node reports whether + // its PCIe slots do hot-plug, so an admin hitting this needs to be told what to look at. + It("should reject a missing defaultResetType and say how to pick one", func() { + obj.Spec.DefaultResetType = "" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("defaultResetType")) + Expect(err.Error()).To(ContainSubstring("hot-plug")) + }) + + // sbr and reflash are valid RecoveryTypes but not platform defaults: sbr is the per-card + // backup and reflash is not a reset. As a cluster-wide default either would apply to every + // wedged GPU the DRA driver reports. + DescribeTable("should reject a defaultResetType that is not a platform reset", + func(rt RecoveryType) { + obj.Spec.DefaultResetType = rt + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("defaultResetType")) + }, + Entry("sbr, the per-card backup", RecoveryTypeSBR), + Entry("reflash, not a reset at all", RecoveryTypeReflash), + Entry("a value outside the enum", RecoveryType("flr")), + ) + + It("should accept amc as a defaultResetType", func() { + obj.Spec.DefaultResetType = RecoveryTypeAMC + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject an invalid subDeviceId format", func() { + obj.Spec.SubDeviceID = "0xGGGG" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("subDeviceId")) + }) + + It("should reject an invalid subVendorId format", func() { + obj.Spec.SubVendorID = "0xZZZZ" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("subVendorId")) + }) + + It("should accept valid optional PCI IDs", func() { + obj.Spec.SubDeviceID = "0xabcd" + obj.Spec.SubVendorID = "0xABCD" + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + Context("approvals validation", func() { + It("should reject an approval with both eventId and selector", func() { + obj.Spec.Approvals = []RecoveryApproval{ + { + EventID: "evt-aabb", + Selector: &ApprovalSelector{RecoveryType: RecoveryTypeSBR}, + }, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("mutually exclusive")) + }) + + It("should reject an approval with neither eventId nor selector", func() { + obj.Spec.Approvals = []RecoveryApproval{{Comment: "no target"}} + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("one of eventId or selector must be set")) + }) + + It("should reject duplicate approval IDs", func() { + obj.Spec.Approvals = []RecoveryApproval{ + {ID: "app-1234", EventID: "evt-aabb"}, + {ID: "app-1234", EventID: "evt-ccdd"}, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("duplicate approval ID")) + }) + + It("should reject persistent=true without a selector", func() { + obj.Spec.Approvals = []RecoveryApproval{ + {EventID: "evt-aabb", Persistent: true}, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("persistent")) + }) + + It("should accept a valid eventId approval", func() { + obj.Spec.Approvals = []RecoveryApproval{ + {ID: "app-1234", EventID: "evt-aabb"}, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should accept a consumed eventId approval (audit trail entry)", func() { + obj.Spec.Approvals = []RecoveryApproval{ + {ID: "app-1234", EventID: "evt-aabb", Consumed: true}, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should accept a valid selector approval", func() { + obj.Spec.Approvals = []RecoveryApproval{ + { + Selector: &ApprovalSelector{RecoveryType: RecoveryTypeReflash}, + }, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should accept a persistent selector approval", func() { + obj.Spec.Approvals = []RecoveryApproval{ + { + Selector: &ApprovalSelector{RecoveryType: RecoveryTypeSBR}, + Persistent: true, + }, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should accept a valid nodeSelector", func() { + obj.Spec.Approvals = []RecoveryApproval{ + { + Selector: &ApprovalSelector{ + NodeSelector: map[string]string{"rack": "rack-04-32", "gpu.intel.com/family": "bmg"}, + }, + }, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject a nodeSelector with an invalid label key", func() { + // labels.SelectorFromSet does not validate, so an invalid key would + // silently match no nodes and the approval would appear to be ignored. + obj.Spec.Approvals = []RecoveryApproval{ + { + Selector: &ApprovalSelector{ + NodeSelector: map[string]string{"not a valid key": "x"}, + }, + }, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("nodeSelector")) + }) + + It("should reject a nodeSelector with an invalid label value", func() { + obj.Spec.Approvals = []RecoveryApproval{ + { + Selector: &ApprovalSelector{ + NodeSelector: map[string]string{"rack": "not a valid value"}, + }, + }, + } + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("nodeSelector")) + }) + }) + + // A namespace name that cannot exist makes the entry a silent no-op: the drain evicts the + // pods the admin meant to protect and nothing in the CR says why. The CRD's item pattern + // catches most of it, but the webhook is what produces a message naming the field and the + // value, and it still runs where the CRD is applied by an older chart. + Context("drain validation", func() { + It("should accept a valid namespacesToSkip list", func() { + obj.Spec.Drain.NamespacesToSkip = []string{"kube-system", "cert-manager"} + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should accept an empty namespacesToSkip list", func() { + obj.Spec.Drain.NamespacesToSkip = nil + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject a namespace name that is not a DNS label", func() { + obj.Spec.Drain.NamespacesToSkip = []string{"Kube System"} + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("namespacesToSkip[0]")) + }) + + It("should reject an empty namespace name", func() { + obj.Spec.Drain.NamespacesToSkip = []string{"kube-system", ""} + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("namespacesToSkip[1]")) + }) + + // A duplicate is harmless to the drain itself, but it is a sign the admin edited the + // list by hand and meant to write two different namespaces. + It("should reject a duplicate namespace", func() { + obj.Spec.Drain.NamespacesToSkip = []string{"kube-system", "kube-system"} + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("duplicate namespace")) + }) + }) + + Context("firmware validation", func() { + validFW := func() *FirmwareSpec { + return &FirmwareSpec{ + Source: FirmwareSource{ + ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:latest"}, + }, + File: "gfx.bin", + } + } + + It("should accept a valid firmware spec", func() { + obj.Spec.Firmware = validFW() + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject an invalid xpuSmi.image reference", func() { + obj.Spec.XpuSmi.Image = "INVALID IMAGE::" + obj.Spec.Firmware = validFW() + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("xpuSmi.image")) + }) + + It("should accept a valid xpuSmi.image", func() { + obj.Spec.XpuSmi.Image = "registry/xpu-smi:latest" + obj.Spec.Firmware = validFW() + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject missing source (no container and no volume)", func() { + fw := validFW() + fw.Source = FirmwareSource{} + obj.Spec.Firmware = fw + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("source")) + }) + + It("should accept a volumeSource instead of containerSource", func() { + fw := validFW() + fw.Source = FirmwareSource{ + VolumeSource: &VolumeFirmwareSource{Name: "my-pvc"}, + } + obj.Spec.Firmware = fw + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject an empty volumeSource name", func() { + fw := validFW() + fw.Source = FirmwareSource{ + VolumeSource: &VolumeFirmwareSource{Name: ""}, + } + obj.Spec.Firmware = fw + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("volumeSource")) + }) + + It("should reject an empty file", func() { + fw := validFW() + fw.File = "" + obj.Spec.Firmware = fw + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("file must not be empty")) + }) + + // The name is interpolated into the reflash Job's shell command unquoted, so the + // next three entries are security checks, not tidiness: a path component escapes the + // firmware mount and a shell metacharacter runs as root in a privileged container. + DescribeTable("should reject an unsafe file name", + func(name, wantMsg string) { + fw := validFW() + fw.File = name + obj.Spec.Firmware = fw + _, err := validator.ValidateCreate(ctx, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring(wantMsg)) + }, + Entry("relative path escape", "../etc/passwd", "path components"), + Entry("absolute path", "/etc/passwd", "path components"), + Entry("subdirectory", "fw/gfx.bin", "path components"), + Entry("space", "fw file.bin", "invalid characters"), + Entry("command substitution", "gfx.bin$(id)", "invalid characters"), + Entry("shell separator", "gfx.bin;rm", "invalid characters"), + ) + }) + }) + + // ── Validator – ValidateUpdate ──────────────────────────────────────────────── + + Context("ValidateUpdate", func() { + It("should accept a valid update with no active reflash events", func() { + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should reject an invalid spec on update", func() { + obj.Spec.DeviceID = "bad" + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + }) + + It("should reject firmware changes while a reflash event is in-progress", func() { + oldObj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-aabb", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeReflash}, + State: RecoveryEventStateInProgress, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v2"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("firmware")) + Expect(err.Error()).To(ContainSubstring("immutable")) + }) + + It("should allow firmware changes when reflash event is succeeded", func() { + oldObj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-aabb", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeReflash}, + State: RecoveryEventStateSucceeded, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v2"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + // missing-firmware, and waiting-approval after a failed image check, are the two situations + // in which the operator is asking the admin to change this very field. Rejecting the change + // there is a deadlock, not protection: the state exists to request an edit, and the guard + // forbade the edit. + // + // The nil -> set case below is the worst of them. It is the ordinary way a reflash gets + // configured — the plan is written, a card drops into FDO mode, the event parks in + // missing-firmware — and there was no way out of it short of deleting the event or the + // whole plan, with the card unrecoverable in the meantime. + It("should allow firmware to be set for the first time while an event is missing-firmware", func() { + oldObj.Spec.Firmware = nil + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-ccdd", + NodeName: "node02", + GPUBDF: "0000:03:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeReflash}, + State: RecoveryEventStateMissingFirmware, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred(), + "missing-firmware means the operator is waiting for this field; refusing to accept "+ + "it leaves the card unrecoverable with no diagnostic pointing at the webhook") + }) + + It("should allow firmware to be corrected while an event is missing-firmware", func() { + oldObj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{VolumeSource: &VolumeFirmwareSource{Name: "fw-pvc"}}, + File: "gfx.bin", + } + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-ccdd", + NodeName: "node02", + GPUBDF: "0000:03:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeReflash}, + State: RecoveryEventStateMissingFirmware, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred(), + "a volume-only source is one of the three things that park an event in "+ + "missing-firmware, so switching it to a container source has to be permitted") + }) + + // The state the user hit in a real cluster: the fwfiles image reference was wrong. The event + // is sent back to waiting-approval with its approval retained, and the correction is the + // only thing that makes the operator check the registry again — so accepting the edit is + // the whole fix, and blocking it would make the admin delete and re-approve. + It("should allow firmware to be corrected after an event failed image verification", func() { + oldObj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/typo:v1"}}, + File: "gfx.bin", + } + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-ccdd", + NodeName: "node02", + GPUBDF: "0000:03:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeReflash}, + State: RecoveryEventStateWaitingApproval, + ApprovalID: "apr-1", + ImageVerifyGeneration: 4, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred(), + "the image check reports an unpullable reference; the correction must be accepted") + }) + + // A blocked reflash is already approved and starts by itself once its node frees up, with + // no further admin action. Editing the spec in that window would flash firmware nobody + // approved — the same hazard as in-progress, just before the Job exists. + It("should reject firmware changes while a reflash event is blocked", func() { + oldObj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-eeff", + NodeName: "node03", + GPUBDF: "0000:04:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeReflash}, + State: RecoveryEventStateBlocked, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v2"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("immutable")) + }) + + // A blocked *reset* says nothing about firmware. Blocking edits on it would freeze the + // field for the duration of an unrelated queue. + It("should allow firmware changes while a blocked event is a reset", func() { + oldObj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v1"}}, + File: "gfx.bin", + } + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-1122", + NodeName: "node04", + GPUBDF: "0000:05:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeSBR}, + State: RecoveryEventStateBlocked, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v2"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + }) + + It("should allow a firmware change when no reflash events exist", func() { + oldObj.Status.Events = []RecoveryEvent{ + { + ID: "evt-aabb", + NodeName: "node01", + GPUBDF: "0000:02:00.0", + RecoveryType: RecoveryTypeSpec{Type: RecoveryTypeSBR}, + State: RecoveryEventStateInProgress, + }, + } + + obj.Spec.Firmware = &FirmwareSpec{ + Source: FirmwareSource{ContainerSource: &ContainerFirmwareSource{Name: "registry/fw:v2"}}, + File: "gfx.bin", + } + + _, err := validator.ValidateUpdate(ctx, oldObj, obj) + Expect(err).NotTo(HaveOccurred()) + }) + }) + + // ── Validator – ValidateDelete ──────────────────────────────────────────────── + + Context("ValidateDelete", func() { + It("should always allow deletion", func() { + _, err := validator.ValidateDelete(ctx, obj) + Expect(err).NotTo(HaveOccurred()) + }) + }) +}) diff --git a/api/v1alpha1/webhook_suite_test.go b/api/v1alpha1/webhook_suite_test.go index 3a41845..57f3123 100644 --- a/api/v1alpha1/webhook_suite_test.go +++ b/api/v1alpha1/webhook_suite_test.go @@ -113,6 +113,9 @@ var _ = BeforeSuite(func() { err = SetupClusterPolicyWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) + err = SetupGPURecoveryPlanWebhookWithManager(mgr) + Expect(err).NotTo(HaveOccurred()) + // +kubebuilder:scaffold:webhook go func() { diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 9d0c599..6f40c13 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -611,6 +611,36 @@ func (in *GPURecoveryPlan) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GPURecoveryPlanCustomDefaulter) DeepCopyInto(out *GPURecoveryPlanCustomDefaulter) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GPURecoveryPlanCustomDefaulter. +func (in *GPURecoveryPlanCustomDefaulter) DeepCopy() *GPURecoveryPlanCustomDefaulter { + if in == nil { + return nil + } + out := new(GPURecoveryPlanCustomDefaulter) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *GPURecoveryPlanCustomValidator) DeepCopyInto(out *GPURecoveryPlanCustomValidator) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new GPURecoveryPlanCustomValidator. +func (in *GPURecoveryPlanCustomValidator) DeepCopy() *GPURecoveryPlanCustomValidator { + if in == nil { + return nil + } + out := new(GPURecoveryPlanCustomValidator) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *GPURecoveryPlanList) DeepCopyInto(out *GPURecoveryPlanList) { *out = *in diff --git a/cmd/main.go b/cmd/main.go index 5021218..0b555bd 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -379,6 +379,10 @@ func main() { setupLog.Error(err, "unable to create webhook", "webhook", "ClusterPolicy") os.Exit(1) } + if err := intelcomv1alpha1.SetupGPURecoveryPlanWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "GPURecoveryPlan") + os.Exit(1) + } } // +kubebuilder:scaffold:builder diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 942cb88..0319f94 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -24,6 +24,26 @@ webhooks: resources: - clusterpolicies sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-intel-com-v1alpha1-gpurecoveryplan + failurePolicy: Fail + name: mgpurecoveryplan-v1alpha1.kb.io + rules: + - apiGroups: + - intel.com + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - gpurecoveryplans + sideEffects: None --- apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration @@ -70,3 +90,23 @@ webhooks: resources: - gpufirmwareupdates sideEffects: None +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /validate-intel-com-v1alpha1-gpurecoveryplan + failurePolicy: Fail + name: vgpurecoveryplan-v1alpha1.kb.io + rules: + - apiGroups: + - intel.com + apiVersions: + - v1alpha1 + operations: + - CREATE + - UPDATE + resources: + - gpurecoveryplans + sideEffects: None