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

Allow arbitrary headers to sent with Bazel Downloader #17829

Closed
thesayyn opened this issue Mar 20, 2023 · 15 comments
Closed

Allow arbitrary headers to sent with Bazel Downloader #17829

thesayyn opened this issue Mar 20, 2023 · 15 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request

Comments

@thesayyn
Copy link
Contributor

Description of the feature request:

Currently, bazel sets some headers when sending an HTTP request to fetch artifacts over the internet. While this works well for most cases, it doesn't work at all when the remote server requires some HTTP headers to be set to special values.

For instance, currently bazel sets all the Accept header to text/html, image/gif, image/jpeg, */* in outgoing HTTP requests. In my case, its Accept header that needs to be set to application/vnd.oci.image.manifest.v1+json so that docker registry sees no problem responding when the image has oci manifest types.

See: #16659 (comment)

Related to #16682

What underlying problem are you trying to solve with this feature?

No response

Which operating system are you running Bazel on?

darwin/arm64

What is the output of bazel info release?

No response

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@thesayyn
Copy link
Contributor Author

cc @Wyverald

@ShreeM01 ShreeM01 added type: feature request untriaged team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Mar 20, 2023
@Wyverald Wyverald added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. and removed team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Mar 20, 2023
@alexeagle
Copy link
Contributor

Note, we'll need to add a workaround in rules_oci for now, however it uses a requires using another tool during the repository rule, making it require a fake toolchain resolution and also the download it makes is not cached anywhere.

@fmeum
Copy link
Collaborator

fmeum commented Sep 11, 2023

@meteorcloudy Do you see any potential issues with allowing rctx.download[_and_extract] to set arbitrary headers?

@thesayyn Would you be interested in contributing this functionality if you get the okay from maintainers? I could help with pre-reviews and references if needed.

@meteorcloudy
Copy link
Member

Do you see any potential issues with allowing rctx.download[_and_extract] to set arbitrary headers?

Not really. @tjgq integrated credential helper for repository download, do you have any concern here?

@tjgq
Copy link
Contributor

tjgq commented Sep 11, 2023

We already merge authentication headers from the credential helper and the argument to rctx.download* so adding additional headers from the Starlark side should work.

@thesayyn
Copy link
Contributor Author

@thesayyn Would you be interested in contributing this functionality if you get the okay from maintainers? I could help with pre-reviews and references if needed.

Yes, I'd be willing to do that.

@thesayyn
Copy link
Contributor Author

@fmeum I'd appreciate your points on #19501

@xuzhenglun
Copy link

xuzhenglun commented Nov 1, 2023

I know this may not be directly related to bazel, but this is a message for others who are directed here by the rules_oci error log.

An checksum failed was throwed, and the error log looks like below:

WARNING: Could not fetch the manifest. Either there was an authentication issue or trying to pull an image with OCI image media types.
Falling back to using `curl`. See https://github.com/bazelbuild/bazel/issues/17829 for the context.
Analyzing: target ... (89 packages loaded, 6572 targets configured)
INFO: Repository rules_oci~1.4.0~oci~distroless_static_linux_amd64 instantiated at:
callstack not available
Repository rule oci_pull defined at:
/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl:427:27: in <toplevel>
ERROR: An error occurred during the fetch of repository 'rules_oci~1.4.0~oci~distroless_static_linux_amd64':
Traceback (most recent call last):
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 378, column 57, in _oci_pull_impl
mf, mf_len, mf_digest = downloader.download_manifest(rctx.attr.identifier, "manifest.json")
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 319, column 74, in lambda
download_manifest = lambda identifier, output: _download_manifest(rctx, state, identifier, output),
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 281, column 18, in _download_manifest
_download(
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 247, column 23, in _download
return download_fn(
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/download.bzl", line 129, column 29, in _download
cache_it = rctx.download(
Error in download: com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException: Checksum was e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 but wanted 6706c73aae2afaa8201d63cc3dda48753c09bcd6c300762251065c0f7e602b25
ERROR: <builtin>: fetching oci_pull rule //:rules_oci~1.4.0~oci~distroless_static_linux_amd64: Traceback (most recent call last):
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 378, column 57, in _oci_pull_impl
mf, mf_len, mf_digest = downloader.download_manifest(rctx.attr.identifier, "manifest.json")
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 319, column 74, in lambda
download_manifest = lambda identifier, output: _download_manifest(rctx, state, identifier, output),
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 281, column 18, in _download_manifest
_download(
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/pull.bzl", line 247, column 23, in _download
return download_fn(
File "/home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0/oci/private/download.bzl", line 129, column 29, in _download
cache_it = rctx.download(
Error in download: com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException: Checksum was e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 but wanted 6706c73aae2afaa8201d63cc3dda48753c09bcd6c300762251065c0f7e602b25
ERROR: /home/admin/.cache/bazel/_bazel_admin/23e125c776c0411833d0c469fe9d82fd/external/rules_oci~1.4.0~oci~distroless_static/BUILD.bazel:1:6: @rules_oci~1.4.0~oci~distroless_static//:distroless_static depends on @rules_oci~1.4.0~oci~distroless_static_linux_amd64//:distroless_static_linux_amd64 in repository @rules_oci~1.4.0~oci~distroless_static_linux_amd64 which failed to fetch. no such package '@rules_oci~1.4.0~oci~distroless_static_linux_amd64//': com.google.devtools.build.lib.bazel.repository.downloader.UnrecoverableHttpException: Checksum was e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855 but wanted 6706c73aae2afaa8201d63cc3dda48753c09bcd6c300762251065c0f7e602b25

I believe the reason that causes this problem is here. To cache the file downloaded from curl, here using ctx.download downloads file from local which is impossible to fetch from remote downloader machine.

To workaround this, flag --experimental_remote_downloader_local_fallback=true could be helpful.

releate:

alexeagle pushed a commit to bazel-contrib/bazel_features that referenced this issue Dec 18, 2023
Coming in 7.1.0, rctx.download will accept a headers parameter allowing
arbitrary headers to be passed to the downloaders.
Link to issue: bazelbuild/bazel#17829

cc: @fmeum
@sushain97
Copy link
Contributor

sushain97 commented Dec 20, 2023

Is this something that will require changes to FetchBlob RPC? That is, https://github.com/bazelbuild/remote-apis/blob/7e33c12ee96192f12b7fbb089e6fe1af6a29ef14/build/bazel/remote/asset/v1/remote_asset.proto#L154 doesn't include a mechanism for the client to specify headers.

@thesayyn
Copy link
Contributor Author

thesayyn commented Jan 3, 2024

I don't there's a mechanism for it for remote downloaders.

@sushain97
Copy link
Contributor

I agree - it sounds like it'll require an addition to the protocol.

Wyverald pushed a commit that referenced this issue Jan 22, 2024
fixes #17829

Closes #19501.

PiperOrigin-RevId: 588750878
Change-Id: I11c982864135d8e1c4420099ce27f67accd3fe20
github-merge-queue bot pushed a commit that referenced this issue Jan 22, 2024
…ct] (#20979)

fixes #17829

Closes #19501.

PiperOrigin-RevId: 588750878
Change-Id: I11c982864135d8e1c4420099ce27f67accd3fe20

Co-authored-by: thesayyn <thesayyn@gmail.com>
Wyverald added a commit that referenced this issue Jan 23, 2024
…ct] (#20979)

fixes #17829

Closes #19501.

PiperOrigin-RevId: 588750878
Change-Id: I11c982864135d8e1c4420099ce27f67accd3fe20

Co-authored-by: thesayyn <thesayyn@gmail.com>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

@sushain97
Copy link
Contributor

I've opened #21490 which is necessary so that rulesets which adopt this feature don't break users of the remote downloader service. If it looks good, I'd appreciate it if we could pull into either 7.1.0 or 7.1.1.

@iancha1992
Copy link
Member

I've opened #21490 which is necessary so that rulesets which adopt this feature don't break users of the remote downloader service. If it looks good, I'd appreciate it if we could pull into either 7.1.0 or 7.1.1.

cc: @Wyverald @meteorcloudy

copybara-service bot pushed a commit that referenced this issue Feb 27, 2024
Related to #17829 and 2697e0c

I don't love this design but according to the Remote Asset API spec, this is an intended use of qualifiers: https://docs.google.com/document/d/10ari9WtTTSv9bqB_UU-oe2gBtaAA7HyQgkpP-RFP80c/edit#heading=h.sixrlhdnkfoa.

cc @Wyverald @jmillikin

Closes #21490.

PiperOrigin-RevId: 610688317
Change-Id: I272f63a6bc4ea432503003ee907ca012f6d641cf
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Feb 27, 2024
Related to bazelbuild#17829 and bazelbuild@2697e0c

I don't love this design but according to the Remote Asset API spec, this is an intended use of qualifiers: https://docs.google.com/document/d/10ari9WtTTSv9bqB_UU-oe2gBtaAA7HyQgkpP-RFP80c/edit#heading=h.sixrlhdnkfoa.

cc @Wyverald @jmillikin

Closes bazelbuild#21490.

PiperOrigin-RevId: 610688317
Change-Id: I272f63a6bc4ea432503003ee907ca012f6d641cf
github-merge-queue bot pushed a commit that referenced this issue Mar 4, 2024
Related to #17829 and
2697e0c

I don't love this design but according to the Remote Asset API spec,
this is an intended use of qualifiers:
https://docs.google.com/document/d/10ari9WtTTSv9bqB_UU-oe2gBtaAA7HyQgkpP-RFP80c/edit#heading=h.sixrlhdnkfoa.

cc @Wyverald @jmillikin

Closes #21490.

Commit
2195baa

PiperOrigin-RevId: 610688317
Change-Id: I272f63a6bc4ea432503003ee907ca012f6d641cf

Co-authored-by: Sushain Cherivirala <sushain@skc.name>
@Damangir
Copy link

We already merge authentication headers from the credential helper and the argument to rctx.download* so adding additional headers from the Starlark side should work.

@tjgq Just want to flag that credential_helper do not override the headers, it just adds another header line. The headers that Bazel sends when credential_helper returns Accept are something like this:

    Accept: text/html, image/gif, image/jpeg, */*\r\n
    User-Agent: Bazel/release 7.1.0\r\n
    Authorization: Bearer XXXXXX\r\n
    Accept-Encoding: gzip\r\n
    Accept: application/octet-stream\r\n

The tests in starlark_repository_test.sh pass only since testing_server.py does not correctly merge headers despite RFC9110.

If you

python src/test/shell/bazel/testing_server.py --dump_headers headers.txt always /dev/null

and then

curl -L http://localhost:43726/ -H "Accept: this" -H "Accept: that" 
cat header.txt
{"Accept": "that"}

Only the last header is captured.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.