Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GrpcRemoteDownloader: optionally propagate credentials to remote server #23578

Closed

Conversation

sluongng
Copy link
Contributor

@sluongng sluongng commented Sep 10, 2024

In a multi-tenancy server deployment setup, the clients might want to
treat the remote downloader server as a pull-through proxy and use it to
download from private storage systems.

Currently, we do support it via --remote_downloader_headers. However
this scheme does not apply to the specific URL, while credentials and
authentication could sometimes be host/domain specific.

Add a flag to let users opt-in to credentials propagation to the remote
server. This is off by default as not all remote servers can be
trusted. When the flag is enabled, URL-specific credentials from Netrc
or a custom credentials helper can be propagated to the remote server.

The server implementation needs to support the new
http_header_url:<url-index>:<header-key> qualifier where the
url-index is a 0-based position of the URL inside the
FetchBlobRequest's uris field. This new qualifier is modeled after the
existing http_header qualifier.

Fixes #23499

@sluongng sluongng force-pushed the sluongng/grpc-downloader-w-creds branch from e83ba32 to 389b613 Compare September 10, 2024 09:14
@sluongng sluongng changed the title sluongng/grpc downloader w creds GrpcRemoteDownloader: optionally propagate credentials to remote server Sep 10, 2024
@sluongng sluongng force-pushed the sluongng/grpc-downloader-w-creds branch 2 times, most recently from 555fd05 to a6d936d Compare September 10, 2024 09:37
@sluongng sluongng marked this pull request as ready for review September 10, 2024 12:08
@sluongng sluongng requested a review from a team as a code owner September 10, 2024 12:08
@github-actions github-actions bot added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Sep 10, 2024
@sluongng
Copy link
Contributor Author

cc: @coeuvre I think the failed test was not caused by my change but I am not certain.

sluongng added a commit to buildbuddy-io/buildbuddy that referenced this pull request Sep 10, 2024
In addition to the checksum.sri qualifier from remote api spec,
Bazel includes it's own set of qualifiers to each FetchBlob requests.

https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java;l=76-85;drc=618c0abbfe518f4e29de523a2e63ca9179050e94

This change adds initial support for the http_header qualifier as well
as acknoledge the existence of bazel.canonical_id header (without
actually using it). Relevant tests were also added to demonstrate the
current behavior.

In a future patch, we may start rejecting unknown qualifiers as
recommended by the Remote Asset API spec and thus, flip the assertions
in the test. See bazelbuild/remote-apis#301
for more info.

Another change to consider in the future is support for custom
url-specific header credentials. This is being proposed in Bazel via
bazelbuild/bazel#23578.
@sluongng
Copy link
Contributor Author

cc: @sushain97, since you added the headers support.

@sluongng sluongng force-pushed the sluongng/grpc-downloader-w-creds branch from a6d936d to fc63914 Compare September 10, 2024 14:03
sluongng added a commit to buildbuddy-io/buildbuddy that referenced this pull request Sep 11, 2024
In addition to the checksum.sri qualifier from remote api spec,
Bazel includes it's own set of qualifiers to each FetchBlob requests.

https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java;l=76-85;drc=618c0abbfe518f4e29de523a2e63ca9179050e94

This change adds initial support for the http_header qualifier as well
as acknoledge the existence of bazel.canonical_id header (without
actually using it). Relevant tests were also added to demonstrate the
current behavior.

In a future patch, we may start rejecting unknown qualifiers as
recommended by the Remote Asset API spec and thus, flip the assertions
in the test. See bazelbuild/remote-apis#301
for more info.

Another change to consider in the future is support for custom
url-specific header credentials. This is being proposed in Bazel via
bazelbuild/bazel#23578.
In a multi-tenancy server deployment setup, the clients might want to
treat the remote downloader server as a pull-through proxy and use it to
download from private storage systems.

Currently, we do support it via --remote_downloader_headers. However
this scheme does not apply to the specific URL, while credentials and
authentication could sometimes be host/domain specific.

Add a flag to let users opt-in to credentials propagation to the remote
server. This is off by default as not all remote servers can be
trusted. When the flag is enabled, URL-specific credentials from Netrc
or a custom credentials helper can be propagated to the remote server.

The server implementation needs to support the new
http_header_url:<url-index>:<header-key> qualifier where the
url-index is a 0-based position of the URL inside the
FetchBlobRequest's uris field. This new qualifier is modeled after the
existing http_header qualifier.
@sluongng sluongng force-pushed the sluongng/grpc-downloader-w-creds branch from fc63914 to e8220b0 Compare September 12, 2024 14:28
@coeuvre coeuvre added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 12, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Remote-Exec Issues and PRs for the Execution (Remote) team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remote Downloader should support using custom credentials
2 participants