Add flag for Oauth request timeout #707
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,9 +4,11 @@ import ( | |
| "encoding/json" | ||
| "errors" | ||
| "fmt" | ||
| "net" | ||
| "net/http" | ||
| "net/url" | ||
| "strings" | ||
| "time" | ||
|
|
||
| log "github.com/sirupsen/logrus" | ||
| "github.com/zalando/skipper/filters" | ||
|
|
@@ -49,13 +51,16 @@ const ( | |
|
|
||
| type ( | ||
| authClient struct { | ||
| url *url.URL | ||
| client *http.Client | ||
| quit chan struct{} | ||
| } | ||
|
|
||
| tokeninfoSpec struct { | ||
| typ roleCheckType | ||
| tokeninfoURL string | ||
| tokenInfoTimeout time.Duration | ||
| authClient *authClient | ||
| } | ||
|
|
||
| filter struct { | ||
|
|
@@ -145,9 +150,32 @@ func intersect(left []string, right []string) bool { | |
| return false | ||
| } | ||
|
|
||
| func createHTTPClient(timeout time.Duration, quit chan struct{}) (*http.Client, error) { | ||
| transport := &http.Transport{ | ||
| DialContext: (&net.Dialer{ | ||
| Timeout: timeout, | ||
| }).DialContext, | ||
| } | ||
|
|
||
| go func() { | ||
| for { | ||
| select { | ||
| case <-time.After(10 * time.Second): | ||
| transport.CloseIdleConnections() | ||
| case <-quit: | ||
| return | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| return &http.Client{ | ||
| Transport: transport, | ||
| }, nil | ||
| } | ||
|
|
||
| // jsonGet requests url with access token in the URL query specified | ||
| // by accessTokenQueryKey, if auth was given and writes into doc. | ||
| func jsonGet(url *url.URL, auth string, doc interface{}, client *http.Client) error { | ||
| if auth != "" { | ||
| q := url.Query() | ||
| q.Set(accessTokenQueryKey, auth) | ||
|
|
@@ -159,7 +187,7 @@ func jsonGet(url *url.URL, auth string, doc interface{}) error { | |
| return err | ||
| } | ||
|
|
||
| rsp, err := client.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -173,50 +201,56 @@ func jsonGet(url *url.URL, auth string, doc interface{}) error { | |
| return d.Decode(doc) | ||
| } | ||
|
|
||
| func newAuthClient(baseURL string, timeout time.Duration) (*authClient, error) { | ||
| u, err := url.Parse(baseURL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| quit := make(chan struct{}) | ||
| client, err := createHTTPClient(timeout, quit) | ||
| if err != nil { | ||
| log.Error("Unable to create http client") | ||
| } | ||
| return &authClient{url: u, client: client, quit: quit}, nil | ||
| } | ||
|
|
||
| func (ac *authClient) getTokeninfo(token string) (map[string]interface{}, error) { | ||
| var a map[string]interface{} | ||
| err := jsonGet(ac.url, token, &a, ac.client) | ||
| return a, err | ||
| } | ||
|
|
||
| // NewOAuthTokeninfoAllScope creates a new auth filter specification | ||
| // to validate authorization for requests. Current implementation uses | ||
| // Bearer tokens to authorize requests and checks that the token | ||
| // contains all scopes. | ||
| func NewOAuthTokeninfoAllScope(OAuthTokeninfoURL string, OAuthTokeninfoTimeout time.Duration) filters.Spec { | ||
| return &tokeninfoSpec{typ: checkOAuthTokeninfoAllScopes, tokeninfoURL: OAuthTokeninfoURL, tokenInfoTimeout: OAuthTokeninfoTimeout} | ||
| } | ||
|
|
||
| // NewOAuthTokeninfoAnyScope creates a new auth filter specification | ||
| // to validate authorization for requests. Current implementation uses | ||
| // Bearer tokens to authorize requests and checks that the token | ||
| // contains at least one scope. | ||
| func NewOAuthTokeninfoAnyScope(OAuthTokeninfoURL string, OAuthTokeninfoTimeout time.Duration) filters.Spec { | ||
| return &tokeninfoSpec{typ: checkOAuthTokeninfoAnyScopes, tokeninfoURL: OAuthTokeninfoURL, tokenInfoTimeout: OAuthTokeninfoTimeout} | ||
| } | ||
|
|
||
| // NewOAuthTokeninfoAllKV creates a new auth filter specification | ||
| // to validate authorization for requests. Current implementation uses | ||
| // Bearer tokens to authorize requests and checks that the token | ||
| // contains all key value pairs provided. | ||
| func NewOAuthTokeninfoAllKV(OAuthTokeninfoURL string, OAuthTokeninfoTimeout time.Duration) filters.Spec { | ||
| return &tokeninfoSpec{typ: checkOAuthTokeninfoAllKV, tokeninfoURL: OAuthTokeninfoURL, tokenInfoTimeout: OAuthTokeninfoTimeout} | ||
| } | ||
|
|
||
| // NewOAuthTokeninfoAnyKV creates a new auth filter specification | ||
| // to validate authorization for requests. Current implementation uses | ||
| // Bearer tokens to authorize requests and checks that the token | ||
| // contains at least one key value pair provided. | ||
| func NewOAuthTokeninfoAnyKV(OAuthTokeninfoURL string, OAuthTokeninfoTimeout time.Duration) filters.Spec { | ||
| return &tokeninfoSpec{typ: checkOAuthTokeninfoAnyKV, tokeninfoURL: OAuthTokeninfoURL, tokenInfoTimeout: OAuthTokeninfoTimeout} | ||
| } | ||
|
|
||
| func (s *tokeninfoSpec) Name() string { | ||
|
|
@@ -251,7 +285,7 @@ func (s *tokeninfoSpec) CreateFilter(args []interface{}) (filters.Filter, error) | |
| return nil, filters.ErrInvalidFilterParameters | ||
| } | ||
|
|
||
| ac, err := newAuthClient(s.tokeninfoURL, s.tokenInfoTimeout) | ||
| if err != nil { | ||
| return nil, filters.ErrInvalidFilterParameters | ||
| } | ||
|
|
@@ -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 { | ||
|
There was a problem hiding this comment. A spec can create filters. |
||
| close(s.authClient.quit) | ||
| } | ||
| } | ||
|
|
||
| func (kv kv) String() string { | ||
| var res []string | ||
| for k, v := range kv { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to call
close(quit), I would the following:close(spec.authClient.quit)from this Close() method@addityasingh @szuecs what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach looks clean to me, since we wouldn't need to pass down
quitchannel all the way down fromskipperinstance creation. And since theClose()is onfilterspec, we wouldn't need to bother about the leaking goroutine as well. @szuecs Thoughts?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the
FilterSpecinterface doesn't have theClosemethod, this approach wouldn't work. Check this https://play.golang.org/p/LkpQRoatvvnThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works in the playground, but not in the skipper test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you found out. An interface can only be casted to, if you use the pointer IIRC.