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

Add a CallOption for modifying the :authority header #5787

Closed
wants to merge 1 commit into from

Conversation

110y
Copy link

@110y 110y commented Nov 10, 2022

This PR just adds a CallOption that makes us able to modify the :authority header per RPC.

Related: #5361

My use case for modifying :authority header per RPC

I'm using Google Cloud Run, a managed container platform, for running my applications.
Google Cloud Run creates a dedicated domain for each deployment like my-service-xxx.run.app.
In addition to this dedicated domain, Google Cloud Run also accepts a HTTP (gRPC) request on the single domain run.app and routes a request based on the :authority header like below:

2022-11-10-grpc-go-pr drawio

This dynamic routing capability is very useful if grpc-go has a CallOption for modifying the :authority header per RPC instead of the per connection basis.
For example, let's assume we have two gRPC Servers, Server A and B, and develop two features for Server B simultaneously. When we deploy each feature as a dedicated server (B-1 and B-2) and if Server A can modify the :authority header for Server B based on its incoming header (metadata), developers can choose which Servers (B-1 or B-2) does Server A send the request dynamically like below:

2022-11-10-grpc-go-pr drawio (3)

To achieve this dynamic routing, we can use the Authority CallOption introduced by this PR like below:

conn, err := grpc.DialContext(ctx, "run.app:443", grpc.WithDefaultCallOptions(grpc.Authority(func(ctx context.Context, authority string) string {
	md, ok := metadata.FromIncomingContext(ctx)
	if !ok {
		return authority
	}

	headers := md.Get("test-service-b-target")
	if len(headers) == 0 {
		return authority
	}

	return fmt.Sprintf("service-b-%s-xxx.run.app", headers[0])
})))

I know there are concerns about modifying the :authority regarding TLS as described in this issue, but I think adding a CallOption for modifying authority might be able to be implemented independently.

P.S.
I know this PR does not include any test for the new CallOption. If this PR is acceptable I'd like to finalize this PR by writing tests, so I would appreciate it if anyone could let me know the right place to write the test for this type of change.

@@ -233,6 +234,29 @@ func (o TrailerCallOption) after(c *callInfo, attempt *csAttempt) {
*o.TrailerAddr = attempt.s.Trailer()
}

// Authority returns a CallOption that mofifies the :authority header for a RPC.
func Authority(modifier func(ctx context.Context, authority string) string) CallOption {
Copy link
Author

@110y 110y Nov 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not fully confident about the signature of this modifier function (this is a user-facing API), but at least, I think it's worth making it have context.Context as its argument since we can use values inside the Context (e.g. metadata) for :authority modification as I described in the description of this PR.

@110y
Copy link
Author

110y commented Nov 10, 2022

I think this PR needs to be tweaked after #5748 has been merged.

@easwars easwars self-assigned this Nov 10, 2022
@easwars
Copy link
Contributor

easwars commented Nov 17, 2022

I know there are concerns about modifying the :authority regarding TLS as described in #5361, but I think adding a CallOption for modifying authority might be able to be implemented independently.

The concerns mentioned in that issue are valid and we would not want to introduce a call option to override the :authority header, without performing the checks mentioned in that issue. Unless the transport credentials agrees with the :authority override, it is not a wise thing to be doing.

Therefore, this PR as it stands currently, is not what we can move forward with. Please let us know if you are willing to update this PR with all the functionality described in #5361.

Thank you.

@easwars easwars assigned 110y and unassigned easwars Nov 17, 2022
@dfawley dfawley added this to the 1.52 Release milestone Nov 18, 2022
@easwars easwars closed this Nov 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants