feat(s3): tag S3 requests with a luigi user agent, document endpoint_url - #3446
Draft
goanpeca wants to merge 4 commits into
Draft
feat(s3): tag S3 requests with a luigi user agent, document endpoint_url#3446goanpeca wants to merge 4 commits into
goanpeca wants to merge 4 commits into
Conversation
Signed-off-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
Signed-off-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
Signed-off-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
Signed-off-by: Gonzalo Peña-Castellanos <goanpeca@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two small, independent changes to the S3 contrib module and its documentation.
1. Identify luigi in the S3 user agent (
luigi/contrib/s3.py,test/contrib/s3_test.py)S3Client.s3now addsluigi/<version>to botocore'suser_agent_extrabefore building the boto3 resource. If the caller already supplies abotocore.config.Config(via[s3]options or kwargs), itsuser_agent_extrais preserved and the luigi token is appended throughConfig.merge(), so no other client setting is overwritten. If there is no config, a freshConfig(user_agent_extra="luigi/<version>")is created.botocore.configis imported locally in the property, matching the existing lazyimport boto3a few lines above.2. Document the
[s3]config section (doc/configuration.rst)Adds an
[s3]section (alphabetically, before[scalding]) that notes keys in the section are passed to the underlying boto3 S3 client, and documentsendpoint_urlandregion_namewith an example.region_nameis worth documenting next toendpoint_url: with a custom endpoint and no region configured, botocore falls back tous-east-1and then signs requests for the wrong region against a bucket that lives elsewhere. No behavior change.Motivation and Context
user_agent_extrais boto3's supported extension point for this, so signing and request behavior are untouched: the only difference is one extra token in theUser-Agentheader. Having it there makes luigi traffic identifiable in object store access logs, which helps when tracing throttling, retries, or unexpected request patterns back to the tool that generated them. Several other data tools tag their S3 clients the same way.The
[s3]section is already read byS3Client._get_s3_config()and every key in it is forwarded to boto3, butdoc/configuration.rstnever mentioned the section at all.endpoint_urlis the option that letsS3Targetwork against any S3-compatible object store (Amazon S3, Backblaze B2, Cloudflare R2, MinIO), and today it is discoverable only by reading the source.Have you tested this? If so, how?
Correcting an earlier version of this description: existing coverage was not unaffected. Adding the
configkwarg to theboto3.resource()call changed the exact call signature that two existing tests pin, sotest_init_without_init_or_configandtest_init_with_configintest/contrib/s3_test.pyfailed on this branch withAssertionError: Expected call: resource('s3', aws_access_key_id=None, aws_secret_access_key=None, aws_session_token=None)against an actual call that also carriesconfig=<botocore.config.Config object>. Both tests are updated in this PR.The two updated tests now go through a small
assert_resource_called_with()helper that still asserts the exact credential kwargs and additionally asserts that the config carries theluigi/<version>user agent suffix, so the assertions are tightened rather than loosened toANY. One test is new,test_init_appends_user_agent_to_given_config, covering the merge branch: a caller suppliedConfig(user_agent_extra="caller/1.0", read_timeout=42)becomescaller/1.0 luigi/<version>withread_timeoutpreserved.Run locally against botocore 1.43.x and moto 4.x, on Python 3.12:
pytest test/contrib/s3_test.py -k test_init: 5 passed.pytest test/contrib/s3_test.py: 68 passed, 1 skipped, plus 4 failures in the multipart size tests. Those 4 also fail on the parent commit715f65cin the same environment (a local moto version artifact in the part size accounting), so they are unrelated to this change.ruff check .andruff format --check .: clean.sphinx-build -W -b html doc: build succeeded, and the rendered[s3]section shows both options as a definition list alongside the neighboring sections.I have not run the full tox matrix locally. Happy to split the two commits into separate PRs if you would rather they land separately.