swift

swift copied to clipboard

While debugging some clients during a keystone outage I noticed that goswift clients always try to authenticate twice before failing. This is due to the changes to fix #21 specifically commit https://github.com/ncw/swift/commit/f7b19fed761cce53042dd89c9d429518ebcb63ad.

I'm using swift with a v3 keystone endpoint and in that case the retry is totally unnecessary as the request does not change and is simply replayed. In general I don't like the idea that the client always retries authentication because one specific implementation (Rackspace) has a funny way of doing things.

I have been thinking about how this could be improved so that the retry is not static and depended on the specific authenticator.

Without changing too much I can think of three ways to tackle this:

1. Make the authentication retry configurable in Connection. Easy to implement but would be difficult to default to the current behaviour of 1 with 0 being the default value ( I don't like tossing around -1's)
2. Add a Retries(c *Connection) bool function to the Authenticator interface. That way the Authenticator can declare if its has alternative ways of authentication based on all config options.
3. Change the Authenticator interface to Request(*Connection) (*http.Request, retry_on_failure, bool, error), which is a variation of 2.

Any feedback on the proposal would this be something worth considering?

Yes that incident wasn't the library's finest hour!

Option 2 sounds like a good one to me. I think Retries should return an integer of how many retries are needed.

This could be an additional interface only implemented by the v2auth which would leave the Authenticator interface unchanged. You'd then test c.Auth for a Retrier interface, something like this...

type Retrier interface {
    Retries() int
}

then

if retrier, ok := c.Auth.(Retrier); ok {
    retries = retrier()
} else {
    retries = 0
}

I'd quite like not to change the public interfaces if possible.

Thinking about this further, perhaps we should revert c2203c0 ? In favour of this plan. I'd like the auth to be retried if possible (transient network errors etc).

Thoughts?

Reverting https://github.com/ncw/swift/commit/c2203c0cfc2831d2f88563c6dd2cfd0f068a78ad would be a mistake. The commit is fixing real bug. When ignoring the error potentially returned by getUrlAndAuthToken the values for targetUrl and authToken are undefined and might just be empty structs with default values.

Re c2203c0 - sorry I didn't think that through properly! I don't think we should revert, but it should retry the auth if it fails (which the logic in this issue removes), so it should make that line look something like

if targetUrl, authToken, err = c.getUrlAndAuthToken(targetUrl, p.OnReAuth); err != nil {
    if retries > 0 {
        retries--
        continue
    }
    return
}

I'm not a fan of built-in retries, I'm not convinced they help in more cases then they do extra useless work and introduce latency. In my experience intermittent network errors are very rare, especially those where connections/requests fail and random and you just have to try 2-3 times for your requests to get through. Its far more likely to have outages of longer duration or permanent errors because of misconfiguration on either client or server side. In those cases the retries just put more strain on the backends by repeating requests that are doomed to fail.

Retrying on any kind of error during authentication, as you suggested does not seem like a good idea to me. If an authentication requests returns a 4xx status code its almost certainly permanent, repeating the same requests again is a waste of resource IMO.

In my experience retrying is a complicated topic that needs careful consideration and backoff and randomisation strategies to be a net positive. Just retrying a few times in a loop is not that effective.

I would be interested in improving this area. Your initial proposal is a step in the right direction - it would be great to see that :-)