From cf2e01b9be2fd74774bf6b3ee268c652ac20ecb9 Mon Sep 17 00:00:00 2001 From: Jon Burdo Date: Fri, 1 May 2026 09:31:21 -0400 Subject: [PATCH] fix(async-upload): add TLS and auth config for base image pull on disconnected clusters The async upload job only passed push_args to skopeo for the destination registry, but the base image pull (e.g. busybox for ModelCar) had no TLS or auth configuration. This caused failures on disconnected clusters where the base image is mirrored to a registry with self-signed certs or requiring authentication. Add two new independent config flags for the base image pull: - MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_TLS_VERIFY (default: true) - MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_CREDENTIALS_PATH (default: none) Ref: RHOAIENG-60454 Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: Jon Burdo --- jobs/async-upload/README.md | 4 +- jobs/async-upload/job/config.py | 4 + jobs/async-upload/job/models.py | 2 + jobs/async-upload/job/upload.py | 9 +- .../samples/sample_job_s3_to_oci.yaml | 6 + jobs/async-upload/tests/test_config.py | 37 +++++++ jobs/async-upload/tests/test_upload.py | 104 ++++++++++++++++++ 7 files changed, 164 insertions(+), 2 deletions(-) diff --git a/jobs/async-upload/README.md b/jobs/async-upload/README.md index d0dd5e385c..ea972047f0 100644 --- a/jobs/async-upload/README.md +++ b/jobs/async-upload/README.md @@ -68,7 +68,9 @@ See asterisks below table for details | MODEL_SYNC_DESTINATION_OCI_REGISTRY | --destination-oci-registry | | ✅\+ | When --destination-type is "oci". Indicates which registry the creds belong to | | MODEL_SYNC_DESTINATION_OCI_USERNAME | --destination-oci-username | | ✅\+ | " | | MODEL_SYNC_DESTINATION_OCI_PASSWORD | --destination-oci-password | | ✅\+ | " | -| MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE | --destination-oci-base-image | busybox:latest | | When --destination-type is "oci". The image to use when pushing to an OCI registry | +| MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE | --destination-oci-base-image | busybox:latest | | When --destination-type is "oci". The base image to use when building the ModelCar OCI image | +| MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_TLS_VERIFY | --destination-oci-base-image-tls-verify | true | | When --destination-type is "oci". TLS verification when pulling the base image. Set to `false` for registries with self-signed certificates (e.g. disconnected clusters) | +| MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_CREDENTIALS_PATH | --destination-oci-base-image-credentials-path | | | When --destination-type is "oci". Path to an auth file (e.g. `.dockerconfigjson`) for pulling the base image from a private or mirrored registry | | MODEL_SYNC_DESTINATION_OCI_ENABLE_TLS_VERIFY | --destination-oci-enable-tls-verify | true | | When --destination-type is "oci". Specifies whether to use TLS when pushing to registry | | MODEL_SYNC_MODEL_UPLOAD_INTENT | --model-upload-intent | update_artifact | | The intent of the upload job. Options include `["create_model", "create_version", "update_artifact"]`. | | MODEL_SYNC_MODEL_ID | --model-id | | ✅\% | The `RegisteredModel.id` | diff --git a/jobs/async-upload/job/config.py b/jobs/async-upload/job/config.py index 83bb71ec06..9e4255b953 100644 --- a/jobs/async-upload/job/config.py +++ b/jobs/async-upload/job/config.py @@ -79,6 +79,8 @@ def _parser() -> cap.ArgumentParser: p.add_argument("--destination-oci-username") p.add_argument("--destination-oci-password") p.add_argument("--destination-oci-base-image", default="public.ecr.aws/docker/library/busybox:latest") + p.add_argument("--destination-oci-base-image-tls-verify", default=True, type=str2bool) + p.add_argument("--destination-oci-base-image-credentials-path", metavar="PATH") # The `type` converter is needed here to support env-based booleans # See: https://github.com/bw2/ConfigArgParse/tree/master?tab=readme-ov-file#special-values p.add_argument("--destination-oci-enable-tls-verify", default=True, type=str2bool) @@ -484,6 +486,8 @@ def get_config(argv: list[str] | None = None) -> AsyncUploadConfig: password=args.destination_oci_password, email=None, base_image=args.destination_oci_base_image, + base_image_tls_verify=args.destination_oci_base_image_tls_verify, + base_image_credentials_path=args.destination_oci_base_image_credentials_path, enable_tls_verify=args.destination_oci_enable_tls_verify, credentials_path=args.destination_oci_credentials_path ) diff --git a/jobs/async-upload/job/models.py b/jobs/async-upload/job/models.py index 815ad3abe8..66339b48f7 100644 --- a/jobs/async-upload/job/models.py +++ b/jobs/async-upload/job/models.py @@ -44,6 +44,8 @@ class OCIConfig(BaseModel): password: str | None = None email: str | None = None base_image: str = "public.ecr.aws/docker/library/busybox:latest" + base_image_tls_verify: bool = True + base_image_credentials_path: str | None = None enable_tls_verify: bool = True @model_validator(mode='after') diff --git a/jobs/async-upload/job/upload.py b/jobs/async-upload/job/upload.py index 1efb97a5a1..bb858a5fa5 100644 --- a/jobs/async-upload/job/upload.py +++ b/jobs/async-upload/job/upload.py @@ -28,12 +28,18 @@ def _get_upload_params(config: AsyncUploadConfig) -> S3Params | OCIParams: ) elif isinstance(destination_config, OCIStorageConfig): push_args = [] + pull_args = [] # Note: These are all skopeo args, see: https://github.com/containers/skopeo/blob/main/docs/skopeo-copy.1.md if not destination_config.enable_tls_verify: push_args.append("--dest-tls-verify=false") if destination_config.credentials_path: push_args.append("--authfile") push_args.append(destination_config.credentials_path) + if not destination_config.base_image_tls_verify: + pull_args.append("--src-tls-verify=false") + if destination_config.base_image_credentials_path: + pull_args.append("--authfile") + pull_args.append(destination_config.base_image_credentials_path) return OCIParams( base_image=destination_config.base_image, @@ -43,7 +49,8 @@ def _get_upload_params(config: AsyncUploadConfig) -> S3Params | OCIParams: oci_password=destination_config.password, # Same as the default backend, but with additional args included custom_oci_backend=utils._get_skopeo_backend( - push_args=push_args + pull_args=pull_args, + push_args=push_args, ), ) else: diff --git a/jobs/async-upload/samples/sample_job_s3_to_oci.yaml b/jobs/async-upload/samples/sample_job_s3_to_oci.yaml index 291194cd0e..fde420d520 100644 --- a/jobs/async-upload/samples/sample_job_s3_to_oci.yaml +++ b/jobs/async-upload/samples/sample_job_s3_to_oci.yaml @@ -113,6 +113,12 @@ spec: value: "/opt/creds/destination/.dockerconfigjson" - name: MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE value: "public.ecr.aws/docker/library/busybox:latest" + # Uncomment to disable TLS verification when pulling the base image (e.g. disconnected clusters with self-signed certs) + # - name: MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_TLS_VERIFY + # value: "false" + # Uncomment to provide auth for pulling the base image from a private/mirrored registry + # - name: MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_CREDENTIALS_PATH + # value: "/opt/creds/base-image/.dockerconfigjson" - name: MODEL_SYNC_DESTINATION_OCI_ENABLE_TLS_VERIFY value: "false" diff --git a/jobs/async-upload/tests/test_config.py b/jobs/async-upload/tests/test_config.py index 414fecc403..d95a64b1a4 100644 --- a/jobs/async-upload/tests/test_config.py +++ b/jobs/async-upload/tests/test_config.py @@ -511,3 +511,40 @@ def test_oci_credentials_file_loading( assert config.destination.email == expected["email"] +def test_base_image_pull_flags_defaults( + source_s3_env_vars, destination_oci_env_vars, update_artifact_intent_env_vars +): + """Test that base image pull flags default to secure values""" + config = get_config([]) + + assert isinstance(config.destination, OCIStorageConfig) + assert config.destination.base_image_tls_verify is True + assert config.destination.base_image_credentials_path is None + + +def test_base_image_pull_flags_from_env( + source_s3_env_vars, destination_oci_env_vars, update_artifact_intent_env_vars, monkeypatch +): + """Test that base image pull flags can be set via environment variables""" + monkeypatch.setenv("MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_TLS_VERIFY", "false") + monkeypatch.setenv("MODEL_SYNC_DESTINATION_OCI_BASE_IMAGE_CREDENTIALS_PATH", "/etc/pull-secret/.dockerconfigjson") + + config = get_config([]) + + assert isinstance(config.destination, OCIStorageConfig) + assert config.destination.base_image_tls_verify is False + assert config.destination.base_image_credentials_path == "/etc/pull-secret/.dockerconfigjson" + + +def test_base_image_pull_flags_from_params( + source_s3_env_vars, destination_oci_env_vars, update_artifact_intent_env_vars +): + """Test that base image pull flags can be set via CLI parameters""" + config = get_config([ + "--destination-oci-base-image-tls-verify", "false", + "--destination-oci-base-image-credentials-path", "/tmp/auth.json", + ]) + + assert isinstance(config.destination, OCIStorageConfig) + assert config.destination.base_image_tls_verify is False + assert config.destination.base_image_credentials_path == "/tmp/auth.json" diff --git a/jobs/async-upload/tests/test_upload.py b/jobs/async-upload/tests/test_upload.py index e3cfe60d3e..b7e96417cd 100644 --- a/jobs/async-upload/tests/test_upload.py +++ b/jobs/async-upload/tests/test_upload.py @@ -13,6 +13,110 @@ UpdateArtifactIntent ) +class TestGetUploadParamsOCIPullArgs: + """Test cases for base image pull_args passed to _get_skopeo_backend""" + + @patch("job.upload.utils._get_skopeo_backend") + def test_pull_args_with_tls_disabled_and_credentials(self, mock_get_backend): + mock_get_backend.return_value = Mock() + + config = AsyncUploadConfig( + source=S3StorageConfig( + bucket="src-bucket", key="src-key", + access_key_id="id", secret_access_key="secret", region="us-east-1" + ), + destination=OCIStorageConfig( + uri="registry.internal/model:v1", registry="registry.internal", + username="user", password="pass", + base_image="registry.internal/busybox:latest", + base_image_tls_verify=False, + base_image_credentials_path="/etc/pull-secret/.dockerconfigjson", + enable_tls_verify=False, + credentials_path="/tmp/push-creds" + ), + model=ModelConfig(intent=UpdateArtifactIntent(artifact_id="123")), + storage=StorageConfig(path="/tmp/test"), + registry=RegistryConfig(server_address="test-server") + ) + + _get_upload_params(config) + + mock_get_backend.assert_called_once() + call_kwargs = mock_get_backend.call_args + pull_args = call_kwargs.kwargs.get("pull_args") or call_kwargs[1].get("pull_args") + push_args = call_kwargs.kwargs.get("push_args") or call_kwargs[1].get("push_args") + + assert "--src-tls-verify=false" in pull_args + assert "--authfile" in pull_args + assert "/etc/pull-secret/.dockerconfigjson" in pull_args + assert "--dest-tls-verify=false" in push_args + assert "--authfile" in push_args + assert "/tmp/push-creds" in push_args + + @patch("job.upload.utils._get_skopeo_backend") + def test_pull_args_defaults_are_empty(self, mock_get_backend): + mock_get_backend.return_value = Mock() + + config = AsyncUploadConfig( + source=S3StorageConfig( + bucket="src-bucket", key="src-key", + access_key_id="id", secret_access_key="secret", region="us-east-1" + ), + destination=OCIStorageConfig( + uri="quay.io/org/model:v1", registry="quay.io", + username="user", password="pass", + base_image="quay.io/quay/busybox:latest", + ), + model=ModelConfig(intent=UpdateArtifactIntent(artifact_id="123")), + storage=StorageConfig(path="/tmp/test"), + registry=RegistryConfig(server_address="test-server") + ) + + _get_upload_params(config) + + mock_get_backend.assert_called_once() + call_kwargs = mock_get_backend.call_args + pull_args = call_kwargs.kwargs.get("pull_args") or call_kwargs[1].get("pull_args") + push_args = call_kwargs.kwargs.get("push_args") or call_kwargs[1].get("push_args") + + assert pull_args == [] + assert push_args == [] + + @patch("job.upload.utils._get_skopeo_backend") + def test_pull_args_independent_from_push_args(self, mock_get_backend): + """Base image pull settings are independent from destination push settings""" + mock_get_backend.return_value = Mock() + + config = AsyncUploadConfig( + source=S3StorageConfig( + bucket="src-bucket", key="src-key", + access_key_id="id", secret_access_key="secret", region="us-east-1" + ), + destination=OCIStorageConfig( + uri="registry.internal/model:v1", registry="registry.internal", + username="user", password="pass", + base_image="registry.internal/busybox:latest", + base_image_tls_verify=False, + base_image_credentials_path="/etc/pull-secret/.dockerconfigjson", + enable_tls_verify=True, + credentials_path=None, + ), + model=ModelConfig(intent=UpdateArtifactIntent(artifact_id="123")), + storage=StorageConfig(path="/tmp/test"), + registry=RegistryConfig(server_address="test-server") + ) + + _get_upload_params(config) + + call_kwargs = mock_get_backend.call_args + pull_args = call_kwargs.kwargs.get("pull_args") or call_kwargs[1].get("pull_args") + push_args = call_kwargs.kwargs.get("push_args") or call_kwargs[1].get("push_args") + + assert "--src-tls-verify=false" in pull_args + assert "--authfile" in pull_args + assert push_args == [] + + class TestGetUploadParams: """Test cases for _get_upload_params function"""