cloudtools / troposphere

troposphere - Python library to create AWS CloudFormation descriptions
BSD 2-Clause "Simplified" License
4.93k stars 1.45k forks source link

Update rds validator logic #2164

Closed tnielsen2 closed 1 year ago

tnielsen2 commented 1 year ago

Troposphere has a new validator released in 4.3.1 that does not allow omitting the Iops attribute when gp3 and small StorageType parameters are passed.

If we passed Iops to an instance under 400GB, with EngineType = mysql, postgres, mariadb; deploying CFN will provide the following deployment error:

2023-07-27T13:15:27.261000+00:00 CREATE_FAILED (AWS::RDS::DBInstance) testrds Resource handler returned message: "You can't specify IOPS or storage throughput for engine mysql and a storage size less than 400. (Service: Rds, Status Code: 400, Request ID: omitted)" (RequestToken: omitted, HandlerErrorCode: InvalidRequest)
2023-07-27T13:15:28.148000+00:00 CREATE_FAILED (AWS::IAM::Role) role Resource creation cancelled
2023-07-27T13:15:28.592000+00:00 ROLLBACK_IN_PROGRESS (AWS::CloudFormation::Stack) dev-omitted-rds The following resource(s) failed to create: [role, testrds]. Rollback requested by user.

Documentation here outlines the possible combinations of parameters related to GP3, EngineType, and conditional values/requirements.

image

Reviewing CloudFormation documentation here states that IOPS is only required for io1.

Note If you specify io1 for the StorageType property, then you must also specify the Iops property.

This PR updates validation logic to allow small storage RDS instances backed by GP3 to be launched with a valid configuration values as per the above linked documentation.

Edit post submittal:

Summary of refspec ride-along changes/fixes:

tnielsen2 commented 1 year ago

As part of this PR, I am attempting to update the code to the latest ref spec, but I am having difficulty getting all CI to pass.

When regen generates the spec, it will sometimes generate the class objects out of order within the Python module.

Example

When I fix those errors, the regen portion of the PR fails, but I can get lint to pass.

The main goal of this PR is to update RDS GP3 validator logic, while attempting to update refspec so CI will pass, but I have been unsuccessful. I seem to be able to only get Lint or Refspec to pass.

@markpeek What is the course for action for this PR? How important is it to have both CI and Refspec generation pass? I looked into patch scripts to address this issue, but I couldn't seem to figure it out on own. Any insight is appreciated.

tnielsen2 commented 1 year ago

With the latest commit, at this moment, lint is failing due to class objects not being generated in the correct order, as well as what seems to be circular dependencies in the connect module with the introduction of new AWS Properties and nested properties.

What is the best way for this to be fixed? And if it is a patch script, what operations would I be using to fix these?

tnielsen2 commented 1 year ago

I have applied the patch changes to get this to pass lint and refspec generation. I have updated the PR with the necessary ride-along refspec updates.

markpeek commented 1 year ago

@tnielsen2 thank you for the PR. Some of the change are needed but there is still an issue with regenerating from the spec. I looked at it this weekend and I think I have a change that should allow this to work better. I'm going to create a branch and start applying the older spec versions into it so we can better reason about the changes. I'll ping back here when I've pushed the new branch out for your review. Thank you for your patience.

tnielsen2 commented 1 year ago

I have rebased from main with upstream changes and this is ready for review after the latest round of discussions/conversation @markpeek

Thanks for all you do!

markpeek commented 1 year ago

Thank you for the PR and getting it rebased.