From fc9af90ec678bed34ecb120a09f0d5236dada9a6 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 2 Sep 2026 12:45:52 +0300 Subject: [PATCH 1/4] OSAC-4488: harden caas conftest token-script and document fixtures Quote the namespace and service-account values with shlex.quote in the oc-create-token command passed to `osac login --token-script`. The osac CLI executes that string in a shell, so unquoted values were a shell-injection vector. Add docstrings to the cli, cluster_template, pull_secret_path, and ssh_public_key_path fixtures. Assisted-by: Claude Code Signed-off-by: Elad Tabak --- tests/e2e/caas/conftest.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/e2e/caas/conftest.py b/tests/e2e/caas/conftest.py index c8f19c0068..4ac0f792f4 100644 --- a/tests/e2e/caas/conftest.py +++ b/tests/e2e/caas/conftest.py @@ -1,6 +1,7 @@ from __future__ import annotations import os +import shlex from collections.abc import Iterator import pytest @@ -17,10 +18,11 @@ # in the shared tenant, which the storage controller skips. @pytest.fixture(scope="session") def cli(namespace: str, fulfillment_address: str, service_account: str) -> Iterator[OsacCLI]: + """Session-scoped osac CLI authenticated with a service-account token.""" instance = OsacCLI( binary=env("OSAC_CLI_PATH", "osac"), address=f"https://{fulfillment_address.rsplit(':', 1)[0]}", - token_script=f"oc create token -n {namespace} {service_account} --as system:admin", + token_script=f"oc create token -n {shlex.quote(namespace)} {shlex.quote(service_account)} --as system:admin", namespace=namespace, ) yield instance @@ -29,14 +31,17 @@ def cli(namespace: str, fulfillment_address: str, service_account: str) -> Itera @pytest.fixture(scope="session") def cluster_template() -> str: + """CaaS cluster template name, overridable via OSAC_CLUSTER_TEMPLATE.""" return env("OSAC_CLUSTER_TEMPLATE", "ocp-ci-small") @pytest.fixture(scope="session") def pull_secret_path() -> str: + """Filesystem path to the OCP pull secret (OSAC_PULL_SECRET_PATH).""" return env("OSAC_PULL_SECRET_PATH") @pytest.fixture(scope="session") def ssh_public_key_path() -> str: + """Filesystem path to the SSH public key, default ~/.ssh/id_rsa.pub (OSAC_SSH_PUBLIC_KEY_PATH).""" return env("OSAC_SSH_PUBLIC_KEY_PATH", os.path.expanduser("~/.ssh/id_rsa.pub")) From dc20d1739ef2808fc076754ef0472314d5ab3f6e Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 2 Sep 2026 12:45:58 +0300 Subject: [PATCH 2/4] OSAC-4488: fix caas cluster-create test edge cases and cleanup Assert node_sets is non-empty before deriving the component count and scaling a worker set, so an empty spec surfaces a clear failure instead of an opaque StopIteration. Delete the ClusterVersion resources created by test_cluster_create_with_version and test_cluster_create_rejected_for_invalid_version in a finally block, so repeated runs do not leave stale versions on the shared cluster. Add a docstring to test_cluster_create. Assisted-by: Claude Code Signed-off-by: Elad Tabak --- tests/e2e/caas/test_cluster_create.py | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/tests/e2e/caas/test_cluster_create.py b/tests/e2e/caas/test_cluster_create.py index 79b3fc5530..b256ab09ce 100644 --- a/tests/e2e/caas/test_cluster_create.py +++ b/tests/e2e/caas/test_cluster_create.py @@ -39,6 +39,9 @@ def test_cluster_create( ssh_public_key_path: str, metering: MeteringCollector, ) -> None: + """Verify the full CaaS cluster lifecycle: create, provision to Ready, version and + releaseImage propagation to the HostedCluster, N+1 metering heartbeat decomposition, + worker scale-up reflected in updated.v1 metering, and deletion.""" name = unique_name("e2e-cluster") uuid = cli.create_cluster( name=name, @@ -85,6 +88,7 @@ def test_cluster_create( # Derive expected N+1 count from cluster spec node_sets = cluster.get("object", {}).get("spec", {}).get("nodeSets", {}) + assert node_sets, "Cluster spec should have at least one node set for the scaling test" expected_components = 1 + len(node_sets) # Verify N+1 heartbeat decomposition @@ -204,6 +208,7 @@ def test_cluster_create_with_version( finally: with contextlib.suppress(subprocess.CalledProcessError): cli.delete_cluster(uuid=uuid) + private_grpc.call_unchecked(service="osac.private.v1.ClusterVersions/Delete", data={"id": version["id"]}) def test_cluster_create_rejected_for_invalid_version( @@ -223,14 +228,18 @@ def _create_with_version(version_name: str) -> tuple[str, int]: data={"object": {"spec": {"template": {"name": cluster_template}, "version": {"name": version_name}}}}, ) - output, rc = _create_with_version(disabled["name"]) - assert rc != 0, f"Expected create to reject disabled version, got: {output}" - assert "disabled" in output.lower(), f"Expected 'disabled' in rejection, got: {output}" + try: + output, rc = _create_with_version(disabled["name"]) + assert rc != 0, f"Expected create to reject disabled version, got: {output}" + assert "disabled" in output.lower(), f"Expected 'disabled' in rejection, got: {output}" - output, rc = _create_with_version(obsolete["name"]) - assert rc != 0, f"Expected create to reject obsolete version, got: {output}" - assert "obsolete" in output.lower(), f"Expected 'obsolete' in rejection, got: {output}" + output, rc = _create_with_version(obsolete["name"]) + assert rc != 0, f"Expected create to reject obsolete version, got: {output}" + assert "obsolete" in output.lower(), f"Expected 'obsolete' in rejection, got: {output}" - output, rc = _create_with_version("4-20-0-e2e-does-not-exist") - assert rc != 0, f"Expected create to reject non-existent version, got: {output}" - assert "not found" in output.lower(), f"Expected 'not found' in rejection, got: {output}" + output, rc = _create_with_version("4-20-0-e2e-does-not-exist") + assert rc != 0, f"Expected create to reject non-existent version, got: {output}" + assert "not found" in output.lower(), f"Expected 'not found' in rejection, got: {output}" + finally: + for version_id in (disabled["id"], obsolete["id"]): + private_grpc.call_unchecked(service="osac.private.v1.ClusterVersions/Delete", data={"id": version_id}) From 244033247b966ed55593e5bd2c9879ac5609beea Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 2 Sep 2026 15:47:02 +0300 Subject: [PATCH 3/4] OSAC-4488: make caas ClusterVersion cleanup robust on failure paths Address self-review findings on the ClusterVersion teardown added for the review-feedback fixes: - test_cluster_create_with_version: move create_cluster inside the try and guard the finally on uuid, so a creation failure still deletes the version. A referenced ClusterVersion cannot be deleted, so wait for full cluster removal before the delete (returns immediately on the happy path). - test_cluster_create_rejected_for_invalid_version: track version ids as they are created and clean up whatever exists, so a mid-setup ensure/update failure does not leak the already-created version. Assisted-by: Claude Code Signed-off-by: Elad Tabak --- tests/e2e/caas/test_cluster_create.py | 49 +++++++++++++++++---------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/tests/e2e/caas/test_cluster_create.py b/tests/e2e/caas/test_cluster_create.py index b256ab09ce..cf3d23981c 100644 --- a/tests/e2e/caas/test_cluster_create.py +++ b/tests/e2e/caas/test_cluster_create.py @@ -166,16 +166,19 @@ def test_cluster_create_with_version( image propagation is covered by test_cluster_create.""" version = private_grpc.ensure_cluster_version(version="4.20.0-e2e", image=TEST_RELEASE_IMAGE) - name = unique_name("e2e-cluster-version") - uuid = cli.create_cluster( - name=name, - template=cluster_template, - version=version["name"], - template_parameter_files={"pull_secret": pull_secret_path}, - template_parameters={"ssh_public_key": Path(ssh_public_key_path).read_text().strip()}, - ) - + # create_cluster runs inside the try so a failure there still triggers the + # ClusterVersion cleanup in the finally (the version already exists by then). + uuid: str | None = None try: + name = unique_name("e2e-cluster-version") + uuid = cli.create_cluster( + name=name, + template=cluster_template, + version=version["name"], + template_parameter_files={"pull_secret": pull_secret_path}, + template_parameters={"ssh_public_key": Path(ssh_public_key_path).read_text().strip()}, + ) + co_name = wait_for_cluster_order_cr(k8s=k8s_hub_client, uuid=uuid) cluster = grpc.get_cluster(cluster_id=uuid) @@ -206,8 +209,14 @@ def test_cluster_create_with_version( wait_for_cluster_deletion(k8s=k8s_hub_client, name=co_name) wait_for_cluster_grpc_removal(grpc=grpc, uuid=uuid) finally: - with contextlib.suppress(subprocess.CalledProcessError): - cli.delete_cluster(uuid=uuid) + if uuid is not None: + with contextlib.suppress(subprocess.CalledProcessError): + cli.delete_cluster(uuid=uuid) + # A referenced ClusterVersion cannot be deleted, so wait for the + # cluster to be fully removed first. Returns immediately on the happy + # path, where the body already waited for removal. + with contextlib.suppress(TimeoutError): + wait_for_cluster_grpc_removal(grpc=grpc, uuid=uuid) private_grpc.call_unchecked(service="osac.private.v1.ClusterVersions/Delete", data={"id": version["id"]}) @@ -216,11 +225,6 @@ def test_cluster_create_rejected_for_invalid_version( ) -> None: """Verify cluster creation is rejected for disabled, obsolete, and non-existent versions.""" - disabled = private_grpc.ensure_cluster_version(version="4.20.0-e2e-disabled", image=TEST_RELEASE_IMAGE) - private_grpc.update_cluster_version(version_id=disabled["id"], enabled=False) - - obsolete = private_grpc.ensure_cluster_version(version="4.20.0-e2e-obsolete", image=TEST_RELEASE_IMAGE) - private_grpc.update_cluster_version(version_id=obsolete["id"], state="CLUSTER_VERSION_STATE_OBSOLETE") def _create_with_version(version_name: str) -> tuple[str, int]: return grpc.call_unchecked( @@ -228,7 +232,18 @@ def _create_with_version(version_name: str) -> tuple[str, int]: data={"object": {"spec": {"template": {"name": cluster_template}, "version": {"name": version_name}}}}, ) + # Track versions as they are created so the finally cleans up whatever + # exists, even if a later ensure/update call raises before the assertions. + created_version_ids: list[str] = [] try: + disabled = private_grpc.ensure_cluster_version(version="4.20.0-e2e-disabled", image=TEST_RELEASE_IMAGE) + created_version_ids.append(disabled["id"]) + private_grpc.update_cluster_version(version_id=disabled["id"], enabled=False) + + obsolete = private_grpc.ensure_cluster_version(version="4.20.0-e2e-obsolete", image=TEST_RELEASE_IMAGE) + created_version_ids.append(obsolete["id"]) + private_grpc.update_cluster_version(version_id=obsolete["id"], state="CLUSTER_VERSION_STATE_OBSOLETE") + output, rc = _create_with_version(disabled["name"]) assert rc != 0, f"Expected create to reject disabled version, got: {output}" assert "disabled" in output.lower(), f"Expected 'disabled' in rejection, got: {output}" @@ -241,5 +256,5 @@ def _create_with_version(version_name: str) -> tuple[str, int]: assert rc != 0, f"Expected create to reject non-existent version, got: {output}" assert "not found" in output.lower(), f"Expected 'not found' in rejection, got: {output}" finally: - for version_id in (disabled["id"], obsolete["id"]): + for version_id in created_version_ids: private_grpc.call_unchecked(service="osac.private.v1.ClusterVersions/Delete", data={"id": version_id}) From 1b0441405b9bf9ce1d74cf0b4efdcc90f8776c13 Mon Sep 17 00:00:00 2001 From: Elad Tabak Date: Wed, 2 Sep 2026 16:32:49 +0300 Subject: [PATCH 4/4] OSAC-4488: skip redundant cluster teardown on the with_version happy path Set uuid = None after the body deletes and waits for full cluster removal, so the finally block only performs cluster teardown on the failure path. Eliminates the redundant delete_cluster + grpc-removal wait that previously ran again in the finally on the happy path. Assisted-by: Claude Code Signed-off-by: Elad Tabak --- tests/e2e/caas/test_cluster_create.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tests/e2e/caas/test_cluster_create.py b/tests/e2e/caas/test_cluster_create.py index cf3d23981c..f3bd2dfc57 100644 --- a/tests/e2e/caas/test_cluster_create.py +++ b/tests/e2e/caas/test_cluster_create.py @@ -208,13 +208,15 @@ def test_cluster_create_with_version( wait_for_cluster_grpc_deleting_or_archived(grpc=grpc, uuid=uuid) wait_for_cluster_deletion(k8s=k8s_hub_client, name=co_name) wait_for_cluster_grpc_removal(grpc=grpc, uuid=uuid) + uuid = None # deleted cleanly; the finally only needs to remove the version finally: + # On the failure path the cluster may still exist and reference the + # version; a referenced ClusterVersion cannot be deleted, so remove the + # cluster and wait for it to be gone first. Skipped entirely on the happy + # path, where the body already deleted the cluster and cleared uuid. if uuid is not None: with contextlib.suppress(subprocess.CalledProcessError): cli.delete_cluster(uuid=uuid) - # A referenced ClusterVersion cannot be deleted, so wait for the - # cluster to be fully removed first. Returns immediately on the happy - # path, where the body already waited for removal. with contextlib.suppress(TimeoutError): wait_for_cluster_grpc_removal(grpc=grpc, uuid=uuid) private_grpc.call_unchecked(service="osac.private.v1.ClusterVersions/Delete", data={"id": version["id"]})