Skip to content

Commit

Permalink
bugfix: ensure that we don't panic on DP validations
Browse files Browse the repository at this point in the history 
Signed-off-by: Vasilis Remmas <vremmas@nvidia.com>
  • Loading branch information
@vasrem
vasrem committed Sep 18, 2024
1 parent a9d5a32 commit aa73e86
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 48 deletions.
82 changes: 56 additions & 26 deletions api/v1alpha1/validator/nicclusterpolicy_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,11 @@ func (w *nicClusterPolicyValidator) validateNicClusterPolicy(in *v1alpha1.NicClu
func (dp *devicePluginSpecWrapper) validateSriovNetworkDevicePlugin(fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList
var sriovNetworkDevicePluginConfigJSON map[string]interface{}

if dp.Config == nil {
return allErrs
}

sriovNetworkDevicePluginConfig := *dp.Config

// Validate if the SRIOV Network Device Plugin Config is a valid json
Expand Down Expand Up @@ -270,38 +275,58 @@ func (dp *devicePluginSpecWrapper) validateSriovNetworkDevicePlugin(fldPath *fie
resourceList, _ := resourceListInterface.([]interface{})
for _, resourceInterface := range resourceList {
resource := resourceInterface.(map[string]interface{})
resourceJSONString, _ := json.Marshal(resource)
resourceJSONLoader := gojsonschema.NewStringLoader(string(resourceJSONString))
var selectorResult *gojsonschema.Result
var selectorErr error
var ok bool
ok, allErrs = validateResourceNamePrefix(resource, allErrs, fldPath, dp)
if !ok {
return allErrs
}
deviceType := resource["deviceType"]
switch deviceType {
case "accelerator":
selectorResult, selectorErr = acceleratorJSONSchema.Validate(resourceJSONLoader)
case "auxNetDevice":
selectorResult, selectorErr = auxNetDeviceJSONSchema.Validate(resourceJSONLoader)
default:
selectorResult, selectorErr = netDeviceJSONSchema.Validate(resourceJSONLoader)
}
if selectorErr != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("Config"), dp.Config,
selectorErr.Error()))
} else if !selectorResult.Valid() {
for _, selectorResultErr := range selectorResult.Errors() {
allErrs = append(allErrs, field.Invalid(fldPath.Child("Config"), dp.Config,
selectorResultErr.Description()))
}
if errs := dp.validateSriovNetworkDevicePluginResource(resource,
acceleratorJSONSchema,
netDeviceJSONSchema,
auxNetDeviceJSONSchema,
fldPath,
); errs != nil {
allErrs = append(allErrs, errs...)
}
}
}
return allErrs
}

// validateSriovNetworkDevicePluginResource validates a SRIOV Network Device Plugin resource
func (dp *devicePluginSpecWrapper) validateSriovNetworkDevicePluginResource(resource map[string]interface{},
acceleratorJSONSchema *gojsonschema.Schema,
netDeviceJSONSchema *gojsonschema.Schema,
auxNetDeviceJSONSchema *gojsonschema.Schema,
fldPath *field.Path,
) field.ErrorList {
var allErrs field.ErrorList
resourceJSONString, _ := json.Marshal(resource)
resourceJSONLoader := gojsonschema.NewStringLoader(string(resourceJSONString))
var selectorResult *gojsonschema.Result
var selectorErr error
var ok bool
ok, allErrs = validateResourceNamePrefix(resource, allErrs, fldPath, dp)
if !ok {
return allErrs
}
deviceType := resource["deviceType"]
switch deviceType {
case "accelerator":
selectorResult, selectorErr = acceleratorJSONSchema.Validate(resourceJSONLoader)
case "auxNetDevice":
selectorResult, selectorErr = auxNetDeviceJSONSchema.Validate(resourceJSONLoader)
default:
selectorResult, selectorErr = netDeviceJSONSchema.Validate(resourceJSONLoader)
}
if selectorErr != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("Config"), dp.Config,
selectorErr.Error()))
} else if !selectorResult.Valid() {
for _, selectorResultErr := range selectorResult.Errors() {
allErrs = append(allErrs, field.Invalid(fldPath.Child("Config"), dp.Config,
selectorResultErr.Description()))
}
}

return allErrs
}

