fix(rds): resolve tags by ARN resource type, not only DB instances - #1649
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves RDS tagging compatibility by resolving ResourceName based on the ARN resource-type segment (e.g., db, cluster, subgrp) instead of always treating it as a DB instance, preventing misleading DBInstanceNotFound failures for valid non-instance resources.
Changes:
- Updated
RdsServicetag operations to dispatch tag reads/writes to instances, clusters, or subnet groups based on parsed ARN resource type. - Added tag storage support to
DbClusterandDbSubnetGroupmodels (mirroringDbInstance). - Expanded
RdsServiceTestcoverage to include subnet group and cluster tag round-trips via ARN, missing-subnet-group fault, and unsupported ARN type rejection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/io/github/hectorvent/floci/services/rds/RdsService.java | Resolves tag targets by ARN resource type and persists tags to the correct backing store. |
| src/main/java/io/github/hectorvent/floci/services/rds/model/DbSubnetGroup.java | Adds tag storage to DB subnet group model. |
| src/main/java/io/github/hectorvent/floci/services/rds/model/DbCluster.java | Adds tag storage to DB cluster model. |
| src/test/java/io/github/hectorvent/floci/services/rds/RdsServiceTest.java | Adds regression and behavior tests for ARN-based tag dispatch and error handling. |
ListTagsForResource, AddTagsToResource and RemoveTagsFromResource unconditionally resolved every ResourceName to a DB instance, so any non-instance ARN (e.g. a DB subnet group) returned 404 DBInstanceNotFound. This broke tools such as the Crossplane Upbound AWS provider, which calls ListTagsForResource on every observe of a DB subnet group and never sees it become ready. Parse the ARN's resource-type segment and dispatch to the matching store (db, cluster, subgrp): return the resource's tags, the resource-specific NotFound fault when it is missing, and InvalidParameterValue for unsupported resource types. Add tag storage to DbCluster and DbSubnetGroup mirroring DbInstance. A bare (non-ARN) resource name still resolves to a DB instance for backwards compatibility.
c175bbe to
8110e8e
Compare
Address PR review: only fall back to a DB-instance lookup for bare, non-ARN resource names. Strings that start with arn: are now parsed strictly and must be RDS ARNs; malformed ARNs and ARNs for other services return 400 InvalidParameterValue instead of a misleading DBInstanceNotFound/DBClusterNotFoundFault.
Address PR review: buildSubnetGroup constructs a fresh DbSubnetGroup with an empty tag map, so modifying a subnet group's subnets dropped any tags added via AddTagsToResource. Carry the existing group's tags onto the rebuilt object before persisting, matching AWS ModifyDBSubnetGroup.
|
Thanks @greptile-apps — valid P1, fixed in 68fd0ee. |
|
The fix looks correct and complete. The one-liner at group.setTags(existing.getTags());is exactly the right place — it runs after The No further concerns on this thread. LGTM. Tip: You can customize Greptile's behavior for this repo with |
|
Thanks @dnlopes, this is a clean fix and the Crossplane repro makes the motivation clear. Reviewed against the RDS model and the moto reference. What checks out:
Two small non blocking notes:
aws rds list-tags-for-resource --resource-name arn:aws:rds:us-east-1:000000000000:mydb
# real AWS: InvalidParameterValue, the resource part must be <type>:<id>
# this PR: resolved as DB instance "mydb" → DBInstanceNotFoundTreating
Neither needs to hold up the merge. Nice work. |
…d-type message Review feedback from floci-io#1649: - An RDS ARN whose resource part is not <type>:<id> now returns 400 InvalidParameterValue (matching real AWS) instead of falling back to a DB-instance lookup; bare non-ARN names keep resolving as DB instances. - The unsupported-resource-type message now states the Floci scope limitation instead of implying those types are untaggable on AWS.
|
Hello @hectorvent, both notes addressed in b8358e1:
|
Summary
RDS tag operations (
ListTagsForResource,AddTagsToResource,RemoveTagsFromResource) resolved everyResourceNameto a DB instance, so any non-instance ARN (DB subnet group, cluster, …) failed with404 DBInstanceNotFoundeven when the resource existed.This PR resolves the tagging
ResourceNameby its ARN resource-type segment (db,cluster,subgrp) and dispatches to the matching store. Tag storage is added toDbClusterandDbSubnetGroup, mirroringDbInstance.400 InvalidParameterValue; a bare (non-ARN) name still resolves to a DB instance for backwards compatibility.DBSubnetGroupNotFoundFault); unsupported RDS resource types returnInvalidParameterValue.modifyDbSubnetGrouppreserves existing tags when subnets are updated.Type of change
fix:)feat:)feat!:orfix!:)AWS Compatibility
This is a bug fix to management-API behavior.
Incorrect behavior: the AWS Crossplane / Upbound provider calls
ListTagsForResourceon every observe of a DB subnet group. Floci returned:so the managed resource never reconciled. Reproducible with the AWS CLI (AWS CLI v2):
Behavior now matches AWS: tags resolve per resource type, and modifying a subnet group preserves its tags.
Checklist
./mvnw testpasses locally