Pass --remote_header's headers to grpc endpoints #10015

AlessandroPatti · 2019-10-14T09:55:42Z

Extends #8245 to gRPC.

buchgr

Thanks for the PR! Can you please add a test to ensure that this actually works and we don't regress?

Can you please explain why headers for caching and execution have different flags? I am somewhat surprised given that Bazel doesn't allow you to mix caching and execution in one invocation.

buchgr · 2019-10-15T09:24:28Z

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

+      }
+      if (remoteOptions.remoteCacheHeaders.size() > 0) {
+        cacheInterceptors = new ArrayList<>(interceptors);
+        cacheInterceptors.add(createExtraHeadersClientInterceptor(remoteOptions.remoteCacheHeaders));


Instead of adding the headers in the module, consider passing the headers to the RemoteSpawnRunner. That way it's easier to write a unit test.

AlessandroPatti · 2019-10-18T08:50:20Z

Can you please explain why headers for caching and execution have different flags? I am somewhat surprised given that Bazel doesn't allow you to mix caching and execution in one invocation.

Bazel does allow to have two different endpoints for cache and executor doesn't it? The idea there was that if both --remote_cache=<host> and --remote_executor=<host>, it is possible to attach different headers to the cache and execution requests.

buchgr · 2019-10-18T09:03:00Z

@AlessandroPatti yes we do but I don't know of anyone setting these two to different values or both at the same time. If you set --remote_executor then --remote_cache automatically defaults to the value of --remote_executor.

So for now I'd prefer to just reuse --remote_headers flag for everything until the use case comes up.

simon0191 · 2019-10-18T09:39:05Z

@AlessandroPatti yes we do but I don't know of anyone setting these two to different values or both at the same time. If you set --remote_executor then --remote_cache automatically defaults to the value of --remote_executor.

So for now I'd prefer to just reuse --remote_headers flag for everything until the use case comes up.

We are using separate services for the CAS and the Scheduler, and therefore require different endpoints for both. I though that Bazel now creates 2 different connections.
CC @eliangidoni @AlessandroPatti

AlessandroPatti · 2019-10-18T09:39:39Z

We do have such use case already. We want to add different headers to cache and execution requests. Now that I think about it, it is not really necessary for the two endpoint to be different, different headers might be necessary according to the request type.

buchgr · 2019-10-18T10:57:06Z

Makes sense. Let's figure out how to keep flags sane then :-). I don't want to have 3 different flags.

How about:

--remote_header: if set applies to all requests for both http and grpc.
--remote_exec_header: if set applies only to Execute() RPCs. Allows to override a header set in --remote_header.

simon0191 · 2019-10-21T17:40:37Z

If we want to pass a header for the CAS and a header for execution the request would look like:

bazel build --remote_header='CAS_SPECIFIC_HEADER=XXX' --remote_exec_header='EXEC_SPECIFIC_HEADER=YYY'

I think it would be better to have a flag that allow us to specify a cas specific header so that it's more explicit what's happening.

bazel build --remote_cas_header='CAS_SPECIFIC_HEADER=XXX' --remote_exec_header='EXEC_SPECIFIC_HEADER=YYY'

And, alternatively have a --remote_header=GENERIC_HEADER=ZZZ which will be passed to both, CAS and Executor, but will be overriden by the specific ones.

AlessandroPatti · 2019-10-28T15:09:02Z

If we want to pass a header for the CAS and a header for execution the request would look like:

bazel build --remote_header='CAS_SPECIFIC_HEADER=XXX' --remote_exec_header='EXEC_SPECIFIC_HEADER=YYY'

I think it would be better to have a flag that allow us to specify a cas specific header so that it's more explicit what's happening.

bazel build --remote_cas_header='CAS_SPECIFIC_HEADER=XXX' --remote_exec_header='EXEC_SPECIFIC_HEADER=YYY'

And, alternatively have a --remote_header=GENERIC_HEADER=ZZZ which will be passed to both, CAS and Executor, but will be overriden by the specific ones.

I'd also add that it would not be possible to pass a header to the CAS and not to the executor.

@buchgr

simon0191 · 2019-11-13T11:25:11Z

@werkt thoughts on this?

werkt · 2019-11-13T14:25:25Z

src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java

+      List<ClientInterceptor> execInterceptors = interceptors;
+      List<ClientInterceptor> cacheInterceptors = interceptors;


This will use the same referenced list for all of the interceptors, which definitely is not what you want. You would need separate List instances, likely with interceptors populated below copied in, to effect different sets of interceptors.

I see the reassignment below accomplishes this, but a bit awkwardly. Composition via a list view would be preferred, as jakob mentioned below, as a parameter to RemoteSpawnRunner instantiation, just so you don't have the potential for reference duplication, and don't have to rely on reference equality.

I am not sure what you meant here. How should the headers be passed with the RemoteSpawnRunner? Also, that module takes care only of remote execution, doesn't it? In that case, how to pass the headers to the cache too?

Coordinated offline. New changes look good, ty.

AlessandroPatti · 2019-11-25T16:29:39Z

Moved the code outside the RemoteModule class and added some tests. @buchgr can you please review the changes?

elianuber · 2019-11-26T12:22:52Z

cc @ola-rozenfeld @ishikhman
Could you take a look at this PR ? It seems that the most important comments were fixed by @AlessandroPatti

rizzatti · 2019-12-09T12:38:25Z

👍

AlessandroPatti · 2019-12-18T12:09:14Z

@buchgr Could you please review these changes? It is the last missing piece for us and we'd love to see it in the next release

Extends bazelbuild#8245 to gRPC. Closes bazelbuild#10015. PiperOrigin-RevId: 286175898

Following #10015. Some requests do not use the custom headers. Closes #10634. PiperOrigin-RevId: 298574179

AlessandroPatti requested review from buchgr, ishikhman and ola-rozenfeld as code owners October 14, 2019 09:55

googlebot added the cla: yes label Oct 14, 2019

jin added the team-Remote-Exec Issues and PRs for the Execution (Remote) team label Oct 14, 2019

jin assigned buchgr Oct 14, 2019

AlessandroPatti force-pushed the apatti/grpc/remote-headers branch from a7dde66 to 08a3cd2 Compare October 15, 2019 08:45

buchgr reviewed Oct 15, 2019

View reviewed changes

werkt reviewed Nov 13, 2019

View reviewed changes

AlessandroPatti force-pushed the apatti/grpc/remote-headers branch 2 times, most recently from b149fd2 to 9873e84 Compare November 25, 2019 13:06

Pass --remote_header's headers to grpc endpoints

19a4d2e

AlessandroPatti force-pushed the apatti/grpc/remote-headers branch from 9873e84 to 19a4d2e Compare November 25, 2019 13:12

Alessandro Patti added 2 commits November 25, 2019 16:13

Add tests for grpc exec extra headers

9b03698

Add tests for grpc cache extra headers

81278bf

ola-rozenfeld removed their request for review December 9, 2019 17:06

bazel-io closed this in 38a7321 Dec 18, 2019

AlessandroPatti deleted the apatti/grpc/remote-headers branch December 23, 2019 10:58

AlessandroPatti pushed a commit to uber-common/bazel that referenced this pull request Jan 13, 2020

Pass --remote_header's headers to grpc endpoints

ce44f66

Extends bazelbuild#8245 to gRPC. Closes bazelbuild#10015. PiperOrigin-RevId: 286175898

AlessandroPatti mentioned this pull request Jan 22, 2020

Intercept capabilities and uploader requests and add custom grpc headers #10634

Closed

bazel-io pushed a commit that referenced this pull request Mar 3, 2020

Intercept capabilities and uploader requests and add custom grpc headers

52c8773

Following #10015. Some requests do not use the custom headers. Closes #10634. PiperOrigin-RevId: 298574179

This was referenced Mar 5, 2020

JWT based authentication support buildbarn/bb-storage#6

Closed

pkg/grpc: Add an Authenticator for OAuth2/OIDC/JWTs buildbarn/bb-storage#48

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass --remote_header's headers to grpc endpoints #10015

Pass --remote_header's headers to grpc endpoints #10015

AlessandroPatti commented Oct 14, 2019

buchgr left a comment

buchgr Oct 15, 2019

AlessandroPatti commented Oct 18, 2019

buchgr commented Oct 18, 2019

simon0191 commented Oct 18, 2019

AlessandroPatti commented Oct 18, 2019

buchgr commented Oct 18, 2019

simon0191 commented Oct 21, 2019

AlessandroPatti commented Oct 28, 2019

simon0191 commented Nov 13, 2019

werkt Nov 13, 2019

werkt Nov 13, 2019

AlessandroPatti Nov 14, 2019

werkt Nov 25, 2019

AlessandroPatti commented Nov 25, 2019

elianuber commented Nov 26, 2019

rizzatti commented Dec 9, 2019

AlessandroPatti commented Dec 18, 2019

		List<ClientInterceptor> execInterceptors = interceptors;
		List<ClientInterceptor> cacheInterceptors = interceptors;

Pass --remote_header's headers to grpc endpoints #10015

Pass --remote_header's headers to grpc endpoints #10015

Conversation

AlessandroPatti commented Oct 14, 2019

buchgr left a comment

Choose a reason for hiding this comment

buchgr Oct 15, 2019

Choose a reason for hiding this comment

AlessandroPatti commented Oct 18, 2019

buchgr commented Oct 18, 2019

simon0191 commented Oct 18, 2019

AlessandroPatti commented Oct 18, 2019

buchgr commented Oct 18, 2019

simon0191 commented Oct 21, 2019

AlessandroPatti commented Oct 28, 2019

simon0191 commented Nov 13, 2019

werkt Nov 13, 2019

Choose a reason for hiding this comment

werkt Nov 13, 2019

Choose a reason for hiding this comment

AlessandroPatti Nov 14, 2019

Choose a reason for hiding this comment

werkt Nov 25, 2019

Choose a reason for hiding this comment

AlessandroPatti commented Nov 25, 2019

elianuber commented Nov 26, 2019

rizzatti commented Dec 9, 2019

AlessandroPatti commented Dec 18, 2019