func validateResourceNamePrefix(resource map[string]interface{},
allErrs field.ErrorList, fldPath *field.Path, dp *devicePluginSpecWrapper) (bool, field.ErrorList) {
resourceName := resource["resourceName"].(string)
Expand All @@ -327,6 +352,11 @@ func validateResourceNamePrefix(resource map[string]interface{},
func (dp *devicePluginSpecWrapper) validateRdmaSharedDevicePlugin(fldPath *field.Path) field.ErrorList {
var allErrs field.ErrorList
var rdmaSharedDevicePluginConfigJSON map[string]interface{}

if dp.Config == nil {
return allErrs
}

rdmaSharedDevicePluginConfig := *dp.Config

// Validate if the RDMA Shared Device Plugin Config is a valid json
Expand Down
56 changes: 34 additions & 22 deletions api/v1alpha1/validator/nicclusterpolicy_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,12 @@ var _ = Describe("Validate", func() {
_, err := validator.ValidateCreate(context.TODO(), nicClusterPolicy)
Expect(err).To(BeNil())
})
It("RDMA no config", func() {
nicClusterPolicy := rdmaDPNicClusterPolicy(nil)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err).NotTo(HaveOccurred())
})
It("Valid RDMA config JSON", func() {
rdmaConfig := `{
"configList": [{
Expand All @@ -223,7 +229,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(rdmaConfig)
nicClusterPolicy := rdmaDPNicClusterPolicy(&rdmaConfig)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -236,7 +242,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(rdmaConfig)
nicClusterPolicy := rdmaDPNicClusterPolicy(&rdmaConfig)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -249,7 +255,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand All @@ -263,7 +269,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring("Invalid Resource name"))
Expand All @@ -276,7 +282,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring("configList is required"))
Expand All @@ -287,7 +293,7 @@ var _ = Describe("Validate", func() {
"resourceName": "rdma_shared_device_a",
"rdmaHcaMax": 63,
"selectors": {}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring("vendors is required"))
Expand All @@ -300,7 +306,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": [15],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand All @@ -314,7 +320,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": [1010]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand All @@ -329,20 +335,26 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"deviceIDs": ["101b"]}}]}`
nicClusterPolicy := rdmaDPNicClusterPolicy(invalidRdmaConfigJSON)
nicClusterPolicy := rdmaDPNicClusterPolicy(&invalidRdmaConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
"Invalid Resource prefix, it must be a valid FQDN"))
})
It("SriovDevicePlugin no config", func() {
nicClusterPolicy := sriovDPNicClusterPolicy(nil)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err).NotTo(HaveOccurred())
})
It("Valid SriovDevicePlugin config JSON", func() {
sriovConfig := `{
"resourceList": [{
"resourceName": "hostdev",
"selectors": {
"vendors": ["15b3"],
"devices": ["101b"]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(sriovConfig)
nicClusterPolicy := sriovDPNicClusterPolicy(&sriovConfig)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -354,7 +366,7 @@ var _ = Describe("Validate", func() {
"selectors": [{
"vendors": ["15b3"],
"devices": ["101b"]}]}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(sriovConfig)
nicClusterPolicy := sriovDPNicClusterPolicy(&sriovConfig)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err).NotTo(HaveOccurred())
Expand All @@ -366,7 +378,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"devices": ["101b"]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand All @@ -379,7 +391,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"devices": ["101b"]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring("Invalid Resource name"))
Expand All @@ -391,7 +403,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"devices": ["101b"]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring("resourceList is required"))
Expand All @@ -401,7 +413,7 @@ var _ = Describe("Validate", func() {
"resourceList": [{
"resourceName": "sriov_network_device_plugin",
"selectors": {}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring("vendors is required"))
Expand All @@ -413,7 +425,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": [15],
"devices": ["101b"]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand All @@ -426,7 +438,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"devices": [1020]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand All @@ -440,7 +452,7 @@ var _ = Describe("Validate", func() {
"selectors": {
"vendors": ["15b3"],
"devices": ["101b"]}}]}`
nicClusterPolicy := sriovDPNicClusterPolicy(invalidSriovConfigJSON)
nicClusterPolicy := sriovDPNicClusterPolicy(&invalidSriovConfigJSON)
validator := nicClusterPolicyValidator{}
_, err := validator.ValidateCreate(context.TODO(), &nicClusterPolicy)
Expect(err.Error()).To(ContainSubstring(
Expand Down Expand Up @@ -906,13 +918,13 @@ var _ = Describe("Validate", func() {
})
})

func rdmaDPNicClusterPolicy(config string) v1alpha1.NicClusterPolicy {
func rdmaDPNicClusterPolicy(config *string) v1alpha1.NicClusterPolicy {
return v1alpha1.NicClusterPolicy{
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Spec: v1alpha1.NicClusterPolicySpec{
RdmaSharedDevicePlugin: &v1alpha1.DevicePluginSpec{
ImageSpecWithConfig: v1alpha1.ImageSpecWithConfig{
Config: &config,
Config: config,
ImageSpec: v1alpha1.ImageSpec{
Image: "k8s-rdma-shared-dev-plugin",
Repository: "ghcr.io/mellanox",
Expand All @@ -925,13 +937,13 @@ func rdmaDPNicClusterPolicy(config string) v1alpha1.NicClusterPolicy {
}
}

func sriovDPNicClusterPolicy(config string) v1alpha1.NicClusterPolicy {
func sriovDPNicClusterPolicy(config *string) v1alpha1.NicClusterPolicy {
return v1alpha1.NicClusterPolicy{
ObjectMeta: metav1.ObjectMeta{Name: "test"},
Spec: v1alpha1.NicClusterPolicySpec{
SriovDevicePlugin: &v1alpha1.DevicePluginSpec{
ImageSpecWithConfig: v1alpha1.ImageSpecWithConfig{
Config: &config,
Config: config,
ImageSpec: v1alpha1.ImageSpec{
Image: "sriov-network-device-plugin",
Repository: "nvcr.io/nvstaging/mellanox",
Expand Down

0 comments on commit aa73e86

Please sign in to comment.