Changed error logging for Lake Formation/IAM permissions issues due to

api structure. Changed testing to match.

---
X-AI-Prompt: Refactor Lake Formation error handling in FeatureGroupManager to remove _has_lake_formation_config() which won't work because our API has no way to record this  and replace separate LF/IAM error messages with a single combined
  _ICEBERG_PERMISSIONS_ERROR_MESSAGE constant covering both governance models (SELECT/DESCRIBE/ALTER for LF, glue:GetTable/glue:UpdateTable for IAM). Apply to both _get_iceberg_properties and _update_iceberg_properties, preserving the more expansive logger.error() calls in the update
  path. Update tests accordingly.
X-AI-Tool: Kiro CLI sisyphus

Author: alexyoung13

sagemaker-mlops/src/sagemaker/mlops/feature_store/feature_group_manager.py

@@ -28,7 +28,10 @@
 from botocore.exceptions import ClientError
 from pyiceberg.catalog import load_catalog
 from tenacity import retry, stop_after_attempt, wait_exponential, retry_if_exception_type
-from sagemaker.mlops.feature_store.feature_utils import _APPROVED_ICEBERG_PROPERTIES
+from sagemaker.mlops.feature_store.feature_utils import (
+    _APPROVED_ICEBERG_PROPERTIES,
+    _ICEBERG_PERMISSIONS_ERROR_MESSAGE,
+)
 
 
 logger = logging.getLogger(__name__)
@@ -77,16 +80,6 @@ class FeatureGroupManager(FeatureGroup):
     if FeatureGroup.__doc__ and __doc__:
         __doc__ = FeatureGroup.__doc__
 
-    def _has_lake_formation_config(self) -> bool:
-        """Check if this feature group was created with Lake Formation governance.
-
-        Note: Returns False if the object was not hydrated via get()/describe
-        (i.e., constructed directly with just a name). In that case, the caller
-        falls back to the generic IAM error message, which is still valid advice.
-        """
-        lf_config = getattr(self, "lake_formation_config", None)
-        return lf_config is not None and lf_config != Unassigned()
-
     def _validate_table_ownership(self, table, database_name: str, table_name: str):
         """Validate that the Iceberg table belongs to this feature group by checking S3 location."""
         table_location = table.metadata.location if table.metadata else None
