Add flag for Oauth request timeout #707

addityasingh · 2018-07-16T23:03:39Z

Close #633

Add flag for Oauth request timeout
Fix test for correct HTTP status code
Revisit the HTTPClient used

lmineiro · 2018-07-17T07:30:14Z

Thanks for the improvements. I added a couple of comments.

lmineiro · 2018-07-17T07:17:06Z

cmd/skipper/main.go

+	oauthCredentialsDirUsage     = "directory where oauth credentials are stored: client.json and user.json"
+	oauthScopeUsage              = "the whitespace separated list of oauth scopes"
+	oauth2TokeninfoURLUsage      = "sets the default tokeninfo URL to query information about an incoming OAuth2 token in oauth2Tokeninfo filters"
+	oauth2TokeninfoTimeoutUsage  = "sets the default tokeninfo request timeout duration"


Let's add some info what a value of 0 means - no timeout, stdlib default (how much?), etc.

Is it enough to add the timeout value of 1500ms from defaultOAuthTokeninfoTimeout ?

lmineiro · 2018-07-17T07:23:56Z

cmd/skipper/main.go

-	oauthCredentialsDirUsage = "directory where oauth credentials are stored: client.json and user.json"
-	oauthScopeUsage          = "the whitespace separated list of oauth scopes"
-	oauth2TokeninfoURLUsage  = "sets the default tokeninfo URL to query information about an incoming OAuth2 token in oauth2Tokeninfo filters"
+	defaultOAuthTokeninfoTimeout = 100 * time.Millisecond // Not sure if this makes sense


This seems too short as a default to me. It should be more relaxed by default and be able to accommodate, let's say, the redirects the default client will follow - which count towards the timeout. I propose something more in the range of 1~2 sec as default. End users should customize that for their own use case. If your use case considers more appropriate 100ms, that's the value you should use.

+1 and please move the defaults to the defaults section, thanks

I have updated this to 1500ms

lmineiro · 2018-07-17T07:28:00Z

skipper.go

@@ -433,6 +433,9 @@ type Options struct {
 	// OAuthTokeninfoURL sets the OAuthTokeninfoURL similar to https://godoc.org/golang.org/x/oauth2#Endpoint
 	OAuthTokeninfoURL string

+	// OAuthTokeninfoTimeout sets timwout duration while calling oauth token service


typo s/timwout/timeout

lmineiro · 2018-07-17T07:29:47Z

filters/auth/oauth.go

@@ -251,7 +261,9 @@ func (s *tokeninfoSpec) CreateFilter(args []interface{}) (filters.Filter, error)
 		return nil, filters.ErrInvalidFilterParameters
 	}

-	ac, err := newAuthClient(s.tokeninfoURL)
+	log.Info("Log the timeout ", s.tokenInfoTimeout)


Please remove this log line

szuecs · 2018-07-17T17:10:47Z

filters/auth/oauth.go

