Skip to content

Conversation

lukebakken
Copy link
Collaborator

@lukebakken lukebakken commented Jul 31, 2024

Fixes #1639

  • Remove all traces of credential refresh from RabbitMQ.Client
  • Add CredentialsRefresher, which does not use timers, and calls refresh based on ValidUntil

Note, there is still a lot of work to get all tests running, but this gives you the idea.

Also, there is a big TODO around integrating these changes with PlainMechanism

cc @danielmarbach @MatthiasWerning

@lukebakken lukebakken added this to the 7.0.0 milestone Jul 31, 2024
@lukebakken lukebakken self-assigned this Jul 31, 2024
Copy link
Contributor

@Tornhoof Tornhoof left a comment

Choose a reason for hiding this comment

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

This is mostly stuff, I noticed when I experimented making the current api async.

I know you're still working on it, but anyway:

@lukebakken
Copy link
Collaborator Author

@Tornhoof I appreciate you taking a look!

@danielmarbach
Copy link
Collaborator

What @Tornhoof said

@lukebakken
Copy link
Collaborator Author

@danielmarbach @MatthiasWerning @Tornhoof - I'm pretty much done with this. I'm going to modify the OAuth2 integration test to include restarting RabbitMQ mid-test, and probably add those annotations to JsonToken.

@lukebakken lukebakken marked this pull request as ready for review August 5, 2024 14:27
@lukebakken lukebakken marked this pull request as draft August 5, 2024 14:27
@lukebakken lukebakken requested a review from Tornhoof August 5, 2024 19:55
@danielmarbach
Copy link
Collaborator

@lukebakken before I take a deeper look I'm curious to hear why you didn't go down the route of collapsing the refreshing into the provider similar to how Azure.Identity does it?

@lukebakken
Copy link
Collaborator Author

before I take a deeper look I'm curious to hear why you didn't go down the route of collapsing the refreshing into the provider similar to how Azure.Identity does it?

Time constraints.

Copy link
Contributor

@Tornhoof Tornhoof left a comment

Choose a reason for hiding this comment

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

This looks a lot better than before :)

Fixes #1639

* Remove all traces of credential refresh from `RabbitMQ.Client`
* Add `CredentialsRefresher`, which does not use timers, and calls refresh based on `ValidUntil`
* Use `ICredentialsProvider` in the auth mechanisms, async
* Pass `CancellationToken` around and correctly dispose Http objects (thanks @Tornhoof)
* Use `SemaphoreSlim`
* Refresh token at 1/3 the interval value.
* Close connection mid-integration test for OAuth2
* Use OAuth2 for EasyNetQ.Management.Client in OAuth2 tests.
* Create separate credentials provider for HTTP API requests.
* Set RabbitMQ logging to debug for OAuth2
* Use `JsonPropertyName`
* Use OAuth2 for HTTP API access. Thanks @MarcialRosales
@lukebakken lukebakken force-pushed the rabbitmq-dotnet-client-1639 branch from 47b5ce7 to 2018045 Compare August 6, 2024 14:25
@lukebakken lukebakken marked this pull request as ready for review August 6, 2024 14:33
@lukebakken
Copy link
Collaborator Author

@danielmarbach @Tornhoof @paulomorgado @MatthiasWerning @MarcialRosales

I plan on merging this in a few hours to produce another version 7 RC.

@danielmarbach
Copy link
Collaborator

The earliest I can review is probably tomorrow

@lukebakken
Copy link
Collaborator Author

Thanks @danielmarbach. I'm going to merge this PR to produce a new RC, but of course I welcome your review. Any changes can be addressed in a follow-up PR.

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.

Timer leak and potential JWT/OAuth2 expiry in TimerBasedCredentialRefresher [OAuth2]
4 participants