Fixes for 0.4.0 release

api/v1alpha1/clusterpolicy_webhook.go

@@ -342,6 +342,13 @@ func (v *ClusterPolicyCustomValidator) ValidateCreate(_ context.Context, cp *Clu
 
 // ValidateUpdate implements webhook.CustomValidator.
 func (v *ClusterPolicyCustomValidator) ValidateUpdate(_ context.Context, _ *ClusterPolicy, cp *ClusterPolicy) (admission.Warnings, error) {
+	// Skip spec validation once deletion has started. The controller drops its
+	// finalizer with a plain Update, which this webhook sees; rejecting it would
+	// leave the policy undeletable if its spec no longer passes current validation.
+	if !cp.DeletionTimestamp.IsZero() {
+		return nil, nil
+	}
+
 	return validateClusterPolicySpec(&cp.Spec)
 }
 

api/v1alpha1/clusterpolicy_webhook_test.go

@@ -23,6 +23,7 @@ import (
 	"github.com/distribution/reference"
 
 	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 const (
@@ -669,6 +670,21 @@ var _ = Describe("ClusterPolicy Webhook", func() {
 			_, err := validator.ValidateUpdate(ctx, old, obj)
 			Expect(err).NotTo(HaveOccurred())
 		})
+
+		// The controller removes its finalizer with a plain Update, which this
+		// webhook sees. If spec validation ran then, a policy whose spec no longer
+		// passes current validation could never be deleted.
+		It("does not block a deletion in progress on an invalid spec", func() {
+			old := validCP()
+			now := metav1.Now()
+			obj.DeletionTimestamp = &now
+			obj.Finalizers = []string{"gpu.intel.com/clusterpolicy-protection"}
+			obj.Spec.DevicePluginSpec.AllowIDs = []string{"0x56c0"}
+			obj.Spec.DevicePluginSpec.DenyIDs = []string{"0x1234"}
+
+			_, err := validator.ValidateUpdate(ctx, old, obj)
+			Expect(err).NotTo(HaveOccurred())
+		})
 	})
 
 	Context("ValidateDelete", func() {

api/v1alpha1/gpufirmwareupdate_types.go

@@ -38,6 +38,7 @@ type GPUFirmwareUpdateSpec struct {
 	ImagePullSecret string `json:"imagePullSecret,omitempty"`
 
 	// Target PCI Device ID in case the node has multiple GPU types. Format is '0xabcd'.
+	// +kubebuilder:validation:Pattern=`^0x[0-9a-f]{4}$`
 	PCIDeviceID string `json:"pciDeviceID,omitempty"`
 
 	// Taint key to be applied to nodes during firmware update.
@@ -101,7 +102,9 @@ type GPUFirmwareUpdateList struct {
 type GPUFirmwareFile struct {
 	// +kubebuilder:validation:Enum=GFX;GFX_DATA;GFX_CODE_DATA;GFX_PSCBIN;AMC;FAN_TABLE;VR_CONFIG;OPROM_CODE;OPROM_DATA
 	Type string `json:"type"`
-	// Filename of the firmware file without any directories.
+	// Filename of the firmware file without any directories. Must start with an
+	// alphanumeric character or '_', which also rules out "." and "..".
+	// +kubebuilder:validation:Pattern=`^[a-zA-Z0-9_][a-zA-Z0-9._-]*$`
 	FileName string `json:"filename"`
 	// SHA256 checksum of the firmware file. Format: sha256:<64 hex characters>.
 	// When set, content.containerImage must be digest-pinned (image@sha256:...).

api/v1alpha1/gpufirmwareupdate_webhook.go

@@ -60,7 +60,12 @@ var inProgressStates = map[string]bool{
 var _ admission.Validator[*GPUFirmwareUpdate] = &GPUFirmwareUpdateCustomValidator{}
 
 func validateInputs(spec *GPUFirmwareUpdateSpec) error {
-	fileNameReg := regexp.MustCompile(`^[a-zA-Z0-9._-]+$`)
+	// The first character must be alphanumeric or '_' so that "." and ".." are
+	// rejected: filepath.Base leaves both unchanged, so the path-component check
+	// below does not catch them. A leading '-' is excluded for the same reason
+	// the name is quoted in the update command - it must not read as a flag.
+	// Keep in sync with the Pattern marker on GPUFirmwareFile.FileName.
+	fileNameReg := regexp.MustCompile(`^[a-zA-Z0-9_][a-zA-Z0-9._-]*$`)
 
 	hasChecksum := false
 
@@ -129,6 +134,16 @@ func (v *GPUFirmwareUpdateCustomValidator) ValidateCreate(newObj context.Context
 
 // ValidateUpdate rejects changes to fields that are immutable once an update is in progress.
 func (v *GPUFirmwareUpdateCustomValidator) ValidateUpdate(_ context.Context, oldFU, newFU *GPUFirmwareUpdate) (admission.Warnings, error) {
+	// Skip input validation once deletion has started. The controller drops its
+	// finalizer with a plain Update, which this webhook sees; rejecting it would
+	// leave the CR undeletable if its spec no longer passes current validation.
+	// The immutability checks below still apply.
+	if newFU.DeletionTimestamp.IsZero() {
+		if err := validateInputs(&newFU.Spec); err != nil {
+			return nil, err
+		}
+	}
+
 	if !inProgressStates[oldFU.Status.State] {
 		return nil, nil
 	}
@@ -154,7 +169,7 @@ func (v *GPUFirmwareUpdateCustomValidator) ValidateUpdate(_ context.Context, old
 		}
 	}
 
-	return nil, validateInputs(&newFU.Spec)
+	return nil, nil
 }
 
 // ValidateDelete implements webhook.CustomValidator so a webhook will be registered for the type GPUFirmwareUpdate.

api/v1alpha1/gpufirmwareupdate_webhook_test.go

@@ -19,6 +19,7 @@ package v1alpha1
 import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 var _ = Describe("GPUFirmwareUpdate Webhook", func() {
@@ -234,6 +235,12 @@ var _ = Describe("GPUFirmwareUpdate Webhook", func() {
 				"someimage.bin&&rm -rf /",
 				"someimage.bin$(echo foo)",
 				"someimage.bin`echo foo`",
+				// filepath.Base leaves "." and ".." unchanged, so the path-component
+				// check above does not reject them - the character allow-list must.
+				".",
+				"..",
+				".hidden.bin",
+				"-rf",
 			}
 
 			for _, fname := range filenames {
@@ -250,6 +257,8 @@ var _ = Describe("GPUFirmwareUpdate Webhook", func() {
 				"0x12G4",
 				"0x123",
 				"0x12345",
+				// Lower-case only, matching every other PCI ID field in the API.
+				"0x12AB",
 			}
 
 			for _, pciId := range pciIds {
@@ -295,12 +304,64 @@ var _ = Describe("GPUFirmwareUpdate Webhook", func() {
 		It("Should validate updates correctly", func() {
 			By("simulating a valid update scenario")
 			oldObj.Status.State = "" // Not started state
-			oldObj.Spec.UpdaterImage = oldImageName
+			oldObj.Spec = GPUFirmwareUpdateSpec{
+				UpdaterImage: oldImageName,
+				UpdateMethod: "canary",
+				Content: GPUFirmwareContent{
+					ContainerImage: updaterImageName,
+					Files:          []GPUFirmwareFile{{Type: "GFX", FileName: "gfx.bin"}},
+				},
+				PCIDeviceID: "0x1234",
+			}
+			obj.Spec = oldObj.Spec
 			obj.Spec.UpdaterImage = newImageName
 			_, err := validator.ValidateUpdate(ctx, oldObj, obj)
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		It("Should validate inputs on updates outside in-progress states", func() {
+			By("rejecting shell metacharacters in a firmware filename")
+			obj.Spec = GPUFirmwareUpdateSpec{
+				UpdaterImage: updaterImageName,
+				UpdateMethod: "direct",
+				Content: GPUFirmwareContent{
+					ContainerImage: updaterImageName,
+					Files:          []GPUFirmwareFile{{Type: "GFX", FileName: `gfx.bin";echo pwned;"`}},
+				},
+				PCIDeviceID: "0x1234",
+			}
+
+			for _, state := range []string{"", "completed", "error"} {
+				oldObj.Status.State = state
+				_, err := validator.ValidateUpdate(ctx, oldObj, obj)
+				Expect(err).To(HaveOccurred(), "state %q", state)
+				Expect(err.Error()).To(ContainSubstring("invalid firmware filename"), "state %q", state)
+			}
+		})
+
+		// The controller removes its finalizer with a plain Update, which this
+		// webhook sees. If input validation ran then, a CR whose spec no longer
+		// passes current validation could never be deleted.
+		It("Should not block a deletion in progress on an invalid spec", func() {
+			now := metav1.Now()
+			obj.DeletionTimestamp = &now
+			obj.Finalizers = []string{"gpufirmwareupdate.intel.com/finalizer"}
+			obj.Spec = GPUFirmwareUpdateSpec{
+				UpdaterImage: updaterImageName,
+				UpdateMethod: "direct",
+				Content: GPUFirmwareContent{
+					ContainerImage: updaterImageName,
+					Files:          []GPUFirmwareFile{{Type: "GFX", FileName: `gfx.bin";echo pwned;"`}},
+				},
+				PCIDeviceID: "0x1234",
+			}
+			oldObj.Status.State = ""
+			oldObj.Spec = *obj.Spec.DeepCopy()
+
+			_, err := validator.ValidateUpdate(ctx, oldObj, obj)
+			Expect(err).NotTo(HaveOccurred())
+		})
+
 		It("Should validate ok if no changes to critical fields", func() {
 			By("simulating a valid update scenario")
 			oldObj.Status.State = "updating"

api/v1alpha1/gpurecoveryplan_types.go

@@ -69,17 +69,17 @@ const (
 
 // GPURecoveryPlanSpec defines the desired state of GPURecoveryPlan.
 type GPURecoveryPlanSpec struct {
-	// DeviceID is the mandatory PCI device ID of the target GPU. Format: '0x' followed by 4 hex digits.
-	// +kubebuilder:validation:Pattern=`^0x[0-9a-fA-F]{4}$`
+	// DeviceID is the mandatory PCI device ID of the target GPU. Format: '0x' followed by 4 lower-case hex digits.
+	// +kubebuilder:validation:Pattern=`^0x[0-9a-f]{4}$`
 	DeviceID string `json:"deviceId"`
 
-	// SubDeviceID is the optional PCI sub-device ID. Format: '0x' followed by 4 hex digits.
-	// +kubebuilder:validation:Pattern=`^0x[0-9a-fA-F]{4}$`
+	// SubDeviceID is the optional PCI sub-device ID. Format: '0x' followed by 4 lower-case hex digits.
+	// +kubebuilder:validation:Pattern=`^0x[0-9a-f]{4}$`
 	// +optional
 	SubDeviceID string `json:"subDeviceId,omitempty"`
 
-	// SubVendorID is the optional PCI sub-vendor ID. Format: '0x' followed by 4 hex digits.
-	// +kubebuilder:validation:Pattern=`^0x[0-9a-fA-F]{4}$`
+	// SubVendorID is the optional PCI sub-vendor ID. Format: '0x' followed by 4 lower-case hex digits.
+	// +kubebuilder:validation:Pattern=`^0x[0-9a-f]{4}$`
 	// +optional
 	SubVendorID string `json:"subVendorId,omitempty"`
 
@@ -261,7 +261,9 @@ type FirmwareSpec struct {
 	Source FirmwareSource `json:"source"`
 
 	// File is the filename of the FDO firmware image to flash, relative to the root of the
-	// source (no path components).
+	// source (no path components). Must start with an alphanumeric character or '_',
+	// which also rules out "." and "..".
+	// +kubebuilder:validation:Pattern=`^[a-zA-Z0-9_][a-zA-Z0-9._-]*$`
 	File string `json:"file"`
 }
 

api/v1alpha1/gpurecoveryplan_webhook.go

@@ -126,12 +126,16 @@ type GPURecoveryPlanCustomValidator struct{}
 
 var _ admission.Validator[*GPURecoveryPlan] = &GPURecoveryPlanCustomValidator{}
 
-var pciIDPattern = regexp.MustCompile(`^0x[0-9a-fA-F]{4}$`)
+var pciIDPattern = regexp.MustCompile(`^0x[0-9a-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._-]+$`)
+// The first character must be alphanumeric or '_', which rules out "." and ".." (filepath.Base
+// leaves both unchanged, so the path-component check does not catch them) and a leading '-'.
+// Keep in sync with the Pattern marker on FirmwareSpec.File, which enforces the same shape in
+// the API server so the schema still rejects these names if this webhook is not in the path.
+var firmwareFileNamePattern = regexp.MustCompile(`^[a-zA-Z0-9_][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) {
@@ -153,8 +157,14 @@ func (v *GPURecoveryPlanCustomValidator) ValidateUpdate(_ context.Context, oldPl
 
 	gpurecoveryplanlog.Info("Validation for GPURecoveryPlan upon update", "name", newPlan.GetName())
 
-	if err := validateRecoveryPlanSpec(&newPlan.Spec); err != nil {
-		return nil, err
+	// Skip spec validation once deletion has started. The controller drops its
+	// finalizer with a plain Update, which this webhook sees; rejecting it would
+	// leave the plan undeletable if its spec no longer passes current validation.
+	// The firmware immutability check below still applies.
+	if newPlan.DeletionTimestamp.IsZero() {
+		if err := validateRecoveryPlanSpec(&newPlan.Spec); err != nil {
+			return nil, err
+		}
 	}
 
 	if firmwareUpdateActive(oldPlan) && !reflect.DeepEqual(oldPlan.Spec.Firmware, newPlan.Spec.Firmware) {
@@ -183,7 +193,7 @@ func validateRecoveryPlanSpec(spec *GPURecoveryPlanSpec) error {
 	}
 
 	if !pciIDPattern.MatchString(spec.DeviceID) {
-		return fmt.Errorf("spec.deviceId %q must match pattern 0x[0-9a-fA-F]{4}", spec.DeviceID)
+		return fmt.Errorf("spec.deviceId %q must match pattern 0x[0-9a-f]{4}", spec.DeviceID)
 	}
 
 	// Mandatory, and restricted to the resets: reflash is not one, and no value is safe to guess.
@@ -199,11 +209,11 @@ func validateRecoveryPlanSpec(spec *GPURecoveryPlanSpec) error {
 	}
 
 	if spec.SubDeviceID != "" && !pciIDPattern.MatchString(spec.SubDeviceID) {
-		return fmt.Errorf("spec.subDeviceId %q must match pattern 0x[0-9a-fA-F]{4}", spec.SubDeviceID)
+		return fmt.Errorf("spec.subDeviceId %q must match pattern 0x[0-9a-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)
+		return fmt.Errorf("spec.subVendorId %q must match pattern 0x[0-9a-f]{4}", spec.SubVendorID)
 	}
 
 	if err := validateApprovals(spec.Approvals); err != nil {

api/v1alpha1/gpurecoveryplan_webhook_test.go

@@ -19,6 +19,7 @@ package v1alpha1
 import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // validPlan returns a minimal valid GPURecoveryPlan for use in tests. Minimal includes
@@ -207,11 +208,26 @@ var _ = Describe("GPURecoveryPlan Webhook", func() {
 
 		It("should accept valid optional PCI IDs", func() {
 			obj.Spec.SubDeviceID = "0xabcd"
-			obj.Spec.SubVendorID = "0xABCD"
+			obj.Spec.SubVendorID = "0x56c0"
 			_, err := validator.ValidateCreate(ctx, obj)
 			Expect(err).NotTo(HaveOccurred())
 		})
 
+		// PCI IDs are compared byte-for-byte against the deviceId attribute the DRA
+		// driver publishes on its ResourceSlices, which is lower-case. An upper-case
+		// ID would validate and then never match a device, so reject it up front.
+		DescribeTable("should reject upper-case hex in PCI IDs",
+			func(mutate func(*GPURecoveryPlan), field string) {
+				mutate(obj)
+				_, err := validator.ValidateCreate(ctx, obj)
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(ContainSubstring(field))
+			},
+			Entry("deviceId", func(p *GPURecoveryPlan) { p.Spec.DeviceID = "0xABCD" }, "deviceId"),
+			Entry("subDeviceId", func(p *GPURecoveryPlan) { p.Spec.SubDeviceID = "0xABCD" }, "subDeviceId"),
+			Entry("subVendorId", func(p *GPURecoveryPlan) { p.Spec.SubVendorID = "0x56C0" }, "subVendorId"),
+		)
+
 		Context("approvals validation", func() {
 			It("should reject an approval with both eventId and selector", func() {
 				obj.Spec.Approvals = []RecoveryApproval{
@@ -468,6 +484,11 @@ var _ = Describe("GPURecoveryPlan Webhook", func() {
 				Entry("space", "fw file.bin", "invalid characters"),
 				Entry("command substitution", "gfx.bin$(id)", "invalid characters"),
 				Entry("shell separator", "gfx.bin;rm", "invalid characters"),
+				// filepath.Base leaves "." and ".." unchanged, so only the character
+				// allow-list stands between these and the reflash command line.
+				Entry("current directory", ".", "invalid characters"),
+				Entry("parent directory", "..", "invalid characters"),
+				Entry("leading dash reads as a flag", "-rf", "invalid characters"),
 			)
 		})
 	})
@@ -486,6 +507,19 @@ var _ = Describe("GPURecoveryPlan Webhook", func() {
 			Expect(err).To(HaveOccurred())
 		})
 
+		// The controller removes its finalizer with a plain Update, which this
+		// webhook sees. If spec validation ran then, a plan whose spec no longer
+		// passes current validation could never be deleted.
+		It("should not block a deletion in progress on an invalid spec", func() {
+			now := metav1.Now()
+			obj.DeletionTimestamp = &now
+			obj.Finalizers = []string{"gpurecoveryplan.intel.com/finalizer"}
+			obj.Spec.DeviceID = "0xABCD"
+
+			_, err := validator.ValidateUpdate(ctx, oldObj, obj)
+			Expect(err).NotTo(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"}},

charts/gpu-base-operator-policy/values.yaml

@@ -37,7 +37,7 @@ dra:
 #           operator: DoesNotExist
 
 xpu:
-  image: ghcr.io/intel/xpumanager/xpumd:v2.1.0@sha256:67b492e40dd3c99a05abac67d45f480261b4521ed4415ff49f660f2db269126
+  image: ghcr.io/intel/xpumanager/xpumd:v2.1.0@sha256:67b492e40dd3c99a05abac67d45f480261b4521ed4415ff49f660f2db2691263
   logLevel: 2
   monitoringResource: monitoring
   configMapOverride: ""

charts/gpu-base-operator/crds/gpufirmwareupdates.yaml

@@ -56,7 +56,10 @@ spec:
                           pattern: ^sha256:[0-9a-f]{64}$
                           type: string
                         filename:
-                          description: Filename of the firmware file without any directories.
+                          description: |-
+                            Filename of the firmware file without any directories. Must start with an
+                            alphanumeric character or '_', which also rules out "." and "..".
+                          pattern: ^[a-zA-Z0-9_][a-zA-Z0-9._-]*$
                           type: string
                         type:
                           enum:
@@ -105,6 +108,7 @@ spec:
               pciDeviceID:
                 description: Target PCI Device ID in case the node has multiple GPU
                   types. Format is '0xabcd'.
+                pattern: ^0x[0-9a-f]{4}$
                 type: string
               tolerations:
                 description: Tolerations needed to be scheduled to the node.