[repository/downloader] Add support for multiple header values #16260
Conversation
Headers can generally be specified multiple times, not just once. This refactoring is prerequisite for adding support for getting credentials from the credential helper. Note that this does not change the Starlark API or the qualifier for the gRPC remote downloader (both of which need to go through the process for incompatible changes - which should hopefully be pretty straight forward to do as this feature doesn't seem to be used that much AFAICT). Progress on bazelbuild#15856
|
@tjgq PTAL |
sgowroji
added
team-Remote-Exec
| // TODO(yannic): Introduce incompatible flag for including all headers, not just the first. | ||
| String value = Iterables.getFirst(subEntry.getValue(), null); | ||
| if (value != null) { | ||
| subObject.addProperty(subEntry.getKey(), value); | ||
| } |
There was a problem hiding this comment.
It's not obvious to me why this is needed. In StarlarkBaseExternalContext#getAuthHeaders, the map is populated from a Starlark dict, which by construction cannot contain multiple values for the same key. Am I missing some other code path where the map could be populated with multiple values?
There was a problem hiding this comment.
The credential helper returns a map<string, list<string>> for headers, so we need this change for adding the headers from the helper. We could implement it in a way that only passes the first (or last) value for the header to the remote server, but that doesn't seem like a good idea IMO.
We can allow setting multiple headers from Starlark by allowing Dict<String, Sequence<String>> in addition to Dict<String, String>, but that would be a follow-up.
There was a problem hiding this comment.
I understand that this change is necessary. What I don't understand is why GrpcRemoteDownloader#addHeadersJson must only call addProperty on the first header value for a given key, and ignore the rest.
- If you're not using a credential helper, headers should come solely from the Starlark dict (unless I'm missing something), therefore there won't be more than one value per key.
- If you are using a credential helper, adding multiple values per key doesn't have to be considered a breaking change, since credential helpers are a new feature (again, unless I'm missing something).
There was a problem hiding this comment.
The JSON is passed as Qualifier in the gRPC download request. The server may or may not look at the JSON (it's not a well-specified part of the assets API), so switching to multiple values here has potential to break things.
We could make the JSON dependent on where a header comes from. But we probably want to merge the headers from Starlark and the credential helper (at least that's my understanding on how the 2 features work together?), and sending a different qualifier based on where credentials come from seems very confusing. That said, with my security hat on, I'm not sure the headers should even be part of the qualifier (esp. in plaintext). The digest of the headers should be sufficient, if sending anything at all.
There was a problem hiding this comment.
The JSON is passed as Qualifier in the gRPC download request. The server may or may not look at the JSON (it's not a well-specified part of the assets API), so switching to multiple values here has potential to break things.
But the breakage could only possibly happen if you're already setting --experimental_credential_helper, right? I would argue that this falls under "APIs and behavior guarded by an --experimental_* flag can change at any time" (https://bazel.build/release/backward-compatibility) and an --incompatible flag might not be necessary.
We could make the JSON dependent on where a header comes from. But we probably want to merge the headers from Starlark and the credential helper (at least that's my understanding on how the 2 features work together?), and sending a different qualifier based on where credentials come from seems very confusing.
I agree that the qualifier should not depend on the provenance of the headers. Although something to ponder is what would happen if the Authentication header is set to the same value from both the Starlark and credential helper sides. Would that cause the header to be sent twice, and if so, would that still work?
That said, with my security hat on, I'm not sure the headers should even be part of the qualifier (esp. in plaintext). The digest of the headers should be sufficient, if sending anything at all.
It might be worth raising this issue with the Remote Execution API working group (but we don't have to block on it).
There was a problem hiding this comment.
Unfortunately, I think it will be confusing no matter what we do. What's the expected behavior if you set both --experimental_credential_helper and the Starlark attribute, but don't set the incompatible flag? Forwarding only one of the headers is guaranteed to be a surprise to someone, even if we make a deterministic choice.
That being said, I won't be opposed to adding the incompatible flag if you so insist. The PR otherwise LGTM (but I'd like @Wyverald to chip in as well, in case there's some subtlety around repository fetching that I don't fully understand).
There was a problem hiding this comment.
I prefer the incompatible flag. Note that I plan to also send a map<string, list<string>> if the incompatible flag is set even if there's no helper involved (in which case we are consistent and unsurprising eventually).
There was a problem hiding this comment.
To confirm, we don't need an incompatible flag for the repo fetching case, right? The Starlark API can just accept both types.
There was a problem hiding this comment.
Yes, I think that would work (although, haven't tried).
There was a problem hiding this comment.
(I assume you're talking about repository_ctx.download(auth={...}) here)
|
@Wyverald Could you please also take a look at this PR, in particular the bits that affect repository fetching? |
Headers can generally be specified multiple times, not just once. This refactoring is prerequisite for adding support for getting credentials from the credential helper. Note that this does not change the Starlark API or the qualifier for the gRPC remote downloader (both of which need to go through the process for incompatible changes - which should hopefully be pretty straight forward to do as this feature doesn't seem to be used that much AFAICT). Progress on bazelbuild#15856 Closes bazelbuild#16260. PiperOrigin-RevId: 474770302 Change-Id: I74620e36481414a41108991bc61321d104a6d39a
Headers can generally be specified multiple times, not just once.
This refactoring is prerequisite for adding support for getting credentials from the credential helper.
Note that this does not change the Starlark API or the qualifier for the gRPC remote downloader (both of which need to go through the process for incompatible changes - which should hopefully be pretty straight forward to do as this feature doesn't seem to be used that much AFAICT).
Progress on #15856