Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ webhooks:
service:
name: {{ include "dynamo-operator.fullname" . }}-webhook-service
namespace: {{ .Release.Namespace }}
path: /validate/nvidia.com/v1beta1/dynamographdeployments
# TODO(1.4): Switch this path to /validate/nvidia.com/v1beta1/dynamographdeployments
# and apiVersions below to v1beta1. The 1.3 operator serves both endpoints.
path: /validate-nvidia-com-v1alpha1-dynamographdeployment
failurePolicy: {{ .Values.webhook.failurePolicy }}
name: vdynamographdeployment.kb.io
{{- if .Values.webhook.namespaceSelector }}
Expand All @@ -93,7 +95,7 @@ webhooks:
- apiGroups:
- nvidia.com
apiVersions:
- v1beta1
- v1alpha1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Helm webhook apiVersion downgrade leaves v1beta1 requests unvalidated

The Helm chart changes the webhook rules from apiVersions: v1beta1 to apiVersions: v1alpha1 for both the validating and mutating DynamoGraphDeployment webhooks (webhook-configuration.yaml:98 and webhook-configuration.yaml:256). While the operator binary serves both v1alpha1 and v1beta1 endpoints (dynamographdeployment_handler.go:140-149 for defaulting, dynamographdeployment_handler.go:197-215 for validation), the Kubernetes API server only dispatches admission requests matching the registered rules. Any direct v1beta1 DynamoGraphDeployment CREATE/UPDATE requests will bypass both validation and defaulting entirely during the 1.3 release window. The TODO comments indicate this is intentional for the migration, but the reviewer should confirm that v1beta1 is not yet served by the CRD or that bypassing validation is acceptable during this period.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

operations:
- CREATE
- UPDATE
Expand Down Expand Up @@ -234,7 +236,9 @@ webhooks:
service:
name: {{ include "dynamo-operator.fullname" . }}-webhook-service
namespace: {{ .Release.Namespace }}
path: /mutate/nvidia.com/v1beta1/dynamographdeployments
# TODO(1.4): Switch this path to /mutate/nvidia.com/v1beta1/dynamographdeployments
# and apiVersions below to v1beta1. The 1.3 operator serves both endpoints.
path: /mutate-nvidia-com-v1alpha1-dynamographdeployment
failurePolicy: {{ .Values.webhook.failurePolicy }}
name: mdynamographdeployment.kb.io
{{- if .Values.webhook.namespaceSelector }}
Expand All @@ -249,7 +253,7 @@ webhooks:
- apiGroups:
- nvidia.com
apiVersions:
- v1beta1
- v1alpha1
operations:
- CREATE
- UPDATE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"strings"

nvidiacomv1alpha1 "github.com/ai-dynamo/dynamo/deploy/operator/api/v1alpha1"
nvidiacomv1beta1 "github.com/ai-dynamo/dynamo/deploy/operator/api/v1beta1"
"github.com/ai-dynamo/dynamo/deploy/operator/internal/consts"
internalwebhook "github.com/ai-dynamo/dynamo/deploy/operator/internal/webhook"
Expand All @@ -34,8 +35,9 @@ import (
)

const (
dgdDefaultingWebhookName = "dynamographdeployment-defaulting-webhook"
dgdDefaultingWebhookPath = "/mutate/nvidia.com/v1beta1/dynamographdeployments"
dgdDefaultingWebhookName = "dynamographdeployment-defaulting-webhook"
dgdV1Alpha1DefaultingWebhookPath = "/mutate-nvidia-com-v1alpha1-dynamographdeployment"
dgdV1Beta1DefaultingWebhookPath = "/mutate/nvidia.com/v1beta1/dynamographdeployments"
)

// DGDDefaulter is a mutating webhook handler that stamps DynamoGraphDeployments
Expand All @@ -46,6 +48,13 @@ type DGDDefaulter struct {
GroveEnabled bool
}

// dgdV1Alpha1Defaulter keeps the previous endpoint available during the
// v1alpha1-to-v1beta1 admission migration. It applies v1beta1 defaulting and
// converts the result back to the object version used by the legacy endpoint.
type dgdV1Alpha1Defaulter struct {
defaulter *DGDDefaulter
}

