Skip to content

Commit 49b560c

Browse files
varunrsekarvarunrsekar
committed
validate version value for attributeSelectors
Signed-off-by: Varun Ramachandra Sekar <vsekar@nvidia.com>
1 parent c2e82f0 commit 49b560c

File tree

6 files changed

+129
-6
lines changed

6 files changed

+129
-6
lines changed

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ toolchain go1.24.1
77
require (
88
github.com/Masterminds/sprig/v3 v3.3.0
99
github.com/NVIDIA/k8s-test-infra v0.0.0-20240806103558-2d7411125519
10+
github.com/blang/semver/v4 v4.0.0
1011
github.com/go-git/go-git/v5 v5.12.0
1112
github.com/go-logr/logr v1.4.2
1213
github.com/google/cel-go v0.23.2
@@ -60,7 +61,6 @@ require (
6061
github.com/asaskevich/govalidator v0.0.0-20230301143203-a9d515a09cc2 // indirect
6162
github.com/aws/aws-sdk-go v1.55.6 // indirect
6263
github.com/beorn7/perks v1.0.1 // indirect
63-
github.com/blang/semver/v4 v4.0.0 // indirect
6464
github.com/blendle/zapdriver v1.3.1 // indirect
6565
github.com/cenkalti/backoff/v4 v4.3.0 // indirect
6666
github.com/cespare/xxhash/v2 v2.3.0 // indirect

‎internal/controller/platform/kserve/nimservice.go‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"strings"
2424
"time"
2525

26+
"github.com/blang/semver/v4"
2627
"github.com/go-logr/logr"
2728
kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
2829
kserveconstants "github.com/kserve/kserve/pkg/constants"
@@ -830,6 +831,26 @@ func (r *NIMServiceReconciler) validateDRAResources(ctx context.Context, nimServ
830831
}
831832
resourceClaimNames[*resource.ResourceClaimName] = true
832833
}
834+
835+
// Check AttributeSelector version values.
836+
for idx, resource := range nimService.Spec.DRAResources {
837+
if resource.ClaimCreationSpec == nil {
838+
continue
839+
}
840+
for deviceIdx, device := range resource.ClaimCreationSpec.Devices {
841+
for attributeIdx, attribute := range device.AttributeSelectors {
842+
if attribute.Value.VersionValue == nil {
843+
continue
844+
}
845+
_, err := semver.Parse(*attribute.Value.VersionValue)
846+
if err != nil {
847+
msg := fmt.Sprintf("spec.draResources[%d].claimCreationSpec.devices[%d].attributeSelectors[%d].value.versionValue.version: invalid version %q: %v", idx, deviceIdx, attributeIdx, *attribute.Value.VersionValue, err)
848+
logger.Error(err, msg, "nimService", nimService.Name)
849+
return false, msg, nil
850+
}
851+
}
852+
}
853+
}
833854
return true, "", nil
834855
}
835856

‎internal/controller/platform/standalone/nimservice.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"strings"
2424
"time"
2525

26+
"github.com/blang/semver/v4"
2627
"github.com/go-logr/logr"
2728
monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
2829
appsv1 "k8s.io/api/apps/v1"
@@ -148,6 +149,25 @@ func (r *NIMServiceReconciler) validateDRAResources(ctx context.Context, nimServ
148149
resourceClaimNames[*resource.ResourceClaimName] = true
149150
}
150151

152+
// Check AttributeSelector version values.
153+
for idx, resource := range nimService.Spec.DRAResources {
154+
if resource.ClaimCreationSpec == nil {
155+
continue
156+
}
157+
for deviceIdx, device := range resource.ClaimCreationSpec.Devices {
158+
for attributeIdx, attribute := range device.AttributeSelectors {
159+
if attribute.Value.VersionValue == nil {
160+
continue
161+
}
162+
_, err := semver.Parse(*attribute.Value.VersionValue)
163+
if err != nil {
164+
msg := fmt.Sprintf("spec.draResources[%d].claimCreationSpec.devices[%d].attributeSelectors[%d].value.versionValue.version: invalid version %q: %v", idx, deviceIdx, attributeIdx, *attribute.Value.VersionValue, err)
165+
logger.Error(err, msg, "nimService", nimService.Name)
166+
return false, msg, nil
167+
}
168+
}
169+
}
170+
}
151171
return true, "", nil
152172
}
153173

‎internal/controller/platform/standalone/nimservice_test.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
145145
Expect(monitoringv1.AddToScheme(scheme)).To(Succeed())
146146
Expect(lwsv1.AddToScheme(scheme)).To(Succeed())
147147
Expect(gatewayv1.Install(scheme)).To(Succeed())
148+
Expect(resourcev1beta2.AddToScheme(scheme)).To(Succeed())
148149

149150
client = fake.NewClientBuilder().WithScheme(scheme).
150151
WithStatusSubresource(&appsv1alpha1.NIMService{}).
@@ -756,6 +757,87 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
756757
Expect(failedCondition.Reason).To(Equal(conditions.ReasonDRAResourcesUnsupported))
757758
Expect(failedCondition.Message).To(Equal("spec.draResources[1].resourceClaimName: duplicate resource claim name: 'test-resource-claim'"))
758759
})
760+
761+
It("should mark NIMService as failed when ClaimCreationSpec.Devices[].AttributeSelectors version value is invalid", func() {
762+
nimService.Spec.DRAResources = []appsv1alpha1.DRAResource{
763+
{
764+
ClaimCreationSpec: &appsv1alpha1.DRAClaimCreationSpec{
765+
Devices: []appsv1alpha1.DRADeviceSpec{
766+
{
767+
Name: "test-device",
768+
Count: 1,
769+
DriverName: "gpu.nvidia.com",
770+
DeviceClassName: "gpu.nvidia.com",
771+
AttributeSelectors: []appsv1alpha1.DRADeviceAttributeSelector{
772+
{
773+
Key: "testKey",
774+
Op: "GreaterThan",
775+
Value: &appsv1alpha1.DRADeviceAttributeSelectorValue{
776+
VersionValue: ptr.To("550.127.08"),
777+
},
778+
},
779+
},
780+
},
781+
},
782+
},
783+
},
784+
}
785+
nimServiceKey := types.NamespacedName{Name: nimService.Name, Namespace: nimService.Namespace}
786+
err := client.Create(context.TODO(), nimService)
787+
Expect(err).NotTo(HaveOccurred())
788+
789+
_, err = reconciler.reconcileNIMService(context.TODO(), nimService)
790+
Expect(err).NotTo(HaveOccurred())
791+
792+
obj := &appsv1alpha1.NIMService{}
793+
err = client.Get(context.TODO(), nimServiceKey, obj)
794+
Expect(err).NotTo(HaveOccurred())
795+
Expect(obj.Status.State).To(Equal(appsv1alpha1.NIMServiceStatusFailed))
796+
failedCondition := getCondition(obj, conditions.Failed)
797+
Expect(failedCondition).NotTo(BeNil())
798+
Expect(failedCondition.Status).To(Equal(metav1.ConditionTrue))
799+
Expect(failedCondition.Reason).To(Equal(conditions.ReasonDRAResourcesUnsupported))
800+
Expect(failedCondition.Message).To(ContainSubstring("spec.draResources[0].claimCreationSpec.devices[0].attributeSelectors[0].value.versionValue.version: invalid version \"550.127.08\":"))
801+
})
802+
803+
It("should succeed with valid ClaimCreationSpec", func() {
804+
nimService.Spec.DRAResources = []appsv1alpha1.DRAResource{
805+
{
806+
ClaimCreationSpec: &appsv1alpha1.DRAClaimCreationSpec{
807+
Devices: []appsv1alpha1.DRADeviceSpec{
808+
{
809+
Name: "test-device",
810+
Count: 1,
811+
DriverName: "gpu.nvidia.com",
812+
DeviceClassName: "gpu.nvidia.com",
813+
AttributeSelectors: []appsv1alpha1.DRADeviceAttributeSelector{
814+
{
815+
Key: "testKey",
816+
Op: "GreaterThan",
817+
Value: &appsv1alpha1.DRADeviceAttributeSelectorValue{
818+
VersionValue: ptr.To("550.127.8"),
819+
},
820+
},
821+
},
822+
},
823+
},
824+
},
825+
},
826+
}
827+
nimServiceKey := types.NamespacedName{Name: nimService.Name, Namespace: nimService.Namespace}
828+
err := client.Create(context.TODO(), nimService)
829+
Expect(err).NotTo(HaveOccurred())
830+
831+
_, err = reconciler.reconcileNIMService(context.TODO(), nimService)
832+
Expect(err).NotTo(HaveOccurred())
833+
834+
obj := &appsv1alpha1.NIMService{}
835+
err = client.Get(context.TODO(), nimServiceKey, obj)
836+
Expect(err).NotTo(HaveOccurred())
837+
Expect(obj.Status.State).NotTo(Equal(appsv1alpha1.NIMServiceStatusFailed))
838+
failedCondition := getCondition(obj, conditions.Failed)
839+
Expect(failedCondition.Status).To(Equal(metav1.ConditionFalse))
840+
})
759841
})
760842

