diff --git a/controller.go b/controller.go index e5567e0..c790160 100644 --- a/controller.go +++ b/controller.go @@ -93,11 +93,10 @@ func newControllerWithVersion(baseURL, apiVersion, apiKey string) (Controller, e Minor: minor, } controller := &controller{client: client, apiVersion: controllerVersion} - // The controllerVersion returned from the function will include any patch version. controller.capabilities, err = controller.readAPIVersionInfo() if err != nil { logger.Debugf("read version failed: %#v", err) - return nil, NewBadVersionInfoError(err) + return nil, errors.Trace(err) } if err := controller.checkCreds(); err != nil { @@ -115,10 +114,8 @@ func newControllerUnknownVersion(args ControllerArgs) (Controller, error) { switch { case err == nil: return controller, nil - case IsBadVersionInfoError(err): - // TODO(babbageclunk): this is bad - it treats transient - // network errors the same as version mismatches. See - // lp:1667095 + case IsUnsupportedVersionError(err): + // This will only come back from readAPIVersionInfo for 410/404. continue default: return nil, errors.Trace(err) @@ -843,9 +840,22 @@ func nextRequestID() int64 { return atomic.AddInt64(&requestNumber, 1) } +func indicatesUnsupportedVersion(err error) bool { + if err == nil { + return false + } + if serverErr, ok := errors.Cause(err).(ServerError); ok { + code := serverErr.StatusCode + return code == http.StatusNotFound || code == http.StatusGone + } + return false +} + func (c *controller) readAPIVersionInfo() (set.Strings, error) { parsed, err := c.get("version") - if err != nil { + if indicatesUnsupportedVersion(err) { + return nil, WrapWithUnsupportedVersionError(err) + } else if err != nil { return nil, errors.Trace(err) } diff --git a/controller_test.go b/controller_test.go index 3928e28..66cb632 100644 --- a/controller_test.go +++ b/controller_test.go @@ -162,6 +162,63 @@ func (s *controllerSuite) TestNewControllerUnsupportedVersionSpecified(c *gc.C) c.Assert(err, jc.Satisfies, IsUnsupportedVersionError) } +func (s *controllerSuite) TestNewControllerNotHidingErrors(c *gc.C) { + // We should only treat 404 and 410 as "this version isn't + // supported". Other errors should be returned up the stack + // unchanged, so we don't confuse transient network errors with + // version mismatches. lp:1667095 + server := NewSimpleServer() + server.AddGetResponse("/api/2.0/users/?op=whoami", http.StatusOK, "underwater woman") + server.AddGetResponse("/api/2.0/version/", http.StatusInternalServerError, "kablooey") + server.Start() + defer server.Close() + + controller, err := NewController(ControllerArgs{ + BaseURL: server.URL, + APIKey: "fake:as:key", + }) + c.Assert(controller, gc.IsNil) + c.Assert(err, gc.ErrorMatches, `ServerError: 500 Internal Server Error \(kablooey\)`) +} + +func (s *controllerSuite) TestNewController410(c *gc.C) { + // We should only treat 404 and 410 as "this version isn't + // supported". Other errors should be returned up the stack + // unchanged, so we don't confuse transient network errors with + // version mismatches. lp:1667095 + server := NewSimpleServer() + server.AddGetResponse("/api/2.0/users/?op=whoami", http.StatusOK, "the answer to all your prayers") + server.AddGetResponse("/api/2.0/version/", http.StatusGone, "cya") + server.Start() + defer server.Close() + + controller, err := NewController(ControllerArgs{ + BaseURL: server.URL, + APIKey: "fake:as:key", + }) + c.Assert(controller, gc.IsNil) + c.Assert(err, jc.Satisfies, IsUnsupportedVersionError) +} + +func (s *controllerSuite) TestNewController404(c *gc.C) { + // We should only treat 404 and 410 as "this version isn't + // supported". Other errors should be returned up the stack + // unchanged, so we don't confuse transient network errors with + // version mismatches. lp:1667095 + server := NewSimpleServer() + server.AddGetResponse("/api/2.0/users/?op=whoami", http.StatusOK, "the answer to all your prayers") + server.AddGetResponse("/api/2.0/version/", http.StatusNotFound, "huh?") + server.Start() + defer server.Close() + + controller, err := NewController(ControllerArgs{ + BaseURL: server.URL, + APIKey: "fake:as:key", + }) + c.Assert(controller, gc.IsNil) + c.Assert(err, jc.Satisfies, IsUnsupportedVersionError) +} + func (s *controllerSuite) TestBootResources(c *gc.C) { controller := s.getController(c) resources, err := controller.BootResources() diff --git a/errors.go b/errors.go index ff83004..be7b09e 100644 --- a/errors.go +++ b/errors.go @@ -65,27 +65,14 @@ func IsUnsupportedVersionError(err error) bool { return ok } -// BadVersionInfoError is more specific than UnsupportedVersionError - -// it means that we couldn't read the version info endpoint, so the -// API version is probably wrong. Used in version selection when -// creating a controller. -type BadVersionInfoError struct { - errors.Err -} - -// NewBadVersionInfoError constructs a new BadVersionInfoError and sets the location. -func NewBadVersionInfoError(err error) error { - uerr := &BadVersionInfoError{Err: errors.NewErr("bad version info: %v", err)} +// WrapWithUnsupportedVersionError constructs a new +// UnsupportedVersionError wrapping the passed error. +func WrapWithUnsupportedVersionError(err error) error { + uerr := &UnsupportedVersionError{Err: errors.NewErr("unsupported version: %v", err)} uerr.SetLocation(1) return errors.Wrap(err, uerr) } -// IsBadVersionInfoError returns true if err is an BadVersionInfoError. -func IsBadVersionInfoError(err error) bool { - _, ok := errors.Cause(err).(*BadVersionInfoError) - return ok -} - // DeserializationError types are returned when the returned JSON data from // the controller doesn't match the code's expectations. type DeserializationError struct { diff --git a/errors_test.go b/errors_test.go index 1174c46..cc6850d 100644 --- a/errors_test.go +++ b/errors_test.go @@ -29,14 +29,6 @@ func (*errorTypesSuite) TestUnexpectedError(c *gc.C) { c.Assert(err.Error(), gc.Equals, "unexpected: wat") } -func (*errorTypesSuite) TestNewBadVersionInfoError(c *gc.C) { - err := errors.New("wat") - err = NewBadVersionInfoError(err) - c.Assert(err, gc.NotNil) - c.Assert(err, jc.Satisfies, IsBadVersionInfoError) - c.Assert(err.Error(), gc.Equals, "bad version info: wat") -} - func (*errorTypesSuite) TestUnsupportedVersionError(c *gc.C) { err := NewUnsupportedVersionError("foo %d", 42) c.Assert(err, gc.NotNil) @@ -44,6 +36,15 @@ func (*errorTypesSuite) TestUnsupportedVersionError(c *gc.C) { c.Assert(err.Error(), gc.Equals, "foo 42") } +func (*errorTypesSuite) TestWrapWithUnsupportedVersionError(c *gc.C) { + err := WrapWithUnsupportedVersionError(errors.New("bad")) + c.Assert(err, gc.NotNil) + c.Assert(err, jc.Satisfies, IsUnsupportedVersionError) + c.Assert(err.Error(), gc.Equals, "unsupported version: bad") + stack := errors.ErrorStack(err) + c.Assert(strings.Split(stack, "\n"), gc.HasLen, 2) +} + func (*errorTypesSuite) TestDeserializationError(c *gc.C) { err := NewDeserializationError("foo %d", 42) c.Assert(err, gc.NotNil)