Allow plugins to add custom schema/authority URL handler rules #17898

thejcannon · 2022-12-29T19:57:57Z

At <company> we're looking at either using a versioned S3 bucket or Artifactory for binary artifact storage. In either case, I'd love to use http_source on a file/resource to have Pants manage the download/caching. But in order to do so Pants would need to make the request with the right auth, which it can't to today.

Ideally, the solution to this problem is flexible enough to handle either case (and many more), so that usually means unions get involved.

This change does 3 things:

adds the ability for plugins to add custom rules which handle file download matched by scheme or authority. This is automatically hooked into by the DownloadFile node (by renaming the existing class NativeDownloadFile for the intrinsic input). The behavior is first-one-wins and existing-behavior-by-default, and the implementing rule must return the URL's Digest.
adds a auth_headers mapping to NativeDownloadFile to be used when making the request but NOT part of the underlying cache key. The expectation being that most authenticated HTTP requests just need the right auth header. The name auth_headers is purposefully a misnomer, as any header could be specified, but is named to nudge the user to only specifying headers required for auth (and is also a hint to the possible sensitivity of the data).
Adds a url handler for S3. Both for exposition and because S3 is an extremely popular storage solution

thejcannon · 2022-12-29T19:58:14Z

Pending tests, then no longer WIP.

kaos

Cool stuff :)

How's the plan to satisfy the botocore dependency when using the S3 url handler?

thejcannon · 2022-12-29T20:20:33Z

You can add botocore to plugins (the test will exercise this). Or if you're me you'll add botocore@git+https://github.com/thejcannon/botocore.git@8de7ace79d71c6f2bc3e02875075b834ada6e6ed which isn't 70MB lol

kaos · 2022-12-29T20:36:23Z

You can add botocore to plugins (the test will exercise this). Or if you're me you'll add botocore@git+https://github.com/thejcannon/botocore.git@8de7ace79d71c6f2bc3e02875075b834ada6e6ed which isn't 70MB lol

Hah, neat trick with the custom fork.. been thinking about doing the same for tensorflow in hopes of reducing the 700MB payload for that dep alone.

I saw the plugins option to solve it, but I'm hesitant if we really want to go down that path as it would mean there'll be a dependency to add to your plugins whenever you enable the S3 backend.

