Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update rds validator logic #2164

Merged
merged 23 commits into from
Aug 9, 2023
Merged

Update rds validator logic #2164

merged 23 commits into from
Aug 9, 2023

Conversation

tnielsen2
Copy link
Contributor

@tnielsen2 tnielsen2 commented Jul 27, 2023

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:

  • update gen.py not to suffer recursion of EvaluationForm resource infinintely. Used same method as outlined in scripts/gen.py for wafv2
  • Patch connect resources to change type to fix out of order circular dependency issues
  • Migrate FSX fix patch from s3 file to FSX file
  • Update S3 patching to accept Encryption Type
  • Update Sagemaker patches that were removed in latest refspec
  • Remove invalid S3 transfer ItemType.
  • Make refspec, and regen

@tnielsen2
Copy link
Contributor Author

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
Copy link
Contributor Author

tnielsen2 commented Jul 27, 2023

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
Copy link
Contributor Author

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
Copy link
Member

markpeek commented Aug 7, 2023

@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 tnielsen2 changed the base branch from main to update-2023-08 August 9, 2023 19:38
@tnielsen2 tnielsen2 changed the base branch from update-2023-08 to main August 9, 2023 19:39
@tnielsen2 tnielsen2 changed the base branch from main to fix_min_instances August 9, 2023 20:32
@tnielsen2 tnielsen2 changed the base branch from fix_min_instances to main August 9, 2023 20:32
@tnielsen2
Copy link
Contributor Author

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 markpeek merged commit 0faace5 into cloudtools:main Aug 9, 2023
6 checks passed
@markpeek
Copy link
Member

markpeek commented Aug 9, 2023

Thank you for the PR and getting it rebased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants