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

reverseproxy: Pull latest proxy changes from stdlib #4266

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Aug 3, 2021

I went through the commits that touched stdlib's reverseproxy.go file in the past couple of years (only a dozen or so), and copied over all the changes that are to code that was copied into Caddy.

The commits I pulled changes from (hopefully apparent from looking at those commits what the changes in this PR are doing):

This may also fix #4247 because of the change to copyResponse to set mlw.flushPending = true right away. /cc @leafac if you'd be willing to try out this PR (you can grab a build from the CI artifacts, or build from this commit with xcaddy) to see if it fixes your usecase.

@francislavoie francislavoie added this to the v2.4.4 milestone Aug 3, 2021
@leafac
Copy link

leafac commented Aug 4, 2021

Oh, nice. I’ll give it a try. Thank you very much!

@mholt
Copy link
Member

mholt commented Aug 4, 2021

These changes look great, thanks Francis. The divergence from bugfixes upstream was something I was worried about. I'll wait to approve this until @leafac reports back whether it resolves his issue.

@mholt mholt added the under review 🧐 Review is pending before merging label Aug 4, 2021
@mholt
Copy link
Member

mholt commented Aug 11, 2021

Should we wait for you, @leafac? (Thanks for trying it out, btw)

@leafac
Copy link

leafac commented Aug 12, 2021

Yeah, sorry about taking too long to answer. I’ll test this first thing when I get to work. 😃

@leafac
Copy link

leafac commented Aug 12, 2021

👍 I tested and I believe it’s working now 🙌

Thank you everyone for your work. You’re awesome!


Here’s what I did in more detail:

  1. I went to this page which includes an artifact called caddy_macOS_go1.16_9c0cd66…. I downloaded it.
  2. I ran this version using my https://github.com/leafac/caddy-express-sse. This is the output I got:
$ curl -Nv https://localhost    
*   Trying ::1...
* TCP_NODELAY set
* Connected to localhost (::1) port 443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-ECDSA-AES256-GCM-SHA384
* ALPN, server accepted to use h2
* Server certificate:
*  subject: [NONE]
*  start date: Aug 12 08:37:31 2021 GMT
*  expire date: Aug 12 20:37:31 2021 GMT
*  subjectAltName: host "localhost" matched cert's "localhost"
*  issuer: CN=Caddy Local Authority - ECC Intermediate
*  SSL certificate verify ok.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x15300f800)
> GET / HTTP/2
> Host: localhost
> User-Agent: curl/7.64.1
> Accept: */*
> 
* Connection state changed (MAX_CONCURRENT_STREAMS == 250)!
< HTTP/2 200 
< content-type: text/event-stream; charset=utf-8
< date: Thu, 12 Aug 2021 08:37:59 GMT
< server: Caddy
< x-powered-by: Express
< 

Note how the headers came right away.

@francislavoie
Copy link
Member Author

Excellent! Thanks for confirming @leafac 🎉

I went through the commits that touched stdlib's `reverseproxy.go` file, and copied over all the changes that are to code that was copied into Caddy.

The commits I pulled changes from:

- golang/go@2cc3473
- golang/go@a5cea06
- golang/go@ecdbffd
- golang/go@2189852
-golang/go@ca3c0df
- golang/go@9c017ff

This may also fix #4247 because of the change to `copyResponse` to set `mlw.flushPending = true` right away.
@mholt mholt removed the under review 🧐 Review is pending before merging label Aug 12, 2021
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, Francis! And thanks for testing it out, @leafac!

@mholt mholt merged commit e6c29ce into master Aug 12, 2021
@mholt mholt deleted the reverseproxy-stdlib-sync branch August 12, 2021 16:48
francislavoie added a commit that referenced this pull request Aug 22, 2021
From reading through the code, I think this code path is now obsoleted by the changes made in #4266.

Basically, `h.flushInterval()` will set the flush interval to `-1` if we're in a bi-directional stream, and the recent PR ensured that `h.copyResponse()` properly flushes headers immediately when the flush interval is non-zero. So now there should be no need to call Flush before calling `h.copyResponse()`.
mholt pushed a commit that referenced this pull request Aug 23, 2021
From reading through the code, I think this code path is now obsoleted by the changes made in #4266.

Basically, `h.flushInterval()` will set the flush interval to `-1` if we're in a bi-directional stream, and the recent PR ensured that `h.copyResponse()` properly flushes headers immediately when the flush interval is non-zero. So now there should be no need to call Flush before calling `h.copyResponse()`.
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.

Caddy Doesn’t Seem to Flush Response Buffer, Breaking Reverse Proxy of Server-Sent Events
3 participants