Skip to content

Have the agent report whether it runs on the gateway, instead of inferring it from the UniFi Console #1108

Description

@tvancott42

Burned out from v2.6.0 push, I'm having Claude write this one.

The short version

The server figures out whether an on-site agent is running on the UniFi gateway by correlating IP addresses against what the site's UniFi Console reports. The agent already knows the answer and could just tell us. Replacing the correlation with a hello field collapses AgentOnGatewayDetector from roughly 500 lines to a lookup, and takes a pile of accumulated workarounds with it.

What's there now

AgentOnGatewayDetector answers one boolean: is this agent running on the gateway itself, or on a separate box? It does it by asking the site's UniFi Console for the gateway's addresses and checking whether the agent's address is one of them.

That sounds simple. It is not, because of one nasty constraint: on an agent site, the console reconnects through that agent's own tunnel. So the thing you need to ask is behind the thing you're asking about. Every caller that shows up before the console is back gets a silent "no" unless you do something about it.

We did something about it, repeatedly:

  • Two cache tiers, one keyed by site and one keyed by (site, agent).
  • Verdicts persisted to SystemSettings (agent.on_gateway, agent.on_gateway.<agentId>) so a restart has something to answer with before the console is up.
  • Seed-from-persisted before starting a refresh, so a refresh that finds the console down can't race an invented "no" into an empty cache.
  • KeepLastAnswer, which re-stamps the existing entry on a degraded refresh so expiry doesn't hammer retries.
  • Two separate address sets, a narrow one (what the console calls the gateway's IP) and a wide one (every address the gateway holds), because an agent on a gateway reports whichever interface the kernel enumerated first and that is frequently an uplink the console never lists.
  • local_ips on the hello (field 7), added specifically because comparing the agent's one self-reported address against the console's list answers "is this the gateway" with a false no.

Seven concurrent dictionaries, four public entry points, and a TODO admitting the entry points "look interchangeable and are not." That last part is #1106.

None of this is bad code. It is a correct solution to a problem we did not need to have.

Why the agent should just say so

Look at what AgentHello already does (src/NetworkOptimizer.AgentProtocol/Protos/agent_tunnel.proto):

  // Whether this agent serves a LAN speed test page at all. Explicitly optional so
  // the server can tell "no" from "did not say": an agent predating this leaves it
  // absent and the server falls back to deciding for itself, while a gateway
  // install answers a definite no without being guessed at by its location.
  optional bool serves_speed_test = 5;
  ...
  optional bool supports_source_bind = 6;

We have already solved this exact shape twice in this exact message, and the comment on field 5 even says the quiet part: a gateway install answers definitively "without being guessed at by its location." That is precisely what we are still doing for the on-gateway question.

The install path already knows, unambiguously. install-agent-gateway.sh exists specifically to do the memory-fenced on-box install, and install-native.sh refuses to run on a gateway at all. If you got the agent onto a gateway, you got there through the gateway installer. And serves_speed_test is already installer-driven config (config.LanSpeedTest), so writing one more fact into agent.json at install time is a path we have walked.

The server, meanwhile, is inferring that same fact by round-tripping a console that is only reachable through the agent in question. It works, and it is backwards.

Proposal

Add the field, keep correlation as the legacy fallback, and let it age out.

Protocol:

  • optional bool on_gateway = 8 on AgentHello, documented in the same style as fields 5 and 6: absent means an older agent that does not say, and the server falls back to IP correlation.

Agent:

  • A key in agent.json, written by install-agent-gateway.sh. That installer is the only supported way onto a gateway, so it is the authority. Absent means "did not say" and the server falls back to correlation, which covers hand-deployed agents and everything installed before the flag existed.
  • Send it on the hello, same as serves_speed_test.
  • Confirm the installer's upgrade path writes the key into an existing agent.json. Re-running the installer keeps the current config by design (so nobody needs a token to upgrade), so the new key has to be added rather than assumed present. Without that, every gateway agent installed before this stays on the correlation path forever.

Deliberately not doing runtime self-detection. ubnt-device-info is the marker install-native.sh uses to refuse, and it does look gateway-exclusive (not on APs, switches, or a UCK G2+), but "looks exclusive today" is an assumption about Ubiquiti's packaging across a product line we do not fully enumerate - nobody here has checked a UNVR. The installer does not have to assume anything, so it should not. Recorded so this does not get helpfully added back later.

Server:

  • AgentTunnelService stores it on the tunnel connection alongside LocalIps.
  • AgentOnGatewayDetector answers from the reported flag when present. Everything else stays only as the pre-flag path.
  • LatestAgentVersion is already 2.6.0, so the "update your agent" prompt is the existing mechanism for getting people onto an agent that reports it. No new nagging needed.

What falls out once the flag is the primary answer:

local_ips should stay regardless. It is useful beyond this question (self-target skipping needs the matched address, not a yes/no), and it is what the legacy correlation path will keep using.

One small thing to sweep up while we're in there

AgentOnGatewayDetector is registered by hand at Program.cs:285 as a plain singleton, so it never joins the ISiteScopedRegistry sweep that SiteManagementService runs on both site delete and site create. Sixteen other registries do. Its slug-keyed dictionaries (_cache, _agentIp, _gatewayIps, _gatewayHostIps) therefore survive a site being removed, and a new site created under the same slug inherits the old site's answer and its gateway address list until the console corrects it.

Low severity: it needs delete-then-recreate-under-the-same-name to fire, and it self-corrects. Worth naming because the interface doc literally warns about this ("Registering the singleton by hand and forgetting the second line is how a registry ends up leaking removed sites"), and because whoever rewrites this class should either wire it up or delete the caches that make it necessary. If the flag lands, most of those dictionaries go away and the question answers itself.

The per-agent caches added in #1107 are fine either way: SiteAgents.Id is PRIMARY KEY AUTOINCREMENT, so ids are never recycled and a re-created site cannot collide with a removed one's keys.

AgentProbeResultSink has the same registry gap across about nine slug-keyed dictionaries, including a console cache. That one is a separate change: it holds SemaphoreSlim instances, so it needs a real teardown callback and the eviction-versus-disposal ordering the interface doc warns about, on a hot ingest path.

Scope notes

This is not urgent and nothing is broken for users today. The current detector gets the right answer in normal operation. The case for doing it is that we are carrying a lot of machinery, and a growing set of "ask the right primitive" footguns, to derive something the other end of the tunnel already knows.

Closes out #1106 if done as described.

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions