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.

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

refactor: use odataerrors and azcore response error #929

Merged
merged 1 commit into from
May 5, 2023

Conversation

aramase
Copy link
Member

@aramase aramase commented May 4, 2023

  • Use odataerrors.ODataError for graph errors; contains the inner error and other details from additionalData that we were previously parsing because of sdk limitations.
  • Use *azcore.ResponseError as part of switch to the new sdk for role assignments and role definitions client.

Validated it locally

./bin/azwi sa create --service-account-name idsa --service-account-namespace oidc --aad-application-name graphtest01 --azure-role Reader --azure-scope "/subscriptions/<SUBSCRIPTION_ID>/resourceGroups/aramasedev/providers/Microsoft.KeyVault/vaults/adevkv" --subscription-id <SUBSCRIPTION_ID> --auth-method cli --service-account-issuer-url https://kubernetes.svc.cluster.local
Thu, 04 May 2023 23:52:58 UTC  serviceaccount/create.go:175  --service-principal-name not specified, falling back to AAD application name  {"warning": true}
Thu, 04 May 2023 23:52:59 UTC  aad-application  create/aadapplication.go:71  created an AAD application  {"name": "graphtest01", "clientID": "b27e3f6b-038e-4c51-8872-5f10823d2832", "objectID": "<OBJECT_ID>"}
Thu, 04 May 2023 23:52:59 UTC  serviceaccount/create.go:175  --service-principal-name not specified, falling back to AAD application name  {"warning": true}
Thu, 04 May 2023 23:52:59 UTC  aad-application  create/aadapplication.go:95  created service principal  {"name": "graphtest01", "clientID": "b27e3f6b-038e-4c51-8872-5f10823d2832", "objectID": "<OBJECT_ID>"}
Thu, 04 May 2023 23:52:59 UTC  service-account  create/serviceaccount.go:94  created kubernetes service account  {"namespace": "oidc", "name": "idsa"}
Thu, 04 May 2023 23:52:59 UTC  federated-identity  create/federatedidentitycredential.go:87  federated credential has been previously created  {"objectID": "<OBJECT_ID>", "subject": "system:serviceaccount:oidc:idsa", "warning": true}
Thu, 04 May 2023 23:52:59 UTC  federated-identity  create/federatedidentitycredential.go:96  added federated credential  {"objectID": "<OBJECT_ID>", "subject": "system:serviceaccount:oidc:idsa"}
Thu, 04 May 2023 23:53:01 UTC  cloud/roleassignments.go:50  Role assignment already exists  {"warning": true, "principalID": "<OBJECT_ID>", "role": "Reader"}
Thu, 04 May 2023 23:53:01 UTC  role-assignment  create/roleassignment.go:81  created role assignment  {"scope": "/subscriptions/<SUBSCRIPTION_ID>/resourceGroups/aramasedev/providers/Microsoft.KeyVault/vaults/adevkv", "role": "Reader", "servicePrincipalObjectID": "<OBJECT_ID>", "roleAssignmentID": null}

@aramase aramase requested a review from enj as a code owner May 4, 2023 23:58
Comment on lines -155 to -159
clientOpts := &armpolicy.ClientOptions{
ClientOptions: azcore.ClientOptions{
Transport: client,
},
}
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 🤔

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2023

Codecov Report

Merging #929 (11ea507) into main (471822d) will increase coverage by 1.31%.
The diff coverage is 17.94%.

@@            Coverage Diff             @@
##             main     #929      +/-   ##
==========================================
+ Coverage   52.11%   53.43%   +1.31%     
==========================================
  Files          36       36              
  Lines        2360     2302      -58     
==========================================
  Hits         1230     1230              
+ Misses       1084     1026      -58     
  Partials       46       46              
Impacted Files Coverage Δ
pkg/cloud/azureclient.go 0.00% <0.00%> (ø)
pkg/cloud/graph.go 3.53% <0.00%> (+0.76%) ⬆️
pkg/cloud/roleassignments.go 0.00% <0.00%> (ø)
pkg/cloud/errors.go 63.63% <45.45%> (+32.52%) ⬆️
...count/phases/create/federatedidentitycredential.go 95.31% <100.00%> (ø)
...cmd/serviceaccount/phases/create/roleassignment.go 94.44% <100.00%> (ø)

@aramase aramase force-pushed the aramase/r/refactor_graph_errors branch from 4d8be55 to 11ea507 Compare May 5, 2023 00:50
pkg/cloud/errors.go Outdated Show resolved Hide resolved
@aramase aramase force-pushed the aramase/r/refactor_graph_errors branch from 11ea507 to 9c5a22a Compare May 5, 2023 17:08
@aramase aramase force-pushed the aramase/r/refactor_graph_errors branch from 9c5a22a to 156a58f Compare May 5, 2023 18:20
@aramase aramase merged commit 8ad5106 into Azure:main May 5, 2023
@aramase aramase deleted the aramase/r/refactor_graph_errors branch May 5, 2023 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants