RDS: lowercase the DBInstanceIdentifier #8564
Conversation
Various RDS API calls treat the DBInstanceIdentifier as lowercase / case insensitive. Documentation for `create_db_instance` says: `DBInstanceIdentifier [...] This parameter is stored as a lowercase string.` https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds/client/create_db_instance.html Documentation for `describe_db_instances` says: `DBInstanceIdentifier [...] This parameter isn’t case-sensitive.` https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds/client/describe_db_instances.html Documentation for `modify_db_instance` says: `DBInstanceIdentifier [...] This value is stored as a lowercase string.` `NewDBInstanceIdentifier [...] This value is stored as a lowercase string.` https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds/client/modify_db_instance.html Documentation for `delete_db_instance` says: `DBInstanceIdentifier [...] This parameter isn’t case-sensitive.` https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds/client/delete_db_instance.html
|
I'll take a look at the test failures a bit later. |
...also for create_db_instance_read_replica(). Documentation for `create_db_instance_read_replica` says: `DBInstanceIdentifier [...] This parameter is stored as a lowercase string.` https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds/client/create_db_instance_read_replica.html
...for reboot_db_instance(). This already works because the backend just calls describe_db_instances() which already uses lower case.
...for promote_read_replica. Documentation for `promote_read_replica` says: DBInstanceIdentifier [...] This value is stored as a lowercase string. https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds/client/promote_read_replica.html
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8564 +/- ##
==========================================
- Coverage 92.65% 92.65% -0.01%
==========================================
Files 1231 1231
Lines 106851 106853 +2
==========================================
Hits 99005 99005
- Misses 7846 7848 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@snordhausen There is a larger issue here, in that all of the RDS models (DBInstance, DBCluster, DBSnapshot, etc.) have case-insensitive identifiers, so I think we need a higher-level solution. Peppering .lower() throughout the code is messy, and it's too easy to miss a spot. We definitely want to handle this at the boundaries and not indiscriminately throughout the code. For example, we could store the identifiers in a case-insensitive dict, we can add a parameter to the filter configuration(s) to indicate if a comparison should be case-insensitive, etc.
If you'd like to consider this a bit more and propose another solution, feel free. If the increased scope is not something you want to tackle, I can add it to my TODO list (as I am currently in the process of refactoring the RDS backend and adding lots of new features).
|
@bpandola What do you think about doing it like this? That lower-cases the DBInstanceIdentifier before it gets in contact with any backend logic and replaces the old code. I think it can be extended for the other cases you mentioned, e.g. with a for-loop in |
Various RDS API calls treat the DBInstanceIdentifier as lowercase / case insensitive.
Documentation for
create_db_instancesays:DBInstanceIdentifier [...] This parameter is stored as a lowercase string.Documentation for
describe_db_instancessays:DBInstanceIdentifier [...] This parameter isn’t case-sensitive.Documentation for
modify_db_instancesays:DBInstanceIdentifier [...] This value is stored as a lowercase string.NewDBInstanceIdentifier [...] This value is stored as a lowercase string.Documentation for
delete_db_instancesays:DBInstanceIdentifier [...] This parameter isn’t case-sensitive.