Reject grpcRouter creation when grpcPort is unspecified; Add warning messages when router is enabled but the exposed service is of type LoadBalancer

Signed-off-by: Sheng Lin <[email protected]>

Author: shengnuo

api/apps/v1alpha1/common_types.go

@@ -30,8 +30,6 @@ import (
 const (
 	// DefaultAPIPort is the default api port.
 	DefaultAPIPort = 8000
-	// DefaultGRPCPort is the default grpc port.
-	DefaultGRPCPort = 50051
 	// DefaultNamedPortAPI is the default name for api port.
 	DefaultNamedPortAPI = "api"
 	// DefaultNamedPortGRPC is the default name for grpc port.

api/apps/v1alpha1/nimservice_types.go

@@ -90,6 +90,7 @@ const (
 
 // NIMServiceSpec defines the desired state of NIMService.
 // +kubebuilder:validation:XValidation:rule="!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled) && self.scale.enabled)", message="autoScaling must be nil or disabled when multiNode is set"
+// +kubebuilder:validation:XValidation:rule="!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled && !has(self.expose.service.grpcPort))", message=".spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled is true"
 // +kubebuilder:validation:XValidation:rule="!(has(self.expose.ingress) && has(self.expose.ingress.enabled) && self.expose.ingress.enabled && has(self.router) && has(self.router.ingress))", message=".spec.expose.ingress is deprecated, and will be removed in a future release. If .spec.expose.ingress is set, please do not set .spec.router.ingress."
 type NIMServiceSpec struct {
 	Image   Image           `json:"image"`
@@ -968,7 +969,10 @@ func (n *NIMService) IsGRPCRouteEnabled() bool {
 }
 
 func (n *NIMService) GetGRPCRouteSpec() gatewayv1.GRPCRouteSpec {
-	return n.Spec.Router.GenerateGatewayGRPCRouteSpec(n.GetNamespace(), n.GetName(), n.GetGRPCServicePort())
+	if n.Spec.Expose.Service.GRPCPort == nil {
+		return gatewayv1.GRPCRouteSpec{}
+	}
+	return n.Spec.Router.GenerateGatewayGRPCRouteSpec(n.GetNamespace(), n.GetName(), *n.Spec.Expose.Service.GRPCPort)
 }
 
 // IsServiceMonitorEnabled returns true if servicemonitor is enabled for NIMService deployment.
@@ -984,14 +988,6 @@ func (n *NIMService) GetServicePort() int32 {
 	return *n.Spec.Expose.Service.Port
 }
 
-// GetGRPCServicePort returns the grpc service port for the NIMService deployment or default port.
-func (n *NIMService) GetGRPCServicePort() int32 {
-	if n.Spec.Expose.Service.GRPCPort == nil {
-		return DefaultGRPCPort
-	}
-	return *n.Spec.Expose.Service.GRPCPort
-}
-
 // GetServiceType returns the service type for the NIMService deployment.
 func (n *NIMService) GetServiceType() string {
 	return string(n.Spec.Expose.Service.Type)
@@ -1372,6 +1368,7 @@ func (n *NIMService) GetGRPCRouteParams() *rendertypes.GRPCRouteParams {
 	params.Name = n.GetName()
 	params.Namespace = n.GetNamespace()
 	params.Labels = n.GetServiceLabels()
+	params.Annotations = n.GetRouterAnnotations()
 	params.Spec = n.GetGRPCRouteSpec()
 	return params
 }

bundle/manifests/apps.nvidia.com_nimpipelines.yaml

@@ -2674,6 +2674,10 @@ spec:
                           is set
                         rule: '!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled)
                           && self.scale.enabled)'
+                      - message: .spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled
+                          is true
+                        rule: '!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled
+                          && !has(self.expose.service.grpcPort))'
                       - message: .spec.expose.ingress is deprecated, and will be removed
                           in a future release. If .spec.expose.ingress is set, please
                           do not set .spec.router.ingress.

bundle/manifests/apps.nvidia.com_nimservices.yaml

@@ -2577,6 +2577,10 @@ spec:
             - message: autoScaling must be nil or disabled when multiNode is set
               rule: '!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled)
                 && self.scale.enabled)'
+            - message: .spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled
+                is true
+              rule: '!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled
+                && !has(self.expose.service.grpcPort))'
             - message: .spec.expose.ingress is deprecated, and will be removed in
                 a future release. If .spec.expose.ingress is set, please do not set
                 .spec.router.ingress.

config/crd/bases/apps.nvidia.com_nimpipelines.yaml

@@ -2674,6 +2674,10 @@ spec:
                           is set
                         rule: '!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled)
                           && self.scale.enabled)'
