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

Reduce ability to create invalid RDS template #29

Merged
merged 1 commit into from Oct 21, 2015
Merged

Conversation

ddgenome
Copy link
Contributor

Implement a complex mechanism for reducing the chance of creating JSON
for an RDS DBInstance that is invalid. My hope is that the complexity
equals and is not greater than the complexity inherent in the
restrictions around RDS creation. There is still one run-time check
around storage encryption since specifying it is only valid on new
instances created within VPCs.

A couple other minor changes:

  • Clean up build.sbt
  • Implement and use EnumFormat for all enumerations JSON serialization

When this change gets merged, the release will have to increment the major version number since this breaks RDS backward compatibility badly.

@sattvik
Copy link

sattvik commented Oct 16, 2015

It's a big set of changes, so it's hard to judge whether it's completely correct, especially not understanding the underlying rules. I do like the enumeration format.

@ddgenome
Copy link
Contributor Author

I stole the enumeration format from @hibikir . I've tried to capture the rules around creation in the Scaladoc, but it is convoluted.

@ddgenome
Copy link
Contributor Author

I forgot to include in the commit and above that I also added support for DeletionPolicy.

@hibikir
Copy link
Contributor

hibikir commented Oct 19, 2015

LGTM. I can't verify that the modeling is perfect, but what I can say is that it's better than the one provided by Amazon. If more changes are needed later, because we didn't quite grasp all the RDS rules, so be it.

@ddgenome
Copy link
Contributor Author

I found a few issues with this PR. There is no good way to map from a Token[X] to X, so as it currently stands the fields that are Ints can't take ParameterRef's as values. To allow that, you'd have to accept Token[Int], and then you can't do any math, like checking that the Iops value is valid, since you won't actually have the value at compile/run time.

While I'd like to keep those checks in, I am not seeing a way to do that, so I'm going to convert to using Tokens.

@sattvik
Copy link

sattvik commented Oct 20, 2015

Hmm… it is possible to code things to know whether you have a raw value or a resource reference, and behave differently depending on the result. However, I haven't spent enough time looking at the code to try to figure out what's the best way to implement that with the minimum amount of impact on the existing API.

@ryan-richt
Copy link
Contributor

Yes, that is what you would have to do, because there are situations where you literally cannot turn a token into an actual thing at generation time (not knowable, like the parameter case), so you'd have to have two paths and get more checks on only one side
This e-mail message may contain privileged and/or confidential information, and is intended to be received only by persons entitled
to receive such information. If you have received this e-mail in error, please notify the sender immediately. Please delete it and
all attachments from any servers, hard drives or any other media. Other use of this e-mail by you is strictly prohibited.

All e-mails and attachments sent and received are subject to monitoring, reading and archival by Monsanto, including its
subsidiaries. The recipient of this e-mail is solely responsible for checking for the presence of "Viruses" or other "Malware".
Monsanto, along with its subsidiaries, accepts no liability for any damage caused by any such code transmitted by or accompanying
this e-mail or any attachment.

The information contained in this email may be subject to the export control laws and regulations of the United States, potentially
including but not limited to the Export Administration Regulations (EAR) and sanctions regulations issued by the U.S. Department of
Treasury, Office of Foreign Asset Controls (OFAC). As a recipient of this information you are obligated to comply with all
applicable U.S. export laws and regulations.

@ddgenome
Copy link
Contributor Author

I was thinking about using something like Either, but then I figured these values will almost always be parameters, not hard-coded. Well, I may do that if for nothing other than it may be worthwhile to keep the logic in for those reading the code.

Implement a complex mechanism for reducing the chance of creating JSON
for an RDS DBInstance that is invalid.  My hope is that the complexity
equals and is not greater than the complexity inherent in the
restrictions around RDS creation.  There is still one run-time check
around storage encryption since specifying it is only valid on new
instances created within VPCs.

There are restrictions on the values of AllocatedStorage when using
Iops but often these values are given as parameter and therefore must
accept Token[Int].  I currently used Either to take Int or Token[Int]
but at some point it would be good to allow one to reach through the
Token to see if the Int is there too test.

Tests were added to cover all of the above.

A couple other minor changes:
-   Clean up build.sbt
-   Implement and use EnumFormat for all enumerations JSON serialization
@ddgenome
Copy link
Contributor Author

I implemented this as Either and fixed a couple other minor things.

ddgenome added a commit that referenced this pull request Oct 21, 2015
Reduce ability to create invalid RDS template
@ddgenome ddgenome merged commit 6b75c99 into master Oct 21, 2015
@ddgenome ddgenome deleted the rds-encrypt branch October 21, 2015 14:33
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.

None yet

4 participants