-
Notifications
You must be signed in to change notification settings - Fork 351
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
Add flag for Oauth request timeout #707
Changes from 1 commit
3fee150
af4dbc5
46406e5
8b20015
369593f
be285b2
b4d7ca8
a48b04e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,10 +128,12 @@ const ( | |
kubernetesNamespaceUsage = "watch only this namespace for ingresses" | ||
|
||
// OAuth2: | ||
oauthURLUsage = "OAuth2 URL for Innkeeper authentication" | ||
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 | ||
oauthURLUsage = "OAuth2 URL for Innkeeper authentication" | ||
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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's add some info what a value of 0 means - no timeout, stdlib default (how much?), etc. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it enough to add the timeout value of 1500ms from |
||
|
||
// connections, timeouts: | ||
idleConnsPerHostUsage = "maximum idle connections per backend host" | ||
|
@@ -235,10 +237,11 @@ var ( | |
kubernetesNamespace string | ||
|
||
// OAuth2: | ||
oauthURL string | ||
oauthScope string | ||
oauthCredentialsDir string | ||
oauth2TokeninfoURL string | ||
oauthURL string | ||
oauthScope string | ||
oauthCredentialsDir string | ||
oauth2TokeninfoURL string | ||
oauth2TokeninfoTimeout time.Duration | ||
|
||
// connections, timeouts: | ||
idleConnsPerHost int | ||
|
@@ -344,6 +347,7 @@ func init() { | |
flag.StringVar(&oauthScope, "oauth-scope", "", oauthScopeUsage) | ||
flag.StringVar(&oauthCredentialsDir, "oauth-credentials-dir", "", oauthCredentialsDirUsage) | ||
flag.StringVar(&oauth2TokeninfoURL, "oauth2-tokeninfo-url", "", oauth2TokeninfoURLUsage) | ||
flag.DurationVar(&oauth2TokeninfoTimeout, "oauth2-tokeninfo-timeout", defaultOAuthTokeninfoTimeout, oauth2TokeninfoTimeoutUsage) | ||
|
||
// connections, timeouts: | ||
flag.IntVar(&idleConnsPerHost, "idle-conns-num", proxy.DefaultIdleConnsPerHost, idleConnsPerHostUsage) | ||
|
@@ -499,10 +503,11 @@ func main() { | |
KubernetesNamespace: kubernetesNamespace, | ||
|
||
// OAuth2: | ||
OAuthUrl: oauthURL, | ||
OAuthScope: oauthScope, | ||
OAuthCredentialsDir: oauthCredentialsDir, | ||
OAuthTokeninfoURL: oauth2TokeninfoURL, | ||
OAuthUrl: oauthURL, | ||
OAuthScope: oauthScope, | ||
OAuthCredentialsDir: oauthCredentialsDir, | ||
OAuthTokeninfoURL: oauth2TokeninfoURL, | ||
OAuthTokeninfoTimeout: oauth2TokeninfoTimeout, | ||
|
||
// connections, timeouts: | ||
IdleConnectionsPerHost: idleConnsPerHost, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import ( | |
"net/http" | ||
"net/url" | ||
"strings" | ||
"time" | ||
|
||
log "github.com/sirupsen/logrus" | ||
"github.com/zalando/skipper/filters" | ||
|
@@ -49,13 +50,15 @@ const ( | |
|
||
type ( | ||
authClient struct { | ||
url *url.URL | ||
url *url.URL | ||
timeout time.Duration | ||
} | ||
|
||
tokeninfoSpec struct { | ||
typ roleCheckType | ||
tokeninfoURL string | ||
authClient *authClient | ||
typ roleCheckType | ||
tokeninfoURL string | ||
tokenInfoTimeout time.Duration | ||
authClient *authClient | ||
} | ||
|
||
filter struct { | ||
|
@@ -147,7 +150,7 @@ func intersect(left []string, right []string) bool { | |
|
||
// 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{}) error { | ||
func jsonGet(url *url.URL, auth string, doc interface{}, timeout time.Duration) error { | ||
if auth != "" { | ||
q := url.Query() | ||
q.Set(accessTokenQueryKey, auth) | ||
|
@@ -159,7 +162,14 @@ func jsonGet(url *url.URL, auth string, doc interface{}) error { | |
return err | ||
} | ||
|
||
rsp, err := http.DefaultClient.Do(req) | ||
var client *http.Client | ||
if timeout != 0 { | ||
client = &http.Client{Timeout: timeout} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will not failover DNS if someone will change the traffic to another loadbalancer for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, right now it’s the same problem, but we have an open issue golang/go#23427 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @szuecs do I need to create the same Transport dialer in the auth filter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @addityasingh you don't need the same, but you need the |
||
} else { | ||
client = http.DefaultClient | ||
} | ||
|
||
rsp, err := client.Do(req) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -173,50 +183,50 @@ func jsonGet(url *url.URL, auth string, doc interface{}) error { | |
return d.Decode(doc) | ||
} | ||
|
||
func newAuthClient(baseURL string) (*authClient, error) { | ||
func newAuthClient(baseURL string, timeout time.Duration) (*authClient, error) { | ||
u, err := url.Parse(baseURL) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return &authClient{url: u}, nil | ||
return &authClient{url: u, timeout: timeout}, nil | ||
} | ||
|
||
func (ac *authClient) getTokeninfo(token string) (map[string]interface{}, error) { | ||
var a map[string]interface{} | ||
err := jsonGet(ac.url, token, &a) | ||
err := jsonGet(ac.url, token, &a, ac.timeout) | ||
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) filters.Spec { | ||
return &tokeninfoSpec{typ: checkOAuthTokeninfoAllScopes, tokeninfoURL: OAuthTokeninfoURL} | ||
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) filters.Spec { | ||
return &tokeninfoSpec{typ: checkOAuthTokeninfoAnyScopes, tokeninfoURL: OAuthTokeninfoURL} | ||
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) filters.Spec { | ||
return &tokeninfoSpec{typ: checkOAuthTokeninfoAllKV, tokeninfoURL: OAuthTokeninfoURL} | ||
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) filters.Spec { | ||
return &tokeninfoSpec{typ: checkOAuthTokeninfoAnyKV, tokeninfoURL: OAuthTokeninfoURL} | ||
func NewOAuthTokeninfoAnyKV(OAuthTokeninfoURL string, OAuthTokeninfoTimeout time.Duration) filters.Spec { | ||
return &tokeninfoSpec{typ: checkOAuthTokeninfoAnyKV, tokeninfoURL: OAuthTokeninfoURL, tokenInfoTimeout: OAuthTokeninfoTimeout} | ||
} | ||
|
||
func (s *tokeninfoSpec) Name() string { | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this log line |
||
|
||
ac, err := newAuthClient(s.tokeninfoURL, s.tokenInfoTimeout) | ||
if err != nil { | ||
return nil, filters.ErrInvalidFilterParameters | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,15 +100,20 @@ logging. See the documentation at: | |
|
||
Getting the code with the test dependencies (`-t` switch): | ||
|
||
1. Get skipper using `go get` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aryszka Can you check if this makes sense? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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? |
||
```sh | ||
go get -t github.com/zalando/skipper/... | ||
|
||
Build and test all packages: | ||
|
||
cd src/github.com/zalando/skipper | ||
make deps | ||
make install | ||
``` | ||
2. Install all dependencies using `glide` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we have a make task, 'deps', that installs the dependencies |
||
```sh | ||
glide install | ||
``` | ||
3. To build the packages and skipper cli, run `make` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'd prefer not using the term 'cli'. |
||
4. To build and test all packages: | ||
``` | ||
make shortcheck | ||
|
||
``` | ||
|
||
#### Kubernetes Ingress | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo s/timwout/timeout |
||
OAuthTokeninfoTimeout time.Duration | ||
|
||
// MaxAuditBody sets the maximum read size of the body read by the audit log filter | ||
MaxAuditBody int | ||
} | ||
|
@@ -655,10 +658,10 @@ func Run(o Options) error { | |
} | ||
|
||
if o.OAuthTokeninfoURL != "" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @szuecs I am not sure if I should check for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No you should not check timeout here |
||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAllScope(o.OAuthTokeninfoURL)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAnyScope(o.OAuthTokeninfoURL)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAllKV(o.OAuthTokeninfoURL)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAnyKV(o.OAuthTokeninfoURL)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAllScope(o.OAuthTokeninfoURL, o.OAuthTokeninfoTimeout)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAnyScope(o.OAuthTokeninfoURL, o.OAuthTokeninfoTimeout)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAllKV(o.OAuthTokeninfoURL, o.OAuthTokeninfoTimeout)) | ||
o.CustomFilters = append(o.CustomFilters, auth.NewOAuthTokeninfoAnyKV(o.OAuthTokeninfoURL, o.OAuthTokeninfoTimeout)) | ||
} | ||
o.CustomFilters = append(o.CustomFilters, logfilter.NewAuditLog(o.MaxAuditBody)) | ||
|
||
|
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 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.
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.
+1 and please move the defaults to the defaults section, thanks
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 have updated this to 1500ms