I would hope there'd be something more automagical perhaps. What if we introduce "extras" on the pantsbuild.pants dist, and the bootstrap uses the proper extras based on enabled backends (for those that have special requirements)?
Another option I see could be to package the backend itself as a dedicated dist (or grouped together with other backends) in order to have the package dependency resolution as usual (this is the option I've leaned towards for the explorer backend; not finalized yet i.e. not publishing it yet)

thejcannon · 2022-12-29T20:38:22Z

This mirrors --stats-log's dependency on hdrhistogram:

pants/src/python/pants/goal/stats_aggregator.py

Line 40 in 06a8d51

    
                       For histogram summaries to work, you must add `hdrhistogram` to `[GLOBAL].plugins`.

The alternative is requesting a PEX with botocore in it (or equivalent). I'd honestly love that but it would require that we generate the auth token in a PEX subprocess which seems waaaay less secure 😓

If/when we peel off "standard-but-separate" plugins, having it depend on botocore would be ideal.

kaos · 2022-12-29T20:40:33Z

Huh, OK then. Still think it's unfortunate, but nothing new. I'll ponder on if we can't find a nicer way to handle this in the future.. :)

src/python/pants/engine/download_file.py

src/rust/engine/src/downloads.rs

tdyas · 2022-12-30T03:45:36Z

src/rust/engine/src/downloads.rs

+    let mut headers = HeaderMap::new();
+    for (k, v) in auth_headers.iter() {
+      headers.insert(
+        HeaderName::from_bytes(k.as_bytes()).unwrap(),


Maybe avoid the .unwrap and return an error instead (using ? operator to just do it inline)? I.e., .map_err(|err| ...)?.

Same comment for the following line.

In that case it'd be a dev mistake, so I'd rather crash-and-burn to make it obvious.

thejcannon · 2022-12-30T17:34:36Z

src/python/pants/engine/download_file.py

+    register both.
+    """
+
+    match_authority: ClassVar[Optional[str]] = None


One thing to consider is allowing wildcards. I suspect at <company> we'll want to use https://*.s3.amazonaws.com. I can upstream that as well.

thejcannon · 2022-12-30T17:43:58Z

I'll also admit I'm not sure how all this works with regions. If anyone wants to try with a non us-east-1 region and see if it crashes, please let me know!

kaos

lgtm

thejcannon · 2022-12-30T20:29:02Z

Just tried it on private artifact and it went boom. I'll post the code soon, and fix the tests later.

thejcannon · 2022-12-30T20:37:08Z

oh and i want to test on a box with no creds, as some files are public

kaos

With #17901 landed, perhaps you want to go back to using None as your mock result sentinel value..? :)

tdyas · 2023-01-03T21:16:07Z

src/python/pants/backend/url_handlers/s3/register.py

+    virtual_hosted_url = f"https://{request.bucket}.s3"
+    if request.region:
+        virtual_hosted_url += f".{request.region}"
+    virtual_hosted_url += f".amazonaws.com/{request.key}"


Is request.key guaranteed to not start with a /?

Yeah. I should add an assert

stuhood · 2023-01-03T22:39:27Z

src/python/pants/engine/fs.py

+@frozen_after_init
+@dataclass(unsafe_hash=True)
+class NativeDownloadFile:
+    """Retrieve the contents of a file via an HTTP GET request or directly for local file:// URLs.
+
+    This request is handled directly by the native engine without any additional coercion by plugins,
+    and therefore should only be used in cases where the URL is known to be publicly accessible.
+    Otherwise, callers should use `DownloadFile`.
+
+    The auth_headers are part of this nodes' cache key for memoization (changing a header invalidates
+    prior results) but are not part of the underlying cache key for the local/remote cache (changing
+    a header won't re-download a file if the file was previously downloaded).
+    """


So, this extra overloading is slightly odd, because as you mention, the "native" implementation already supports multiple URL schemes.

What was the reasoning behind doing this in Python via a union, rather than by adding another implementation on the rust side? A preference for boto? A binary size concern? A desire to use the @union to plug in other private implementations? Because it seems like it would be simpler to continue to expand the existing scheme based switching on the Rust side (which would get you retry for the auth portion as well), rather than have two code paths parsing URLs to decide which implementation to use.

There's a closed PR in your inbox that has this on the rust side... But much less powerful.

Three big things stick out:

handling auth headers for any URL. As I said in the PR description we might use artifactory, so vanilla http with an auth header is required. That's currently impossible to have rust-side with a flexible enough client-side

s3 URLs come in many flavors as the S3 tests show. Trying to match them all is folly.

bloating the engine binary with AWS is just plain rude

Would this be clearer if we just made this type private?

Would this be clearer if we just made this type private?

Yea, I would feel better about that. I know that we don't have any explicitly public / stable plugin APIs, but it'd be good not to commit to this being an extension point if we think that support for s3 / auth are things that we should support natively (I think that they are.)

To be clear, the native type would be private. The extension point would remain public.

How would you use the extension point if the native type was private?

Same way I am today, but with an underscore 😌

More honestly, we'd just keep usage internal to the repo I guess

Oh maybe there's confusion. Plugin code is currently using DownloadFile, which will become the extension point. Then the extender(?) Is the one using the "native" type.

I have an example usage in this PR for S3, check it out!

Oh and maybe you didn't see, the plugin implementation boils down to NativeDownloadFile, so we still get retry and fast-return-if-cached

stuhood · 2023-01-04T17:35:02Z

You’ve put a lot of effort into this, and I think that we could probably port it elsewhere and deprecate the existing API fairly easily if we find a better solution. Thanks!

thejcannon · 2023-01-04T21:34:16Z

To wrap up the botocore issue above: https://pypi.org/project/botocore-a-la-carte/

initial impl

0c92c8a

thejcannon added the category:new feature label Dec 29, 2022

thejcannon requested review from stuhood, benjyw and Eric-Arellano December 29, 2022 19:57

thejcannon changed the title ~~WIP: Allow plugins to add custom schema/authority handler rules~~ WIP: Allow plugins to add custom schema/authority URL handler rules Dec 29, 2022

use botocore for auth

fd8992e

kaos reviewed Dec 29, 2022

View reviewed changes

thats it

4fd98dc

thejcannon changed the title ~~WIP: Allow plugins to add custom schema/authority URL handler rules~~ Allow plugins to add custom schema/authority URL handler rules Dec 29, 2022

that did it

f241c08

thejcannon mentioned this pull request Dec 30, 2022

(WIP) Support s3 schemes in DownloadFile URLs #17897

Closed

tdyas reviewed Dec 30, 2022

View reviewed changes

src/python/pants/engine/download_file.py Show resolved Hide resolved

src/rust/engine/src/downloads.rs Outdated Show resolved Hide resolved

tdyas reviewed Dec 30, 2022

View reviewed changes

comments!

6e1da5d

thejcannon commented Dec 30, 2022

View reviewed changes

kaos approved these changes Dec 30, 2022

View reviewed changes

now it actually works

672a718

kaos approved these changes Dec 31, 2022

View reviewed changes

creds and queries

b807f36

tdyas approved these changes Jan 3, 2023

View reviewed changes

stuhood reviewed Jan 3, 2023

View reviewed changes

stuhood approved these changes Jan 4, 2023

View reviewed changes

thejcannon merged commit d098b58 into pantsbuild:main Jan 4, 2023

thejcannon deleted the user-defined-download branch January 4, 2023 18:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow plugins to add custom schema/authority URL handler rules #17898

Allow plugins to add custom schema/authority URL handler rules #17898

thejcannon commented Dec 29, 2022 •

edited

Loading

thejcannon commented Dec 29, 2022

kaos left a comment

thejcannon commented Dec 29, 2022

kaos commented Dec 29, 2022

thejcannon commented Dec 29, 2022 •

edited

Loading

kaos commented Dec 29, 2022

tdyas Dec 30, 2022

tdyas Dec 30, 2022

thejcannon Dec 30, 2022

thejcannon Dec 30, 2022

thejcannon commented Dec 30, 2022

kaos left a comment

thejcannon commented Dec 30, 2022

thejcannon commented Dec 30, 2022

kaos left a comment

tdyas Jan 3, 2023

thejcannon Jan 3, 2023

stuhood Jan 3, 2023

thejcannon Jan 3, 2023

thejcannon Jan 3, 2023

stuhood Jan 4, 2023 •

edited

Loading

thejcannon Jan 4, 2023

stuhood Jan 4, 2023

thejcannon Jan 4, 2023

thejcannon Jan 4, 2023

thejcannon Jan 4, 2023

stuhood commented Jan 4, 2023

thejcannon commented Jan 4, 2023

Allow plugins to add custom schema/authority URL handler rules #17898

Allow plugins to add custom schema/authority URL handler rules #17898

Conversation

thejcannon commented Dec 29, 2022 • edited Loading

thejcannon commented Dec 29, 2022

kaos left a comment

Choose a reason for hiding this comment

thejcannon commented Dec 29, 2022

kaos commented Dec 29, 2022

thejcannon commented Dec 29, 2022 • edited Loading

kaos commented Dec 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejcannon commented Dec 30, 2022

kaos left a comment

Choose a reason for hiding this comment

thejcannon commented Dec 30, 2022

thejcannon commented Dec 30, 2022

kaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Jan 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Jan 4, 2023

thejcannon commented Jan 4, 2023

thejcannon commented Dec 29, 2022 •

edited

Loading

thejcannon commented Dec 29, 2022 •

edited

Loading

stuhood Jan 4, 2023 •

edited

Loading