+                      - message: .spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled
+                          is true
+                        rule: '!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled
+                          && !has(self.expose.service.grpcPort))'
                       - message: .spec.expose.ingress is deprecated, and will be removed
                           in a future release. If .spec.expose.ingress is set, please
                           do not set .spec.router.ingress.

config/crd/bases/apps.nvidia.com_nimservices.yaml

@@ -2577,6 +2577,10 @@ spec:
             - message: autoScaling must be nil or disabled when multiNode is set
               rule: '!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled)
                 && self.scale.enabled)'
+            - message: .spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled
+                is true
+              rule: '!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled
+                && !has(self.expose.service.grpcPort))'
             - message: .spec.expose.ingress is deprecated, and will be removed in
                 a future release. If .spec.expose.ingress is set, please do not set
                 .spec.router.ingress.

config/default/kustomization.yaml

@@ -1,5 +1,5 @@
 # Adds namespace to all resources.
-namespace: k8s-nim-operator-system
+namespace: nemo
 
 # Value of this field is prepended to the
 # names of all resources, e.g. a deployment named

deployments/helm/k8s-nim-operator/crds/apps.nvidia.com_nimpipelines.yaml

@@ -2674,6 +2674,10 @@ spec:
                           is set
                         rule: '!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled)
                           && self.scale.enabled)'
+                      - message: .spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled
+                          is true
+                        rule: '!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled
+                          && !has(self.expose.service.grpcPort))'
                       - message: .spec.expose.ingress is deprecated, and will be removed
                           in a future release. If .spec.expose.ingress is set, please
                           do not set .spec.router.ingress.

deployments/helm/k8s-nim-operator/crds/apps.nvidia.com_nimservices.yaml

@@ -2577,6 +2577,10 @@ spec:
             - message: autoScaling must be nil or disabled when multiNode is set
               rule: '!(has(self.multiNode) && has(self.scale) && has(self.scale.enabled)
                 && self.scale.enabled)'
+            - message: .spec.expose.service.grpcPort must be set when .spec.router.gateway.grpcRoutesEnabled
+                is true
+              rule: '!(has(self.router.gateway) && self.router.gateway.grpcRoutesEnabled
+                && !has(self.expose.service.grpcPort))'
             - message: .spec.expose.ingress is deprecated, and will be removed in
                 a future release. If .spec.expose.ingress is set, please do not set
                 .spec.router.ingress.

internal/webhook/apps/v1alpha1/nimservice_webhook_validation_helper.go

@@ -111,6 +111,23 @@ func validateNIMServiceSpec(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Pa
 	warningList = append(warningList, w...)
 	errList = append(errList, err...)
 
+	w, err = validateExposeRouterConfiguration(spec, fldPath)
+	warningList = append(warningList, w...)
+	errList = append(errList, err...)
+
+	return warningList, errList
+}
+
+func validateExposeRouterConfiguration(spec *appsv1alpha1.NIMServiceSpec, fldPath *field.Path) (admission.Warnings, field.ErrorList) {
+	warningList := admission.Warnings{}
+	errList := field.ErrorList{}
+	if spec.Expose.Service.Type == corev1.ServiceTypeLoadBalancer && (spec.Router.Gateway != nil || spec.Router.Ingress != nil) {
+		warningList = append(warningList, "spec.expose.service.type is set to LoadBalancer, but spec.router.gateway or spec.router.ingress is also set. This creates two entry points for the service. Consider only using one of them.")
+	}
+
+	if spec.Router.Gateway != nil && spec.Router.Gateway.GRPCRoutesEnabled && spec.Expose.Service.GRPCPort == nil {
+		errList = append(errList, field.Required(fldPath.Child("expose").Child("service").Child("grpcPort"), "must be set when .spec.router.gateway.grpcRoutesEnabled is true"))
+	}
 	return warningList, errList
 }