Implemented flushing interval #23

agentgonzo · 2019-01-23T16:14:36Z

Flush responses for streaming upstreams

Description

When proxying streaming responses, it would not flush the response writer buffer until some seemingly random point (maybe the number of bytes?). This makes it flush every 1 second by default, but with a configurable interval.

Motivation and Context

If you have an upstream that drip-feeds data (eg, a jenkins instance that streams its logs as they get generated), the proxy would lag behind, presumably until a buffer got filled enough for it to flush it.

How Has This Been Tested?

Manually tested against an upstream that responded with streamed data over the course of many seconds.

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

When proxying streaming responses, it would not flush the response writer buffer until some seemingly random point (maybe the number of bytes?). This makes it flush every 1 second by default, but with a configurable interval.

JoelSpeed

I'd like to change the default flag value to preserve existing behaviour which means there's a few changes to be made. But am generally happy with it.

The only thing I'm considering is that having this new single option to the NewReverseProxy like that seems a bit ugly (when considering there are helpers that configure it already) and I'm wondering if we can refactor that somehow. Any thoughts on this @syscll?

JoelSpeed · 2019-01-25T14:32:09Z

main.go

@@ -43,6 +43,7 @@ func main() {
 	flagSet.Bool("skip-provider-button", false, "will skip sign-in-page to directly reach the next step: oauth/start")
 	flagSet.Bool("skip-auth-preflight", false, "will skip authentication for OPTIONS requests")
 	flagSet.Bool("ssl-insecure-skip-verify", false, "skip validation of certificates presented when using HTTPS")
+	flagSet.Duration("flush-interval", time.Duration(1)*time.Second, "period between response flushing when streaming responses")


I think this should default to 0 rather than 1 since this technically changes the behaviour. I appreciate it improves the behaviour but someone may be relying on the old behaviour. (You also won't need any of that casting stuff there since Durations are int64 anyway)

Suggested change

flagSet.Duration("flush-interval", time.Duration(1)*time.Second, "period between response flushing when streaming responses")

flagSet.Duration("flush-interval", 0, "period between response flushing when streaming responses")

I understand keeping things backwardly compatible, but TBH the existing behaviour sucks and I would consider a bug. When proxying logs, it doesn't even flush on newlines - you just get the response stopping halfway through a word.

I'd strongly urge the default to be 1s (from experience of using it) but if you still want it to be 0, let me know here and I'll change it.

I think you are right, the existing behaviour sucks, maybe I'm being too cautious but I think for now I'd like to make the default 0, with the view to changing it to 1 after the change has had some further testing and we have given warning that it will be changing (we should put a note in the release note that the plan is to flip the default in the next release). WDYT @agentgonzo @syscll ?

In honesty, I think you're being too cautious. :-)
The existing behaviour is bad - without putting a too finer point on it, if people are relying on this (bad) behaviour, then something is very broken in what they are trying to do achieve with their deployments. I see no reasonable situation where you would want the existing behaviour

JoelSpeed · 2019-01-25T14:36:08Z

oauthproxy_test.go

@@ -38,7 +38,7 @@ func TestNewReverseProxy(t *testing.T) {
 	backendHost := net.JoinHostPort(backendHostname, backendPort)
 	proxyURL, _ := url.Parse(backendURL.Scheme + "://" + backendHost + "/")

-	proxyHandler := NewReverseProxy(proxyURL)
+	proxyHandler := NewReverseProxy(proxyURL, time.Second)


Unless we are testing the flushing can we set the value to 0 as would be the default.

JoelSpeed · 2019-01-25T14:36:32Z

oauthproxy_test.go

@@ -60,7 +60,7 @@ func TestEncodedSlashes(t *testing.T) {
 	defer backend.Close()

 	b, _ := url.Parse(backend.URL)
-	proxyHandler := NewReverseProxy(b)
+	proxyHandler := NewReverseProxy(b, time.Second)


Same as above

JoelSpeed · 2019-01-25T14:37:49Z

CHANGELOG.md

@@ -10,6 +10,8 @@
 - [#21](https://github.com/pusher/oauth2_proxy/pull/21) Docker Improvement (@yaegashi)
  - Move Docker base image from debian to alpine
  - Install ca-certificates in docker image
+- [#23](https://github.com/pusher/oauth2_proxy/pull/21) Flushed streaming responses
+  - Long-running upstream responses will get flushed every <timeperiod> (1 second by default)


This will need rewording to reflect the 0 second default

loshz · 2019-01-25T15:28:20Z

The only thing I'm considering is that having this new single option to the NewReverseProxy like that seems a bit ugly (when considering there are helpers that configure it already) and I'm wondering if we can refactor that somehow. Any thoughts on this @syscll?

I suppose we could use a helper method, or rely on callers of this functions to set the proxy.FlushInterval manually. But, I don't mind the current approach as it's explicit.

Could also have a new wrapper func:

 func NewReverseProxyWithFlushInterval(target *url.URL, flushInterval time.Duration) *httputil.ReverseProxy {

But I'm not sure if I like doing this like that.

JoelSpeed · 2019-01-25T15:32:30Z

But, I don't mind the current approach as it's explicit.

Yeah cool, so @agentgonzo let's just change the flag value for the default and we will get this one merged. Thanks!

# Conflicts: # CHANGELOG.md

jtnord · 2019-01-31T13:55:49Z

I agree with @agentgonzo here - the default means that the proxy is broken out of the box with streaming which is just bad behaviour. If people want the broken behaviour for some reason they can opt into it - but forcing users to opt out of brokenness to get something that works is just weird.

loshz · 2019-01-31T13:58:00Z

After thinking about this a little more, I also agree with @agentgonzo and @jtnord.

JoelSpeed

Let's leave it as is then

Set SSI ON for dev

Implemented flushing interval

f55515e

When proxying streaming responses, it would not flush the response writer buffer until some seemingly random point (maybe the number of bytes?). This makes it flush every 1 second by default, but with a configurable interval.

agentgonzo requested a review from a team January 23, 2019 16:14

agentgonzo added 2 commits January 23, 2019 16:16

flushing CHANGELOG

34fa893

gofmt and goimports

c4dca63

JoelSpeed requested changes Jan 25, 2019

View reviewed changes

JoelSpeed requested a review from loshz January 25, 2019 14:40

JoelSpeed assigned loshz and JoelSpeed Jan 25, 2019

JoelSpeed added the enhancement label Jan 25, 2019

Merge remote-tracking branch 'origin/master' into flush

a08b342

# Conflicts: # CHANGELOG.md

JoelSpeed approved these changes Jan 31, 2019

View reviewed changes

loshz approved these changes Jan 31, 2019

View reviewed changes

JoelSpeed merged commit 01c5f5a into oauth2-proxy:master Jan 31, 2019

loshz mentioned this pull request Feb 8, 2019

Update release notes for v3.1.0 #50

Merged

3 tasks

michael-freidgeim-webjet pushed a commit to MNF/oauth2-proxy that referenced this pull request Sep 4, 2021

Merge pull request oauth2-proxy#23 from Webjet/ssi

c7a0b21

Set SSI ON for dev

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented flushing interval #23

Implemented flushing interval #23

agentgonzo commented Jan 23, 2019 •

edited

JoelSpeed left a comment

JoelSpeed Jan 25, 2019

agentgonzo Jan 25, 2019

agentgonzo Jan 25, 2019

JoelSpeed Jan 29, 2019

agentgonzo Jan 29, 2019

JoelSpeed Jan 25, 2019

JoelSpeed Jan 25, 2019

JoelSpeed Jan 25, 2019

loshz commented Jan 25, 2019

JoelSpeed commented Jan 25, 2019

jtnord commented Jan 31, 2019

loshz commented Jan 31, 2019

JoelSpeed left a comment

	flagSet.Duration("flush-interval", time.Duration(1)*time.Second, "period between response flushing when streaming responses")
	flagSet.Duration("flush-interval", 0, "period between response flushing when streaming responses")

Implemented flushing interval #23

Implemented flushing interval #23

Conversation

agentgonzo commented Jan 23, 2019 • edited

Description

Motivation and Context

How Has This Been Tested?

Checklist:

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loshz commented Jan 25, 2019

JoelSpeed commented Jan 25, 2019

jtnord commented Jan 31, 2019

loshz commented Jan 31, 2019

JoelSpeed left a comment

Choose a reason for hiding this comment

agentgonzo commented Jan 23, 2019 •

edited