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

Improve error from communicating with Azure API. #45572

Merged

Conversation

bigkevmcd
Copy link
Contributor

Issue:

Fixes: #45010
Fixes: #45005

Problem

The upstream package (github.com/manicminer/hamilton) was dropping errors from the underlying Go HTTP Transport, this was being reported by the Go HTTP Client as an error, and the original error was lost, for example, if you had egress rules that prevented access, any "connection" errors would be lost.,

Solution

This changes the behaviour of our use of the hamilton msgraph client to return errors when connecting which will pass the message through.

This is a reimplementation of the fix that was upstreamed here manicminer/hamilton#280

Testing

This would require blocking of access to Azure's endpoints, or a misconfiguration of the Graph API custom endpoint, see https://ranchermanager.docs.rancher.com/how-to-guides/new-user-guides/authentication-permissions-and-global-configuration/authentication-config/configure-azure-ad "custom endpoints" to point to an endpoint that could not be connected with.

Without the fix, you will get a message http: RoundTripper implementation (*retryablehttp.RoundTripper) returned a nil *Response with a nil error with the fix, if you have a custom endpoint that is invalid, you will get...Get "https://localhost/v1.0/test-tenant/users/test-user-id": dial tcp [::1]:80: connect: connection refused or something similar.

Engineering Testing

Manual Testing

See above.

Automated Testing

  • Test types added/modified:
    • Unit
    • Integration (Go Framework)
    • Integration (v2prov Framework)
    • Validation (Go Framework)
    • Other - Explain: EXPLAIN
    • None
    • REMOVE NOT APPLICABLE BULLET POINTS ABOVE
  • If "None" - Reason: EXPLAIN THE REASON
  • If "None" - GH Issue/PR: LINK TO GH ISSUE/PR TO ADD TESTS

Summary: TODO

QA Testing Considerations

Regressions Considerations

TODO

Existing / newly added automated tests that provide evidence there are no regressions:

  • TODO

@bigkevmcd bigkevmcd force-pushed the improve-hamilton-error-handling branch 2 times, most recently from 91eb02f to 4c9fab8 Compare May 28, 2024 13:02
@bigkevmcd bigkevmcd force-pushed the improve-hamilton-error-handling branch 2 times, most recently from 918f862 to 79dcb07 Compare May 30, 2024 09:54
This changes the behaviour of our use of the hamilton msgraph client to
return errors when connecting which will pass the message through.

This is a reimplementation of the fix that was upstreamed here
manicminer/hamilton#280
@bigkevmcd bigkevmcd force-pushed the improve-hamilton-error-handling branch from 79dcb07 to f144890 Compare May 30, 2024 12:31
Copy link
Contributor

@crobby crobby left a comment

Choose a reason for hiding this comment

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

lgtm. Appears to be similar to the upstreamed approach and has nice tests.

Copy link
Contributor

@enrichman enrichman left a comment

Choose a reason for hiding this comment

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

Love it :)

@bigkevmcd bigkevmcd merged commit 895ab75 into rancher:release/v2.8 May 31, 2024
2 checks passed
@bigkevmcd
Copy link
Contributor Author

Validation Template

What was fixed, or what change has occurred

Failing to connect to upstream Azure endpoints will get a better error message.

Areas or cases that should be tested

  • Configure authentication for Azure AD and enter a custom endpoint that points to a site/port that will not connect

What areas could experience regressions

  • None, we retained the existing behaviour except for the error-case, which wasn't working

Are the repro steps accurate/minimal?

See the test comments above for more detail.

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.

None yet

3 participants