Don't return UnsupportedVersionError for unexpected errors from version check

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)
 	}
 

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()

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 {

errors_test.go

@@ -29,21 +29,22 @@ 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)
 	c.Assert(err, jc.Satisfies, IsUnsupportedVersionError)
 	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)