rabbitmq / rabbitmq-stream-dotnet-client

RabbitMQ client for the stream protocol
https://rabbitmq.github.io/rabbitmq-stream-dotnet-client/stable/htmlsingle/index.html
Other
122 stars 41 forks source link

Force localhost connections when client connects to localhost #305

Closed Zerpet closed 1 year ago

Zerpet commented 1 year ago

Fixes #296

It is common to have a local instance of RabbitMQ running locally, either via a local rabbit (e.g. installed from source), or inside a container (e.g. Docker or local Kubernetes). In both cases, our client may not work without providing an address resolver, or without changing the advertised host/port in rabbit via an env variable. Specifically, our clients won't work if the client can't resolve the hostname of the local machine.

For exampe, a laptop may have a hostname mylaptop.some-company.com. Rabbit won't be impacted as long as it works in a single-node configuration. However, the client smart features to locate the stream leaders and replicas will get "confused" because it can't resolve the hostname.

This commit changes the smart layer, so that it won't try to locate the stream replicas if the client is connected to localhost. Instead, it will force the producer/consumer connections to be localhost, and it will assume that stream replicas/leader are in localhost. This is true for single-node rabbits in local development environments. The smart feature to locate leader/replicas remains unchanged for non-local connections.

Zerpet commented 1 year ago

Requires some manual QA before merging.

@Gsantomaggio @lukebakken could we introduce this in a patch release? Arguably it doesn't add new functionality, but a slight change in behaviour.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 57.74% and project coverage change: -0.20% :warning:

Comparison is base (c9f7563) 92.86% compared to head (3624282) 92.66%.

:exclamation: Current head 3624282 differs from pull request most recent head 049343f. Consider uploading reports for the commit 049343f to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #305 +/- ## ========================================== - Coverage 92.86% 92.66% -0.20% ========================================== Files 112 112 Lines 9742 9795 +53 Branches 774 786 +12 ========================================== + Hits 9047 9077 +30 - Misses 532 546 +14 - Partials 163 172 +9 ``` | [Files Changed](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-dotnet-client/pull/305?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq) | Coverage Δ | | |---|---|---| | [Tests/FilterTest.cs](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-dotnet-client/pull/305?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq#diff-VGVzdHMvRmlsdGVyVGVzdC5jcw==) | `95.65% <0.00%> (-4.35%)` | :arrow_down: | | [Tests/RawProducerSystemTests.cs](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-dotnet-client/pull/305?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq#diff-VGVzdHMvUmF3UHJvZHVjZXJTeXN0ZW1UZXN0cy5jcw==) | `97.56% <42.85%> (-1.39%)` | :arrow_down: | | [Tests/SuperStreamConsumerTests.cs](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-dotnet-client/pull/305?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq#diff-VGVzdHMvU3VwZXJTdHJlYW1Db25zdW1lclRlc3RzLmNz) | `94.24% <45.45%> (-3.51%)` | :arrow_down: | | [RabbitMQ.Stream.Client/StreamSystem.cs](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-dotnet-client/pull/305?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq#diff-UmFiYml0TVEuU3RyZWFtLkNsaWVudC9TdHJlYW1TeXN0ZW0uY3M=) | `81.62% <84.84%> (-1.46%)` | :arrow_down: | ... and [2 files with indirect coverage changes](https://app.codecov.io/gh/rabbitmq/rabbitmq-stream-dotnet-client/pull/305/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=rabbitmq)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Zerpet commented 1 year ago

Looks good to me, should we do #305 (comment) ?

I don't think we should mark the test to be skipped. In my commit, the test is marked "maybe skip", and it will always run, at least to the point where the SkipException is thrown.

Allow me to do manual QA for a couple hours before we consider merging.

Zerpet commented 1 year ago

Rebased branched on top of main. We should be good to merge after CI passes.