Repository navigation
PMM-15389 Assume an IAM role for AWS RDS monitoring - #5944
Merged
Merged
Conversation
AddRemoteRDSNodeParams had no instance_id field and `pmm-admin inventory add node remote-rds` had no flag for it, so every remote-RDS node created through the inventory path stored an empty identifier. rds_exporter then received `instance: ""`, logged "No scraper for <region>/, skipping." and collected nothing while the agent reported AGENT_STATUS_RUNNING. PMM-13157 split address from instance_id but wired the new field through the management API only; the inventory API and CLI were never updated. This adds instance_id to AddRemoteRDSNodeParams and to the CLI, and refuses the broken state at source: an empty identifier is rejected at node creation (InvalidArgument), and attaching an rds_exporter to a remote_rds node that lacks one is rejected at agent creation (FailedPrecondition), which also covers rows created before this fix. The stale "DB instance identifier" comment on the address field is corrected in both RemoteRDSNode and AddRemoteRDSNodeParams. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5944 +/- ##
==========================================
+ Coverage 43.59% 53.35% +9.75%
==========================================
Files 415 559 +144
Lines 43134 43417 +283
Branches 0 587 +587
==========================================
+ Hits 18804 23163 +4359
+ Misses 22454 20247 -2207
+ Partials 1876 7 -1869 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4nte
marked this pull request as ready for review
September 14, 2026 11:29
4nte
requested review from
JiriCtvrtka and
ademidoff
and removed request for
a team
September 14, 2026 11:29
The help text already said it was required, but Kong accepted an omitted value and sent an empty identifier the server then rejected. Enforce it at parse time (CodeRabbit review on #5943). Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Adopt the assumed-role implementation from #5804 (by Fergal Kearns) onto current main, squashed into one commit. PMM can assume an AWS IAM role using its own ambient credentials instead of long-lived access keys, for both RDS discovery (on PMM Server) and rds_exporter scraping (on the pmm-agent host). Adds aws_role_arn across the RDS API surface, mutually exclusive with the access/secret key; assumes the role once per partition during discovery; groups rds_exporter processes by credential identity; and exposes --aws-role-arn on the pmm-admin RDS commands. AWS SDK bumped to the versions already on main, with service/sts promoted to a direct dependency. Docs are intentionally excluded; they land via #5838. Known defects from the #5804 review are fixed in follow-up commits on this branch. Original PR: #5804 Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Fixes two defects in the DiscoverRDS assume-role path found reviewing #5804. The STS AssumeRole call ran on the raw request context, before the awsDiscoverTimeout region-scan deadline was applied, and the HTTP client had no timeout of its own. A slow or unreachable STS endpoint could hang DiscoverRDS for minutes. The assume now runs under its own awsDiscoverTimeout deadline and the HTTP client carries a matching per-request ceiling. The role ARN's partition was never checked against settings.AWSPartitions. A role in a partition PMM is not configured to scan could assume successfully and then fail every scanned region, or return nothing with no error. The partition is now rejected up front with FailedPrecondition, before any network call. stsRegionForRoleARN returns the partition for this check. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
A pmm-agent older than 3.4.0 bundles an rds_exporter that assumes an IAM role from empty static credentials rather than the ambient chain, so the sts:AssumeRole is never signed and the exporter dies with EmptyStaticCreds. Before this, a current server accepted --aws-role-arn for such an agent, returned success, and left the exporter failing with no server-side signal. CreateAgent and ChangeAgent now reject a role-based rds_exporter whose pmm-agent is below PMMAgentMinVersionForAWSRoleARN (3.4.0-0), with FailedPrecondition. Static-key exporters are unaffected. An agent with no reported version is treated as unsupported, which is the safe default. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Clearing the role ARN (--aws-role-arn="") without also supplying keys leaves the exporter with empty AWS options, so rds_exporter falls back to the pmm-agent host's ambient credentials - often a broader identity than the role the operator deliberately chose. This transition was silent: the CLI printed only "cleared AWS role ARN". Ambient credentials are a legitimate mode, so this is not rejected; instead it is made explicit. The pmm-admin change command now states that the exporter will use the host's ambient credentials when the ARN is cleared without keys, the flag help spells out the mutual-exclusion and clear semantics, and ChangeRDSExporter logs a Warn covering the API and UI callers that do not see the CLI message. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
The 3.4.0 role-ARN gate rejects a role-based rds_exporter whose pmm-agent reports no version. TestRoster/GetFallbackHandlesRoleARN creates one on the built-in pmm-server agent, which the test fixtures seed without a version, so the gate refused it. Give that agent a supported version in the test, as any running 3.4.0+ server would report once the built-in agent connects. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
The pmm-agent 3.4.0 gate rejected a role ARN whenever IsAgentSupported returned any error, which includes the 'no version info' case for a pmm-agent that has not connected yet. Reject only AgentNotSupportedError (a pmm-agent known to be too old); an unreported version no longer blocks storing the config and the gate re-checks once the agent connects. This is what the api-test TestRDSExporter/WithRoleARN and the feature's own intent expect. Signed-off-by: Ante Gulin <ante.gulin@percona.com> (cherry picked from commit 1ebc19b)
Shorten the change-agent role-ARN help to satisfy lll, drop the named returns on stsRegionForRoleARN, and remove a redundant .Querier selector in the roster test (golangci-lint --fix). Signed-off-by: Ante Gulin <ante.gulin@percona.com>
4nte
force-pushed
the
PMM-15389-assume-role
branch
from
September 14, 2026 12:10
42619c1 to
1520890
Compare
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (5)
📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review. WalkthroughThe change adds AWS IAM role ARN support to RDS exporter agents and RDS discovery. API contracts validate role ARNs and reject combinations with static AWS keys. Agent models derive credential identities and check PMM Agent version support. Discovery assumes roles through STS and scans the role’s partition. Remote RDS node creation accepts an instance identifier, normalizes it, and checks uniqueness by region. API responses, CLI commands, exporter configuration, and grouping carry the new AWS options. Sequence Diagram(s)sequenceDiagram
participant Client
participant DiscoverRDS
participant STS
participant RDS
Client->>DiscoverRDS: Submit role ARN
DiscoverRDS->>STS: Assume role
STS-->>DiscoverRDS: Return temporary credentials
DiscoverRDS->>RDS: Scan role partition regions
RDS-->>DiscoverRDS: Return discovery results
DiscoverRDS-->>Client: Return results
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Remote RDS nodes may retain identifiers that cannot match an instance, and concurrent creation may produce duplicate instance records. Resolve or explicitly accept those risks before merging. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: de5c0115-40d4-4a09-b055-06f29df8adbb
⛔ Files ignored due to path filters (3)
api/inventory/v1/agents.pb.gois excluded by!**/*.pb.goapi/management/v1/agent.pb.gois excluded by!**/*.pb.goapi/management/v1/rds.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (40)
admin/commands/inventory/add_agent_rds_exporter.goadmin/commands/inventory/change_agent_rds_exporter.goadmin/commands/inventory/change_agent_rds_exporter_test.goapi-tests/inventory/agents_rds_exporter_test.goapi-tests/management/rds_test.goapi/inventory/v1/agents.pb.validate.goapi/inventory/v1/agents.protoapi/inventory/v1/json/client/agents_service/add_agent_responses.goapi/inventory/v1/json/client/agents_service/change_agent_responses.goapi/inventory/v1/json/client/agents_service/get_agent_responses.goapi/inventory/v1/json/client/agents_service/list_agents_responses.goapi/inventory/v1/json/v1.jsonapi/management/v1/agent.pb.validate.goapi/management/v1/agent.protoapi/management/v1/json/client/management_service/add_service_responses.goapi/management/v1/json/client/management_service/discover_rds_responses.goapi/management/v1/json/client/management_service/list_agents_responses.goapi/management/v1/json/client/management_service/list_services_responses.goapi/management/v1/json/v1.jsonapi/management/v1/rds.pb.validate.goapi/management/v1/rds.protoapi/swagger/swagger-dev.jsonapi/swagger/swagger.jsongo.modmanaged/models/agent_helpers.gomanaged/models/agent_helpers_test.gomanaged/models/agent_model.gomanaged/models/agent_model_test.gomanaged/models/agentversion.gomanaged/services/agents/rds.gomanaged/services/agents/rds_test.gomanaged/services/agents/roster.gomanaged/services/agents/roster_test.gomanaged/services/agents/state.gomanaged/services/agents/state_test.gomanaged/services/converters.gomanaged/services/inventory/agents.gomanaged/services/management/agent.gomanaged/services/management/rds.gomanaged/services/management/rds_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.
Address CodeRabbit review on #5944: - The role-ARN gate allowed on any non-AgentNotSupportedError, so a present-but-malformed pmm-agent version (and, on the change path, a PMM Agent lookup error) silently persisted an unverifiable role ARN. Add an ErrAgentVersionNotReported sentinel and allow only that case; reject the rest. - DiscoverRDS reported an STS timeout as FailedPrecondition; return DeadlineExceeded for context cancellation/deadline instead. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
⚠️ Outside diff range comments (1)
managed/services/management/rds.go (1)
604-609: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSupport
aws-iso-bthrough the complete settings and discovery paths.
ValidateAWSPartitionsrejectsaws-iso-b, soUpdateSettingsreturnsInvalidArgumentbefore the partition is saved.listRegionsalso has noaws-iso-bentry, andstsRegionForRoleARNrejects its role ARNs before the STS call.Add
aws-iso-btoAWSPartitions(). Moveus-isob-east-1into anaws-iso-bRDS region set. Add thestsDefaultRegionmapping. Update the tests that currently expectaws-iso-bto be unsupported. The API schema already accepts generic strings, and the default partition remainsaws.Required fix
func AWSPartitions() []string { return []string{ "aws", "aws-cn", "aws-iso", + "aws-iso-b", "aws-us-gov", } } var stsDefaultRegion = map[string]string{ "aws": "us-east-1", "aws-cn": "cn-north-1", "aws-us-gov": "us-gov-west-1", "aws-iso": "us-iso-east-1", + "aws-iso-b": "us-isob-east-1", }"aws-iso": { "rds": { - "us-iso-east-1", "us-iso-west-1", "us-isob-east-1", + "us-iso-east-1", "us-iso-west-1", + }, + }, + "aws-iso-b": { + "rds": { + "us-isob-east-1", }, },
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 5af51db9-4424-42a6-a409-014fc52e8c4b
📒 Files selected for processing (4)
managed/models/agent_helpers.gomanaged/models/agent_helpers_test.gomanaged/models/agentversion.gomanaged/services/management/rds.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.
This was referenced Sep 16, 2026
4nte
added a commit
to Percona-Lab/pmm-submodules
that referenced
this pull request
Sep 16, 2026
The FB pinned PMM-15389-rds-assume-iam-role, the single branch this work started on. It was since split into a reviewable stack, percona/pmm#5943 for the instance_id fix with percona/pmm#5944 stacked on top, replayed onto a newer main and given two further commits: golangci-lint fixes and a tightened role-ARN version gate with an STS timeout. Pin PMM-15389-assume-role instead, so the images carry what is actually under review rather than the branch review moved off. It contains the instance_id commits too, being stacked on that base, so one entry still covers the whole stack. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
|
|
Cover the required flag and the request body: parsing fails without --instance-id, and the value reaches remote_rds.instance_id. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Nodes created before 3.4.0 or through the inventory API hold the bare identifier in address, not the endpoint. Say so on both messages, describe the instance_id fallback and lowercasing, and call RemoteRDSNode.instance_id a DB instance identifier rather than an AWS instance ID, which reads like an EC2 ID. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Add the instance_id field on AddRemoteRDSNodeParams to the buf breaking baseline. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Migration 110 copied the address into instance_id as typed, and the management API stores the identifier it is given, so existing remote RDS nodes can carry a mixed-case identifier. AWS stores DB instance identifiers in lowercase and rds_exporter matches them exactly, so such a node passes the new guard and still scrapes nothing. Lowercase existing identifiers and the backfilled bare addresses, the same rule createNodeWithID now applies to new nodes. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
The file committed in bdbbe96 came from make -C api gen without the formatting pass that make gen runs last, so CI's format check restored the blank line gofumpt inserts before the depIdxs declaration. Regenerated with make gen and make format in the devcontainer; that blank line is the only change. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
…PMM-15389-assume-role # Conflicts: # managed/models/agent_helpers_test.go # managed/services/agents/state_test.go
ademidoff
approved these changes
Sep 28, 2026
DiscoverRDS always assumed the IAM role against the partition's home region (us-east-1 for commercial AWS), even when PMM Server had a region configured. A server whose egress is limited to one region could never reach that STS endpoint and failed with "Timed out assuming role". Assume the role in the region the AWS SDK resolved from AWS_REGION, AWS_DEFAULT_REGION or the profile, and keep the partition default only as the fallback for a server with no region configured. A configured region outside the role's partition cannot issue its credentials, so reject it up front with a FailedPrecondition that names the variable, before any network call. Only the STS call moves. Region scanning, the partition allow-list in settings, and the discovery deadlines are unchanged. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
AWS_REGION and AWS_DEFAULT_REGION now steer which STS endpoint PMM Server uses to assume an RDS role, so they are documented input rather than unknown variables. Skip them in the environment parser alongside the existing AWS_ACCESS_KEY and AWS_SECRET_KEY case instead of logging "unknown environment variable" at startup. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Renumber the RDS instance_id backfill migration from 119 to 120, since main's 119 clears unused environment variable names. Point its test at the new number. Keep the AWS options validation and role ARN version gate in CreateAgent, and save through main's new insertAgent helper. Take main's AWS SDK versions; sts becomes a direct dependency. Regenerate api/descriptor.bin. Switch the RDS role gate test from the removed models.ChangeAgent to the changeAgent test helper. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Extract the role assumption block of DiscoverRDS into assumeRDSRole to fix the nestif lint finding. Behavior is unchanged. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
4nte
requested review from
fabio-silva and
matejkubinec
and removed request for
a team
October 7, 2026 09:00
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
94d9bc41-25fd-4276-ad3a-c188b79adc7f
⛔ Files ignored due to path filters (3)
api/descriptor.binis excluded by!**/*.binapi/inventory/v1/agents.pb.gois excluded by!**/*.pb.goapi/inventory/v1/nodes.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (39)
admin/commands/inventory/add_node_remote_rds_test.goadmin/commands/inventory/change_agent_rds_exporter_test.goapi-tests/inventory/agents_rds_exporter_test.goapi/inventory/v1/agents.pb.validate.goapi/inventory/v1/agents.protoapi/inventory/v1/json/client/agents_service/change_agent_responses.goapi/inventory/v1/json/client/nodes_service/add_node_responses.goapi/inventory/v1/json/client/nodes_service/get_node_responses.goapi/inventory/v1/json/client/nodes_service/list_nodes_responses.goapi/inventory/v1/json/v1.jsonapi/inventory/v1/nodes.pb.validate.goapi/inventory/v1/nodes.protoapi/management/v1/json/client/management_service/add_service_responses.goapi/management/v1/json/v1.jsonapi/swagger/swagger-dev.jsonapi/swagger/swagger.jsongo.modmanaged/models/agent_helpers.gomanaged/models/agent_helpers_test.gomanaged/models/agent_model.gomanaged/models/agent_model_test.gomanaged/models/agentversion.gomanaged/models/database.gomanaged/models/database_test.gomanaged/models/node_helpers.gomanaged/services/agents/rds.gomanaged/services/agents/rds_test.gomanaged/services/agents/state.gomanaged/services/agents/state_test.gomanaged/services/converters.gomanaged/services/inventory/agents.gomanaged/services/inventory/agents_test.gomanaged/services/inventory/nodes.gomanaged/services/inventory/nodes_test.gomanaged/services/inventory/services_test.gomanaged/services/management/rds.gomanaged/services/management/rds_test.gomanaged/utils/envvars/parser.gomanaged/utils/envvars/parser_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
fabio-silva
approved these changes
Oct 7, 2026
A blank instance_id was stored as is and could never match an RDS instance. Trim it, and treat a blank value like an omitted one. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
Keep the branch's roster lookup in the conflict with main's encrypted access key fix: findRDSExportersByCredentials already matches on the decrypted credentials in Go, and covers role ARNs as well. Signed-off-by: Ante Gulin <ante.gulin@percona.com>
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
6 participants
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.
Adds the ability to assume an AWS IAM role for RDS monitoring: supply a role ARN instead of static access keys, including a role in a different AWS account. PMM assumes the role with its ambient credentials and refreshes them automatically.
Stacked on #5943 (the
instance_iddelivery-surface fix); base branch isPMM-15389-rds-inventory-cli.Adopts the original contributor's feature (#5804, fergalhk - preserved as author on the adopt commit) and adds four fixes on top:
Verified end-to-end on real AWS (assume-role proven via CloudTrail +
aws_rds_*/rdsosmetrics_*metrics).Related
doc-3.10.0)