Say the RDS log was refused, instead of that it held no plans - #2634
Conversation
Deployed the RDS log-API route to the PostgreSQL monitoring host and
read what it actually reported:
collection_log: pg_plan_capture | SUCCESS | rows=0
| no new auto_explain plans in the RDS log window
app log: rds:DescribeDBLogFiles ... is not authorized ...
because no identity-based policy allows
Nothing was read. The log was never opened. And the row says, in as many
words, that it was opened and was empty.
The ingestor caught every failure, warned, and returned 0, and the
runner turned 0 into that sentence. Three states shared one value: a
true negative, a denied call, and a cluster mid-failover.
It is also a regression against the route it replaced. pg_read_file
answers the same situation with PERMISSIONS and a message naming the
grant; the managed path - the one a real fleet is on - is the one that
went quiet. The app-log warning is not a substitute, because
collection_log is where collection health is read, and it was saying
this collector was fine.
Tolerating the failure stays right: one target's IAM gap must not take
the cycle down. What changes is that the cycle now says which kind of
nothing it found. An authorization refusal degrades to PERMISSIONS with
a message that says the grant is on the MONITORING HOST's IAM role and
not the database login - a distinction nothing else in this product has
to make, because every other collector reaches its target through a
connection string. Everything else stays loud. A permanent-sounding
status on a throttle or a failover is how a real outage gets read as a
configuration choice.
The refusal is matched on the message as well as the SDK type, because
that is the shape the fleet actually produced, and through inner
exceptions, because the SDK wraps.
Second defect, same area, found reading the dispatch afterwards:
IsAwsRds is never assigned on the PostgreSQL connect path. It comes from
a T-SQL detection query, so it is false for every PostgreSQL target -
which makes the second half of pg_plan_capture's `IsAurora || IsAwsRds`
unreachable and sends plain RDS PostgreSQL, managed and with no
filesystem, down the pg_read_file route to fail naming a database grant
that would never have helped. Now derived from the endpoint, which
RdsEndpoint already parses.
Aurora was unaffected the whole time because IsAurora carried the
routing. That is exactly why the fleet could not show it: every
PostgreSQL target on it is Aurora, so the dead half was never
load-bearing.
Fixes #2633.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /* #2633: derived from the ENDPOINT, because nothing else here can see it. IsAwsRds is | ||
| probed with a T-SQL detection query on the SQL Server path, so before this it was | ||
| silently false for every PostgreSQL target — which made the second half of | ||
| pg_plan_capture's `IsAurora || IsAwsRds` dispatch unreachable and sent plain RDS | ||
| PostgreSQL down the pg_read_file route, where a managed instance has no filesystem to | ||
| read and the failure names a grant that would never have helped. Aurora was unaffected | ||
| because IsAurora carries it, which is why the fleet never showed this. */ | ||
| IsAwsRds = RdsEndpoint.TryParse( | ||
| new NpgsqlConnectionStringBuilder(connectionString).Host) is not null, |
There was a problem hiding this comment.
This fixes Target.IsAwsRds (the field pg_plan_capture's dispatch reads), but ConnectPostgresAsync's returned ServerRuntime never sets the top-level ServerRuntime.IsAwsRds (DarlingServerConnector.cs:55) — only Target.IsAwsRds gets the derived value here. The SQL Server path sets both (isAwsRds flows into Target.IsAwsRds at line 297 and ServerRuntime.IsAwsRds at line 311); the new Postgres path only does the former.
ProbeAsync builds ConnectionProbeResult.IsAwsRds from runtime.IsAwsRds (line 420), not runtime.Target.IsAwsRds, so for an actual RDS PostgreSQL target test_connect/add_servers will keep reporting isAwsRds: false in DarlingCommandExecutor.MapProbeResult (line 481) even after this fix — while the collector loop's dispatch (DarlingWorker.cs:4711) correctly treats the same target as RDS. Operator-facing preflight output disagrees with actual runtime behavior for the exact fact this PR is about.
No PostgreSQL collector currently gates AppliesTo on IsAwsRds, so DescribeProbeFacts's skipped-collector count isn't affected today, but the raw isAwsRds JSON field returned by test_connect/add_servers is directly wrong.
Suggest also setting IsAwsRds on the returned ServerRuntime here, mirroring the SQL Server path.
|
Reviewed. This is Darling-only (PostgreSQL/RDS plan capture has no Lite equivalent — Lite never opens an Correctness of the two claimed fixes:
No other correctness, security, or T-SQL-style issues found — no T-SQL in this diff. |
1 participant
Fixes #2633. Found by deploying the RDS log-API route to the PostgreSQL monitoring host and reading what it actually reported.
The two rows
Nothing was read. The log was never opened. The row says, in as many words, that it was opened and was empty.
IngestAsynccaught every failure, warned, and returned 0; the runner turned 0 into that sentence. Three states shared one value — a true negative, a denied call, and a cluster mid-failover.It is a regression against the route it replaced:
pg_read_fileanswers the same situation withPERMISSIONSand a message naming the grant. The managed path — the one a real fleet is on — is the one that went quiet. The app-log warning is not a substitute, becausecollection_logis where collection health is read, and it was saying this collector was fine.What changes
Tolerating the failure stays right; one target's IAM gap must not take the cycle down. The cycle now says which kind of nothing it found.
An authorization refusal degrades to
PERMISSIONSwith a message making a distinction nothing else in this product has to make:Every other failure stays loud. A permanent-sounding status on a throttle or a failover is how a real outage gets read as a configuration choice.
The refusal is matched on the message as well as the SDK type — that is the shape the fleet actually produced — and through inner exceptions, because the SDK wraps.
Second defect, found reading the dispatch afterwards
ConnectPostgresAsyncsetsEngine,PostgresMajorVersion,PostgresVersionNum,IsAuroraandIsInRecovery. It never setsIsAwsRds— that comes from a T-SQL detection query on the SQL Server path.So
IsAwsRdsis false for every PostgreSQL target, the second half of that condition is unreachable, and plain RDS PostgreSQL — managed, no filesystem, not Aurora — falls topg_read_fileand fails naming a database grant that would never have helped.Now derived from the endpoint, which
RdsEndpointalready parses.Aurora was unaffected the whole time because
IsAuroracarried the routing. That is exactly why the fleet could not show it: every PostgreSQL target on it is Aurora, so the dead half was never load-bearing.Verified on the fleet
The route itself works — after the upgrade the Aurora target correctly took the RDS API path rather than
pg_read_file, which is what surfaced the reporting bug in the first place. Enabling capture end to end needs three prod changes on the target (auto_explaininshared_preload_libraries, which is static and needs a reboot;auto_explain.log_min_durationandlog_format; and%Qinlog_line_prefix, currently absent) plus the two IAM actions above — noted on #2538, not done here.