761843
It("should delete Deployment when the NIMService is deleted", func() {

‎internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import (
2828
apiresource "k8s.io/apimachinery/pkg/api/resource"
2929
"k8s.io/apimachinery/pkg/util/validation/field"
3030

31-
utilversion "k8s.io/apimachinery/pkg/util/version"
31+
"github.com/blang/semver/v4"
3232

3333
appsv1alpha1 "github.com/NVIDIA/k8s-nim-operator/api/apps/v1alpha1"
3434
"github.com/NVIDIA/k8s-nim-operator/internal/k8sutil"
@@ -311,9 +311,9 @@ func validateDRADeviceAttributeSelectorValue(attribute *appsv1alpha1.DRADeviceAt
311311
}
312312

313313
if attribute.VersionValue != nil {
314-
_, err := utilversion.ParseSemantic(*attribute.VersionValue)
314+
_, err := semver.Parse(*attribute.VersionValue)
315315
if err != nil {
316-
errList = append(errList, field.Invalid(fldPath, attribute.VersionValue, "must be a valid semantic version"))
316+
errList = append(errList, field.Invalid(fldPath, attribute.VersionValue, fmt.Sprintf("must be a valid semantic version: %v", err)))
317317
}
318318
}
319319
return errList

‎internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -594,7 +594,7 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) {
594594
Key: "driver-version",
595595
Op: appsv1alpha1.DRADeviceAttributeSelectorOpEqual,
596596
Value: &appsv1alpha1.DRADeviceAttributeSelectorValue{
597-
VersionValue: ptr.To("not-a-version"),
597+
VersionValue: ptr.To("550.127.08"),
598598
},
599599
},
600600
},
@@ -604,7 +604,7 @@ func TestValidateDRAResourcesConfiguration(t *testing.T) {
604604
},
605605
k8sVersion: "v1.34.0",
606606
wantErrs: 1,
607-
wantErrMsgs: []string{"spec.draResources[0].claimCreationSpec.devices[0].attributeSelectors[0].value: Invalid value: \"not-a-version\": must be a valid semantic version"},
607+
wantErrMsgs: []string{"spec.draResources[0].claimCreationSpec.devices[0].attributeSelectors[0].value: Invalid value: \"550.127.08\": must be a valid semantic version: Patch number must not contain leading zeroes \"08\""},
608608
},
609609
{
610610
name: "claimCreationSpec with invalid quantity selector - invalid op",

0 commit comments

Comments
 (0)