refactor: use odataerrors and azcore response error

Signed-off-by: Anish Ramasekar <[email protected]>

go.mod

@@ -16,7 +16,6 @@ require (
 	github.com/kelseyhightower/envconfig v1.4.0
 	github.com/microsoft/kiota-authentication-azure-go v0.6.0
 	github.com/microsoft/kiota-http-go v0.16.2
-	github.com/microsoft/kiota-serialization-json-go v0.9.3
 	github.com/microsoftgraph/msgraph-sdk-go v0.61.0
 	github.com/open-policy-agent/cert-controller v0.5.0
 	github.com/pkg/errors v0.9.1
@@ -39,6 +38,7 @@ require (
 require (
 	github.com/Azure/go-autorest/autorest/adal v0.9.23 // indirect
 	github.com/microsoft/kiota-abstractions-go v0.19.1 // indirect
+	github.com/microsoft/kiota-serialization-json-go v0.9.3 // indirect
 )
 
 require (

pkg/cloud/azureclient.go

@@ -152,18 +152,12 @@ func getClient(env azure.Environment, subscriptionID string, credential azcore.T
 		return nil, errors.Wrap(err, "failed to create request adapter")
 	}
 
-	clientOpts := &armpolicy.ClientOptions{
-		ClientOptions: azcore.ClientOptions{
-			Transport: client,
-		},
-	}
-
-	roleAssignmentsClient, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, credential, clientOpts)
+	roleAssignmentsClient, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, credential, nil)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create role assignments client")
 	}
 
-	roleDefinitionsClient, err := armauthorization.NewRoleDefinitionsClient(credential, clientOpts)
+	roleDefinitionsClient, err := armauthorization.NewRoleDefinitionsClient(credential, nil)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to create role definitions client")
 	}

pkg/cloud/errors.go

@@ -5,9 +5,8 @@ import (
 	"net/http"
 	"strings"
 
-	"github.com/Azure/go-autorest/autorest"
-	jsonserialization "github.com/microsoft/kiota-serialization-json-go"
-	"github.com/microsoftgraph/msgraph-sdk-go/models"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors"
 	"github.com/pkg/errors"
 )
 
