Skip to content

[6.0.0] [remote/downloader] Don't include headers in FetchBlobRequest#16677

Merged
meteorcloudy merged 3 commits intobazelbuild:release-6.0.0from
EngFlow:yannic-cherry-pick-9296068be5e3808eb03a3b61f3af3a2c88f7ab7d
Nov 10, 2022
Merged

[6.0.0] [remote/downloader] Don't include headers in FetchBlobRequest#16677
meteorcloudy merged 3 commits intobazelbuild:release-6.0.0from
EngFlow:yannic-cherry-pick-9296068be5e3808eb03a3b61f3af3a2c88f7ab7d

Conversation

@Yannic
Copy link
Copy Markdown
Contributor

@Yannic Yannic commented Nov 7, 2022

Including the headers in the request is very inefficient as credentials should never change the content of the downloaded archive. In fact, given that Bazel verifies the checksum of the downloaded file, the credentials cannot possibly used in a way where they influence the outcome of the download (other than deciding whether or not the user is allowed to download the blob at all). Hence, the credentials should not be included in the request.

Users that need to send credentials to the remote downloader should do so by passing the credentials as metadata to the gRPC call.

Note that the remote downloader is behind an experimental flag, so this change does not need to go thorugh the incompatible change process.

Closes #16595.

PiperOrigin-RevId: 485576157
Change-Id: I8afc7c818e4eed63ac1f70c3e4c2115a1d365830

Including the headers in the request is very inefficient as credentials should never change the content of the downloaded archive. In fact, given that Bazel verifies the checksum of the downloaded file, the credentials cannot possibly used in a way where they influence the outcome of the download (other than deciding whether or not the user is allowed to download the blob at all). Hence, the credentials should not be included in the request.

Users that need to send credentials to the remote downloader should do so by passing the credentials as metadata to the gRPC call.

Note that the remote downloader is behind an experimental flag, so this change does not need to go thorugh the incompatible change process.

Closes bazelbuild#16595.

PiperOrigin-RevId: 485576157
Change-Id: I8afc7c818e4eed63ac1f70c3e4c2115a1d365830
@Yannic Yannic requested a review from ShreeM01 as a code owner November 7, 2022 17:37
@Yannic
Copy link
Copy Markdown
Contributor Author

Yannic commented Nov 7, 2022

cc @tjgq

@Yannic Yannic changed the title [remote/downloader] Don't include headers in FetchBlobRequest [6.0.0] [remote/downloader] Don't include headers in FetchBlobRequest Nov 7, 2022
@ShreeM01 ShreeM01 added team-Remote-Exec Issues and PRs for the Execution (Remote) team awaiting-review PR is awaiting review from an assigned reviewer labels Nov 7, 2022
@Yannic
Copy link
Copy Markdown
Contributor Author

Yannic commented Nov 9, 2022

@meteorcloudy can you help getting this merged please?

@meteorcloudy meteorcloudy requested a review from tjgq November 10, 2022 09:14
@meteorcloudy
Copy link
Copy Markdown
Member

@tjgq Please LGTM if this looks good to be cherry-picked for 6.0 ;)

@meteorcloudy meteorcloudy enabled auto-merge (squash) November 10, 2022 09:15
@tjgq tjgq 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 Nov 10, 2022
@meteorcloudy
Copy link
Copy Markdown
Member

@Yannic Please also rebase this one so that it'll get merged ;)

@Yannic
Copy link
Copy Markdown
Contributor Author

Yannic commented Nov 10, 2022

done

@meteorcloudy meteorcloudy merged commit 7aa69a9 into bazelbuild:release-6.0.0 Nov 10, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Nov 10, 2022
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.

5 participants