feat(consul): advertise-address override + per-listener service registration - #48
Open
krital wants to merge 1 commit into
Open
feat(consul): advertise-address override + per-listener service registration#48krital wants to merge 1 commit into
krital wants to merge 1 commit into
Conversation
…tration
Builds on the consul-service-registration work with the two pieces our
edge fleet (Nomad bridge-networked aggregators, Tailscale-only routing
between nodes) still needs before the orchestration-side registration
workarounds can be retired:
1. ConsulServiceAddress system property. The auto-detected local address
is the one this process can SEE, which inside a bridge-networked
container is the bridge-internal IP: reachable from the host's Consul
agent, unroutable from every other node. Deployments whose nodes only
share an overlay (WireGuard/Tailscale) must register the overlay
address or cross-node discovery returns an address the consumer
cannot reach. When set, the override is used as the ServiceAddress
and health-check target for the main service and all listener
services; unset keeps today's behaviour.
2. Per-listener service registration, derived from the endpoint entries
already present in meta ("mqtt" -> "tcp://0.0.0.0:1883/"): each
URI-shaped entry registers as maps-<key> (id <uniqueName>-<key>) with
the listener's port. Consumers discover a PROTOCOL, not the server --
a bridge or edge node asks for the mqtt endpoint instead of asking
for mapsMessaging and guessing ports. Non-URI meta entries are left
as plain metadata. Every listener service carries a TCP check so
DeregisterCriticalServiceAfter reaps it when the process dies;
UDP-transported listeners (mavlink) cannot be TCP-probed, so the REST
endpoint stands in as the process-liveness proxy.
Six new unit tests cover the override (main + listeners), one-service-
per-endpoint, plain-metadata skipping, the UDP liveness proxy, and the
no-REST-endpoint guard.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
1 participant
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.
Hi Matt — we ran the new consul-service-registration code on our REPMUS fleet today (both aggregators, MAPS daily 13-Aug). The TCP check + explicit address registration works and is live. This PR adds the two remaining pieces that would let us delete our orchestration-side registration workarounds entirely.
1.
ConsulServiceAddresssystem property — advertise address overrideWhat we observed live: the auto-detected address registers as
172.26.64.x— the Nomad bridge-internal IP. The host's Consul agent can reach it (check passes), but no other node can route to it. Our fleet's nodes only share a Tailscale overlay, so cross-node discovery must return the tailnet address. Today we do that with Nomad-side service stanzas; with this property (-DConsulServiceAddress=100.x.y.z) MAPS can register the routable address itself. Unset ⇒ exactly today's behaviour.The health check deliberately follows the advertised address: the local agent runs the check, and an advertise address the host itself can't reach is a misconfiguration that should fail visibly.
2. Per-listener service registration
metaalready carries every listener endpoint (mqtt → tcp://0.0.0.0:1883/). Each URI-shaped entry now also registers as its own servicemaps-<key>(id<uniqueName>-<key>) with the listener's port — so consumers discover a protocol ("give me the mqtt endpoint") instead of discoveringmapsMessagingand guessing ports. Non-URI meta entries are untouched metadata. Naming follows themaps-<protocol>convention we already use fleet-side, so these registrations drop in as replacements.Since there's no explicit deregistration path, every listener service carries a check so
DeregisterCriticalServiceAfterreaps it: TCP listeners are probed on their own port; UDP listeners (mavlink) use the REST endpoint as a process-liveness proxy.Tests
Six new unit tests alongside your existing
EcwidConsulManagerTest(override on main + listeners, one-service-per-endpoint, plain-metadata skipped, UDP proxy check, no-REST guard).Honest caveat
We could not run the suite locally: a clean
developmentcheckout currently fails to compile against the rollingloggingsnapshot (ConfigLogMessages is not abstract and does not override abstract method getParameterCount() in LogMessage— the snapshot'sLogMessageinterface has drifted). You'll likely want CI/your environment to validate. The new code is exercised by the added tests and mirrors the structure ofcreateService.