diff --git a/api/v1alpha1/clusterpolicy_webhook.go b/api/v1alpha1/clusterpolicy_webhook.go index 7c1312f..0434913 100644 --- a/api/v1alpha1/clusterpolicy_webhook.go +++ b/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) } diff --git a/api/v1alpha1/clusterpolicy_webhook_test.go b/api/v1alpha1/clusterpolicy_webhook_test.go index 9404be2..4869011 100644 --- a/api/v1alpha1/clusterpolicy_webhook_test.go +++ b/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() { diff --git a/api/v1alpha1/gpufirmwareupdate_types.go b/api/v1alpha1/gpufirmwareupdate_types.go index 825cb32..85510e9 100644 --- a/api/v1alpha1/gpufirmwareupdate_types.go +++ b/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:...). diff --git a/api/v1alpha1/gpufirmwareupdate_webhook.go b/api/v1alpha1/gpufirmwareupdate_webhook.go index 9316a59..ca33d24 100644 --- a/api/v1alpha1/gpufirmwareupdate_webhook.go +++ b/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. diff --git a/api/v1alpha1/gpufirmwareupdate_webhook_test.go b/api/v1alpha1/gpufirmwareupdate_webhook_test.go index f7bdc09..7c4c6db 100644 --- a/api/v1alpha1/gpufirmwareupdate_webhook_test.go +++ b/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" diff --git a/api/v1alpha1/gpurecoveryplan_types.go b/api/v1alpha1/gpurecoveryplan_types.go index 7199273..ec38a62 100644 --- a/api/v1alpha1/gpurecoveryplan_types.go +++ b/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"` } diff --git a/api/v1alpha1/gpurecoveryplan_webhook.go b/api/v1alpha1/gpurecoveryplan_webhook.go index bff4d61..86af5ca 100644 --- a/api/v1alpha1/gpurecoveryplan_webhook.go +++ b/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 { diff --git a/api/v1alpha1/gpurecoveryplan_webhook_test.go b/api/v1alpha1/gpurecoveryplan_webhook_test.go index 2074311..bf7ab8d 100644 --- a/api/v1alpha1/gpurecoveryplan_webhook_test.go +++ b/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"}}, diff --git a/charts/gpu-base-operator-policy/values.yaml b/charts/gpu-base-operator-policy/values.yaml index 0c655db..0e5edd9 100644 --- a/charts/gpu-base-operator-policy/values.yaml +++ b/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: "" diff --git a/charts/gpu-base-operator/crds/gpufirmwareupdates.yaml b/charts/gpu-base-operator/crds/gpufirmwareupdates.yaml index 9251844..2324b18 100644 --- a/charts/gpu-base-operator/crds/gpufirmwareupdates.yaml +++ b/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. diff --git a/charts/gpu-base-operator/crds/gpurecoveryplans.yaml b/charts/gpu-base-operator/crds/gpurecoveryplans.yaml index 08b6bdc..ec13d92 100644 --- a/charts/gpu-base-operator/crds/gpurecoveryplans.yaml +++ b/charts/gpu-base-operator/crds/gpurecoveryplans.yaml @@ -158,8 +158,8 @@ spec: type: string deviceId: description: 'DeviceID is the mandatory PCI device ID of the target - GPU. Format: ''0x'' followed by 4 hex digits.' - pattern: ^0x[0-9a-fA-F]{4}$ + GPU. Format: ''0x'' followed by 4 lower-case hex digits.' + pattern: ^0x[0-9a-f]{4}$ type: string drain: default: @@ -201,7 +201,9 @@ spec: file: description: |- 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 "..". + pattern: ^[a-zA-Z0-9_][a-zA-Z0-9._-]*$ type: string source: description: Source specifies where the firmware file can be found. @@ -250,13 +252,13 @@ spec: type: boolean subDeviceId: description: 'SubDeviceID is the optional PCI sub-device ID. Format: - ''0x'' followed by 4 hex digits.' - pattern: ^0x[0-9a-fA-F]{4}$ + ''0x'' followed by 4 lower-case hex digits.' + pattern: ^0x[0-9a-f]{4}$ type: string subVendorId: description: 'SubVendorID is the optional PCI sub-vendor ID. Format: - ''0x'' followed by 4 hex digits.' - pattern: ^0x[0-9a-fA-F]{4}$ + ''0x'' followed by 4 lower-case hex digits.' + pattern: ^0x[0-9a-f]{4}$ type: string timeouts: default: diff --git a/cmd/main.go b/cmd/main.go index b69c965..c30c547 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -383,20 +383,18 @@ func main() { os.Exit(1) } - // nolint:goconst - if os.Getenv("DISABLE_WEBHOOKS") != "true" { - if err := intelcomv1alpha1.SetupGPUFirmwareUpdateWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "GPUFirmwareUpdate") - os.Exit(1) - } - if err := intelcomv1alpha1.SetupClusterPolicyWebhookWithManager(mgr); err != nil { - 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) - } + // Webhooks are critical for operator's FW update and recovery operations. + if err := intelcomv1alpha1.SetupGPUFirmwareUpdateWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create webhook", "webhook", "GPUFirmwareUpdate") + os.Exit(1) + } + if err := intelcomv1alpha1.SetupClusterPolicyWebhookWithManager(mgr); err != nil { + 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/crd/bases/intel.com_gpufirmwareupdates.yaml b/config/crd/bases/intel.com_gpufirmwareupdates.yaml index 9251844..2324b18 100644 --- a/config/crd/bases/intel.com_gpufirmwareupdates.yaml +++ b/config/crd/bases/intel.com_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. diff --git a/config/crd/bases/intel.com_gpurecoveryplans.yaml b/config/crd/bases/intel.com_gpurecoveryplans.yaml index 08b6bdc..ec13d92 100644 --- a/config/crd/bases/intel.com_gpurecoveryplans.yaml +++ b/config/crd/bases/intel.com_gpurecoveryplans.yaml @@ -158,8 +158,8 @@ spec: type: string deviceId: description: 'DeviceID is the mandatory PCI device ID of the target - GPU. Format: ''0x'' followed by 4 hex digits.' - pattern: ^0x[0-9a-fA-F]{4}$ + GPU. Format: ''0x'' followed by 4 lower-case hex digits.' + pattern: ^0x[0-9a-f]{4}$ type: string drain: default: @@ -201,7 +201,9 @@ spec: file: description: |- 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 "..". + pattern: ^[a-zA-Z0-9_][a-zA-Z0-9._-]*$ type: string source: description: Source specifies where the firmware file can be found. @@ -250,13 +252,13 @@ spec: type: boolean subDeviceId: description: 'SubDeviceID is the optional PCI sub-device ID. Format: - ''0x'' followed by 4 hex digits.' - pattern: ^0x[0-9a-fA-F]{4}$ + ''0x'' followed by 4 lower-case hex digits.' + pattern: ^0x[0-9a-f]{4}$ type: string subVendorId: description: 'SubVendorID is the optional PCI sub-vendor ID. Format: - ''0x'' followed by 4 hex digits.' - pattern: ^0x[0-9a-fA-F]{4}$ + ''0x'' followed by 4 lower-case hex digits.' + pattern: ^0x[0-9a-f]{4}$ type: string timeouts: default: diff --git a/internal/controller/gpurecoveryplan_reflash_test.go b/internal/controller/gpurecoveryplan_reflash_test.go index 098490c..a2e022d 100644 --- a/internal/controller/gpurecoveryplan_reflash_test.go +++ b/internal/controller/gpurecoveryplan_reflash_test.go @@ -212,12 +212,6 @@ var _ = Describe("GPURecoveryPlan Controller: reflash Jobs", func() { }, Entry("no firmware at all", "plan-reflash-nofw", "evt-reflash-nofw", nil, "spec.firmware"), - Entry("a source but no file", "plan-reflash-nofile", "evt-reflash-nofile", - &intelv1a1.FirmwareSpec{ - Source: intelv1a1.FirmwareSource{ - ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: fwImage}, - }, - }, "spec.firmware"), // volumeSource is accepted by the CRD but not acted on: the reflash Job copies firmware // out of a container image. Parking says so rather than building a Job whose // initContainer would copy from an image that holds no firmware. @@ -229,5 +223,48 @@ var _ = Describe("GPURecoveryPlan Controller: reflash Jobs", func() { File: reflashFile, }, "containerSource"), ) + + // Kept out of the table above because the state is no longer expressible through the + // API server: spec.firmware.file carries a Pattern that an empty string fails, so the + // plan is created with a real filename and the in-memory copy is emptied afterwards. + // The controller guard is still worth exercising - it is what stands between an + // unset filename and a privileged reflash Job built around "/update/". + It("should park in missing-firmware when the firmware filename is empty", func() { + r := newTestReconciler() + p := reflashPlan("plan-reflash-nofile", "evt-reflash-nofile", containerFirmware()) + p.Spec.Firmware.File = "" + + Expect(r.createRecoveryJob(ctx, p, &p.Status.Events[0])).To(Succeed()) + + evt := p.Status.Events[0] + Expect(evt.State).To(Equal(intelv1a1.RecoveryEventStateMissingFirmware)) + Expect(evt.JobName).To(BeEmpty()) + Expect(evt.StateMessage).To(ContainSubstring("spec.firmware")) + Expect(p.Status.Messages).To(ContainElement(ContainSubstring("spec.firmware"))) + + expectNoJob(recoveryJobName("evt-reflash-nofile", 0)) + }) + + // The schema layer for the same condition: with no webhook in the path, an empty + // filename is rejected by the API server itself. + It("should reject an empty firmware filename at the API server", func() { + p := &intelv1a1.GPURecoveryPlan{ + ObjectMeta: metav1.ObjectMeta{Name: "plan-reflash-emptyfile"}, + Spec: intelv1a1.GPURecoveryPlanSpec{ + DefaultResetType: intelv1a1.RecoveryTypeSlot, + DeviceID: "0xabcd", + XpuSmi: intelv1a1.XpuSmiSpec{Image: "registry/xpu-smi:latest"}, + Firmware: &intelv1a1.FirmwareSpec{ + Source: intelv1a1.FirmwareSource{ + ContainerSource: &intelv1a1.ContainerFirmwareSource{Name: fwImage}, + }, + }, + }, + } + + err := k8sClient.Create(ctx, p) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("spec.firmware.file")) + }) }) }) diff --git a/test/e2e/e2e_test.go b/test/e2e/e2e_test.go index 6d835e7..5e878e7 100644 --- a/test/e2e/e2e_test.go +++ b/test/e2e/e2e_test.go @@ -485,6 +485,76 @@ var _ = Describe("Kubectl apply", Ordered, func() { }) }) +func runTestSSLPodAndVerifyTLS(namespace, endpoint string) { + podName, err := createTestSSLPod(namespace, endpoint) + Expect(err).NotTo(HaveOccurred(), "Failed to deploy testssl.sh pod") + + defer func() { + err := deletePod(namespace, podName) + Expect(err).NotTo(HaveOccurred(), "Failed to delete testssl.sh pod") + }() + + Eventually(waitForTestSSLPodToBecomeComplete).WithTimeout(5 * time.Minute).Should(Succeed()) + + By("checking the TLS ciphers used by the operator's webhook") + cmd := exec.Command("kubectl", "logs", podName, "-n", namespace) + output, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed to get logs from testssl.sh pod") + + By(output) + + // TLS + Expect(output).To(ContainSubstring("TLS 1.1 not offered")) + Expect(output).To(ContainSubstring("TLS 1.3 not offered")) + Expect(output).To(Not(ContainSubstring("TLS 1.2 not offered"))) + Expect(output).To(ContainSubstring("TLS 1.2 offered (OK)")) + + // No http/2 + Expect(output).To(ContainSubstring("ALPN/HTTP2 http/1.1 (offered)")) +} + +func runTestNMAPPodAndVerifyPorts(allowedPorts map[string]bool) { + By("deploy nmap pod") + nmapPod, err := createTestNMAPPod(namespace, getControllerPodIP(namespace)) + Expect(err).NotTo(HaveOccurred(), "Failed to deploy testnmap pod") + + defer func() { + err := deletePod(namespace, nmapPod) + Expect(err).NotTo(HaveOccurred(), "Failed to delete testnmap pod") + }() + + Eventually(waitForTestNMAPPodToBecomeComplete).WithTimeout(2 * time.Minute).Should(Succeed()) + + By("checking NMAP scan output") + cmd := exec.Command("kubectl", "logs", nmapPod, "-n", namespace) + output, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed to get logs from testnmap pod") + + By(output) + + foundPorts := map[string]bool{} + + // Extract open ports from nmap output using regex + // Host: 10.244.0.64 () Ports: + // 8081/open/tcp//blackice-icecap///, 9443/open/tcp//tungsten-https/// Ignored State: closed (65533) + // ^^^^ ^^^^ + portRe := regexp.MustCompile(`(\d+)/open`) + + matches := portRe.FindAllStringSubmatch(string(output), -1) + Expect(matches).NotTo(BeEmpty(), "No open ports found in nmap output") + + for _, match := range matches { + port := match[1] + Expect(port).To(BeKeyOf(allowedPorts), "unexpected open port: %s", port) + + foundPorts[port] = true + } + + for port := range allowedPorts { + Expect(foundPorts).To(HaveKey(port), "expected open port not found in nmap output: %s", port) + } +} + var _ = Describe("Helm", Ordered, Label("helm"), func() { Context("install", func() { operatorInstallArgsBase := []string{"install", "--create-namespace", "-n", namespace, helmOperatorName, helmOperatorChartPath, "--wait"} @@ -621,82 +691,61 @@ var _ = Describe("Helm", Ordered, Label("helm"), func() { serviceName := "intel-gpu-base-operator-webhook-service" serviceIP := getServiceClusterIP(serviceName, namespace) - podName, err := createTestSSLPod(namespace, serviceIP) - Expect(err).NotTo(HaveOccurred(), "Failed to deploy testssl.sh pod") + runTestSSLPodAndVerifyTLS(namespace, serviceIP) + }) - defer func() { - err := deletePod(namespace, podName) - Expect(err).NotTo(HaveOccurred(), "Failed to delete testssl.sh pod") - }() + It("check operator's metrics TLS cipher selection", Label("helm", "tls", "long"), func() { + operatorHelmArgs := []string{} + operatorHelmArgs = append(operatorHelmArgs, operatorInstallArgsBase...) + operatorHelmArgs = append(operatorHelmArgs, "--set", "metrics.enabled=true") - Eventually(waitForTestSSLPodToBecomeComplete).WithTimeout(5 * time.Minute).Should(Succeed()) + By("install operator helm chart with metrics enabled") + cmd := exec.Command("helm", operatorHelmArgs...) + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed to deploy operator helm chart") - By("checking the TLS ciphers used by the operator's webhook") - cmd = exec.Command("kubectl", "logs", podName, "-n", namespace) - output, err := utils.Run(cmd) - Expect(err).NotTo(HaveOccurred(), "Failed to get logs from testssl.sh pod") + By("deploy testssl.sh pod") - By(output) + serviceName := helmOperatorName + "-controller-manager-metrics-service" + serviceIP := getServiceClusterIP(serviceName, namespace) - // TLS - Expect(output).To(ContainSubstring("TLS 1.1 not offered")) - Expect(output).To(ContainSubstring("TLS 1.3 not offered")) - Expect(output).To(Not(ContainSubstring("TLS 1.2 not offered"))) - Expect(output).To(ContainSubstring("TLS 1.2 offered (OK)")) + // Metrics service is exposed on port 8443 + serviceIP = fmt.Sprintf("%s:8443", serviceIP) - // No http/2 - Expect(output).To(ContainSubstring("ALPN/HTTP2 http/1.1 (offered)")) + runTestSSLPodAndVerifyTLS(namespace, serviceIP) }) It("check operator's open ports", Label("helm", "ports", "long"), func() { - By("install operator helm chart") + By("install operator helm chart with metrics enabled") cmd := exec.Command("helm", operatorInstallArgsBase...) _, err := utils.Run(cmd) Expect(err).NotTo(HaveOccurred(), "Failed to deploy operator helm chart") - By("deploy nmap pod") - nmapPod, err := createTestNMAPPod(namespace, getControllerPodIP(namespace)) - Expect(err).NotTo(HaveOccurred(), "Failed to deploy testnmap pod") - - defer func() { - err := deletePod(namespace, nmapPod) - Expect(err).NotTo(HaveOccurred(), "Failed to delete testnmap pod") - }() - - Eventually(waitForTestNMAPPodToBecomeComplete).WithTimeout(2 * time.Minute).Should(Succeed()) - - By("checking NMAP scan output") - cmd = exec.Command("kubectl", "logs", nmapPod, "-n", namespace) - output, err := utils.Run(cmd) - Expect(err).NotTo(HaveOccurred(), "Failed to get logs from testnmap pod") - - By(output) - allowedPorts := map[string]bool{ "9443": true, // webhook "8081": true, // healthz } - foundPorts := map[string]bool{} - // Extract open ports from nmap output using regex - // Host: 10.244.0.64 () Ports: - // 8081/open/tcp//blackice-icecap///, 9443/open/tcp//tungsten-https/// Ignored State: closed (65533) - // ^^^^ ^^^^ - portRe := regexp.MustCompile(`(\d+)/open`) + runTestNMAPPodAndVerifyPorts(allowedPorts) + }) - matches := portRe.FindAllStringSubmatch(string(output), -1) - Expect(matches).NotTo(BeEmpty(), "No open ports found in nmap output") + It("check operator's open ports with metrics enabled", Label("helm", "ports", "long"), func() { + operatorHelmArgs := []string{} + operatorHelmArgs = append(operatorHelmArgs, operatorInstallArgsBase...) + operatorHelmArgs = append(operatorHelmArgs, "--set", "metrics.enabled=true") - for _, match := range matches { - port := match[1] - Expect(port).To(BeKeyOf(allowedPorts), "unexpected open port: %s", port) + By("install operator helm chart with metrics enabled") + cmd := exec.Command("helm", operatorHelmArgs...) + _, err := utils.Run(cmd) + Expect(err).NotTo(HaveOccurred(), "Failed to deploy operator helm chart") - foundPorts[port] = true + allowedPorts := map[string]bool{ + "9443": true, // webhook + "8081": true, // healthz + "8443": true, // metrics } - for port := range allowedPorts { - Expect(foundPorts).To(HaveKey(port), "expected open port not found in nmap output: %s", port) - } + runTestNMAPPodAndVerifyPorts(allowedPorts) }) It("device plugin with xpumd", Label("deviceplugin", "xpum"), func() {