fix: prioritise service instance address over Consul node address in GetDownstreamHost#2370
Closed
TomPallister wants to merge 1 commit intodevelopfrom
Closed
fix: prioritise service instance address over Consul node address in GetDownstreamHost#2370TomPallister wants to merge 1 commit intodevelopfrom
TomPallister wants to merge 1 commit intodevelopfrom
Conversation
…GetDownstreamHost Fixes #2325. The regression introduced in 23.3 incorrectly used the Consul node's Name as the downstream host when a node was present, causing connection failures in Kubernetes and other environments where services and Consul run on separate hosts. The fix restores the pre-23.3 behaviour: entry.Service.Address is used when non-empty, with node.Address and then node.Name as fallbacks. Unit tests are updated to cover all three branches explicitly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Tom, nice to see you! 🙂 The problem is deeper than it may seem at first glance. Please see the related issue #2208.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2325
Regression introduced in Ocelot 23.3 where
GetDownstreamHostincorrectly used the Consul node'sNameas the downstream host whenever a node was present, breaking environments (especially Kubernetes) where Consul nodes and service instances run on different hosts.Changes
DefaultConsulServiceBuilder.GetDownstreamHost: Reversed the priority logic to useentry.Service.Addressfirst (the actual service instance address), falling back tonode.Addressthennode.Nameonly when the service address is unavailable.GetDownstreamHost_BothBranches_NameOrAddresstest (which validated the broken behaviour) with two new tests covering all branches of the corrected logic.Before (broken, 23.3+)
After (fixed)
Test plan
GetDownstreamHost_ServiceAddressPresent_ReturnsServiceAddress— verifies service address is used even when a node is presentGetDownstreamHost_ServiceAddressEmpty_FallsBackToNodeAddressThenName— verifies fallback tonode.Addressthennode.Namewhen service address is empty/nullDefaultConsulServiceBuilderTestspass🤖 Generated with Claude Code