Skip to content

Keep S3 Dag bundle downloads within the configured directory - #73756

Merged
potiuk merged 3 commits into
apache:mainfrom
jingi723:fix/s3-sync-directory-prefix
Oct 5, 2026
Merged

potiuk merged 3 commits into
apache:mainfrom
jingi723:fix/s3-sync-directory-prefix

Conversation

@jingi723

@jingi723 jingi723 commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

S3DagBundle documents its prefix as a subdirectory and checks that directory during initialization, but passes the original string prefix to the download hook. With prefix="dags", an object such as dags_archive/old.py matches the S3 listing and then fails local path conversion. An object named exactly dags also makes initialization fail when downloaded to the bundle directory itself.

Append / to non-empty directory prefixes in S3DagBundle.refresh() before calling the hook. This aligns downloads with the bundle's directory validation. The configured prefix, local file layout, URLs, and locking remain unchanged. Empty prefixes still download the whole bucket, and prefixes already ending in / are unchanged.

The revised patch leaves S3Hook.sync_to_local_dir() unchanged: generic S3 prefixes retain their string-prefix meaning. The regression now exercises bundle initialization and subsequent refresh through the real hook and boto3 with Moto, including stale local file cleanup and preservation of the excluded remote objects.

For configurations without a trailing slash, the listing request changes from, for example, dags to dags/. IAM policies using an exact s3:prefix condition must allow that directory prefix. This behavior change is documented in the bundle guide and pending changelog note.

Validation:

  • The eight bundle regression cases have four failures on unchanged upstream (two ValueError, two IsADirectoryError) and all pass with the fix. The four already-delimited cases pass on both versions.
  • S3 hook and bundle suites: 172 passed, 1 warning, on Python 3.10.21 in the official Linux/amd64 Breeze CI image.
  • Regular and manual prek checks pass on the final PR diff, including provider mypy. Selective-check also completes successfully.

No live AWS or IAM policy evaluation was performed. The broader dependent-provider, older-Airflow, scheduler/worker integration, and OS/Python matrices were not run locally.


Was generative AI tooling used to co-author this PR?
  • Yes — OpenAI Codex (GPT-6).

Generated-by: OpenAI Codex (GPT-6) following the guidelines

@jingi723
jingi723 requested a review from o-nikolas as a code owner September 26, 2026 11:09
@boring-cyborg boring-cyborg Bot added area:providers provider:amazon AWS/Amazon - related issues labels Sep 26, 2026
@boring-cyborg

boring-cyborg Bot commented Sep 26, 2026

Copy link
Copy Markdown

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our prek-hooks will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example Dag that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@rjgoyln rjgoyln left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Just one small nit on the test.

One non-blocking note: GCSHook.sync_to_local_dir has the same prefix/relative_to pattern, but that can be addressed separately.

Just my thoughts, feel free to resolve it.


Drafted by Claude Code (Opus 5); reviewed by @rjgoyln before posting.

Comment thread providers/amazon/tests/unit/amazon/aws/hooks/test_s3.py Outdated
@jingi723
jingi723 force-pushed the fix/s3-sync-directory-prefix branch from d0484a7 to df16438 Compare September 28, 2026 11:11

@vincbeck vincbeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I understand the confusion but S3 prefix are not folders/directories.

For example, syncing dags also lists dags_archive/old.py, which cannot be made relative to the dags directory

This is valid to me, if you specify dags as prefix then dags_archive/old.py should be listed

@jingi723
jingi723 force-pushed the fix/s3-sync-directory-prefix branch from df16438 to 5207787 Compare September 29, 2026 04:25
@jingi723 jingi723 changed the title Keep S3 directory sync within the requested prefix Keep S3 Dag bundle downloads within the configured directory Sep 29, 2026
@jingi723

jingi723 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Following up after moving the prefix normalization into S3DagBundle. The targeted tests and local checks passed. Could a maintainer approve the pending CI runs and take another look when available? Thanks!

jingi723 and others added 3 commits October 5, 2026 18:29
S3 lists keys using string prefixes, so a directory prefix without a
trailing slash also selects sibling files and directories. Those keys
cannot be mapped below the requested local directory and prevent a Dag
bundle from refreshing.
The exact-prefix case fails differently from sibling keys, so its name should make that boundary clear in CI failures.
The bundle validates a directory prefix during initialization, but downloads with a raw string prefix. Matching objects outside that directory can prevent initialization and refresh.
@potiuk
potiuk force-pushed the fix/s3-sync-directory-prefix branch from 5207787 to 9a354fa Compare October 5, 2026 16:29

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving. I checked the containment logic against .., absolute keys, keys that share the start of the prefix or equal it, trailing/double slashes and symlinks, and none escapes the bundle directory. One small pre-existing gap for a follow-up: a key like dags/. doesn't end in /, so it isn't skipped, and pathlib reduces it to the bundle directory itself. The download then fails on every refresh, which lets anyone who can write under the prefix block bundle updates. Rejecting an empty or . relative path before downloading would close it.


Drafted-by: Claude Code (Opus 5.5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit 86aef85 into apache:main Oct 5, 2026
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants