Adddressing review comments

Signed-off-by: Vishesh Tanksale <vtanksale@nvidia.com>

Author: visheshtanksale

api/apps/v1alpha1/nimcache_types.go

@@ -48,6 +48,7 @@ type NIMCacheSpec struct {
 	GroupID *int64 `json:"groupID,omitempty"`
 	// CertConfig is the name of the ConfigMap containing the custom certificates.
 	// for secure communication.
+	//
 	// Deprecated: use `Proxy` instead to configure custom certificates for using proxy.
 	// +optional
 	CertConfig *CertConfig `json:"certConfig,omitempty"`

api/apps/v1alpha1/nimservice_types.go

@@ -275,17 +275,6 @@ func (n *NIMService) GetStandardEnv() []corev1.EnvVar {
 			Name:  "NIM_CACHE_PATH",
 			Value: utils.DefaultModelStorePath,
 		},
-		{
-			Name: "NGC_API_KEY",
-			ValueFrom: &corev1.EnvVarSource{
-				SecretKeyRef: &corev1.SecretKeySelector{
-					LocalObjectReference: corev1.LocalObjectReference{
-						Name: n.Spec.AuthSecret,
-					},
-					Key: "NGC_API_KEY",
-				},
-			},
-		},
 		{
 			Name:  "OUTLINES_CACHE_DIR",
 			Value: "/tmp/outlines",
@@ -362,6 +351,11 @@ func (n *NIMService) getLWSCommonEnv() []corev1.EnvVar {
 	return env
 }
 
+// GetLWSLeaderEnvFrom returns the env from sources for the leader worker set.
+func (n *NIMService) GetLWSCommonEnvFrom() []corev1.EnvFromSource {
+	return n.GetEnvFrom()
+}
+
 func (n *NIMService) GetLWSLeaderEnv() []corev1.EnvVar {
 	env := n.getLWSCommonEnv()
 
@@ -566,6 +560,23 @@ func (n *NIMService) GetEnv() []corev1.EnvVar {
 	return envVarList
 }
 
+// GetEnvFrom returns merged slice of standard and user specified env from sources.
+func (n *NIMService) GetEnvFrom() []corev1.EnvFromSource {
+	if n.Spec.AuthSecret != "" {
+		return []corev1.EnvFromSource{
+			{
+				SecretRef: &corev1.SecretEnvSource{
+					LocalObjectReference: corev1.LocalObjectReference{
+						Name: n.Spec.AuthSecret,
+					},
+				},
+			},
+		}
+	}
+	// no secrets to source the env variables
+	return []corev1.EnvFromSource{}
+}
+
 // GetImage returns container image for the NIMService.
 func (n *NIMService) GetImage() string {
 	return fmt.Sprintf("%s:%s", n.Spec.Image.Repository, n.Spec.Image.Tag)
@@ -1047,6 +1058,7 @@ func (n *NIMService) GetDeploymentParams() *rendertypes.DeploymentParams {
 	// Set container spec
 	params.ContainerName = n.GetContainerName()
 	params.Env = n.GetEnv()
+	params.EnvFrom = n.GetEnvFrom()
 	params.Args = n.GetArgs()
 	params.Command = n.GetCommand()
 	params.Resources = n.GetResources()
@@ -1118,6 +1130,8 @@ func (n *NIMService) GetLWSParams() *rendertypes.LeaderWorkerSetParams {
 	params.Command = n.GetCommand()
 	params.LeaderEnvs = n.GetLWSLeaderEnv()
 	params.WorkerEnvs = n.GetLWSWorkerEnv()
+	params.LeaderEnvFrom = n.GetLWSCommonEnvFrom()
+	params.WorkerEnvFrom = n.GetLWSCommonEnvFrom()
 	params.UserID = n.GetUserID()
 	params.GroupID = n.GetGroupID()
 	params.Image = n.GetImage()

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

@@ -54,6 +54,7 @@ spec:
                 description: |-
                   CertConfig is the name of the ConfigMap containing the custom certificates.
                   for secure communication.
+
                   Deprecated: use `Proxy` instead to configure custom certificates for using proxy.
                 properties:
                   mountPath:

internal/controller/platform/standalone/nimservice.go

@@ -462,26 +462,6 @@ func (r *NIMServiceReconciler) reconcileNIMService(ctx context.Context, nimServi
 				Value: utils.DefaultModelStorePath,
 			}}, deploymentParams.Env)
 		}
-		// If NIMCache is not provided, and the model is from Hugging Face, add the HF_TOKEN to the environment variables
-		if nimService.GetNIMCacheName() == "" {
-			env := utils.FindEnvByValue(deploymentParams.Env, "NIM_MODEL_NAME")
-			if env != nil {
-				if strings.HasPrefix(env.Value, "hf://") {
-					deploymentParams.Env = utils.RemoveEnvVar(deploymentParams.Env, "NGC_API_KEY")
-					envVars, err := k8sutil.GetSecretEnvVars(r.GetClient(), ctx, nimService.GetNamespace(), nimService.Spec.AuthSecret)
-					if err != nil {
-						return ctrl.Result{}, err
-					}
-					env = utils.FindEnvByValue(envVars, "HF_TOKEN")
-					if env != nil {
-						deploymentParams.Env = utils.MergeEnvVars(envVars, deploymentParams.Env)
-					} else {
-						return ctrl.Result{}, fmt.Errorf("HF_TOKEN not found in secret %s", env.Value)
-					}
-
-				}
-			}
-		}
 		// Setup volume mounts with model store
 		deploymentParams.Volumes = nimService.GetVolumes(*modelPVC)
 		deploymentParams.VolumeMounts = nimService.GetVolumeMounts(*modelPVC)

internal/controller/platform/standalone/nimservice_test.go

@@ -1107,17 +1107,6 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
 					Name:  "NIM_CACHE_PATH",
 					Value: "/model-store",
 				},
-				{
-					Name: "NGC_API_KEY",
-					ValueFrom: &corev1.EnvVarSource{
-						SecretKeyRef: &corev1.SecretKeySelector{
-							LocalObjectReference: corev1.LocalObjectReference{
-								Name: "",
-							},
-							Key: "NGC_API_KEY",
-						},
-					},
-				},
 				{
 					Name:  "OUTLINES_CACHE_DIR",
 					Value: "/tmp/outlines",
@@ -1209,17 +1198,6 @@ var _ = Describe("NIMServiceReconciler for a standalone platform", func() {
 					Name:  "NIM_CACHE_PATH",
 					Value: "/model-store",
 				},
-				{
-					Name: "NGC_API_KEY",
-					ValueFrom: &corev1.EnvVarSource{
-						SecretKeyRef: &corev1.SecretKeySelector{
-							LocalObjectReference: corev1.LocalObjectReference{
-								Name: "",
-							},
-							Key: "NGC_API_KEY",
-						},
-					},
-				},
 				{
 					Name:  "OUTLINES_CACHE_DIR",
 					Value: "/tmp/outlines",

internal/k8sutil/k8sutil.go

@@ -412,32 +412,3 @@ func ControllerOwnsIfCRDExists(discoveryClient discovery.DiscoveryInterface,
 		},
 	)
 }
-
-func GetSecretEnvVars(cli client.Client, ctx context.Context, namespace, secretName string) ([]corev1.EnvVar, error) {
-	secret := &corev1.Secret{}
-	err := cli.Get(ctx, client.ObjectKey{
-		Namespace: namespace,
-		Name:      secretName,
-	}, secret)
-	if err != nil {
-		return nil, fmt.Errorf("failed to get secret %s: %w", secretName, err)
-	}
-
-	// Convert the secret data into EnvVars
-	envVars := []corev1.EnvVar{}
-	for key := range secret.Data {
-		envVars = append(envVars, corev1.EnvVar{
-			Name: key,
-			ValueFrom: &corev1.EnvVarSource{
-				SecretKeyRef: &corev1.SecretKeySelector{
-					LocalObjectReference: corev1.LocalObjectReference{
-						Name: secretName,
-					},
-					Key: key,
-				},
-			},
-		})
-	}
-
-	return envVars, nil
-}

internal/render/types/types.go

@@ -71,6 +71,7 @@ type DeploymentParams struct {
 	Volumes            []corev1.Volume
 	VolumeMounts       []corev1.VolumeMount
 	Env                []corev1.EnvVar
+	EnvFrom            []corev1.EnvFromSource
 	Resources          *corev1.ResourceRequirements
 	NodeSelector       map[string]string
 	Tolerations        []corev1.Toleration
@@ -112,6 +113,8 @@ type LeaderWorkerSetParams struct {
 	LeaderVolumeMounts []corev1.VolumeMount
 	WorkerEnvs         []corev1.EnvVar
 	LeaderEnvs         []corev1.EnvVar
+	WorkerEnvFrom      []corev1.EnvFromSource
+	LeaderEnvFrom      []corev1.EnvFromSource
 	Resources          *corev1.ResourceRequirements
 	NodeSelector       map[string]string
 	Tolerations        []corev1.Toleration

internal/utils/utils.go

@@ -494,22 +494,3 @@ func IsVersionGreaterThanOrEqual(version string, minVersion string) bool {
 	}
 	return cv.AtLeast(mv)
 }
-
-func RemoveEnvVar(envs []corev1.EnvVar, name string) []corev1.EnvVar {
-	result := make([]corev1.EnvVar, 0, len(envs))
-	for _, e := range envs {
-		if e.Name != name {
-			result = append(result, e)
-		}
-	}
-	return result
-}
-
-func FindEnvByValue(envs []corev1.EnvVar, key string) *corev1.EnvVar {
-	for i := range envs {
-		if envs[i].Name == key {
-			return &envs[i]
-		}
-	}
-	return nil
-}

manifests/deployment.yaml

@@ -98,6 +98,10 @@ spec:
         {{- if .Env }}
           {{- .Env | yaml | nindent 10 }}
         {{- end }}
+        envFrom:
+        {{- if .EnvFrom }}
+          {{- .EnvFrom | yaml | nindent 10 }}
+        {{- end }}
         {{- with .Resources }}
         resources:
           {{ . | yaml | nindent 10 }}