-	rsp, err := http.DefaultClient.Do(req)
+	var client *http.Client
+	if timeout != 0 {
+		client = &http.Client{Timeout: timeout}


This will not failover DNS if someone will change the traffic to another loadbalancer for example.
We need to have the workaround as in the kubernetes dataclient or the proxy.
See https://github.com/zalando/skipper/blob/master/dataclients/kubernetes/kube.go#L310-L337.

@szuecs Is this still true since the introduction of Transport.IdleConnTimeout in go 1.7? I think that new setting which defaults to 90secs is good enough to mitigate that problem.

No, right now it’s the same problem, but we have an open issue golang/go#23427

@szuecs do I need to create the same Transport dialer in the auth filter?

@addityasingh you don't need the same, but you need the for {} loop below the transport to close idle connections to make the http client to do a DNS call to make DNS based failover happen.

szuecs

the DNS problem should be fixed and the default as commented from @lmineiro and me cheanged.

addityasingh · 2018-07-19T19:25:00Z

skipper.go

@@ -655,10 +658,10 @@ func Run(o Options) error {
 	}

 	if o.OAuthTokeninfoURL != "" {


@szuecs I am not sure if I should check for the o.OAuthTokeninfoTimeout field here, since we always will have a default. What do you think?

No you should not check timeout here

addityasingh · 2018-07-19T19:25:25Z

readme.md

@@ -100,15 +100,20 @@ logging. See the documentation at:

 Getting the code with the test dependencies (`-t` switch):

+1. Get skipper using `go get`


@aryszka Can you check if this makes sense?

@addityasingh have you checked the 'Working with the code' section below? The reason for this 'Installation' section is left more open, is that 1/ 'go get' actually installs the skipper command in GOPATH/bin, 2/ it leaves the choice between using the latest or the bound version of the dependencies to the user.

This doesn't mean I am not open to improve the readme, but I think it should be a discussion in a separate PR. Can you please move the changes to the readme into a new, separate PR?

addityasingh · 2018-07-19T19:27:00Z

filters/auth/oauth.go

@@ -145,9 +149,35 @@ func intersect(left []string, right []string) bool {
 	return false
 }

+func createHttpClient(timeout time.Duration) *http.Client {
+	var client *http.Client
+	if timeout != 0 {


@szuecs IMO, since we will always have a default value for the OAuthTokeninfoTimeout , it doesn't make sense to have a default client and always use the one with timeout. Thoughts ?

Yes use always timeouts

szuecs · 2018-07-19T21:35:56Z

filters/auth/oauth.go

+
+		client = &http.Client{Transport: transport}
+
+		go func() {


We need to have a quit channel to not leak this goroutine

At which part of the lifecycle should the quit channel be closed?

@szuecs I did not find any lifecycle method in oauth filter similar to Close() method in the kubernetes ingress controller. I have added the quit channel to return from the goroutine but still not sure at which point to send to the quit channel. I checked the Explicit Cancellation in go pipleines concurrency patterns but could not find the point where to trigger the quit channel

We have a sigterm handler and use quit channels right now. Please check the kubernetes dataclient or the client connection pool in proxy/proxy.go

I checked the quit channel implementations in those 2 but the kubernetes/dataclients and proxy/proxy.go have Close() methods, but a filter doesn't. Should I implement a custom dataclient with Close() method for the oauth filter?

aryszka · 2018-07-19T21:36:22Z

readme.md

-    make deps
-    make install
+```
+2. Install all dependencies using `glide`


we have a make task, 'deps', that installs the dependencies

aryszka · 2018-07-19T21:38:17Z

readme.md

+```sh
+glide install
+```
+3. To build the packages and skipper cli, run `make`


i'd prefer not using the term 'cli'.

aryszka · 2018-07-19T21:51:44Z

filters/auth/oauth.go

+
+		client = &http.Client{Transport: transport}
+
+		go func() {


i wonder if this is required. If it is required, then we should make it possible to return from this goroutine.

aryszka · 2018-07-19T21:52:06Z

filters/auth/oauth.go

@@ -159,7 +189,8 @@ func jsonGet(url *url.URL, auth string, doc interface{}) error {
 		return err
 	}

-	rsp, err := http.DefaultClient.Do(req)
+	client := createHttpClient(timeout)


we should not create a new client on every request.

aryszka · 2018-07-19T21:55:07Z

cmd/skipper/main.go

@@ -57,6 +57,9 @@ const (
 	defaultTLSHandshakeTimeoutBackend = 60 * time.Second
 	defaultMaxIdleConnsBackend        = 0

+	// OAuth2:
+	defaultOAuthTokeninfoTimeout = 1500 * time.Millisecond // Not sure if this makes sense


It makes sense, but still a bit opinionated in my view 😄 . How about time.Minute?

I have changed it to 2 * time.Second

aryszka · 2018-07-23T15:19:20Z

filters/auth/oauth.go

 	u, err := url.Parse(baseURL)
 	if err != nil {
 		return nil, err
 	}
-	return &authClient{url: u}, nil
+
+	quit := make(chan struct{})


to call close(quit), I would the following:

add a Close() method to the filterspec

call close(spec.authClient.quit) from this Close() method

for normal skipper operation do nothing, do not call close, because there's going to be only a single instance of the spec

for tests, always call Close() on the spec

document that in case someone uses filter spec as a library in a custom build, they need to call Close() if they may create an arbitrary number of spec instances

@addityasingh @szuecs what do you think?

sounds good to me

This approach looks clean to me, since we wouldn't need to pass down quit channel all the way down from skipper instance creation. And since the Close() is on filterspec, we wouldn't need to bother about the leaking goroutine as well. @szuecs Thoughts?

Since the FilterSpec interface doesn't have the Close method, this approach wouldn't work. Check this https://play.golang.org/p/LkpQRoatvvn

You might be able to type cast it. Keep in mind if you need access from another package you have to export the type.

See: https://play.golang.org/p/BK0kM25jnjb

It works in the playground, but not in the skipper test

I see you found out. An interface can only be casted to, if you use the pointer IIRC.

szuecs · 2018-07-26T08:01:10Z

filters/auth/oauth.go

@@ -280,6 +314,13 @@ func (s *tokeninfoSpec) CreateFilter(args []interface{}) (filters.Filter, error)
 	return f, nil
 }

+// Close cleans-up the quit channel used for this spec
+func (s *tokeninfoSpec) Close() {
+	if s.authClient.quit != nil {


A spec can create filters.
I think you have to close it from the filter, that's why you get a nil ptr panic.
See https://github.com/zalando/skipper/pull/707/files#diff-82513d1c973748c3c6b7fa0677fd3fd4L259, which shows that you store it in the filter and not in the spec.

lmineiro · 2018-07-27T08:40:19Z

👍

aryszka · 2018-07-27T10:20:00Z

👍

aryszka · 2018-07-27T10:20:16Z

Thanks a lot for this PR! 😄

#633 Add flag for Oauth request timeout

3fee150

lmineiro suggested changes Jul 17, 2018

View reviewed changes

szuecs reviewed Jul 17, 2018

View reviewed changes

szuecs requested changes Jul 17, 2018

View reviewed changes

#633 Fix code review comments

af4dbc5

addityasingh commented Jul 19, 2018

View reviewed changes

addityasingh changed the title ~~[WIP]: Add flag for Oauth request timeout~~ Add flag for Oauth request timeout Jul 19, 2018

szuecs reviewed Jul 19, 2018

View reviewed changes

aryszka reviewed Jul 19, 2018

View reviewed changes

addityasingh added 2 commits July 21, 2018 20:17

#633 Revert README and fix code comments

46406e5

#633 Change the defalt timeout

8b20015

aryszka reviewed Jul 23, 2018

View reviewed changes

addityasingh added 2 commits July 25, 2018 19:09

#633 Add quit channel to close on Close() of spec

369593f

#633 Add pointer for tokenInfoSpec

be285b2

szuecs reviewed Jul 26, 2018

View reviewed changes

addityasingh added 2 commits July 26, 2018 22:27

#633 Fix breaking test and add doc

b4d7ca8

#633 Fix breaking tests and change slice implementation

a48b04e

szuecs approved these changes Jul 27, 2018

View reviewed changes

lmineiro approved these changes Jul 27, 2018

View reviewed changes

aryszka merged commit e6d8e2d into zalando:master Jul 27, 2018

addityasingh deleted the add-oauth-timeout-flag branch July 27, 2018 11:56

		@@ -655,10 +658,10 @@ func Run(o Options) error {
		}

		if o.OAuthTokeninfoURL != "" {

		@@ -100,15 +100,20 @@ logging. See the documentation at:

		Getting the code with the test dependencies (`-t` switch):

		1. Get skipper using `go get`

Add flag for Oauth request timeout #707

Add flag for Oauth request timeout #707

Conversation

addityasingh commented Jul 16, 2018 • edited Loading

lmineiro commented Jul 17, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szuecs 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

addityasingh Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

szuecs Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

addityasingh Jul 23, 2018 • edited Loading

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

addityasingh Jul 21, 2018 • edited Loading

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

szuecs Jul 23, 2018 • edited Loading

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

lmineiro commented Jul 27, 2018

aryszka commented Jul 27, 2018

aryszka commented Jul 27, 2018

addityasingh commented Jul 16, 2018 •

edited

Loading

addityasingh Jul 22, 2018 •

edited

Loading

szuecs Jul 22, 2018 •

edited

Loading

addityasingh Jul 23, 2018 •

edited

Loading

addityasingh Jul 21, 2018 •

edited

Loading

szuecs Jul 23, 2018 •

edited

Loading