Skip to content

Conversation

@alkaline-0
Copy link
Contributor

@alkaline-0 alkaline-0 commented Oct 30, 2025

Description

  1. Gate Lake Formation (LF) tag management strictly on configuration: tags should not be managed unless LF is explicitly set and configured.
  2. Add tests to ensure no LF calls are made when LF config is not set, preventing unintended GetResourceLFTags requests.

Related Issues

Changes:

  1. implemented a test in /tests/load/pipeline/test_athena.py
  2. Changed the check in AthenaClient to verify that enabled flag is true before calling manage_lf_tags() since the resolver does not change that value even tho it initializes an object of lakeformation even tho lakeformation config obj is None.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 30, 2025

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
docs 62eed5d Commit Preview URL

Branch Preview URL
Nov 06 2025, 02:47 PM

@alkaline-0 alkaline-0 force-pushed the fix/3165-athena-lakeformation-permissions-required-when-not-set branch 2 times, most recently from e78bbf6 to bdaa57f Compare October 30, 2025 18:16
@alkaline-0 alkaline-0 self-assigned this Oct 30, 2025
@sh-rp sh-rp marked this pull request as ready for review November 3, 2025 11:23
Copy link
Collaborator

@sh-rp sh-rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks good, small changes to the tests are needed

def test_athena_lakeformation_config_gating(
destination_config: DestinationTestConfiguration, mocker
) -> None:
pytest.importorskip("pyathena")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be needed. This test is only selected when the athena destination is active(see destination_config setup above) and in that case, the athena extra will also be present.

destinations_configs(default_sql_configs=True, subset=["athena"]),
ids=lambda x: x.name,
)
def test_athena_lakeformation_config_gating(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these kind of tests are "more secure" if you parametrize them to have one run with lf enabled and check that the mocked function is called and then one run where it is not and thus the mocked function is not called

Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! this type of test is sufficient - clearly we deal with configuration problem, not edge behavior of athena

…d on configuration. Add tests to ensure tags are not applied when Lake Formation config is not set. This fixes the bug of lakeformation GetResourceLFTags bug
…rmation gating configuration. Update test to ensure proper mocking of connection methods.
… tests for Athena Lake Formation configuration.
@alkaline-0 alkaline-0 force-pushed the fix/3165-athena-lakeformation-permissions-required-when-not-set branch from bdaa57f to 62eed5d Compare November 6, 2025 14:41
@alkaline-0 alkaline-0 requested a review from sh-rp November 6, 2025 16:47
Copy link
Collaborator

@sh-rp sh-rp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alkaline-0 alkaline-0 merged commit 8a16442 into devel Nov 12, 2025
67 of 72 checks passed
@alkaline-0 alkaline-0 deleted the fix/3165-athena-lakeformation-permissions-required-when-not-set branch November 12, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants