feat(notebook): add secret handling for private registry image pull (#1265)

* feat(notebook): add secret handling for private registry image pull

This secret is created by the admin deploying the platform and is
not user accessible.

* chore: update patch_sesssion for new image pull secret implementation

* fix: add missing None check for private registry secret handling

* chore: simplify secret return handling

When going through this if, the secret is created, thus returning early
is the right action.

Co-authored-by: Flora Thiebaut <flora@leafty.dev>

* chore: make created image pull secret adoption explicit

While this is the default value, this make the intention
clearer with regard to the use of a pre-created secret
such as the one used for the private image registry used
when building images from a private repository.

* feat(builds): validate private/public image prefix to be different

This avoids accidentally sending images built from
private repositories to the same registry used for
public repositories.

* feat: add tests for image prefix validation

* chore: fix format issues

---------

Co-authored-by: Flora Thiebaut <flora@leafty.dev>

Author: sgaist

bases/renku_data_services/data_api/app.py

@@ -222,6 +222,7 @@ def register_all_handlers(app: Sanic, dm: DependencyManager) -> Sanic:
         storage_repo=dm.storage_repo,
         user_repo=dm.kc_user_repo,
         git_repositories_repo=dm.git_repositories_repo,
+        builds_config=dm.config.builds,
     )
     platform_config = PlatformConfigBP(
         name="platform_config",

components/renku_data_services/notebooks/blueprints.py

@@ -32,6 +32,7 @@
 from renku_data_services.notebooks.image_check import ImageCheckRepository
 from renku_data_services.project.db import ProjectRepository, ProjectSessionSecretRepository
 from renku_data_services.repositories.db import GitRepositoriesRepository
+from renku_data_services.session.config import BuildsConfig
 from renku_data_services.session.db import SessionRepository
 from renku_data_services.storage.db import StorageRepository
 from renku_data_services.users.db import UserRepo
@@ -61,6 +62,7 @@ class NotebooksNewBP(CustomBlueprint):
     user_repo: UserRepo
     metrics: MetricsService
     git_repositories_repo: GitRepositoriesRepository
+    builds_config: BuildsConfig
 
     def start(self) -> BlueprintFactoryResponse:
         """Start a session with the new operator."""
@@ -92,6 +94,7 @@ async def _handler(
                 image_check_repo=self.image_check_repo,
                 data_source_repo=self.data_source_repo,
                 git_repositories_repo=self.git_repositories_repo,
+                builds_config=self.builds_config,
             )
             status = 201 if created else 200
             return json(session.as_apispec().model_dump(exclude_none=True, mode="json"), status)
@@ -164,6 +167,7 @@ async def _handler(
                 metrics=self.metrics,
                 image_check_repo=self.image_check_repo,
                 data_source_repo=self.data_source_repo,
+                builds_config=self.builds_config,
             )
             return json(new_session.as_apispec().model_dump(exclude_none=True, mode="json"))
 

components/renku_data_services/notebooks/core_sessions.py

@@ -105,6 +105,7 @@
 from renku_data_services.project.models import Project, SessionSecret
 from renku_data_services.repositories.db import GitRepositoriesRepository
 from renku_data_services.repositories.models import Metadata as RepoMetadata
+from renku_data_services.session.config import BuildsConfig
 from renku_data_services.session.db import SessionRepository
 from renku_data_services.session.models import SessionLauncher
 from renku_data_services.users.db import UserRepo
@@ -617,7 +618,8 @@ def __format_image_pull_secret(secret_name: str, access_token: str, registry_dom
             data={".dockerconfigjson": registry_secret},
             metadata=V1ObjectMeta(name=secret_name),
             type="kubernetes.io/dockerconfigjson",
-        )
+        ),
+        adopt=True,
     )
 
 
@@ -648,6 +650,7 @@ async def get_image_pull_secret(
     user: APIUser,
     internal_gitlab_user: APIUser,
     image_check_repo: ImageCheckRepository,
+    builds_config: BuildsConfig,
 ) -> ExtraSecret | None:
     """Get an image pull secret."""
 
@@ -657,6 +660,18 @@ async def get_image_pull_secret(
     if v2_secret:
         return v2_secret
 
+    if (
+        builds_config.enabled
+        and builds_config.build_output_private_image_prefix is not None
+        and image.startswith(builds_config.build_output_private_image_prefix)
+    ):
+        return ExtraSecret(
+            V1Secret(
+                metadata=V1ObjectMeta(name=builds_config.pull_private_image_secret_name),
+            ),
+            adopt=False,
+        )
+
     if (
         nb_config.enable_internal_gitlab
         and isinstance(user, AuthenticatedAPIUser)
@@ -740,6 +755,7 @@ async def start_session(
     image_check_repo: ImageCheckRepository,
     data_source_repo: DataSourceRepository,
     git_repositories_repo: GitRepositoriesRepository,
+    builds_config: BuildsConfig,
 ) -> tuple[AmaltheaSessionV1Alpha1, bool]:
     """Start an Amalthea session.
 
@@ -910,6 +926,7 @@ async def start_session(
         user=user,
         internal_gitlab_user=internal_gitlab_user,
         image_check_repo=image_check_repo,
+        builds_config=builds_config,
     )
     if image_secret:
         session_extras = session_extras.concat(SessionExtraResources(secrets=[image_secret]))
@@ -969,7 +986,9 @@ async def start_session(
         metadata=Metadata(name=server_name, annotations=annotations),
         spec=AmaltheaSessionSpec(
             location=session_location,
-            imagePullSecrets=[ImagePullSecret(name=image_secret.name, adopt=True)] if image_secret else [],
+            imagePullSecrets=[ImagePullSecret(name=image_secret.name, adopt=image_secret.adopt)]
+            if image_secret
+            else [],
             codeRepositories=[],
             hibernated=False,
             reconcileStrategy=ReconcileStrategy.whenFailedOrHibernated,
@@ -1069,6 +1088,7 @@ async def patch_session(
     image_check_repo: ImageCheckRepository,
     data_source_repo: DataSourceRepository,
     metrics: MetricsService,
+    builds_config: BuildsConfig,
 ) -> AmaltheaSessionV1Alpha1:
     """Patch an Amalthea session."""
     session = await nb_config.k8s_v2_client.get_session(session_id, user.id)
@@ -1220,6 +1240,7 @@ async def patch_session(
         image_check_repo=image_check_repo,
         user=user,
         internal_gitlab_user=internal_gitlab_user,
+        builds_config=builds_config,
     )
     if image_pull_secret:
         session_extras = session_extras.concat(SessionExtraResources(secrets=[image_pull_secret]))

components/renku_data_services/session/config.py

@@ -5,6 +5,7 @@
 from datetime import timedelta
 
 from pydantic import ValidationError as PydanticValidationError
+from sanic_ext.exceptions import ValidationError
 
 from renku_data_services.app_config import logging
 from renku_data_services.session import crs as session_crs
@@ -38,6 +39,7 @@ class BuildsConfig:
     build_platform_overrides: dict[str, BuildPlatformOverrides] | None = None
     push_secret_name: str | None = None
     push_private_secret_name: str | None = None
+    pull_private_image_secret_name: str | None = None
     buildrun_retention_after_failed: timedelta | None = None
     buildrun_retention_after_succeeded: timedelta | None = None
     buildrun_build_timeout: timedelta | None = None
@@ -50,6 +52,14 @@ def from_env(cls) -> "BuildsConfig":
         enabled = os.environ.get("IMAGE_BUILDERS_ENABLED", "false").lower() == "true"
         build_output_image_prefix = os.environ.get("BUILD_OUTPUT_IMAGE_PREFIX")
         build_output_private_image_prefix = os.environ.get("BUILD_OUTPUT_PRIVATE_IMAGE_PREFIX")
+
+        if (
+            build_output_image_prefix is not None
+            and build_output_private_image_prefix is not None
+            and build_output_image_prefix == build_output_private_image_prefix
+        ):
+            raise ValidationError("Public and private builds cannot use the same image prefix")
+
         build_builder_image = os.environ.get("BUILD_BUILDER_IMAGE")
         build_run_image = os.environ.get("BUILD_RUN_IMAGE")
         build_strategy_name = os.environ.get("BUILD_STRATEGY_NAME")
@@ -60,6 +70,7 @@ def from_env(cls) -> "BuildsConfig":
             )
         push_secret_name = os.environ.get("BUILD_PUSH_SECRET_NAME")
         push_private_secret_name = os.environ.get("BUILD_PUSH_PRIVATE_SECRET_NAME")
+        pull_private_image_secret_name = os.environ.get("BUILD_PULL_PRIVATE_SECRET_NAME")
         buildrun_retention_after_failed_seconds = int(os.environ.get("BUILD_RUN_RETENTION_AFTER_FAILED_SECONDS") or "0")
         buildrun_retention_after_failed = (
             timedelta(seconds=buildrun_retention_after_failed_seconds)
@@ -134,6 +145,7 @@ def from_env(cls) -> "BuildsConfig":
             build_platform_overrides=build_platform_overrides,
             push_secret_name=push_secret_name or None,
             push_private_secret_name=push_private_secret_name or None,
+            pull_private_image_secret_name=pull_private_image_secret_name or None,
             buildrun_retention_after_failed=buildrun_retention_after_failed,
             buildrun_retention_after_succeeded=buildrun_retention_after_succeeded,
             buildrun_build_timeout=buildrun_build_timeout,

test/bases/renku_data_services/data_api/test_sessions.py

@@ -7,11 +7,13 @@
 import pytest
 from kr8s.objects import new_class
 from pytest import FixtureRequest
+from sanic_ext.exceptions import ValidationError
 from sanic_testing.testing import SanicASGITestClient, TestingResponse
 from syrupy.filters import props
 
 from renku_data_services import errors
 from renku_data_services.data_api.dependencies import DependencyManager
+from renku_data_services.session.config import BuildsConfig
 from renku_data_services.session.constants import BUILD_RUN_GVK
 from renku_data_services.session.models import EnvVar
 from renku_data_services.users.models import UserInfo
@@ -55,6 +57,28 @@ def cleanup():
     return launch_session_helper
 
 
+@pytest.mark.parametrize(
+    "prefix,private_prefix,must_raise",
+    [
+        ("dummy/image-prefix", "dummy/private-image-prefix", False),
+        (None, "dummy/private-image-prefix", False),
+        ("dummy/image-prefix", None, False),
+        ("dummy/private-image-prefix", "dummy/private-image-prefix", True),
+    ],
+)
+def test_same_build_image_prefix(monkeypatch, prefix, private_prefix, must_raise) -> None:
+    if prefix is not None:
+        monkeypatch.setenv("BUILD_OUTPUT_IMAGE_PREFIX", prefix)
+    if private_prefix is not None:
+        monkeypatch.setenv("BUILD_OUTPUT_PRIVATE_IMAGE_PREFIX", private_prefix)
+
+    if must_raise:
+        with pytest.raises(ValidationError):
+            _ = BuildsConfig.from_env()
+    else:
+        _ = BuildsConfig.from_env()
+
+
 @pytest.mark.asyncio
 async def test_get_all_session_environments(
     sanic_client: SanicASGITestClient, unauthorized_headers, create_session_environment, snapshot