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