@@ -20,7 +19,7 @@ const (
 
 // GraphError is a custom error type for Graph API errors.
 type GraphError struct {
-	PublicError *models.PublicError
+	Errorable odataerrors.MainErrorable
 }
 
 // IsNotFound returns true if the given error is a NotFound error.
@@ -31,67 +30,42 @@ func IsNotFound(err error) bool {
 // IsRoleAssignmentAlreadyDeleted returns true if the given error is a role assignment already deleted error.
 // Ref: https://docs.microsoft.com/en-us/rest/api/authorization/role-assignments/delete#response
 func IsRoleAssignmentAlreadyDeleted(err error) bool {
-	derr := autorest.DetailedError{}
+	derr := &azcore.ResponseError{}
 	return errors.As(err, &derr) && derr.StatusCode == http.StatusNoContent
 }
 
-// IsAlreadyExists parses the error message to check if it's resource already exists error.
-func IsAlreadyExists(err error) bool {
-	derr := autorest.DetailedError{}
+// IsRoleAssignmentExists returns true if the given error is a role assignment already exists error.
+func IsRoleAssignmentExists(err error) bool {
+	derr := &azcore.ResponseError{}
 	return errors.As(err, &derr) && derr.StatusCode == http.StatusConflict
 }
 
 // IsFederatedCredentialNotFound returns true if the given error is a federated credential not found error.
 func IsFederatedCredentialNotFound(err error) bool {
 	gerr := GraphError{}
-	return errors.As(err, &gerr) && *gerr.PublicError.GetCode() == GraphErrorCodeResourceNotFound
+	return errors.As(err, &gerr) && *gerr.Errorable.GetCode() == GraphErrorCodeResourceNotFound
 }
 
 // IsFederatedCredentialAlreadyExists returns true if the given error is a federated credential already exists error.
 // E1202 22:40:05.500821  867104 main.go:57] "failed to add federated identity credential" err="code: Request_MultipleObjectsWithSameKeyValue, message: FederatedIdentityCredential with name aramase-default-cred already exists."
 func IsFederatedCredentialAlreadyExists(err error) bool {
 	gerr := GraphError{}
-	return errors.As(err, &gerr) && *gerr.PublicError.GetCode() == GraphErrorCodeMultipleObjectsWithSameKeyValue
+	return errors.As(err, &gerr) && *gerr.Errorable.GetCode() == GraphErrorCodeMultipleObjectsWithSameKeyValue
 }
 
-// GetGraphError returns the public error message from the additional info.
+// maybeExtractGraphError returns the additional information from the graph API error.
 // ref: https://docs.microsoft.com/en-us/graph/errors#error-resource-type
 // errors returned by the graph API aren't serialized today and this is a known issue: https://github.com/microsoftgraph/msgraph-sdk-go-core/issues/1
-func GetGraphError(additionalData map[string]interface{}) (*GraphError, error) {
-	if additionalData == nil || additionalData["error"] == nil {
-		return nil, nil
+func maybeExtractGraphError(err error) error {
+	var oerr *odataerrors.ODataError
+	if errors.As(err, &oerr) {
+		return &GraphError{Errorable: oerr.GetError()}
 	}
-	e := models.NewPublicError()
-	e.SetAdditionalData(additionalData)
 
-	ad := additionalData["error"].(map[string]*jsonserialization.JsonParseNode)
-	// error code string for the error that occurred
-	code, err := ad["code"].GetStringValue()
-	if err != nil {
-		return nil, err
-	}
-	// developer ready message about the error that occurred. This should not be displayed to the user directly.
-	message, err := ad["message"].GetStringValue()
-	if err != nil {
-		return nil, err
-	}
-	// Optional. Additional error objects that may be more specific than the top level error.
-	innerError, err := ad["innerError"].GetObjectValue(models.CreatePublicInnerErrorFromDiscriminatorValue)
-	if err != nil {
-		return nil, err
-	}
-
-	e.SetCode(code)
-	e.SetMessage(message)
-	e.SetInnerError(innerError.(*models.PublicInnerError))
-
-	return &GraphError{e}, nil
+	return err
 }
 
 // Error returns the error message.
 func (e GraphError) Error() string {
-	if e.PublicError == nil {
-		return ""
-	}
-	return fmt.Sprintf("code: %s, message: %s", *e.PublicError.GetCode(), *e.PublicError.GetMessage())
+	return fmt.Sprintf("code: %s, message: %s", *e.Errorable.GetCode(), *e.Errorable.GetMessage())
 }

pkg/cloud/errors_test.go

@@ -3,9 +3,9 @@ package cloud
 import (
 	"testing"
 
-	"github.com/Azure/go-autorest/autorest"
-	"github.com/Azure/go-autorest/autorest/to"
-	"github.com/microsoftgraph/msgraph-sdk-go/models"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
+	"github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors"
 	"github.com/pkg/errors"
 )
 
@@ -43,18 +43,18 @@ func TestIsRoleAssignmentAlreadyDeleted(t *testing.T) {
 		want      bool
 	}{
 		{
-			name:      "not autorest detailed error",
+			name:      "not azcore response error",
 			actualErr: errors.New("role assignment already deleted"),
 			want:      false,
 		},
 		{
 			name:      "status code doesn't match",
-			actualErr: &autorest.DetailedError{StatusCode: 404, Message: "role assignment not found"},
+			actualErr: &azcore.ResponseError{StatusCode: 404, ErrorCode: "role assignment not found"},
 			want:      false,
 		},
 		{
 			name:      "role assignment already deleted error",
-			actualErr: autorest.DetailedError{StatusCode: 204, Message: "role assignment already deleted"},
+			actualErr: &azcore.ResponseError{StatusCode: 204, ErrorCode: "role assignment already deleted"},
 			want:      true,
 		},
 	}
@@ -68,32 +68,32 @@ func TestIsRoleAssignmentAlreadyDeleted(t *testing.T) {
 	}
 }
 
-func TestIsAlreadyExists(t *testing.T) {
+func TestIsRoleAssignmentExists(t *testing.T) {
 	tests := []struct {
 		name      string
 		actualErr error
 		want      bool
 	}{
 		{
-			name:      "not autorest detailed error",
+			name:      "not azcore response error",
 			actualErr: errors.New("resource already exists"),
 			want:      false,
 		},
 		{
 			name:      "status code doesn't match",
-			actualErr: &autorest.DetailedError{StatusCode: 401, Message: "authorization failed"},
+			actualErr: &azcore.ResponseError{StatusCode: 401, ErrorCode: "authorization failed"},
 			want:      false,
 		},
 		{
 			name:      "resource already exists error",
-			actualErr: autorest.DetailedError{StatusCode: 409, Message: "resource already exists"},
+			actualErr: &azcore.ResponseError{StatusCode: 409, ErrorCode: "resource already exists"},
 			want:      true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := IsAlreadyExists(tt.actualErr); got != tt.want {
+			if got := IsRoleAssignmentExists(tt.actualErr); got != tt.want {
 				t.Errorf("IsAlreadyExists() = %v, want %v", got, tt.want)
 			}
 		})
@@ -114,17 +114,17 @@ func TestIsFederatedCredentialNotFound(t *testing.T) {
 		{
 			name: "graph error code doesn't match",
 			actualErr: func() error {
-				err := GraphError{PublicError: models.NewPublicError()}
-				err.PublicError.SetCode(to.StringPtr("random_error_code"))
+				err := GraphError{Errorable: odataerrors.NewMainError()}
+				err.Errorable.SetCode(to.Ptr("random_error_code"))
 				return err
 			},
 			want: false,
 		},
 		{
 			name: "graph error resource not found",
 			actualErr: func() error {
-				err := GraphError{PublicError: models.NewPublicError()}
-				err.PublicError.SetCode(to.StringPtr(GraphErrorCodeResourceNotFound))
+				err := GraphError{Errorable: odataerrors.NewMainError()}
+				err.Errorable.SetCode(to.Ptr(GraphErrorCodeResourceNotFound))
 				return err
 			},
 			want: true,
@@ -154,17 +154,17 @@ func TestIsFederatedCredentialAlreadyExists(t *testing.T) {
 		{
 			name: "graph error code doesn't match",
 			actualErr: func() error {
-				err := GraphError{PublicError: models.NewPublicError()}
-				err.PublicError.SetCode(to.StringPtr("random_error_code"))
+				err := GraphError{Errorable: odataerrors.NewMainError()}
+				err.Errorable.SetCode(to.Ptr("random_error_code"))
 				return err
 			},
 			want: false,
 		},
 		{
 			name: "graph error resource already exists",
 			actualErr: func() error {
-				err := GraphError{PublicError: models.NewPublicError()}
-				err.PublicError.SetCode(to.StringPtr(GraphErrorCodeMultipleObjectsWithSameKeyValue))
+				err := GraphError{Errorable: odataerrors.NewMainError()}
+				err.Errorable.SetCode(to.Ptr(GraphErrorCodeMultipleObjectsWithSameKeyValue))
 				return err
 			},
 			want: true,

pkg/cloud/graph.go

@@ -27,15 +27,9 @@ func (c *AzureClient) CreateServicePrincipal(ctx context.Context, appID string,
 	mlog.Debug("Creating service principal for application", "id", appID)
 	sp, err := c.graphServiceClient.ServicePrincipals().Post(ctx, body, nil)
 	if err != nil {
-		return nil, err
-	}
-	graphErr, err := GetGraphError(sp.GetAdditionalData())
-	if err != nil {
-		return nil, err
-	}
-	if graphErr != nil {
-		return nil, *graphErr
+		return nil, maybeExtractGraphError(err)
 	}
+
 	return sp, nil
 }
 
@@ -47,15 +41,9 @@ func (c *AzureClient) CreateApplication(ctx context.Context, displayName string)
 	mlog.Debug("Creating application", "displayName", displayName)
 	app, err := c.graphServiceClient.Applications().Post(ctx, body, nil)
 	if err != nil {
-		return nil, err
-	}
-	graphErr, err := GetGraphError(app.GetAdditionalData())
-	if err != nil {
-		return nil, err
-	}
-	if graphErr != nil {
-		return nil, *graphErr
+		return nil, maybeExtractGraphError(err)
 	}
+
 	return app, nil
 }
 
@@ -71,15 +59,9 @@ func (c *AzureClient) GetServicePrincipal(ctx context.Context, displayName strin
 
 	resp, err := c.graphServiceClient.ServicePrincipals().Get(ctx, spGetOptions)
 	if err != nil {
-		return nil, err
-	}
-	graphErr, err := GetGraphError(resp.GetAdditionalData())
-	if err != nil {
-		return nil, err
-	}
-	if graphErr != nil {
-		return nil, *graphErr
+		return nil, maybeExtractGraphError(err)
 	}
+
 	if len(resp.GetValue()) == 0 {
 		return nil, errors.Errorf("service principal %s not found", displayName)
 	}
@@ -98,15 +80,9 @@ func (c *AzureClient) GetApplication(ctx context.Context, displayName string) (m
 
 	resp, err := c.graphServiceClient.Applications().Get(ctx, appGetOptions)
 	if err != nil {
-		return nil, err
-	}
-	graphErr, err := GetGraphError(resp.GetAdditionalData())
-	if err != nil {
-		return nil, err
-	}
-	if graphErr != nil {
-		return nil, *graphErr
+		return nil, maybeExtractGraphError(err)
 	}
+
 	if len(resp.GetValue()) == 0 {
 		return nil, errors.Errorf("application with display name '%s' not found", displayName)
 	}
@@ -129,17 +105,10 @@ func (c *AzureClient) DeleteApplication(ctx context.Context, objectID string) er
 func (c *AzureClient) AddFederatedCredential(ctx context.Context, objectID string, fic models.FederatedIdentityCredentialable) error {
 	mlog.Debug("Adding federated credential", "objectID", objectID)
 
-	fic, err := c.graphServiceClient.ApplicationsById(objectID).FederatedIdentityCredentials().Post(ctx, fic, nil)
-	if err != nil {
-		return err
-	}
-	graphErr, err := GetGraphError(fic.GetAdditionalData())
-	if err != nil {
-		return err
-	}
-	if graphErr != nil {
-		return *graphErr
+	if _, err := c.graphServiceClient.ApplicationsById(objectID).FederatedIdentityCredentials().Post(ctx, fic, nil); err != nil {
+		return maybeExtractGraphError(err)
 	}
+
 	return nil
 }
 
@@ -160,15 +129,9 @@ func (c *AzureClient) GetFederatedCredential(ctx context.Context, objectID, issu
 
 	resp, err := c.graphServiceClient.ApplicationsById(objectID).FederatedIdentityCredentials().Get(ctx, ficGetOptions)
 	if err != nil {
-		return nil, err
-	}
-	graphErr, err := GetGraphError(resp.GetAdditionalData())
-	if err != nil {
-		return nil, err
-	}
-	if graphErr != nil {
-		return nil, *graphErr
+		return nil, maybeExtractGraphError(err)
 	}
+
 	for _, fic := range resp.GetValue() {
 		if *fic.GetIssuer() == issuer {
 			return fic, nil