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

[Feature Request] Support h2c in vhost server #4563

Open
11 tasks
sword-jin opened this issue Nov 28, 2024 · 8 comments · May be fixed by #4606
Open
11 tasks

[Feature Request] Support h2c in vhost server #4563

sword-jin opened this issue Nov 28, 2024 · 8 comments · May be fixed by #4606

Comments

@sword-jin
Copy link
Contributor

Describe the feature request

Like the title, we can enable h2c support in vhost server, there is a PR already

#4561

Describe alternatives you've considered

No response

Affected area

  • Docs
  • Installation
  • Performance and Scalability
  • Security
  • User Experience
  • Test and Release
  • Developer Infrastructure
  • Client Plugin
  • Server Plugin
  • Extensions
  • Others
@sword-jin
Copy link
Contributor Author

@fatedier I think it's safe to wrapper the handler with h2c if we don't want to introduce new optional parameter

@fatedier
Copy link
Owner

Could you provide relevant documentation or examples from other projects? Also, please add an end-to-end (e2e) test case to ensure existing functionality is not affected.

@sword-jin
Copy link
Contributor Author

I see, I will write a e2e test case, I think we can see the real user case in the e2e case

@sword-jin
Copy link
Contributor Author

sword-jin commented Nov 29, 2024

I can't reprocude the bug when running all the components locally, this is my playground https://github.com/sword-jin/caddy-frp-grpc-streaming/blob/be0c28f7a1a0972a6103b8434d43f73ea51f9939/cmd/client/main.go#L46

sequenceDiagram
    autonumber
    actor B as Browser
    box Purple On Cloud
    participant C as Caddy
    participant FS as Frps
    end
    box Grey On Local
    participant FC as Frpc
    participant LS as LocalServer
    end
    
    B ->> C: request 443
    C ->> C: tls termination
    C ->> FS: reverse proxy to 8888
    FS ->>+ FC: notify the client new connection
    FC ->>- FS: new tcp connection
    FS ->> FC: forward request
    FC ->> LocalServer: forward request
Loading

This diagram Illuminates how a request reach out the local server. This repo trys to build the same worfklow, but fortunately it doesn't work.

In the playground, we use a mock certificate, and the client skip the validation

When the frps and caddy on real cloud with a real tls certificate, the client always gets stream error: internal: protocol error: no Grpc-Status trailer: unexpected EOF at https://github.com/sword-jin/caddy-frp-grpc-streaming/blob/be0c28f7a1a0972a6103b8434d43f73ea51f9939/cmd/client/main.go#L46-L48

Form the logs of caddy and frps and local server, the request has been handled successfully, the problem is the trailer header is missing.

What I did to fix is add a h2c layer in frp vhost server and use http2 transport

I check the frps's response, left is without h2c, right is with h2c, the difference is left doesn't have Trailer:Grpc-Status which cause the error

image

@fatedier
Copy link
Owner

fatedier commented Dec 5, 2024

@fatedier I think it's safe to wrapper the handler with h2c if we don't want to introduce new optional parameter

Can the code be modified in this way? And verify that it does not affect other requests?

@sword-jin
Copy link
Contributor Author

"golang.org/x/net/http2/h2c" is completely compatible to the existing http1.1, we can verify it doesn't affect all the current behavior(by current e2e test

but for http2.transparent, we need another option flag

@fatedier
Copy link
Owner

A quick look shows that server-side support for h2c seems compatible and can be directly added. However, proxying to the backend using http2.Transport is incompatible. Perhaps we could split the work and submit the former first.

For proxying to the backend, there are two main approaches to consider:

Achieving compatibility with both, possibly through an upgrade mechanism or a fallback mechanism.
Allowing the backend service to configure its own interaction protocol.
I prefer the second approach, but the current code has limited support in this regard. There has always been a plan for future refactoring to provide more fine-grained configuration support.

@sword-jin
Copy link
Contributor Author

I agree, #4582 adding h2c wapper is compatible, for http2.Transport, I'd like to take a deep look or you can support the second approach if you are free.

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