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

Setting Host part of address url to :authority pseudo header #1567

Merged
merged 1 commit into from Aug 6, 2020

Conversation

everpeace
Copy link
Contributor

fixes #1566

What I've changed

This PR sets custom :authority pseudo header with Host part of address url via grpc.WithAuthority dial option.

This is because:

I think current --addr option uses net.URL even though custom connection helpers like kube-pod://, dockercontainer://. So, I think that taking url.Host part wouldn't cause a serial problem.

@everpeace everpeace force-pushed the fix-authority-header branch 2 times, most recently from 9c40be7 to bb037de Compare July 14, 2020 16:19
@tonistiigi
Copy link
Member

@AkihiroSuda wdyt?

If this is better then why isn't it the default in go-grpc?

@tonistiigi tonistiigi added this to the v0.8.0 milestone Jul 21, 2020
grpc-go uses a slightly different naming scheme(https://github.com/grpc/grpc/blob/master/doc/naming.md)
This will end up setting rfc non-complient :authority header to address string (e.g. tcp://127.0.0.1:1234).
So, this commit changes to sets right authority header via WithAuthority DialOption.

Signed-off-by: Shingo Omura <everpeace@gmail.com>
@everpeace
Copy link
Contributor Author

everpeace commented Jul 31, 2020

@tonistiigi @AkihiroSuda I posted #1620 in another approach to fix the issue. Another one doesn't contain any breaking changes. How about it?

@everpeace
Copy link
Contributor Author

@AkihiroSuda oh, thanks for your approval. I'm closing another PR. Thanks.

@everpeace
Copy link
Contributor Author

@tonistiigi @AkihiroSuda Could you kindly merge this PR? Are there any concern about this PR?

@tonistiigi tonistiigi merged commit 279d686 into moby:master Aug 6, 2020
@edrevo
Copy link
Contributor

edrevo commented Aug 28, 2020

A question about how this change propagates: does this change affect the dockerfile frontend or does it affect the buildkit code? I'm asking in order to understand if I need to wait for a new release of buildkit and then wait until that release gets integrated into docker, or if I can just # syntax = docker/dockerfile:N as soon as version N gets released in order to get this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buildctl sends malformed :authority header when connecting to Buildkitd TCP service
4 participants