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

[v2.8] Backport hamilton error fix #45010

Closed

Conversation

bigkevmcd
Copy link
Contributor

@bigkevmcd bigkevmcd commented Apr 3, 2024

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 changed the base branch from release/v2.9 to release/v2.8 April 3, 2024 08:36
@bigkevmcd bigkevmcd force-pushed the backport-hamilton-error-fix branch 5 times, most recently from 3065c67 to 0d33119 Compare April 3, 2024 15:03
@bigkevmcd bigkevmcd changed the title Backport hamilton error fix [v2.8] Backport hamilton error fix Apr 3, 2024
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 backport-hamilton-error-fix branch from 0d33119 to e6476ae Compare April 4, 2024 08:17
@bigkevmcd bigkevmcd requested a review from maxsokolovsky April 5, 2024 10:30
@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.

1 participant