// NewDGDDefaulter creates a new DGDDefaulter with the given operator version.
func NewDGDDefaulter(operatorVersion string, groveEnabled bool) *DGDDefaulter {
return &DGDDefaulter{
Expand All @@ -62,8 +71,6 @@ func NewDGDDefaulter(operatorVersion string, groveEnabled bool) *DGDDefaulter {
// On CREATE: stamps nvidia.com/dynamo-operator-origin-version with the operator version.
// On UPDATE/DELETE: the origin version annotation is immutable once set.
func (d *DGDDefaulter) Default(ctx context.Context, obj runtime.Object) error {
logger := log.FromContext(ctx).WithName(dgdDefaultingWebhookName)

if err := internalwebhook.ValidateAdmissionGVK(ctx, nvidiacomv1beta1.DynamoGraphDeploymentGVK); err != nil {
return err
}
Expand All @@ -72,6 +79,14 @@ func (d *DGDDefaulter) Default(ctx context.Context, obj runtime.Object) error {
if !ok {
return fmt.Errorf("expected DynamoGraphDeployment but got %T", obj)
}
return d.defaultV1Beta1(ctx, dgd)
}

func (d *DGDDefaulter) defaultV1Beta1(
ctx context.Context,
dgd *nvidiacomv1beta1.DynamoGraphDeployment,
) error {
logger := log.FromContext(ctx).WithName(dgdDefaultingWebhookName)

req, err := admission.RequestFromContext(ctx)
if err != nil {
Expand Down Expand Up @@ -119,9 +134,45 @@ func (d *DGDDefaulter) isGrovePathway(dgd *nvidiacomv1beta1.DynamoGraphDeploymen

// RegisterWithManager registers the defaulting webhook with the manager.
func (d *DGDDefaulter) RegisterWithManager(mgr manager.Manager) error {
webhook := admission.
betaWebhook := admission.
WithCustomDefaulter(mgr.GetScheme(), &nvidiacomv1beta1.DynamoGraphDeployment{}, d).
WithRecoverPanic(true)
mgr.GetWebhookServer().Register(dgdDefaultingWebhookPath, webhook)
mgr.GetWebhookServer().Register(dgdV1Beta1DefaultingWebhookPath, betaWebhook)

// Keep the v1alpha1 endpoint in the binary before the Helm registration
// moves to v1beta1. This lets an upgrade switch the registration only after
// all running operators already serve both endpoints.
alphaDefaulter := &dgdV1Alpha1Defaulter{defaulter: d}
alphaWebhook := admission.
WithCustomDefaulter(mgr.GetScheme(), &nvidiacomv1alpha1.DynamoGraphDeployment{}, alphaDefaulter).
WithRecoverPanic(true)
mgr.GetWebhookServer().Register(dgdV1Alpha1DefaultingWebhookPath, alphaWebhook)
return nil
}

func (d *dgdV1Alpha1Defaulter) Default(ctx context.Context, obj runtime.Object) error {
if err := internalwebhook.ValidateAdmissionGVK(ctx, nvidiacomv1alpha1.DynamoGraphDeploymentGVK); err != nil {
return err
}

alpha, ok := obj.(*nvidiacomv1alpha1.DynamoGraphDeployment)
if !ok {
return fmt.Errorf("expected DynamoGraphDeployment but got %T", obj)
}

beta, err := internalwebhook.ConvertDynamoGraphDeploymentToV1Beta1(alpha)
if err != nil {
return err
}
if err := d.defaulter.defaultV1Beta1(ctx, beta); err != nil {
return err
}

converted, err := internalwebhook.ConvertDynamoGraphDeploymentToV1Alpha1(beta)
if err != nil {
return err
}
converted.TypeMeta = alpha.TypeMeta
*alpha = *converted
return nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"context"
"testing"

nvidiacomv1alpha1 "github.com/ai-dynamo/dynamo/deploy/operator/api/v1alpha1"
nvidiacomv1beta1 "github.com/ai-dynamo/dynamo/deploy/operator/api/v1beta1"
"github.com/ai-dynamo/dynamo/deploy/operator/internal/consts"
admissionv1 "k8s.io/api/admission/v1"
Expand Down Expand Up @@ -434,3 +435,27 @@ func TestDGDDefaulter_DefaultsGroveMinAvailable(t *testing.T) {
})
}
}

func TestDGDV1Alpha1Defaulter_Default(t *testing.T) {
dgd := &nvidiacomv1alpha1.DynamoGraphDeployment{
ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"},
Spec: nvidiacomv1alpha1.DynamoGraphDeploymentSpec{
Services: map[string]*nvidiacomv1alpha1.DynamoComponentDeploymentSharedSpec{
"worker": {
ComponentType: consts.ComponentTypeWorker,
},
},
},
}
defaulter := &dgdV1Alpha1Defaulter{defaulter: NewDGDDefaulter("0.9.0", false)}

if err := defaulter.Default(admissionCtx(admissionv1.Create, nvidiacomv1alpha1.DynamoGraphDeploymentGVK), dgd); err != nil {
t.Fatalf("Default() unexpected error: %v", err)
}
if got := dgd.Annotations[consts.KubeAnnotationDynamoOperatorOriginVersion]; got != "0.9.0" {
t.Errorf("origin annotation = %q, want %q", got, "0.9.0")
}
if got := dgd.Spec.Services["worker"].Replicas; got == nil || *got != 1 {
t.Errorf("worker replicas = %v, want 1", got)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved.
* SPDX-License-Identifier: Apache-2.0
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package webhook

import (
"fmt"

nvidiacomv1alpha1 "github.com/ai-dynamo/dynamo/deploy/operator/api/v1alpha1"
nvidiacomv1beta1 "github.com/ai-dynamo/dynamo/deploy/operator/api/v1beta1"
)

// ConvertDynamoGraphDeploymentToV1Beta1 converts an admission object from the
// v1alpha1 spoke to the v1beta1 hub used by the DGD admission logic.
func ConvertDynamoGraphDeploymentToV1Beta1(src *nvidiacomv1alpha1.DynamoGraphDeployment) (*nvidiacomv1beta1.DynamoGraphDeployment, error) {
dst := &nvidiacomv1beta1.DynamoGraphDeployment{}
if err := src.ConvertTo(dst); err != nil {
return nil, fmt.Errorf("convert v1alpha1 DynamoGraphDeployment to v1beta1: %w", err)
}
return dst, nil
}

// ConvertDynamoGraphDeploymentToV1Alpha1 converts a v1beta1 hub object to the
// v1alpha1 spoke expected by the legacy mutation endpoint.
func ConvertDynamoGraphDeploymentToV1Alpha1(src *nvidiacomv1beta1.DynamoGraphDeployment) (*nvidiacomv1alpha1.DynamoGraphDeployment, error) {
dst := &nvidiacomv1alpha1.DynamoGraphDeployment{}
if err := dst.ConvertFrom(src); err != nil {
return nil, fmt.Errorf("convert v1beta1 DynamoGraphDeployment to v1alpha1: %w", err)
}
return dst, nil
}
Loading
Loading