Skip to content

Commit 159f30b

Browse files
committed
fix: add scaling adapter
Signed-off-by: Julien Mancuso <[email protected]>
1 parent 03604d6 commit 159f30b

File tree

3 files changed

+38
-30
lines changed

3 files changed

+38
-30
lines changed

deploy/cloud/operator/internal/webhook/validation/dynamographdeployment.go

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -68,37 +68,42 @@ func (v *DynamoGraphDeploymentValidator) Validate() (admission.Warnings, error)
6868
}
6969

7070
// ValidateUpdate performs stateful validation comparing old and new DynamoGraphDeployment.
71+
// userInfo is used for identity-based validation (replica protection).
72+
// If userInfo is nil, replica changes for DGDSA-enabled services are rejected (fail closed).
7173
// Returns warnings and error.
72-
func (v *DynamoGraphDeploymentValidator) ValidateUpdate(old *nvidiacomv1alpha1.DynamoGraphDeployment) (admission.Warnings, error) {
73-
return v.ValidateUpdateWithUserInfo(old, nil)
74-
}
74+
func (v *DynamoGraphDeploymentValidator) ValidateUpdate(old *nvidiacomv1alpha1.DynamoGraphDeployment, userInfo *authenticationv1.UserInfo) (admission.Warnings, error) {
75+
var warnings admission.Warnings
7576

76-
// ValidateUpdateWithUserInfo performs stateful validation with user identity checking.
77-
// When userInfo is provided, it validates that only allowed controllers can modify
78-
// replicas for services with scaling adapter enabled.
79-
// Returns warnings and error.
80-
func (v *DynamoGraphDeploymentValidator) ValidateUpdateWithUserInfo(old *nvidiacomv1alpha1.DynamoGraphDeployment, userInfo *authenticationv1.UserInfo) (admission.Warnings, error) {
81-
// Validate that BackendFramework is not changed (immutable)
82-
if v.deployment.Spec.BackendFramework != old.Spec.BackendFramework {
83-
warning := "Changing spec.backendFramework may cause unexpected behavior"
84-
return admission.Warnings{warning}, fmt.Errorf("spec.backendFramework is immutable and cannot be changed after creation")
77+
// Validate immutable fields
78+
if err := v.validateImmutableFields(old, &warnings); err != nil {
79+
return warnings, err
8580
}
8681

8782
// Validate replicas changes for services with scaling adapter enabled
88-
if userInfo != nil {
89-
if err := v.validateReplicasChanges(old, *userInfo); err != nil {
90-
return nil, err
91-
}
83+
// Pass userInfo (may be nil - will fail closed for DGDSA-enabled services)
84+
if err := v.validateReplicasChanges(old, userInfo); err != nil {
85+
return warnings, err
9286
}
9387

94-
return nil, nil
88+
return warnings, nil
89+
}
90+
91+
// validateImmutableFields checks that immutable fields have not been changed.
92+
// Appends warnings to the provided slice.
93+
func (v *DynamoGraphDeploymentValidator) validateImmutableFields(old *nvidiacomv1alpha1.DynamoGraphDeployment, warnings *admission.Warnings) error {
94+
if v.deployment.Spec.BackendFramework != old.Spec.BackendFramework {
95+
*warnings = append(*warnings, "Changing spec.backendFramework may cause unexpected behavior")
96+
return fmt.Errorf("spec.backendFramework is immutable and cannot be changed after creation")
97+
}
98+
return nil
9599
}
96100

97101
// validateReplicasChanges checks if replicas were changed for services with scaling adapter enabled.
98102
// Only authorized service accounts (operator controller, planner) can modify these fields.
99-
func (v *DynamoGraphDeploymentValidator) validateReplicasChanges(old *nvidiacomv1alpha1.DynamoGraphDeployment, userInfo authenticationv1.UserInfo) error {
103+
// If userInfo is nil, all replica changes for DGDSA-enabled services are rejected (fail closed).
104+
func (v *DynamoGraphDeploymentValidator) validateReplicasChanges(old *nvidiacomv1alpha1.DynamoGraphDeployment, userInfo *authenticationv1.UserInfo) error {
100105
// If the request comes from an authorized service account, allow the change
101-
if internalwebhook.CanModifyDGDReplicas(userInfo) {
106+
if userInfo != nil && internalwebhook.CanModifyDGDReplicas(*userInfo) {
102107
return nil
103108
}
104109

deploy/cloud/operator/internal/webhook/validation/dynamographdeployment_handler.go

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323

2424
nvidiacomv1alpha1 "github.com/ai-dynamo/dynamo/deploy/cloud/operator/api/v1alpha1"
2525
internalwebhook "github.com/ai-dynamo/dynamo/deploy/cloud/operator/internal/webhook"
26+
authenticationv1 "k8s.io/api/authentication/v1"
2627
"k8s.io/apimachinery/pkg/runtime"
2728
"sigs.k8s.io/controller-runtime/pkg/log"
2829
"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -92,22 +93,23 @@ func (h *DynamoGraphDeploymentHandler) ValidateUpdate(ctx context.Context, oldOb
9293
}
9394

9495
// Get user info from admission request context for identity-based validation
96+
var userInfo *authenticationv1.UserInfo
9597
req, err := admission.RequestFromContext(ctx)
9698
if err != nil {
97-
logger.Error(err, "failed to get admission request from context, skipping user-based validation")
98-
// Fall back to basic validation without user info
99-
updateWarnings, err := validator.ValidateUpdate(oldDeployment)
100-
if err != nil {
101-
return updateWarnings, err
102-
}
103-
warnings = append(warnings, updateWarnings...)
104-
return warnings, nil
99+
logger.Error(err, "failed to get admission request from context, replica changes for DGDSA-enabled services will be rejected")
100+
// userInfo remains nil - validateReplicasChanges will fail closed
101+
} else {
102+
userInfo = &req.UserInfo
105103
}
106104

107105
// Validate stateful rules (immutability + replicas protection)
108-
updateWarnings, err := validator.ValidateUpdateWithUserInfo(oldDeployment, &req.UserInfo)
106+
updateWarnings, err := validator.ValidateUpdate(oldDeployment, userInfo)
109107
if err != nil {
110-
logger.Info("validation failed", "error", err.Error(), "user", req.UserInfo.Username)
108+
username := "<unknown>"
109+
if userInfo != nil {
110+
username = userInfo.Username
111+
}
112+
logger.Info("validation failed", "error", err.Error(), "user", username)
111113
return updateWarnings, err
112114
}
113115

deploy/cloud/operator/internal/webhook/validation/dynamographdeployment_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -419,7 +419,8 @@ func TestDynamoGraphDeploymentValidator_ValidateUpdate(t *testing.T) {
419419
for _, tt := range tests {
420420
t.Run(tt.name, func(t *testing.T) {
421421
validator := NewDynamoGraphDeploymentValidator(tt.newDeployment)
422-
warnings, err := validator.ValidateUpdate(tt.oldDeployment)
422+
// Pass nil userInfo - these tests don't modify replicas, so it's safe
423+
warnings, err := validator.ValidateUpdate(tt.oldDeployment, nil)
423424

424425
if (err != nil) != tt.wantErr {
425426
t.Errorf("DynamoGraphDeploymentValidator.ValidateUpdate() error = %v, wantErr %v", err, tt.wantErr)

0 commit comments

Comments
 (0)