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

[ARO-12518] Map other error's http status codes to correct result type. #3995

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

pepedocs
Copy link
Collaborator

@pepedocs pepedocs commented Dec 6, 2024

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-12518

What this PR does / why we need it:

NOTE: I have to recreate this PR from a direct branch of this repo as my previous PR from my fork was not picked up by azure CI (see
#3981)

What
Map other known backend errors returned by azure to their correct result type when logging them.

Why
Currently, these known errors are mapped to "InternalServerError" regardless of their nature (e.g. 5xx, 4xx) and logged as an InternalServerError. This breaks our CIF SLO reporting as it depends on parsing these logs.

Example UserErrors that are mapped to InternalServerError

network.SubnetsClient#CreateOrUpdate: Failure sending request: StatusCode=403 – Original Error: Code="LinkedAuthorizationFailed" Message= (backend)

ClientSecretCredential authentication failed. FromClientSecret(): http call(https://login.microsoftonline.com/xxx/oauth2/v2.0/token)(POST) error: reply status code was 400:

Test plan for issue:

Unit Test

  • Ensure the following
    • ResponseError and DetailedError types' StatusCodes are utilized when mapping to error result type

Manual Test

  • Mocked network.SubnetsClient#CreateOrUpdate: to use an invalid subnet so it will return a ResponseError and checked if it was logged as UserError.

Integration Tests

  • Please suggest a method if you think it's necessary.

Is there any documentation that needs to be updated for this PR?

  • None

How do you know this will function as expected in production?

  • Tests were satisfied

@fahlmant
Copy link
Collaborator

fahlmant commented Dec 6, 2024

@pepedocs Is this different than #3981 ?

@pepedocs
Copy link
Collaborator Author

pepedocs commented Dec 6, 2024

@pepedocs Is this different than #3981 ?

@fahlmant The same changes, but I have to create this new PR from a direct branch (recommended) from this repo. The previous one was from my fork (which I'm used to), but azure CI does not anymore pick it up.

@edisonLcardenas
Copy link
Collaborator

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@edisonLcardenas
Copy link
Collaborator

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@edisonLcardenas
Copy link
Collaborator

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@edisonLcardenas
Copy link
Collaborator

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pepedocs
Copy link
Collaborator Author

pepedocs commented Jan 6, 2025

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kimorris27 kimorris27 merged commit 1071de8 into master Jan 6, 2025
24 checks passed
@kimorris27 kimorris27 deleted the pepedocs/ARO-12518 branch January 6, 2025 22:11
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.

5 participants