@@ -187,17 +180,9 @@ def _get_iceberg_properties(
 
         except ClientError as e:
             if e.response["Error"]["Code"] == "AccessDeniedException":
-                if self._has_lake_formation_config():
-                    raise PermissionError(
-                        f"Access denied reading Iceberg properties for '{self.feature_group_name}'. "
-                        f"This feature group uses Lake Formation governance — ensure you have "
-                        f"SELECT and DESCRIBE permissions on the table in Lake Formation, "
-                        f"in addition to IAM permissions."
-                    ) from e
                 raise PermissionError(
                     f"Access denied reading Iceberg properties for '{self.feature_group_name}'. "
-                    f"Ensure your role has glue:GetTable permission "
-                    f"on the feature group's Glue table."
+                    f"{_ICEBERG_PERMISSIONS_ERROR_MESSAGE}"
                 ) from e
             raise RuntimeError(
                 f"Failed to get Iceberg properties for '{self.feature_group_name}': {e}"
@@ -298,17 +283,9 @@ def _update_iceberg_properties(
 
         except ClientError as e:
             if e.response["Error"]["Code"] == "AccessDeniedException":
-                if self._has_lake_formation_config():
-                    raise PermissionError(
-                        f"Access denied updating Iceberg properties for '{self.feature_group_name}'. "
-                        f"This feature group uses Lake Formation governance — ensure you have "
-                        f"ALTER permission on the table in Lake Formation, "
-                        f"in addition to IAM permissions."
-                    ) from e
                 raise PermissionError(
                     f"Access denied updating Iceberg properties for '{self.feature_group_name}'. "
-                    f"Ensure your role has glue:UpdateTable permission "
-                    f"on the feature group's Glue table."
+                    f"{_ICEBERG_PERMISSIONS_ERROR_MESSAGE}"
                 ) from e
             logger.error(
                 f"Failed to update Iceberg properties for feature group "

sagemaker-mlops/src/sagemaker/mlops/feature_store/feature_utils.py

@@ -66,6 +66,13 @@
     "write.target-file-size-bytes"
 }
 
+_ICEBERG_PERMISSIONS_ERROR_MESSAGE = (
+    "If this feature group uses Lake Formation governance, ensure you have "
+    "SELECT, DESCRIBE, and ALTER permissions on the table in Lake Formation, "
+    "in addition to IAM permissions.\n"
+    "If this feature group uses IAM governance, ensure your role has "
+    "glue:GetTable and glue:UpdateTable permissions on the feature group's Glue table."
+) 
 
 def _get_athena_client(session: Session):
     """Get Athena client from session."""

sagemaker-mlops/tests/unit/sagemaker/mlops/feature_store/test_iceberg_properties.py

@@ -995,41 +995,6 @@ def test_passes_session_and_region_to_get_iceberg_properties(self, mock_get_clie
         mock_get_iceberg.assert_called_once_with(session=mock_session, region="us-east-1")
 
 
-class TestHasLakeFormationConfig:
-    """Tests for _has_lake_formation_config method."""
-
-    def test_returns_false_when_attribute_missing(self):
-        """Test returns False when lake_formation_config attribute does not exist."""
-        fg = MagicMock(spec=FeatureGroupManager)
-        fg._has_lake_formation_config = FeatureGroupManager._has_lake_formation_config.__get__(fg)
-        # MagicMock(spec=...) won't have lake_formation_config since it's not on the class
-        del fg.lake_formation_config
-        assert fg._has_lake_formation_config() is False
-
-    def test_returns_false_when_none(self):
-        """Test returns False when lake_formation_config is None."""
-        fg = MagicMock(spec=FeatureGroupManager)
-        fg._has_lake_formation_config = FeatureGroupManager._has_lake_formation_config.__get__(fg)
-        fg.lake_formation_config = None
-        assert fg._has_lake_formation_config() is False
-
-    def test_returns_false_when_unassigned(self):
-        """Test returns False when lake_formation_config is Unassigned()."""
-        from sagemaker.core.shapes import Unassigned
-
-        fg = MagicMock(spec=FeatureGroupManager)
-        fg._has_lake_formation_config = FeatureGroupManager._has_lake_formation_config.__get__(fg)
-        fg.lake_formation_config = Unassigned()
-        assert fg._has_lake_formation_config() is False
-
-    def test_returns_true_when_config_present(self):
-        """Test returns True when lake_formation_config has a real value."""
-        fg = MagicMock(spec=FeatureGroupManager)
-        fg._has_lake_formation_config = FeatureGroupManager._has_lake_formation_config.__get__(fg)
-        fg.lake_formation_config = MagicMock()
-        assert fg._has_lake_formation_config() is True
-
-
 class TestGetIcebergPropertiesAccessDenied:
     """Tests for AccessDeniedException handling in _get_iceberg_properties."""
 
@@ -1052,26 +1017,16 @@ def _make_client_error(self, code):
         return ClientError({"Error": {"Code": code, "Message": "denied"}}, "GetTable")
 
     @patch("sagemaker.mlops.feature_store.feature_group_manager.load_catalog")
-    def test_access_denied_with_lake_formation_raises_permission_error(self, mock_load_catalog):
-        """Test PermissionError with LF message when AccessDenied and LF config present."""
-        mock_catalog = MagicMock()
-        mock_catalog.load_table.side_effect = self._make_client_error("AccessDeniedException")
-        mock_load_catalog.return_value = mock_catalog
-        self.fg._has_lake_formation_config.return_value = True
-
-        with pytest.raises(PermissionError, match="Lake Formation governance"):
-            self.fg._get_iceberg_properties(session=MagicMock())
-
-    @patch("sagemaker.mlops.feature_store.feature_group_manager.load_catalog")
-    def test_access_denied_without_lake_formation_raises_permission_error(self, mock_load_catalog):
-        """Test PermissionError with IAM message when AccessDenied and no LF config."""
+    def test_access_denied_raises_permission_error_with_combined_message(self, mock_load_catalog):
+        """Test PermissionError with combined LF/IAM message on AccessDenied."""
         mock_catalog = MagicMock()
         mock_catalog.load_table.side_effect = self._make_client_error("AccessDeniedException")
         mock_load_catalog.return_value = mock_catalog
-        self.fg._has_lake_formation_config.return_value = False
 
-        with pytest.raises(PermissionError, match="glue:GetTable"):
+        with pytest.raises(PermissionError, match="Lake Formation governance") as exc_info:
             self.fg._get_iceberg_properties(session=MagicMock())
+        assert "glue:GetTable" in str(exc_info.value)
+        assert "glue:UpdateTable" in str(exc_info.value)
 
     @patch("sagemaker.mlops.feature_store.feature_group_manager.load_catalog")
     def test_non_access_denied_client_error_raises_runtime_error(self, mock_load_catalog):
@@ -1090,7 +1045,6 @@ def test_access_denied_permission_error_chains_original(self, mock_load_catalog)
         mock_catalog = MagicMock()
         mock_catalog.load_table.side_effect = original
         mock_load_catalog.return_value = mock_catalog
-        self.fg._has_lake_formation_config.return_value = False
 
         with pytest.raises(PermissionError) as exc_info:
             self.fg._get_iceberg_properties(session=MagicMock())
@@ -1121,31 +1075,22 @@ def _setup_get_result(self):
         }
         return mock_table
 
-    def test_access_denied_with_lake_formation_raises_permission_error(self):
-        """Test PermissionError with LF message when AccessDenied and LF config present."""
+    def test_access_denied_raises_permission_error_with_combined_message(self):
+        """Test PermissionError with combined LF/IAM message on AccessDenied."""
         mock_table = self._setup_get_result()
         mock_table.transaction().__enter__().set_properties.side_effect = self._make_client_error("AccessDeniedException")
-        self.fg._has_lake_formation_config.return_value = True
 
         props = IcebergProperties(properties={"write.target-file-size-bytes": "536870912"})
-        with pytest.raises(PermissionError, match="Lake Formation governance"):
-            self.fg._update_iceberg_properties(iceberg_properties=props)
-
-    def test_access_denied_without_lake_formation_raises_permission_error(self):
-        """Test PermissionError with IAM message when AccessDenied and no LF config."""
-        mock_table = self._setup_get_result()
-        mock_table.transaction().__enter__().set_properties.side_effect = self._make_client_error("AccessDeniedException")
-        self.fg._has_lake_formation_config.return_value = False
-
-        props = IcebergProperties(properties={"write.target-file-size-bytes": "536870912"})
-        with pytest.raises(PermissionError, match="glue:UpdateTable"):
+        with pytest.raises(PermissionError, match="Lake Formation governance") as exc_info:
             self.fg._update_iceberg_properties(iceberg_properties=props)
+        assert "glue:GetTable" in str(exc_info.value)
+        assert "glue:UpdateTable" in str(exc_info.value)
+        assert "ALTER" in str(exc_info.value)
 
     def test_non_access_denied_client_error_raises_runtime_error(self):
         """Test RuntimeError for non-AccessDenied ClientError."""
         mock_table = self._setup_get_result()
         mock_table.transaction().__enter__().set_properties.side_effect = self._make_client_error("InternalServiceException")
-        self.fg._has_lake_formation_config.return_value = False
 
         props = IcebergProperties(properties={"write.target-file-size-bytes": "536870912"})
         with pytest.raises(RuntimeError, match="Failed to update Iceberg properties"):
@@ -1156,19 +1101,8 @@ def test_access_denied_permission_error_chains_original(self):
         mock_table = self._setup_get_result()
         original = self._make_client_error("AccessDeniedException")
         mock_table.transaction().__enter__().set_properties.side_effect = original
-        self.fg._has_lake_formation_config.return_value = True
 
         props = IcebergProperties(properties={"write.target-file-size-bytes": "536870912"})
         with pytest.raises(PermissionError) as exc_info:
             self.fg._update_iceberg_properties(iceberg_properties=props)
         assert exc_info.value.__cause__ is original
-
-    def test_access_denied_lf_message_mentions_alter(self):
-        """Test that LF error message mentions ALTER permission."""
-        mock_table = self._setup_get_result()
-        mock_table.transaction().__enter__().set_properties.side_effect = self._make_client_error("AccessDeniedException")
-        self.fg._has_lake_formation_config.return_value = True
-
-        props = IcebergProperties(properties={"write.target-file-size-bytes": "536870912"})
-        with pytest.raises(PermissionError, match="ALTER permission"):
-            self.fg._update_iceberg_properties(iceberg_properties=props)