Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: use odataerrors and azcore response error #929

Merged
merged 1 commit into from
May 5, 2023
Merged

refactor: use odataerrors and azcore response error #929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 (
Expand Down
10 changes: 2 additions & 8 deletions pkg/cloud/azureclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
}
Comment on lines -155 to -159
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This http client is specific to the graph sdk and causes issues when used with the azure-sdk-for-go clients, so removing it (this was not wired before either)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious, what issues does it cause? This means we don't get logs for any calls this code makes right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the error

PUT https://management.azure.com/subscriptions/<SUBSCRIPTION_ID>/resourceGroups/aramasedev/providers/Microsoft.KeyVault/vaults/<KEYVAULT_NAME>/providers/Microsoft.Authorization/roleAssignments/a91b41b8-f7ed-4eaa-8b57-34df7ccb90e4
--------------------------------------------------------------------------------
RESPONSE 400: 400 Bad Request
ERROR CODE: InvalidRequestContent
--------------------------------------------------------------------------------
{
  "error": {
    "code": "InvalidRequestContent",
    "message": "The content of your request was not valid, and the original object could not be deserialized. Exception message: 'Unable to translate bytes [8B] at index 1 from specified code page to Unicode.'"
  }
}
--------------------------------------------------------------------------------

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea how using the wrapped client would cause that 🤔


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")
}
Expand Down
58 changes: 16 additions & 42 deletions pkg/cloud/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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.
Expand All @@ -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())
}
38 changes: 19 additions & 19 deletions pkg/cloud/errors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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,
},
}
Expand All @@ -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)
}
})
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
63 changes: 13 additions & 50 deletions pkg/cloud/graph.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}

Expand All @@ -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)
}
Expand All @@ -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)
}
Expand All @@ -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
}

Expand All @@ -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
Expand Down