diff --git a/CHANGELOG.md b/CHANGELOG.md index 1929e87fdc..a6cfd625d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,6 +88,7 @@ request adding CHANGELOG notes for breaking (!) changes and possibly other secti - Deprecated `ALLOW_EXTERNAL_TABLE_LOCATION`. Use `ALLOW_EXTERNAL_METADATA_FILE_LOCATION` for external metadata file locations, including catalog config `polaris.config.allow.external.metadata.file.location`. ### Fixes +- Python CLI `setup apply` now exits with an error after any setup operation fails, while still attempting the remaining operations. Previously, individual failures were logged but the command reported success and exited with status 0. - Python CLI `tables list`, `tables get`, and `tables delete` commands now exit with status 1 when catalog API requests fail. Previously, these commands printed an error but exited successfully. - Default table storage locations with object-storage prefixing enabled are now percent-encoded with UTF-8 instead of the JVM default charset. Previously the persisted location of a table under a non-ASCII namespace or table name depended on the platform default charset, so the same table could resolve to a different (or lossy) location on a non-UTF-8 JVM. - Async task execution (table cleanup, manifest and batch file cleanup) now retries when a handler returns false on transient errors (e.g. IO or delete failures). Previously `false` was swallowed with only a warning log and the task was never retried via the existing retry mechanism. diff --git a/client/python/apache_polaris/cli/command/setup.py b/client/python/apache_polaris/cli/command/setup.py index 72bbd5c6b2..1f0831995d 100644 --- a/client/python/apache_polaris/cli/command/setup.py +++ b/client/python/apache_polaris/cli/command/setup.py @@ -20,11 +20,11 @@ import logging import yaml import json -from dataclasses import dataclass +from dataclasses import dataclass, field from typing import Dict, Optional, List, Any, Set from apache_polaris.cli.command import Command -from apache_polaris.cli.exceptions import CliError +from apache_polaris.cli.exceptions import CliError, CLI_ERROR_EXIT_CODE from apache_polaris.cli.constants import ( PrincipalType, Subcommands, @@ -76,6 +76,7 @@ class SetupCommand(Command): _existing_catalogs: Optional[Set[str]] = None _existing_principals: Optional[Set[str]] = None _catalog_api: Optional[Any] = None + _failure_count: int = field(default=0, init=False, repr=False) def _get_catalog_api(self, api: PolarisDefaultApi) -> Any: """Get or create and cache the IcebergCatalogAPI client.""" @@ -91,7 +92,7 @@ def _get_existing_principals(self, api: PolarisDefaultApi) -> Set[str]: p.name for p in api.list_principals().principals } except Exception: - logger.exception("Failed to fetch existing principals") + self._record_failure("Failed to fetch existing principals") self._existing_principals = set() return self._existing_principals @@ -103,7 +104,7 @@ def _get_existing_principal_roles(self, api: PolarisDefaultApi) -> Set[str]: role.name for role in api.list_principal_roles().roles } except Exception: - logger.exception("Failed to fetch existing principal roles") + self._record_failure("Failed to fetch existing principal roles") self._existing_principal_roles = set() return self._existing_principal_roles @@ -125,8 +126,8 @@ def _get_existing_catalog_roles( # In dry-run, a 404 is expected if the catalog doesn't exist yet is_404 = getattr(e, "status", None) == 404 or "(404)" in str(e) if not (self.dry_run and is_404): - logger.warning( - f"Failed to fetch catalog roles for catalog '{catalog_name}': {e}" + self._record_failure( + f"Failed to fetch catalog roles for catalog '{catalog_name}'" ) self._existing_catalog_roles[catalog_name] = set() return self._existing_catalog_roles[catalog_name] @@ -138,7 +139,7 @@ def _get_existing_catalogs(self, api: PolarisDefaultApi) -> Set[str]: try: self._existing_catalogs = {c.name for c in api.list_catalogs().catalogs} except Exception: - logger.exception("Failed to fetch existing catalogs") + self._record_failure("Failed to fetch existing catalogs") self._existing_catalogs = set() return self._existing_catalogs @@ -572,7 +573,12 @@ def execute(self, api: PolarisDefaultApi) -> None: dry_run=self.dry_run, ) logger.info(f"--- Finished processing catalog: {catalog_name} ---") - if self.dry_run: + if self._failure_count: + raise CliError( + f"Setup apply failed; setup errors: {self._failure_count}", + exit_code=CLI_ERROR_EXIT_CODE, + ) + elif self.dry_run: logger.info("=== Dry-Run Finished ===") else: logger.info("=== Setup Apply Process Completed Successfully ===") @@ -601,6 +607,10 @@ def _log_dry_run( message += f" with details:\n{details_str}" logger.info(message) + def _record_failure(self, message: str) -> None: + self._failure_count += 1 + logger.exception(message) + def _create_principals( self, api: PolarisDefaultApi, @@ -609,6 +619,9 @@ def _create_principals( ) -> None: """Create principals and assign them to principal roles.""" logger.info("--- Processing principals ---") + if not principals_config: + logger.info("--- Finished processing principals ---") + return existing_principals = self._get_existing_principals(api) for principal_name, principal_data in principals_config.items(): if principal_name in existing_principals: @@ -640,7 +653,7 @@ def _create_principals( ) existing_principals.add(principal_name) except Exception: - logger.exception( + self._record_failure( f"Failed to create principal '{principal_name}'" ) # Assign roles @@ -657,6 +670,7 @@ def _create_principals( logger.warning( f"Skipping assignment for non-existing principal role '{role_name}' for principal '{principal_name}'" ) + self._failure_count += 1 continue if dry_run: @@ -681,7 +695,7 @@ def _create_principals( f"Assigned principal '{principal_name}' to role '{role_name}' successfully." ) except Exception: - logger.exception( + self._record_failure( f"Failed to assign principal '{principal_name}' to role '{role_name}'" ) logger.info("--- Finished processing principals ---") @@ -694,6 +708,9 @@ def _create_principal_roles( ) -> None: """Create principal roles.""" logger.info("--- Processing principal roles ---") + if not principal_roles_config: + logger.info("--- Finished processing principal roles ---") + return self._get_existing_principal_roles(api) for role_name in principal_roles_config: @@ -722,7 +739,9 @@ def _create_principal_roles( if self._existing_principal_roles is not None: self._existing_principal_roles.add(role_name) except Exception: - logger.exception(f"Failed to create principal role '{role_name}'") + self._record_failure( + f"Failed to create principal role '{role_name}'" + ) logger.info("--- Finished processing principal roles ---") def _map_storage_properties(self, catalog_data: Dict[str, Any]) -> Dict[str, Any]: @@ -883,6 +902,7 @@ def _create_catalogs( logger.warning( f"External catalog '{catalog_name}' is missing the required 'connection' info block." ) + self._failure_count += 1 overall_success = False continue command_args.update(conn_args) @@ -954,7 +974,7 @@ def _create_catalogs( logger.info(f"Catalog '{catalog_name}' created successfully.") existing_catalogs.add(catalog_name) except Exception: - logger.exception(f"Failed to create catalog '{catalog_name}'") + self._record_failure(f"Failed to create catalog '{catalog_name}'") overall_success = False logger.info("--- Finished processing catalogs ---") return overall_success @@ -968,8 +988,14 @@ def _create_catalog_roles( ) -> None: """Create catalog roles, assign them to principal roles, and grant privileges.""" logger.info(f"--- Processing catalog roles for catalog: {catalog_name} ---") + if not roles_config: + logger.info( + f"--- Finished processing catalog roles for catalog: {catalog_name} ---" + ) + return existing_roles_in_catalog = self._get_existing_catalog_roles(api, catalog_name) - self._get_existing_principal_roles(api) + if any(role_data.get("assign_to") for role_data in roles_config.values()): + self._get_existing_principal_roles(api) for role_name, role_data in roles_config.items(): if role_name in existing_roles_in_catalog: logger.info( @@ -1001,7 +1027,7 @@ def _create_catalog_roles( # Update the cache with the newly created role existing_roles_in_catalog.add(role_name) except Exception: - logger.exception( + self._record_failure( f"Failed to create catalog role '{role_name}' in catalog '{catalog_name}'" ) continue @@ -1015,6 +1041,7 @@ def _create_catalog_roles( logger.warning( f"Skipping assignment of catalog role '{role_name}' to non-existing principal role '{principal_role_name}'" ) + self._failure_count += 1 continue if dry_run: self._log_dry_run( @@ -1040,7 +1067,7 @@ def _create_catalog_roles( f"Assigned catalog role '{role_name}' to principal role '{principal_role_name}' successfully." ) except Exception: - logger.exception( + self._record_failure( f"Failed to assign catalog role '{role_name}' to principal role '{principal_role_name}'" ) # Grant privileges @@ -1115,7 +1142,7 @@ def _grant_privilege( cmd.execute(api) logger.info(f"Successfully granted {log_message}") except Exception: - logger.exception(f"Failed to grant {log_message}") + self._record_failure(f"Failed to grant {log_message}") def _create_namespaces( self, @@ -1176,7 +1203,7 @@ def _create_namespaces( existing_namespaces.add(".".join(ns)) listed_parents.add(parent_ns) except Exception: - logger.exception( + self._record_failure( f"Failed to list sub-namespaces for '{parent_ns}'" ) @@ -1216,7 +1243,7 @@ def _create_namespaces( ) existing_namespaces.add(ns_name) except Exception: - logger.exception( + self._record_failure( f"Failed to create namespace '{ns_name}' in catalog '{catalog_name}'" ) logger.info( @@ -1254,9 +1281,16 @@ def _create_policies_and_attachments( except NotFoundException: policy_exists = False except Exception: - logger.warning( - f"Could not verify existence of policy '{policy_name}', attempting creation." - ) + # A real apply's create attempt determines whether this failure is terminal. + if dry_run: + self._record_failure( + f"Could not verify existence of policy '{policy_name}'" + ) + else: + logger.warning( + f"Could not verify existence of policy '{policy_name}', attempting creation.", + exc_info=True, + ) policy_exists = False if policy_exists: logger.info( @@ -1314,7 +1348,7 @@ def _create_policies_and_attachments( ) logger.info(f"Policy '{policy_name}' created successfully.") except Exception: - logger.exception(f"Failed to create policy '{policy_name}'") + self._record_failure(f"Failed to create policy '{policy_name}'") continue # Attachments attachments = policy_data.get("attach", []) @@ -1355,7 +1389,7 @@ def _create_policies_and_attachments( cmd.execute(api) logger.info(f"Successfully attached policy '{policy_name}'") except Exception: - logger.exception(f"Failed to attach policy '{policy_name}'") + self._record_failure(f"Failed to attach policy '{policy_name}'") logger.info(f"--- Finished processing policies for catalog: {catalog_name} ---") def _validate_entity( diff --git a/client/python/tests/test_setup_command.py b/client/python/tests/test_setup_command.py index 7a849c7745..d7f075b418 100644 --- a/client/python/tests/test_setup_command.py +++ b/client/python/tests/test_setup_command.py @@ -20,6 +20,9 @@ import io from unittest.mock import patch, MagicMock, mock_open from cli_test_utils import CLITestBase, INVALID_ARGS +from apache_polaris.cli.command.setup import SetupCommand +from apache_polaris.cli.constants import Subcommands +from apache_polaris.cli.exceptions import CliError, CLI_ERROR_EXIT_CODE from apache_polaris.sdk.management import ( PolarisCatalog, CatalogProperties, @@ -39,7 +42,14 @@ def test_setup_validation(self) -> None: @patch( "apache_polaris.cli.command.setup.open", new_callable=mock_open, - read_data="principals:\n quickstart_user:\n roles:\n - quickstart_user_role", + read_data=( + "principal_roles:\n" + " - quickstart_user_role\n" + "principals:\n" + " quickstart_user:\n" + " roles:\n" + " - quickstart_user_role" + ), ) @patch("apache_polaris.cli.command.setup.os.path.isfile") def test_setup_apply_dry_run( @@ -50,6 +60,138 @@ def test_setup_apply_dry_run( self.mock_execute(mock_client, ["setup", "apply", "config.yaml", "--dry-run"]) mock_client.list_principals.assert_called() + def test_setup_apply_succeeds_without_failures(self) -> None: + mock_client = self.build_mock_client() + mock_client.list_principal_roles.return_value.roles = [] + command = SetupCommand( + setup_subcommand=Subcommands.APPLY, + setup_config="config.yaml", + _config_cache={"principal_roles": ["successful-role"]}, + ) + + command.execute(mock_client) + + self.assertEqual(command._failure_count, 0) + mock_client.create_principal_role.assert_called_once() + + @patch("apache_polaris.cli.command.setup.os.path.isfile") + def test_setup_dry_run_reports_lookup_failures( + self, mock_isfile: MagicMock + ) -> None: + mock_client = self.build_mock_client() + mock_isfile.return_value = True + mock_client.list_principals.side_effect = RuntimeError("listing unavailable") + setup_yaml = "principals:\n quickstart_user: {}" + + with ( + patch( + "apache_polaris.cli.command.setup.open", + mock_open(read_data=setup_yaml), + ), + self.assertRaises(CliError) as cm, + ): + self.mock_execute( + mock_client, + ["setup", "apply", "config.yaml", "--dry-run"], + ) + + self.assertEqual(cm.exception.exit_code, CLI_ERROR_EXIT_CODE) + self.assertIn("setup errors: 1", str(cm.exception)) + mock_client.create_principal.assert_not_called() + + @patch("apache_polaris.cli.command.setup.os.path.isfile") + def test_setup_dry_run_reports_missing_role_assignments( + self, mock_isfile: MagicMock + ) -> None: + mock_client = self.build_mock_client() + mock_isfile.return_value = True + setup_yaml = "principals:\n user:\n roles:\n - missing-role" + + with ( + patch( + "apache_polaris.cli.command.setup.open", + mock_open(read_data=setup_yaml), + ), + self.assertRaises(CliError) as cm, + ): + self.mock_execute( + mock_client, + ["setup", "apply", "config.yaml", "--dry-run"], + ) + + self.assertEqual(cm.exception.exit_code, CLI_ERROR_EXIT_CODE) + self.assertIn("setup errors: 1", str(cm.exception)) + + def test_setup_dry_run_ignores_missing_catalog_roles(self) -> None: + mock_client = self.build_mock_client() + mock_client.list_catalog_roles.side_effect = RuntimeError("(404)") + command = SetupCommand( + setup_subcommand=Subcommands.APPLY, + dry_run=True, + ) + + self.assertEqual( + command._get_existing_catalog_roles(mock_client, "new-catalog"), set() + ) + self.assertEqual(command._failure_count, 0) + + @patch("apache_polaris.cli.command.setup.PolicyAPI") + def test_setup_dry_run_reports_policy_lookup_failures( + self, mock_policy_api: MagicMock + ) -> None: + mock_client = self.build_mock_client() + mock_policy_api.return_value.load_policy.side_effect = RuntimeError( + "listing unavailable" + ) + command = SetupCommand( + setup_subcommand=Subcommands.APPLY, + dry_run=True, + ) + + command._create_policies_and_attachments( + mock_client, + "catalog", + { + "policy": { + "namespace": "namespace", + "type": "data-compaction", + "content": {}, + } + }, + dry_run=True, + ) + + self.assertEqual(command._failure_count, 1) + + @patch("apache_polaris.cli.command.setup.PolicyAPI") + def test_setup_apply_recovers_policy_lookup_failure( + self, mock_policy_api: MagicMock + ) -> None: + mock_client = self.build_mock_client() + mock_policy_api.return_value.load_policy.side_effect = RuntimeError( + "listing unavailable" + ) + command = SetupCommand( + setup_subcommand=Subcommands.APPLY, + dry_run=False, + ) + + command._create_policies_and_attachments( + mock_client, + "catalog", + { + "policy": { + "namespace": "namespace", + "type": "data-compaction", + "content": {}, + } + }, + dry_run=False, + ) + + self.assertEqual(command._failure_count, 0) + mock_policy_api.return_value.create_policy.assert_called_once() + @patch("apache_polaris.cli.command.setup.os.path.isfile") def test_setup_apply_s3_optional_fields(self, mock_isfile: MagicMock) -> None: mock_client = self.build_mock_client() @@ -65,6 +207,58 @@ def test_setup_apply_s3_optional_fields(self, mock_isfile: MagicMock) -> None: # role_arn should be None, NOT an empty string self.assertIsNone(call_args.catalog.storage_config_info.role_arn) self.assertEqual(call_args.catalog.name, "s3-catalog") + mock_client.list_principals.assert_not_called() + mock_client.list_principal_roles.assert_not_called() + mock_client.list_catalog_roles.assert_not_called() + + @patch("apache_polaris.cli.command.setup.os.path.isfile") + def test_setup_apply_reports_missing_external_catalog_connection( + self, mock_isfile: MagicMock + ) -> None: + mock_client = self.build_mock_client() + mock_isfile.return_value = True + setup_yaml = "catalogs:\n - name: broken\n type: external" + + with ( + patch( + "apache_polaris.cli.command.setup.open", + mock_open(read_data=setup_yaml), + ), + self.assertRaises(CliError) as cm, + ): + self.mock_execute(mock_client, ["setup", "apply", "config.yaml"]) + + self.assertEqual(cm.exception.exit_code, CLI_ERROR_EXIT_CODE) + self.assertIn("setup errors: 1", str(cm.exception)) + mock_client.create_catalog.assert_not_called() + + @patch("apache_polaris.cli.command.setup.os.path.isfile") + def test_setup_apply_reports_provisioning_failures( + self, mock_isfile: MagicMock + ) -> None: + mock_client = self.build_mock_client() + mock_isfile.return_value = True + mock_client.list_principal_roles.side_effect = RuntimeError( + "listing unavailable" + ) + mock_client.create_principal_role.side_effect = [ + RuntimeError("backend unavailable"), + None, + ] + setup_yaml = "principal_roles:\n - failed-role\n - successful-role" + + with ( + patch( + "apache_polaris.cli.command.setup.open", + mock_open(read_data=setup_yaml), + ), + self.assertRaises(CliError) as cm, + ): + self.mock_execute(mock_client, ["setup", "apply", "config.yaml"]) + + self.assertEqual(cm.exception.exit_code, CLI_ERROR_EXIT_CODE) + self.assertIn("setup errors: 2", str(cm.exception)) + self.assertEqual(mock_client.create_principal_role.call_count, 2) @patch( "apache_polaris.cli.command.setup.open", new_callable=mock_open, read_data="{}"