Merge pull request juju#9504 from timClicks/oci-invalidate-credential…

…-on-auth-error juju#9504 We make use of the juju [`HandleCredentialError`][1] callback mechanism to mark credentials as invalid within Juju when that's detected client-side. All API calls that require a [`CallProviderContext`][2] have had this callback added. [1]: https://github.com/juju/juju/provider/common [2]: https://github.com/juju/juju/environs/context ## Description of change This change enables Juju to short circuit subsequent requests to the API with credentials that are no longer valid. This reduces the load on the server by preventing workers (for example) to continually poll the API with requests. The short circuit behaviour makes OCI consistent with the other providers. ## QA steps prdesc bootstrap into oci prdesc deploy a workload prdesc invalidate the user's credentials (via the oracle console?) prdesc attempt to perform another action within juju This should fail fast and further. ## Documentation changes None. This change shouldn't be user-facing, except that authorisation errors are now handled more gracefully. ## Bug reference n/a
MalloZup · Nov 29, 2018 · 2468f78 · 2468f78
2 parents ca80373 + b7782b8
commit 2468f78
Show file tree

Hide file tree

Showing 9 changed files with 310 additions and 45 deletions.
diff --git a/provider/common/errors.go b/provider/common/errors.go
@@ -84,8 +84,7 @@ var AuthorisationFailureStatusCodes = set.NewInts(
 )
 
 // MaybeHandleCredentialError determines if a given error relates to an invalid credential.
-//  If it is, the credential is invalidated and the return bool is true.
-// Original error is returned untouched.
+// If it is, the credential is invalidated and the return bool is true.
 func MaybeHandleCredentialError(isAuthError func(error) bool, err error, ctx context.ProviderCallContext) bool {
 	denied := isAuthError(errors.Cause(err))
 	if ctx != nil && denied {
@@ -98,7 +97,6 @@ func MaybeHandleCredentialError(isAuthError func(error) bool, err error, ctx con
 }
 
 // HandleCredentialError determines if a given error relates to an invalid credential.
-// If it is, the credential is invalidated. Original error is returned untouched.
 func HandleCredentialError(isAuthError func(error) bool, err error, ctx context.ProviderCallContext) {
 	MaybeHandleCredentialError(isAuthError, err, ctx)
 }
diff --git a/provider/oci/common/errors.go b/provider/oci/common/errors.go
@@ -0,0 +1,61 @@
+// Copyright 2018 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details
+
+package common
+
+import (
+	"github.com/juju/collections/set"
+	ociCommon "github.com/oracle/oci-go-sdk/common"
+
+	"github.com/juju/juju/environs/context"
+	"github.com/juju/juju/provider/common"
+)
+
+// Oracle bundles authorisation errors into HTTP 401, HTTP 404 and
+// HTTP 409, without specifically indicating that it's an authorisation
+// failure. Each response includes a Code field that we can match against,
+// but they remain intentionally ambiguous.
+//
+// The codes we match against are:
+//   - HTTP 401 ("NotAuthenticated")
+//   - HTTP 404 ("NotAuthorizedOrNotFound")
+//   - HTTP 409 ("NotAuthorizedOrResourceAlreadyExists")
+//
+// As we're not generating any API calls manually, it's unlikely
+// that we'll be striking URIs that don't exist at all, therefore we assume
+// auth issues are causing the errors.
+//
+// For more details, see https://docs.cloud.oracle.com/iaas/Content/API/References/apierrors.htm
+var authErrorCodes = set.NewStrings(
+	"NotAuthenticated",
+	"NotAuthorizedOrResourceAlreadyExists",
+	"NotAuthorizedOrNotFound",
+)
+
+// IsAuthorisationFailure reports whether the error is related to
+// attempting to access the provider with invalid or expired credentials.
+func IsAuthorisationFailure(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	serviceError, ok := err.(ociCommon.ServiceError)
+	if !ok {
+		// Just to double check, also try the SDK's
+		// implementation. This isn't checked first, because
+		// it is hard to test.
+		serviceError, ok = ociCommon.IsServiceError(err)
+	}
+
+	if ok && authErrorCodes.Contains(serviceError.GetCode()) {
+		return true
+	}
+
+	return false
+}
+
+// HandleCredentialError marks the current credentials as invalid internally
+//  if Oracle believes that they are expired
+func HandleCredentialError(err error, ctx context.ProviderCallContext) {
+	common.HandleCredentialError(IsAuthorisationFailure, err, ctx)
+}
diff --git a/provider/oci/common/errors_test.go b/provider/oci/common/errors_test.go
@@ -0,0 +1,60 @@
+// Copyright 2018 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package common_test
+
+import (
+	"fmt"
+
+	ocicommon "github.com/oracle/oci-go-sdk/common"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/errors"
+	"github.com/juju/juju/provider/oci/common"
+	"github.com/juju/juju/testing"
+	jc "github.com/juju/testing/checkers"
+)
+
+type errorsSuite struct {
+	testing.BaseSuite
+}
+
+var _ = gc.Suite(&errorsSuite{})
+
+type MockServiceError struct {
+	ocicommon.ServiceError
+
+	code string
+}
+
+func (a MockServiceError) Error() string {
+	return fmt.Sprintf("Mocked error %s", a.GetCode())
+}
+
+func (a MockServiceError) GetCode() string { return a.code }
+
+func (s *errorsSuite) TestServiceErrorsCanTriggerIsAuthorisationFailure(c *gc.C) {
+	err := MockServiceError{code: "NotAuthenticated"}
+	result := common.IsAuthorisationFailure(err)
+	c.Assert(result, jc.IsTrue)
+
+	err = MockServiceError{code: "InternalServerError"}
+	result = common.IsAuthorisationFailure(err)
+	c.Assert(result, jc.IsFalse)
+}
+
+func (s *errorsSuite) TestUnknownErrorsDoNotTriggerIsAuthorisationFailure(c *gc.C) {
+	err1 := errors.New("unknown")
+	for _, err := range []error{
+		err1,
+		errors.Trace(err1),
+		errors.Annotate(err1, "really unknown"),
+	} {
+		c.Assert(common.IsAuthorisationFailure(err), jc.IsFalse)
+	}
+}
+
+func (s *errorsSuite) TestNilDoesNotTriggerIsAuthorisationFailure(c *gc.C) {
+	result := common.IsAuthorisationFailure(nil)
+	c.Assert(result, jc.IsFalse)
+}