diff --git a/k8s/helm/templates/core/controller-role.yaml b/k8s/helm/templates/core/controller-role.yaml index 198a31e6f0..5940186710 100644 --- a/k8s/helm/templates/core/controller-role.yaml +++ b/k8s/helm/templates/core/controller-role.yaml @@ -38,7 +38,11 @@ rules: verbs: ["get", "list", "create", "delete"] - apiGroups: [""] resources: ["secrets"] - verbs: ["create", "delete"] + # get/list are required by the deployments k8s backend: it reads a managed + # Secret to enforce the ownership-label guard before create (409 conflict) and + # before delete. Without get, delete_deployment fails with 403 and the + # Deployment is stuck in DELETING. Mirrors the configmaps verb set. + verbs: ["get", "list", "create", "delete"] {{- if .Values.rbac.volcanoEnabled }} # Volcano - apiGroups: ["batch.volcano.sh"] diff --git a/plugins/nemo-deployments/openapi/openapi.yaml b/plugins/nemo-deployments/openapi/openapi.yaml index 914622736a..00234c8f4b 100644 --- a/plugins/nemo-deployments/openapi/openapi.yaml +++ b/plugins/nemo-deployments/openapi/openapi.yaml @@ -1365,12 +1365,13 @@ components: - name - image title: RequestContainer - description: Public request container; env entries cannot carry secretRef. + description: Public request container. RequestEnvVar: not: required: - value - valueFrom + - secretRef properties: name: type: string @@ -1382,12 +1383,20 @@ components: title: Valuefrom additionalProperties: true type: object + secretRef: + $ref: '#/components/schemas/SecretRef' additionalProperties: false type: object required: - name title: RequestEnvVar - description: Public request env vars; secretRef is controller-managed and response-only. + description: "Public request env vars.\n\nAn env var carries exactly one value\ + \ source: a plaintext ``value``, a\n``valueFrom`` projection, or a ``secretRef``\ + \ pointing at a Platform secret.\n``secretRef`` values are resolved by the\ + \ substrate backend at deploy time \u2014\ndocker injects the resolved value\ + \ as a plaintext container env var, while\nk8s materializes a single per-deployment\ + \ ``Secret`` and mounts it via\n``envFrom`` so the plaintext never lands in\ + \ the pod manifest." ResourceRequirements: properties: limits: diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py index 2b6ccab028..2856322bc0 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/backend.py @@ -21,7 +21,7 @@ from nemo_deployments_plugin.backends.k8s.config import K8sExecutorConfig from nemo_deployments_plugin.backends.labels import deployment_identity_labels from nemo_deployments_plugin.entities import Deployment, DeploymentConfig -from nemo_deployments_plugin.secrets import SecretResolutionError, resolve_deployment_config_secrets +from nemo_deployments_plugin.secrets import SecretResolutionError, resolve_deployment_secret_env from nemo_platform_plugin.client.adapter import client_from_platform from nemo_platform_plugin.entities.client import AsyncEntitiesClient from nemo_platform_plugin.entity_client import NemoEntitiesClient, NemoEntityNotFoundError @@ -103,7 +103,10 @@ async def create_deployment( ) -> BackendStatusUpdate: try: config = await self._load_deployment_config(workspace, config_name) - config = await resolve_deployment_config_secrets(self._sdk, config) + # k8s keeps secret_ref env vars intact and mounts their resolved + # values through a single per-deployment Secret via envFrom, so the + # plaintext never lands in the pod manifest. + secret_env = await resolve_deployment_secret_env(self._sdk, config) except NemoEntityNotFoundError: return BackendStatusUpdate( status="FAILED", @@ -126,6 +129,7 @@ async def create_deployment( backend_config=backend_config, config=config, executor_image_pull_secrets=self._executor_config.image_pull_secrets, + secret_env=secret_env, ) return await job_ops.create_job( @@ -138,6 +142,7 @@ async def create_deployment( backend_config=backend_config, config=config, executor_image_pull_secrets=self._executor_config.image_pull_secrets, + secret_env=secret_env, ) async def read_status(self, *, workspace: str, name: str) -> BackendStatusUpdate: diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/compiler.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/compiler.py index bb8340c1f6..0a7a66db39 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/compiler.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/compiler.py @@ -19,7 +19,11 @@ from nemo_deployments_plugin.auth_proxy import build_auth_proxy_container from nemo_deployments_plugin.backends.k8s.client import k8s_client_module from nemo_deployments_plugin.backends.k8s.status import resource_labels_match -from nemo_deployments_plugin.backends.labels import k8s_deployment_configmap_name, k8s_volume_resource_name +from nemo_deployments_plugin.backends.labels import ( + k8s_deployment_configmap_name, + k8s_deployment_secret_name, + k8s_volume_resource_name, +) from nemo_deployments_plugin.entities import ( Affinity, ConfigFile, @@ -55,10 +59,25 @@ def merged_volume_mounts(config: DeploymentConfig, container: Container) -> list def build_env_vars(container: Container) -> list[Any]: + """Build plaintext ``V1EnvVar`` entries for a container. + + Env vars carrying a ``secret_ref`` are intentionally skipped here — their + values live in the per-deployment managed ``Secret`` and are injected via + ``envFrom`` (see :func:`build_secret_env_from`), so the plaintext never + appears in the pod manifest. + """ k8s = k8s_client_module() return [k8s.client.V1EnvVar(name=item.name, value=item.value) for item in container.env if item.value is not None] +def build_secret_env_from(secret_name: str | None) -> list[Any]: + """Build the ``envFrom`` projection for a deployment's managed Secret.""" + if secret_name is None: + return [] + k8s = k8s_client_module() + return [k8s.client.V1EnvFromSource(secret_ref=k8s.client.V1SecretEnvSource(name=secret_name))] + + def build_resource_requirements(container: Container) -> Any | None: limits = container.resources.limits or None requests = container.resources.requests or None @@ -98,12 +117,18 @@ def build_volume_mounts(mounts: list[VolumeMount]) -> list[Any]: ] -def build_container_spec(container: Container, *, volume_mounts: list[VolumeMount] | None = None) -> Any: +def build_container_spec( + container: Container, + *, + volume_mounts: list[VolumeMount] | None = None, + secret_name: str | None = None, +) -> Any: k8s = k8s_client_module() kwargs: dict[str, Any] = { "name": container.name, "image": container.image, "env": build_env_vars(container) or None, + "env_from": build_secret_env_from(secret_name) or None, "resources": build_resource_requirements(container), } if container.command: @@ -123,6 +148,8 @@ class CompiledWorkload: configmap_body: Any | None configmap_name: str | None service_containers: tuple[Container, ...] + secret_body: Any | None = None + secret_name: str | None = None def _reraise_api_unless(exc: ApiException, *allowed_statuses: int) -> None: @@ -256,19 +283,21 @@ def build_container( *, config: DeploymentConfig, include_probes: bool, + secret_name: str | None = None, ) -> Any: """Build a V1Container from a plugin Container.""" k8s = k8s_client_module() mounts = merged_volume_mounts(config, container) if config.config_files: mounts = [*mounts, *_config_file_mounts(config.config_files)] - base = build_container_spec(container, volume_mounts=mounts or None) + base = build_container_spec(container, volume_mounts=mounts or None, secret_name=secret_name) kwargs: dict[str, Any] = { "name": base.name, "image": base.image, "command": base.command, "args": base.args, "env": base.env, + "env_from": base.env_from, "resources": base.resources, "volume_mounts": build_volume_mounts(mounts) if mounts else base.volume_mounts, "ports": _build_container_ports(container.ports) or None, @@ -335,6 +364,34 @@ def build_configmap_body( ) +def build_secret_body( + *, + workspace: str, + deployment_name: str, + labels: dict[str, str], + secret_env: dict[str, str], +) -> Any | None: + """Build the per-deployment managed ``V1Secret`` for resolved secret env vars. + + Returns ``None`` when the deployment references no secrets. The Secret holds + every resolved secret env var (keyed by env var name) and is mounted into + containers via ``envFrom`` so the plaintext never lands in the pod manifest. + """ + if not secret_env: + return None + k8s = k8s_client_module() + return k8s.client.V1Secret( + api_version="v1", + kind="Secret", + type="Opaque", + metadata=k8s.client.V1ObjectMeta( + name=k8s_deployment_secret_name(workspace, deployment_name), + labels=labels, + ), + string_data=dict(secret_env), + ) + + def _build_config_file_volume(configmap_name: str, config_files: list[ConfigFile]) -> Any: k8s = k8s_client_module() items = [ @@ -369,8 +426,14 @@ def compile_workload( k8s_config: K8sDeploymentConfig | None, pod_restart_policy: RestartPolicy, executor_image_pull_secrets: list[ImagePullSecret] | None = None, + secret_env: dict[str, str] | None = None, ) -> CompiledWorkload: - """Compile pod spec kwargs and optional ConfigMap for a Job or Deployment.""" + """Compile pod spec kwargs and optional ConfigMap/Secret for a Job or Deployment. + + ``secret_env`` maps env var names to resolved plaintext secret values. When + non-empty, a single per-deployment ``Secret`` is compiled and mounted into + every container via ``envFrom``. + """ validate_workload_config(config) pvc_mounts = _collect_pvc_mounts(config) volumes = build_pod_volumes(workspace=workspace, mounts=pvc_mounts) @@ -384,23 +447,34 @@ def compile_workload( if configmap_name is not None: volumes = [*volumes, _build_config_file_volume(configmap_name, config.config_files)] + secret_body = build_secret_body( + workspace=workspace, + deployment_name=deployment_name, + labels=labels, + secret_env=secret_env or {}, + ) + secret_name = secret_body.metadata.name if secret_body is not None else None + ordered_init = list(_ordered_init_containers(config)) - # Auth-proxy sidecar (native sidecar with restartPolicy=Always) is appended so - # it starts before the main workload and keeps running. No-op when the config - # does not request it or platform auth is disabled. - auth_proxy = build_auth_proxy_container(config) - if auth_proxy is not None: - ordered_init.append(auth_proxy) init_containers = [ build_container( container, config=config, include_probes=container.restart_policy == NATIVE_SIDECAR_RESTART_POLICY, + secret_name=secret_name, ) for container in ordered_init ] + # Auth-proxy sidecar (native sidecar with restartPolicy=Always) is appended so + # it starts before the main workload and keeps running. No-op when the config + # does not request it or platform auth is disabled. It is a platform-managed + # container and deliberately does NOT receive the workload's secret envFrom. + auth_proxy = build_auth_proxy_container(config) + if auth_proxy is not None: + init_containers.append(build_container(auth_proxy, config=config, include_probes=True)) main_containers = [ - build_container(container, config=config, include_probes=True) for container in config.containers + build_container(container, config=config, include_probes=True, secret_name=secret_name) + for container in config.containers ] pod_spec_kwargs: dict[str, Any] = { @@ -434,6 +508,8 @@ def compile_workload( configmap_body=configmap_body, configmap_name=configmap_name, service_containers=tuple(config.containers), + secret_body=secret_body, + secret_name=secret_name, ) @@ -499,3 +575,67 @@ def delete_configmap( core_v1.delete_namespaced_config_map(name=name, namespace=namespace, _request_timeout=timeout) except ApiException as exc: _reraise_api_unless(exc, 404) + + +def create_secret( + core_v1: Any, + *, + namespace: str, + body: Any, + expected_labels: dict[str, str], + timeout: float | None, +) -> None: + try: + core_v1.create_namespaced_secret(namespace=namespace, body=body, _request_timeout=timeout) + except ApiException as exc: + _reraise_api_unless(exc, 409) + existing = core_v1.read_namespaced_secret( + name=body.metadata.name, + namespace=namespace, + _request_timeout=timeout, + ) + if not resource_labels_match(existing, expected_labels): + raise + + +def delete_secret_best_effort( + core_v1: Any, + *, + namespace: str, + name: str | None, + expected_labels: dict[str, str], + timeout: float | None, +) -> None: + if name is None: + return + try: + delete_secret( + core_v1, + namespace=namespace, + name=name, + expected_labels=expected_labels, + timeout=timeout, + ) + except ApiException: + logger.debug("Best-effort Secret cleanup failed for %s", name, exc_info=True) + + +def delete_secret( + core_v1: Any, + *, + namespace: str, + name: str, + expected_labels: dict[str, str], + timeout: float | None, +) -> None: + try: + secret = core_v1.read_namespaced_secret(name=name, namespace=namespace, _request_timeout=timeout) + except ApiException as exc: + _reraise_api_unless(exc, 404) + return + if not resource_labels_match(secret, expected_labels): + return + try: + core_v1.delete_namespaced_secret(name=name, namespace=namespace, _request_timeout=timeout) + except ApiException as exc: + _reraise_api_unless(exc, 404) diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py index fb4405b256..48a4eddfad 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/deployments.py @@ -22,8 +22,11 @@ DeploymentConfigError, compile_workload, create_configmap, + create_secret, delete_configmap, delete_configmap_best_effort, + delete_secret, + delete_secret_best_effort, validate_config_for_deployment, ) from nemo_deployments_plugin.backends.k8s.jobs import ( @@ -45,6 +48,7 @@ deployment_identity_labels, k8s_deployment_configmap_name, k8s_deployment_resource_name, + k8s_deployment_secret_name, managed_by_label_selector, ) from nemo_deployments_plugin.entities import Container, DeploymentConfig, K8sDeploymentConfig @@ -78,6 +82,7 @@ def build_deployment_body( deployment_name: str, k8s_config: K8sDeploymentConfig | None, executor_image_pull_secrets: list | None = None, + secret_env: dict[str, str] | None = None, ) -> BuiltDeployment: """Build an ``apps/v1.Deployment`` for create and its compiled workload.""" k8s = k8s_client_module() @@ -91,6 +96,7 @@ def build_deployment_body( k8s_config=k8s_config, pod_restart_policy="Always", executor_image_pull_secrets=executor_image_pull_secrets, + secret_env=secret_env, ) deployment = k8s.client.V1Deployment( api_version="apps/v1", @@ -227,6 +233,7 @@ async def create_deployment( backend_config: dict[str, Any], config: DeploymentConfig, executor_image_pull_secrets: list | None = None, + secret_env: dict[str, str] | None = None, ) -> BackendStatusUpdate: resource_name = k8s_deployment_resource_name(workspace, name) try: @@ -249,6 +256,7 @@ async def create_deployment( deployment_name=name, k8s_config=k8s_config, executor_image_pull_secrets=executor_image_pull_secrets, + secret_env=secret_env, ) deployment_body = built.deployment compiled = built.compiled @@ -261,7 +269,9 @@ async def create_deployment( apps_v1 = clients.apps_v1 core_v1 = clients.core_v1 - def _rollback_partial_create(*, deployment_created: bool, delete_configmap: bool) -> None: + def _rollback_partial_create( + *, deployment_created: bool, should_delete_configmap: bool, should_delete_secret: bool + ) -> None: if deployment_created: try: apps_v1.delete_namespaced_deployment( @@ -272,7 +282,7 @@ def _rollback_partial_create(*, deployment_created: bool, delete_configmap: bool ) except ApiException as cleanup_exc: _log_cleanup_ignored(resource_name, cleanup_exc) - if delete_configmap: + if should_delete_configmap: delete_configmap_best_effort( core_v1, namespace=namespace, @@ -280,18 +290,46 @@ def _rollback_partial_create(*, deployment_created: bool, delete_configmap: bool expected_labels=identity_labels, timeout=timeout, ) + if should_delete_secret: + delete_secret_best_effort( + core_v1, + namespace=namespace, + name=compiled.secret_name, + expected_labels=identity_labels, + timeout=timeout, + ) def _create() -> Any: deployment_created = False - configmap_written = compiled.configmap_body is not None - if configmap_written: - create_configmap( + secret_written = compiled.secret_body is not None + if secret_written: + create_secret( core_v1, namespace=namespace, - body=compiled.configmap_body, + body=compiled.secret_body, expected_labels=identity_labels, timeout=timeout, ) + configmap_written = compiled.configmap_body is not None + if configmap_written: + try: + create_configmap( + core_v1, + namespace=namespace, + body=compiled.configmap_body, + expected_labels=identity_labels, + timeout=timeout, + ) + except Exception: + if secret_written: + delete_secret_best_effort( + core_v1, + namespace=namespace, + name=compiled.secret_name, + expected_labels=identity_labels, + timeout=timeout, + ) + raise try: apps_v1.create_namespaced_deployment( namespace=namespace, @@ -309,6 +347,14 @@ def _create() -> Any: expected_labels=identity_labels, timeout=timeout, ) + if secret_written: + delete_secret_best_effort( + core_v1, + namespace=namespace, + name=compiled.secret_name, + expected_labels=identity_labels, + timeout=timeout, + ) raise deployment = apps_v1.read_namespaced_deployment( @@ -317,10 +363,15 @@ def _create() -> Any: _request_timeout=timeout, ) if not resource_labels_match(deployment, identity_labels): - _rollback_partial_create(deployment_created=deployment_created, delete_configmap=configmap_written) + _rollback_partial_create( + deployment_created=deployment_created, + should_delete_configmap=configmap_written, + should_delete_secret=secret_written, + ) return deployment - delete_configmap_on_service_failure = deployment_created and configmap_written + should_delete_configmap_on_service_failure = deployment_created and configmap_written + should_delete_secret_on_service_failure = deployment_created and secret_written try: core_v1.create_namespaced_service( namespace=namespace, @@ -337,13 +388,15 @@ def _create() -> Any: if not resource_labels_match(existing_service, identity_labels): _rollback_partial_create( deployment_created=deployment_created, - delete_configmap=delete_configmap_on_service_failure, + should_delete_configmap=should_delete_configmap_on_service_failure, + should_delete_secret=should_delete_secret_on_service_failure, ) raise return deployment _rollback_partial_create( deployment_created=deployment_created, - delete_configmap=delete_configmap_on_service_failure, + should_delete_configmap=should_delete_configmap_on_service_failure, + should_delete_secret=should_delete_secret_on_service_failure, ) raise return deployment @@ -441,6 +494,7 @@ async def delete_deployment( ) -> BackendStatusUpdate: resource_name = k8s_deployment_resource_name(workspace, name) configmap_name = k8s_deployment_configmap_name(workspace, name) + secret_name = k8s_deployment_secret_name(workspace, name) try: k8s_config = resolve_k8s_deployment_config(backend_config) namespace = resolve_deployment_namespace(default_namespace=default_namespace, k8s_config=k8s_config) @@ -448,6 +502,23 @@ async def delete_deployment( apps_v1 = clients.apps_v1 core_v1 = clients.core_v1 + def _delete_config_resources() -> None: + """Delete the ConfigMap and managed Secret for this deployment.""" + delete_configmap( + core_v1, + namespace=namespace, + name=configmap_name, + expected_labels=expected_labels, + timeout=timeout, + ) + delete_secret( + core_v1, + namespace=namespace, + name=secret_name, + expected_labels=expected_labels, + timeout=timeout, + ) + def _delete() -> str | None: try: deployment = apps_v1.read_namespaced_deployment( @@ -466,13 +537,7 @@ def _delete() -> str | None: except ApiException as service_read_exc: if service_read_exc.status != 404: raise - delete_configmap( - core_v1, - namespace=namespace, - name=configmap_name, - expected_labels=expected_labels, - timeout=timeout, - ) + _delete_config_resources() return None if resource_labels_match(service, expected_labels): try: @@ -484,13 +549,7 @@ def _delete() -> str | None: except ApiException as service_exc: if service_exc.status != 404: raise - delete_configmap( - core_v1, - namespace=namespace, - name=configmap_name, - expected_labels=expected_labels, - timeout=timeout, - ) + _delete_config_resources() return None raise if not resource_labels_match(deployment, expected_labels): @@ -510,13 +569,7 @@ def _delete() -> str | None: except ApiException as exc: if exc.status != 404: raise - delete_configmap( - core_v1, - namespace=namespace, - name=configmap_name, - expected_labels=expected_labels, - timeout=timeout, - ) + _delete_config_resources() return "deleted" result = await asyncio.to_thread(_delete) @@ -528,6 +581,13 @@ def _delete() -> str | None: expected_labels=expected_labels, timeout=timeout, ) + delete_secret_best_effort( + core_v1, + namespace=namespace, + name=secret_name, + expected_labels=expected_labels, + timeout=timeout, + ) return BackendStatusUpdate( status="FAILED", status_message=f"Deployment {resource_name} exists but is not managed by this plugin", diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.py index 1c02d79bfe..c7e1d3ee9a 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/k8s/jobs.py @@ -18,8 +18,11 @@ DeploymentConfigError, compile_workload, create_configmap, + create_secret, delete_configmap, delete_configmap_best_effort, + delete_secret, + delete_secret_best_effort, validate_config_for_job, ) from nemo_deployments_plugin.backends.k8s.status import ( @@ -38,6 +41,7 @@ deployment_identity_labels, k8s_deployment_configmap_name, k8s_deployment_resource_name, + k8s_deployment_secret_name, managed_by_label_selector, ) from nemo_deployments_plugin.constants import MANAGED_BY_LABEL @@ -117,6 +121,7 @@ def build_job_body( deployment_name: str, k8s_config: K8sDeploymentConfig | None, executor_image_pull_secrets: list | None = None, + secret_env: dict[str, str] | None = None, ) -> BuiltJob: """Build a ``batch/v1.Job`` for create.""" k8s = k8s_client_module() @@ -128,6 +133,7 @@ def build_job_body( k8s_config=k8s_config, pod_restart_policy=config.restart_policy, executor_image_pull_secrets=executor_image_pull_secrets, + secret_env=secret_env, ) job = k8s.client.V1Job( api_version="batch/v1", @@ -189,6 +195,7 @@ async def create_job( backend_config: dict[str, Any], config: DeploymentConfig, executor_image_pull_secrets: list | None = None, + secret_env: dict[str, str] | None = None, ) -> BackendStatusUpdate: job_name = k8s_deployment_resource_name(workspace, name) try: @@ -211,6 +218,7 @@ async def create_job( deployment_name=name, k8s_config=k8s_config, executor_image_pull_secrets=executor_image_pull_secrets, + secret_env=secret_env, ) body = built.job compiled = built.compiled @@ -218,16 +226,47 @@ async def create_job( batch_v1 = clients.batch_v1 core_v1 = clients.core_v1 - def _create() -> Any: - configmap_written = compiled.configmap_body is not None + def _cleanup_config_resources(*, configmap_written: bool, secret_written: bool) -> None: if configmap_written: - create_configmap( + delete_configmap_best_effort( + core_v1, + namespace=namespace, + name=compiled.configmap_name, + expected_labels=identity_labels, + timeout=timeout, + ) + if secret_written: + delete_secret_best_effort( + core_v1, + namespace=namespace, + name=compiled.secret_name, + expected_labels=identity_labels, + timeout=timeout, + ) + + def _create() -> Any: + secret_written = compiled.secret_body is not None + if secret_written: + create_secret( core_v1, namespace=namespace, - body=compiled.configmap_body, + body=compiled.secret_body, expected_labels=identity_labels, timeout=timeout, ) + configmap_written = compiled.configmap_body is not None + if configmap_written: + try: + create_configmap( + core_v1, + namespace=namespace, + body=compiled.configmap_body, + expected_labels=identity_labels, + timeout=timeout, + ) + except Exception: + _cleanup_config_resources(configmap_written=False, secret_written=secret_written) + raise try: return batch_v1.create_namespaced_job( namespace=namespace, @@ -242,23 +281,12 @@ def _create() -> Any: _request_timeout=timeout, ) if not resource_labels_match(job, identity_labels): - if configmap_written: - delete_configmap_best_effort( - core_v1, - namespace=namespace, - name=compiled.configmap_name, - expected_labels=identity_labels, - timeout=timeout, - ) + _cleanup_config_resources( + configmap_written=configmap_written, + secret_written=secret_written, + ) return job - if configmap_written: - delete_configmap_best_effort( - core_v1, - namespace=namespace, - name=compiled.configmap_name, - expected_labels=identity_labels, - timeout=timeout, - ) + _cleanup_config_resources(configmap_written=configmap_written, secret_written=secret_written) raise job = await asyncio.to_thread(_create) @@ -345,6 +373,24 @@ async def delete_job( batch_v1 = clients.batch_v1 core_v1 = clients.core_v1 configmap_name = k8s_deployment_configmap_name(workspace, name) + secret_name = k8s_deployment_secret_name(workspace, name) + + def _delete_config_resources() -> None: + """Delete the ConfigMap and managed Secret for this job.""" + delete_configmap( + core_v1, + namespace=namespace, + name=configmap_name, + expected_labels=expected_labels, + timeout=timeout, + ) + delete_secret( + core_v1, + namespace=namespace, + name=secret_name, + expected_labels=expected_labels, + timeout=timeout, + ) def _delete() -> str | None: try: @@ -355,13 +401,7 @@ def _delete() -> str | None: ) except ApiException as exc: if exc.status == 404: - delete_configmap( - core_v1, - namespace=namespace, - name=configmap_name, - expected_labels=expected_labels, - timeout=timeout, - ) + _delete_config_resources() return None raise if not resource_labels_match(job, expected_labels): @@ -372,13 +412,7 @@ def _delete() -> str | None: propagation_policy="Background", _request_timeout=timeout, ) - delete_configmap( - core_v1, - namespace=namespace, - name=configmap_name, - expected_labels=expected_labels, - timeout=timeout, - ) + _delete_config_resources() return "deleted" result = await asyncio.to_thread(_delete) @@ -390,6 +424,13 @@ def _delete() -> str | None: expected_labels=expected_labels, timeout=timeout, ) + delete_secret_best_effort( + core_v1, + namespace=namespace, + name=secret_name, + expected_labels=expected_labels, + timeout=timeout, + ) return BackendStatusUpdate( status="FAILED", status_message=f"Job {job_name} exists but is not managed by this plugin", diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py index 38157f07ba..90002a79be 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/backends/labels.py @@ -92,6 +92,14 @@ def k8s_deployment_configmap_name(workspace: str, deployment_name: str) -> str: ) +def k8s_deployment_secret_name(workspace: str, deployment_name: str) -> str: + """Kubernetes Secret name for a deployment's resolved secret env vars.""" + return k8s_safe_name( + f"dep-sec-{workspace}-{deployment_name}", + hash_input=f"{deployment_key(workspace, deployment_name)}/secret", + ) + + def deployment_identity_labels( workspace: str, name: str, diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/schema.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/schema.py index 8c8a23e233..6fe2e6059d 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/schema.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/schema.py @@ -22,6 +22,7 @@ Probe, ResourceRequirements, RestartPolicy, + SecretRef, Volume, VolumeBackendConfig, VolumeMount, @@ -36,33 +37,48 @@ def _default_request_access_modes() -> list[AccessMode]: class RequestEnvVar(BaseModel): - """Public request env vars; secretRef is controller-managed and response-only.""" + """Public request env vars. + + An env var carries exactly one value source: a plaintext ``value``, a + ``valueFrom`` projection, or a ``secretRef`` pointing at a Platform secret. + ``secretRef`` values are resolved by the substrate backend at deploy time — + docker injects the resolved value as a plaintext container env var, while + k8s materializes a single per-deployment ``Secret`` and mounts it via + ``envFrom`` so the plaintext never lands in the pod manifest. + """ name: str value: str | None = None value_from: dict[str, Any] | None = Field(default=None, alias="valueFrom") + secret_ref: SecretRef | None = Field(default=None, alias="secretRef") model_config = ConfigDict( populate_by_name=True, extra="forbid", json_schema_extra={ # Keep the OpenAPI contract aligned with validate_single_source. - "not": {"required": ["value", "valueFrom"]}, + "not": {"required": ["value", "valueFrom", "secretRef"]}, }, ) @model_validator(mode="after") def validate_single_source(self) -> RequestEnvVar: - if self.value is not None and self.value_from is not None: - raise ValueError("EnvVar may define only one of value or valueFrom") + sources = (self.value, self.value_from, self.secret_ref) + if sum(source is not None for source in sources) > 1: + raise ValueError("EnvVar may define only one of value, valueFrom, or secretRef") return self def to_entity(self) -> EnvVar: - return EnvVar(name=self.name, value=self.value, valueFrom=self.value_from) + return EnvVar( + name=self.name, + value=self.value, + valueFrom=self.value_from, + secretRef=self.secret_ref, + ) class RequestContainer(BaseModel): - """Public request container; env entries cannot carry secretRef.""" + """Public request container.""" name: str image: str diff --git a/plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.py b/plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.py index 36766ab9de..1fafb2eeca 100644 --- a/plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.py +++ b/plugins/nemo-deployments/src/nemo_deployments_plugin/secrets.py @@ -43,7 +43,13 @@ async def resolve_secret_ref(sdk: AsyncNeMoPlatform, secret_ref: SecretRef) -> s async def resolve_deployment_config_secrets(sdk: AsyncNeMoPlatform, config: DeploymentConfig) -> DeploymentConfig: - """Return an execution-only copy whose secret references have resolved values.""" + """Return an execution-only copy whose secret references have plaintext values. + + Used by substrates that cannot mount a managed secret object (docker, + openshell): every ``secret_ref`` env var is resolved to a plaintext + ``EnvVar.value``. Vars that resolve to ``None`` (best-effort NGC only) are + omitted so mock/local images can still start. + """ resolved = config.model_copy(deep=True) for container in (*resolved.init_containers, *resolved.containers): env: list[EnvVar] = [] @@ -55,22 +61,65 @@ async def resolve_deployment_config_secrets(sdk: AsyncNeMoPlatform, config: Depl return resolved +async def resolve_deployment_secret_env(sdk: AsyncNeMoPlatform, config: DeploymentConfig) -> dict[str, str]: + """Collect resolved secret values for every ``secret_ref`` env var. + + Used by the k8s substrate to materialize a single per-deployment ``Secret`` + that is mounted via ``envFrom``, so plaintext never lands in the pod + manifest. The returned mapping is keyed by the env var name across all + containers. Later containers win on duplicate names, matching the + single-Secret-per-deployment projection. + + Best-effort NGC semantics are preserved: a ``secret_ref`` that resolves to + ``None`` is omitted rather than raising. Unauthorized references and + secret-service access failures remain hard errors via ``_resolve_secret_value``. + """ + secret_env: dict[str, str] = {} + for container in (*config.init_containers, *config.containers): + for item in container.env: + if item.secret_ref is None: + continue + value = await _resolve_secret_value(sdk, item) + if value is not None: + secret_env[item.name] = value + return secret_env + + async def _resolve_env_var(sdk: AsyncNeMoPlatform, item: EnvVar) -> EnvVar | None: - """Resolve an authorized secret-backed environment variable. + """Resolve a secret-backed environment variable to a plaintext ``EnvVar``. - NGC credentials are best-effort: when neither the configured secret nor the - process-environment fallback is available, omit the variable so mock/local - NIM images can still start. Unauthorized references and secret-service - access failures remain hard errors. + Non-secret vars pass through unchanged. Secret vars that resolve to ``None`` + (best-effort NGC only) are omitted (returns ``None``). """ if item.secret_ref is None: return item - ngc_secret_ref = platform_ngc_secret_ref() - if item.name != "NGC_API_KEY" or item.secret_ref != ngc_secret_ref: - raise SecretResolutionError(f"Unsupported secret reference for environment variable {item.name!r}") - value = await resolve_secret_ref(sdk, item.secret_ref) - if value is None: - value = os.environ.get(get_platform_config().ngc_api_key_env_var) + value = await _resolve_secret_value(sdk, item) if value is None: return None return EnvVar(name=item.name, value=value) + + +async def _resolve_secret_value(sdk: AsyncNeMoPlatform, item: EnvVar) -> str | None: + """Resolve the plaintext value for a secret-backed env var. + + NGC credentials are best-effort: when neither the configured secret nor the + process-environment fallback is available, return ``None`` so callers can + omit the variable and mock/local NIM images can still start. All other + references are resolved via the Secrets service; a missing secret is a hard + error, as is any secret-service access failure. + """ + if item.secret_ref is None: + raise SecretResolutionError(f"Environment variable {item.name!r} has no secret reference to resolve") + value = await resolve_secret_ref(sdk, item.secret_ref) + if value is not None: + return value + + # Only reached when the Secrets service had no value: fall back to the NGC + # process-env only for the platform NGC key, otherwise it is a hard error. + is_ngc = item.name == "NGC_API_KEY" and item.secret_ref == platform_ngc_secret_ref() + if is_ngc: + return os.environ.get(get_platform_config().ngc_api_key_env_var) + raise SecretResolutionError( + f"Secret {item.secret_ref.workspace}/{item.secret_ref.name} for environment " + f"variable {item.name!r} could not be resolved" + ) diff --git a/plugins/nemo-deployments/tests/unit/backends/k8s/test_backend.py b/plugins/nemo-deployments/tests/unit/backends/k8s/test_backend.py index e5f09efc7c..12d516c037 100644 --- a/plugins/nemo-deployments/tests/unit/backends/k8s/test_backend.py +++ b/plugins/nemo-deployments/tests/unit/backends/k8s/test_backend.py @@ -59,23 +59,26 @@ def test_effective_namespace_ignores_blank_pod_namespace(monkeypatch: pytest.Mon @pytest.mark.asyncio -async def test_create_resolves_secrets_before_compiling_workload( +async def test_create_passes_secret_env_and_unmodified_config( k8s_backend: K8sDeploymentBackend, mock_entities: AsyncMock, mock_sdk: MagicMock, ) -> None: + # k8s keeps the stored config's secret_ref env vars intact and passes the + # resolved secret values separately as secret_env (mounted via a managed + # Secret + envFrom), so plaintext never lands in the pod manifest. stored = DeploymentConfig( name="cfg", workspace="default", containers=[Container(name="server", image="example:latest")], ) - resolved = stored.model_copy(deep=True) + secret_env = {"APP_TOKEN": "app-token-value"} mock_entities.get.return_value = stored with ( patch( - "nemo_deployments_plugin.backends.k8s.backend.resolve_deployment_config_secrets", - AsyncMock(return_value=resolved), + "nemo_deployments_plugin.backends.k8s.backend.resolve_deployment_secret_env", + AsyncMock(return_value=secret_env), ) as resolve_mock, patch( "nemo_deployments_plugin.backends.k8s.backend.deployment_ops.create_deployment", @@ -92,4 +95,5 @@ async def test_create_resolves_secrets_before_compiling_workload( resolve_mock.assert_awaited_once_with(mock_sdk, stored) assert create_mock.await_args is not None - assert create_mock.await_args.kwargs["config"] is resolved + assert create_mock.await_args.kwargs["config"] is stored + assert create_mock.await_args.kwargs["secret_env"] == secret_env diff --git a/plugins/nemo-deployments/tests/unit/backends/k8s/test_compiler.py b/plugins/nemo-deployments/tests/unit/backends/k8s/test_compiler.py index ae79541679..d5f035cc26 100644 --- a/plugins/nemo-deployments/tests/unit/backends/k8s/test_compiler.py +++ b/plugins/nemo-deployments/tests/unit/backends/k8s/test_compiler.py @@ -13,6 +13,9 @@ DeploymentConfigError, _build_probe, build_configmap_body, + build_env_vars, + build_secret_body, + build_secret_env_from, compile_workload, configmap_data_key, validate_config_for_deployment, @@ -20,14 +23,17 @@ ) from nemo_deployments_plugin.backends.k8s.deployments import build_deployment_body from nemo_deployments_plugin.backends.k8s.jobs import build_job_body +from nemo_deployments_plugin.backends.labels import k8s_deployment_secret_name from nemo_deployments_plugin.entities import ( ConfigFile, Container, ContainerPort, + EnvVar, ExecAction, HTTPGetAction, K8sDeploymentConfig, Probe, + SecretRef, ) from nemo_platform_plugin.config import ImagePullSecret @@ -301,3 +307,95 @@ def test_validate_rejects_duplicate_listen_ports() -> None: def test_build_configmap_body_none_when_empty() -> None: assert build_configmap_body(workspace="default", deployment_name="task", labels={}, config_files=[]) is None + + +def test_build_secret_body_none_when_empty() -> None: + assert build_secret_body(workspace="default", deployment_name="task", labels={}, secret_env={}) is None + + +def test_build_secret_body_holds_values_and_labels() -> None: + labels = {"managed-by": "nemo-deployments", "nemo.nvidia.com/deployment-name": "task"} + secret = build_secret_body( + workspace="default", + deployment_name="task", + labels=labels, + secret_env={"APP_TOKEN": "value-a", "OTHER": "value-b"}, + ) + serialized = _serialized(secret) + assert serialized["kind"] == "Secret" + assert serialized["type"] == "Opaque" + assert serialized["metadata"]["name"] == k8s_deployment_secret_name("default", "task") + assert serialized["metadata"]["labels"] == labels + assert serialized["stringData"] == {"APP_TOKEN": "value-a", "OTHER": "value-b"} + + +def test_build_env_vars_skips_secret_ref_entries() -> None: + container = Container( + name="main", + image="alpine", + env=[ + EnvVar(name="PLAIN", value="v"), + EnvVar(name="APP_TOKEN", secretRef=SecretRef(workspace="default", name="app-token")), + ], + ) + env = build_env_vars(container) + names = {item.name for item in env} + assert names == {"PLAIN"} + + +def test_build_secret_env_from_empty_without_secret() -> None: + assert build_secret_env_from(None) == [] + + +def test_build_secret_env_from_projects_secret_ref() -> None: + env_from = build_secret_env_from("dep-sec-abc") + serialized = [_serialized(item) for item in env_from] + assert serialized == [{"secretRef": {"name": "dep-sec-abc"}}] + + +def test_compile_workload_mounts_secret_via_env_from() -> None: + config = sample_always_config().model_copy( + update={ + "containers": [ + Container( + name="main", + image="nginx:alpine", + ports=[ContainerPort(name="http", containerPort=8080)], + env=[EnvVar(name="APP_TOKEN", secretRef=SecretRef(workspace="default", name="app-token"))], + ) + ] + } + ) + compiled = compile_workload( + config=config, + workspace="default", + deployment_name="task", + labels={"managed-by": "nemo-deployments"}, + k8s_config=None, + pod_restart_policy="Always", + secret_env={"APP_TOKEN": "app-token-value"}, + ) + assert compiled.secret_body is not None + assert compiled.secret_name == k8s_deployment_secret_name("default", "task") + pod_spec = _serialized(compiled.pod_spec_kwargs) + main = pod_spec["containers"][0] + assert main["envFrom"] == [{"secretRef": {"name": k8s_deployment_secret_name("default", "task")}}] + # The secret value never appears as a plaintext env var in the pod spec. + assert "env" not in main or all(entry.get("value") != "app-token-value" for entry in main["env"]) + + +def test_compile_workload_no_secret_when_secret_env_empty() -> None: + config = sample_always_config() + compiled = compile_workload( + config=config, + workspace="default", + deployment_name="task", + labels={"managed-by": "nemo-deployments"}, + k8s_config=None, + pod_restart_policy="Always", + secret_env={}, + ) + assert compiled.secret_body is None + assert compiled.secret_name is None + pod_spec = _serialized(compiled.pod_spec_kwargs) + assert "envFrom" not in pod_spec["containers"][0] diff --git a/plugins/nemo-deployments/tests/unit/backends/k8s/test_deployments.py b/plugins/nemo-deployments/tests/unit/backends/k8s/test_deployments.py index 03b24e39f6..86ca52f195 100644 --- a/plugins/nemo-deployments/tests/unit/backends/k8s/test_deployments.py +++ b/plugins/nemo-deployments/tests/unit/backends/k8s/test_deployments.py @@ -26,8 +26,16 @@ MANAGED_BY_KEY, k8s_deployment_configmap_name, k8s_deployment_resource_name, + k8s_deployment_secret_name, +) +from nemo_deployments_plugin.entities import ( + ConfigFile, + Container, + ContainerPort, + DeploymentConfig, + EnvVar, + SecretRef, ) -from nemo_deployments_plugin.entities import ConfigFile, Container, ContainerPort from nemo_platform_plugin.entity_client import NemoEntityNotFoundError @@ -264,6 +272,152 @@ async def test_create_deployment_adopted_service_failure_keeps_configmap( mock_k8s_clients.core_v1.delete_namespaced_config_map.assert_not_called() +def _config_with_secret_env() -> DeploymentConfig: + base = sample_always_config() + container = base.containers[0].model_copy( + update={"env": [EnvVar(name="APP_TOKEN", secretRef=SecretRef(workspace="default", name="app-token"))]} + ) + return base.model_copy(update={"containers": [container]}) + + +@pytest.mark.asyncio +async def test_create_deployment_creates_managed_secret( + deployment_ops_clients: MagicMock, mock_k8s_clients: MagicMock +) -> None: + config = _config_with_secret_env() + mock_k8s_clients.apps_v1.create_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.apps_v1.read_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.core_v1.list_namespaced_pod.return_value = MagicMock(items=[]) + + update = await deployment_ops.create_deployment( + deployment_ops_clients, + default_namespace="default", + workspace="default", + name="task", + config_name="config1", + labels={}, + backend_config={}, + config=config, + secret_env={"APP_TOKEN": "app-token-value"}, + ) + + assert update.status != "FAILED" + mock_k8s_clients.core_v1.create_namespaced_secret.assert_called_once() + secret_body = mock_k8s_clients.core_v1.create_namespaced_secret.call_args.kwargs["body"] + assert secret_body.metadata.name == k8s_deployment_secret_name("default", "task") + assert secret_body.string_data == {"APP_TOKEN": "app-token-value"} + + +@pytest.mark.asyncio +async def test_create_deployment_no_secret_when_secret_env_empty( + deployment_ops_clients: MagicMock, mock_k8s_clients: MagicMock +) -> None: + config = sample_always_config() + mock_k8s_clients.apps_v1.create_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.apps_v1.read_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.core_v1.list_namespaced_pod.return_value = MagicMock(items=[]) + + await deployment_ops.create_deployment( + deployment_ops_clients, + default_namespace="default", + workspace="default", + name="task", + config_name="config1", + labels={}, + backend_config={}, + config=config, + secret_env={}, + ) + + mock_k8s_clients.core_v1.create_namespaced_secret.assert_not_called() + + +@pytest.mark.asyncio +async def test_create_deployment_rolls_back_secret_when_service_create_fails( + deployment_ops_clients: MagicMock, mock_k8s_clients: MagicMock +) -> None: + config = _config_with_secret_env() + mock_k8s_clients.apps_v1.create_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.apps_v1.read_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.core_v1.create_namespaced_service.side_effect = ApiException(status=500) + identity_labels = always_identity_labels(backoff_limit=config.backoff_limit) + mock_k8s_clients.core_v1.read_namespaced_secret.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels=identity_labels), + ) + + update = await deployment_ops.create_deployment( + deployment_ops_clients, + default_namespace="default", + workspace="default", + name="task", + config_name="config1", + labels={}, + backend_config={}, + config=config, + secret_env={"APP_TOKEN": "app-token-value"}, + ) + + assert update.status == "FAILED" + mock_k8s_clients.core_v1.create_namespaced_secret.assert_called_once() + mock_k8s_clients.core_v1.delete_namespaced_secret.assert_called_once() + + +@pytest.mark.asyncio +async def test_delete_deployment_removes_managed_secret( + deployment_ops_clients: MagicMock, mock_k8s_clients: MagicMock +) -> None: + identity_labels = always_identity_labels() + mock_k8s_clients.apps_v1.read_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.core_v1.read_namespaced_config_map.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels=identity_labels), + ) + mock_k8s_clients.core_v1.read_namespaced_secret.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels=identity_labels), + ) + + update = await deployment_ops.delete_deployment( + deployment_ops_clients, + default_namespace="default", + workspace="default", + name="task", + backend_config={}, + expected_labels=identity_labels, + ) + + assert update.status == "SUCCEEDED" + mock_k8s_clients.core_v1.delete_namespaced_secret.assert_called_once_with( + name=k8s_deployment_secret_name("default", "task"), + namespace="default", + _request_timeout=mock_k8s_clients.request_timeout, + ) + + +@pytest.mark.asyncio +async def test_delete_deployment_skips_foreign_secret( + deployment_ops_clients: MagicMock, mock_k8s_clients: MagicMock +) -> None: + identity_labels = always_identity_labels() + mock_k8s_clients.apps_v1.read_namespaced_deployment.return_value = mock_deployment() + mock_k8s_clients.core_v1.read_namespaced_config_map.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels=identity_labels), + ) + # A secret with mismatched labels must not be deleted. + mock_k8s_clients.core_v1.read_namespaced_secret.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels={MANAGED_BY_KEY: "other-plugin"}), + ) + + await deployment_ops.delete_deployment( + deployment_ops_clients, + default_namespace="default", + workspace="default", + name="task", + backend_config={}, + expected_labels=identity_labels, + ) + + mock_k8s_clients.core_v1.delete_namespaced_secret.assert_not_called() + + @pytest.mark.asyncio async def test_read_status_accepts_init_containers( k8s_backend, mock_k8s_clients: MagicMock, mock_entities: AsyncMock diff --git a/plugins/nemo-deployments/tests/unit/backends/k8s/test_jobs.py b/plugins/nemo-deployments/tests/unit/backends/k8s/test_jobs.py index 7d373641b1..2036cc6b7a 100644 --- a/plugins/nemo-deployments/tests/unit/backends/k8s/test_jobs.py +++ b/plugins/nemo-deployments/tests/unit/backends/k8s/test_jobs.py @@ -3,6 +3,7 @@ from __future__ import annotations +from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock import pytest @@ -11,8 +12,9 @@ from nemo_deployments_plugin.backends.k8s import jobs as job_ops from nemo_deployments_plugin.backends.k8s.client import KubernetesClients from nemo_deployments_plugin.backends.k8s.jobs import job_backoff_limit, trim_log_text, validate_config_for_job -from nemo_deployments_plugin.backends.labels import MANAGED_BY_KEY +from nemo_deployments_plugin.backends.labels import MANAGED_BY_KEY, k8s_deployment_secret_name from nemo_deployments_plugin.constants import MANAGED_BY_LABEL +from nemo_deployments_plugin.entities import DeploymentConfig, EnvVar, SecretRef from nemo_deployments_plugin.types import RestartPolicy from nemo_platform_plugin.entity_client import NemoEntityNotFoundError @@ -210,6 +212,67 @@ async def test_delete_job_rejects_foreign(job_ops_clients: MagicMock, mock_k8s_c mock_k8s_clients.batch_v1.delete_namespaced_job.assert_not_called() +def _job_config_with_secret_env() -> DeploymentConfig: + config = sample_config(restart_policy="Never") + config.containers[0].env = [EnvVar(name="APP_TOKEN", secretRef=SecretRef(workspace="default", name="app-token"))] + return config + + +@pytest.mark.asyncio +async def test_create_job_creates_managed_secret(job_ops_clients: MagicMock, mock_k8s_clients: MagicMock) -> None: + config = _job_config_with_secret_env() + mock_k8s_clients.batch_v1.create_namespaced_job.return_value = mock_job(active=1) + + await job_ops.create_job( + job_ops_clients, + default_namespace="default", + workspace="default", + name="task", + config_name="config1", + labels={}, + backend_config={}, + config=config, + secret_env={"APP_TOKEN": "app-token-value"}, + ) + + mock_k8s_clients.core_v1.create_namespaced_secret.assert_called_once() + secret_body = mock_k8s_clients.core_v1.create_namespaced_secret.call_args.kwargs["body"] + assert secret_body.metadata.name == k8s_deployment_secret_name("default", "task") + assert secret_body.string_data == {"APP_TOKEN": "app-token-value"} + container = mock_k8s_clients.batch_v1.create_namespaced_job.call_args.kwargs["body"].spec.template.spec.containers[ + 0 + ] + assert container.env_from[0].secret_ref.name == k8s_deployment_secret_name("default", "task") + + +@pytest.mark.asyncio +async def test_delete_job_removes_managed_secret(job_ops_clients: MagicMock, mock_k8s_clients: MagicMock) -> None: + identity_labels = job_identity_labels() + mock_k8s_clients.batch_v1.read_namespaced_job.return_value = mock_job(complete=True) + mock_k8s_clients.core_v1.read_namespaced_config_map.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels=identity_labels), + ) + mock_k8s_clients.core_v1.read_namespaced_secret.return_value = SimpleNamespace( + metadata=SimpleNamespace(labels=identity_labels), + ) + + update = await job_ops.delete_job( + job_ops_clients, + default_namespace="default", + workspace="default", + name="task", + backend_config={}, + expected_labels=identity_labels, + ) + + assert update.status == "SUCCEEDED" + mock_k8s_clients.core_v1.delete_namespaced_secret.assert_called_once_with( + name=k8s_deployment_secret_name("default", "task"), + namespace="default", + _request_timeout=mock_k8s_clients.request_timeout, + ) + + @pytest.mark.asyncio async def test_list_managed_deployment_names(k8s_backend, mock_k8s_clients: MagicMock) -> None: listed = MagicMock() diff --git a/plugins/nemo-deployments/tests/unit/test_deployment_config_secret_refs.py b/plugins/nemo-deployments/tests/unit/test_deployment_config_secret_refs.py index ea5ee59ca8..65d749a701 100644 --- a/plugins/nemo-deployments/tests/unit/test_deployment_config_secret_refs.py +++ b/plugins/nemo-deployments/tests/unit/test_deployment_config_secret_refs.py @@ -7,28 +7,34 @@ import pytest from nemo_deployments_plugin.api.v2.deployment_configs import create_deployment_config -from nemo_deployments_plugin.entities import DeploymentConfig +from nemo_deployments_plugin.entities import DeploymentConfig, SecretRef from nemo_deployments_plugin.schema import CreateDeploymentConfigRequest, RequestContainer, RequestEnvVar from pydantic import ValidationError -def test_request_env_var_rejects_secret_ref() -> None: - with pytest.raises(ValidationError, match="secretRef"): - RequestEnvVar.model_validate( - { - "name": "NGC_API_KEY", - "secretRef": {"workspace": "system", "name": "ngc-api-key"}, - } - ) +def test_request_env_var_accepts_secret_ref() -> None: + env = RequestEnvVar.model_validate( + { + "name": "APP_TOKEN", + "secretRef": {"workspace": "default", "name": "app-token"}, + } + ) + assert env.secret_ref == SecretRef(workspace="default", name="app-token") + entity = env.to_entity() + assert entity.secret_ref == SecretRef(workspace="default", name="app-token") + assert entity.value is None -def test_request_env_var_schema_forbids_value_and_value_from_together() -> None: +def test_request_env_var_schema_forbids_multiple_sources_together() -> None: schema = RequestEnvVar.model_json_schema(by_alias=True) - assert schema["not"] == {"required": ["value", "valueFrom"]} + assert schema["not"] == {"required": ["value", "valueFrom", "secretRef"]} with pytest.raises(ValidationError, match="only one"): RequestEnvVar(name="FOO", value="bar", valueFrom={"fieldRef": {"fieldPath": "metadata.name"}}) + with pytest.raises(ValidationError, match="only one"): + RequestEnvVar(name="FOO", value="bar", secretRef=SecretRef(workspace="default", name="app-token")) + def test_create_request_accepts_plain_env_values() -> None: body = CreateDeploymentConfigRequest( diff --git a/plugins/nemo-deployments/tests/unit/test_secrets.py b/plugins/nemo-deployments/tests/unit/test_secrets.py index ebeb2329ea..62d0a80479 100644 --- a/plugins/nemo-deployments/tests/unit/test_secrets.py +++ b/plugins/nemo-deployments/tests/unit/test_secrets.py @@ -13,6 +13,7 @@ SecretResolutionError, platform_ngc_secret_ref, resolve_deployment_config_secrets, + resolve_deployment_secret_env, ) from nemo_platform_plugin.client.errors import NemoClientError, NotFoundError @@ -151,18 +152,131 @@ async def test_secret_access_error_does_not_fall_back_to_process_environment( @pytest.mark.asyncio -async def test_resolve_deployment_config_secrets_rejects_untrusted_reference() -> None: +async def test_resolve_deployment_config_secrets_resolves_arbitrary_reference() -> None: config = _config( - SecretRef(workspace="system", name="unrelated-secret"), - env_name="EXFILTRATED_VALUE", + SecretRef(workspace="default", name="app-token"), + env_name="APP_TOKEN", ) + response = MagicMock() + response.data.return_value = SimpleNamespace(value="app-token-value") + secrets = AsyncMock() + secrets.access_secret.return_value = response + platform = SimpleNamespace( + ngc_api_key_secret="system/ngc-api-key", + ngc_api_key_env_var="NGC_API_KEY", + ) + + with ( + patch("nemo_deployments_plugin.secrets.client_from_platform", return_value=secrets), + patch("nemo_deployments_plugin.secrets.get_platform_config", return_value=platform), + ): + resolved = await resolve_deployment_config_secrets(MagicMock(), config) + + assert resolved.containers[0].env[0].value == "app-token-value" + assert resolved.containers[0].env[0].secret_ref is None + # The stored config is never mutated with a plaintext value. + assert config.containers[0].env[0].value is None + + +@pytest.mark.asyncio +async def test_resolve_deployment_config_secrets_missing_arbitrary_reference_raises() -> None: + config = _config( + SecretRef(workspace="default", name="app-token"), + env_name="APP_TOKEN", + ) + secrets = AsyncMock() + secrets.access_secret.side_effect = _not_found() platform = SimpleNamespace( ngc_api_key_secret="system/ngc-api-key", ngc_api_key_env_var="NGC_API_KEY", ) with ( + patch("nemo_deployments_plugin.secrets.client_from_platform", return_value=secrets), patch("nemo_deployments_plugin.secrets.get_platform_config", return_value=platform), - pytest.raises(SecretResolutionError, match="Unsupported"), + pytest.raises(SecretResolutionError, match="could not be resolved"), ): await resolve_deployment_config_secrets(MagicMock(), config) + + +@pytest.mark.asyncio +async def test_resolve_deployment_secret_env_collects_values_across_containers() -> None: + config = DeploymentConfig( + name="cfg", + workspace="default", + initContainers=[ + Container( + name="init", + image="busybox", + env=[EnvVar(name="INIT_TOKEN", secretRef=SecretRef(workspace="default", name="init-token"))], + ) + ], + containers=[ + Container( + name="server", + image="nvcr.io/nim/test:latest", + env=[ + EnvVar(name="APP_TOKEN", secretRef=SecretRef(workspace="default", name="app-token")), + EnvVar(name="PLAINTEXT", value="not-a-secret"), + ], + ) + ], + ) + + async def _access_secret(*, name: str, workspace: str) -> MagicMock: + response = MagicMock() + response.data.return_value = SimpleNamespace(value=f"{workspace}/{name}-value") + return response + + secrets = AsyncMock() + secrets.access_secret.side_effect = _access_secret + platform = SimpleNamespace( + ngc_api_key_secret="system/ngc-api-key", + ngc_api_key_env_var="NGC_API_KEY", + ) + + with ( + patch("nemo_deployments_plugin.secrets.client_from_platform", return_value=secrets), + patch("nemo_deployments_plugin.secrets.get_platform_config", return_value=platform), + ): + secret_env = await resolve_deployment_secret_env(MagicMock(), config) + + assert secret_env == { + "INIT_TOKEN": "default/init-token-value", + "APP_TOKEN": "default/app-token-value", + } + # The config's secret_ref env vars remain intact (mounted via envFrom, not plaintext). + assert config.containers[0].env[0].secret_ref == SecretRef(workspace="default", name="app-token") + + +@pytest.mark.asyncio +async def test_resolve_deployment_secret_env_empty_when_no_secret_refs() -> None: + config = DeploymentConfig( + name="cfg", + workspace="default", + containers=[Container(name="server", image="busybox", env=[EnvVar(name="PLAIN", value="v")])], + ) + secret_env = await resolve_deployment_secret_env(MagicMock(), config) + assert secret_env == {} + + +@pytest.mark.asyncio +async def test_resolve_deployment_secret_env_omits_unresolved_ngc_key( + monkeypatch: pytest.MonkeyPatch, +) -> None: + config = _config(SecretRef(workspace="system", name="ngc-api-key")) + secrets = AsyncMock() + secrets.access_secret.side_effect = _not_found() + monkeypatch.delenv("NGC_API_KEY", raising=False) + platform = SimpleNamespace( + ngc_api_key_secret="system/ngc-api-key", + ngc_api_key_env_var="NGC_API_KEY", + ) + + with ( + patch("nemo_deployments_plugin.secrets.client_from_platform", return_value=secrets), + patch("nemo_deployments_plugin.secrets.get_platform_config", return_value=platform), + ): + secret_env = await resolve_deployment_secret_env(MagicMock(), config) + + assert secret_env == {}