Refactor AWS 2

[evansiroky]

Checklist

• Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
• Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
• The description lists all applicable issues this PR seeks to resolve
• The description lists any configuration setting(s) that differ from the default settings
• All tests and CI builds passing

Description

After looking through the first AWS refactor, it became clear that the AWSUtils class was becoming quite cluttered. Also, there were still a lot of methods in various classes that had logic that would be better placed elsewhere. Therefore, I moved a whole bunch of stuff around as follows:

• All AWS-related classes have been put into the common.util.aws package
• The AWSUtils class has been broken up into the EC2Utils, IAMUtils and S3Utils classes.
• The AWS credential handling has been moved into the AWSClientManager class
• Moves most ServerController#terminateInstances methods into EC2Utils
• Moves most OtpServer validation methods from ServerController into OtpServer
• Moves the fetchEC2Instances and one of the fetchEC2InstanceSummaries methods from DeploymentController into EC2Utils
• Moves the FeedStore bucket and prefix variables into S3Utils

Also, the PR now fixes a bug so that role-based session credentials are properly refreshed.

[codecov-commenter]

Codecov Report

Merging #341 into dev will decrease coverage by 0.09%.
The diff coverage is 0.41%.

@@             Coverage Diff              @@
##                dev     #341      +/-   ##
============================================
- Coverage     23.72%   23.62%   -0.10%     
- Complexity      545      546       +1     
============================================
  Files           172      175       +3     
  Lines          8998     9027      +29     
  Branches       1225     1224       -1     
============================================
- Hits           2135     2133       -2     
- Misses         6654     6686      +32     
+ Partials        209      208       -1     

Flag	Coverage Δ	Complexity Δ
#unit_tests	23.62% <0.41%> (-0.10%)	546.00 <1.00> (+1.00)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ	Complexity Δ
...conveyal/datatools/common/utils/ExpiringAsset.java	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...l/datatools/common/utils/aws/AWSClientManager.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...atatools/common/utils/aws/CheckedAWSException.java	0.00% <ø> (ø)	0.00 <0.00> (?)
.../conveyal/datatools/common/utils/aws/EC2Utils.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...atatools/common/utils/aws/EC2ValidationResult.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
.../conveyal/datatools/common/utils/aws/IAMUtils.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...m/conveyal/datatools/common/utils/aws/S3Utils.java	0.00% <0.00%> (ø)	0.00 <0.00> (?)
...tools/editor/controllers/api/EditorController.java	33.20% <0.00%> (-3.86%)	18.00 <0.00> (ø)
...ols/editor/controllers/api/SnapshotController.java	15.21% <0.00%> (ø)	2.00 <0.00> (ø)
...datatools/editor/jobs/ExportSnapshotToGTFSJob.java	36.95% <0.00%> (ø)	4.00 <0.00> (ø)
... and 21 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cab4b4...b43aaa0. Read the comment docs.

[landonreed]

With the package change it's really difficult to tell which lines of code are different in this PR review. Does it make sense to move these files back into the util folder for this PR and then submit a separate PR that just moves all of the files into the new package?

[landonreed]

Can we move the EC2 config validation somewhere else? By moving it here, we're just ballooning this file (instead of ServerController.

[evansiroky]

I believe this is the best place for all of these validation utils. Every one of these validation methods is directly tied to an OtpServer instance.

[landonreed]

• verifyS3WritePermissions could be generalized and moved to S3Utils (doesn't need to be OtpServer specific).
• isEmpty could be moved to Utils or some other
• validateAmiId, validateKeyName, validateSecurityGroupId could be generalized and moved to EC2Utils (where there are already some other validation methods)
• validateIamInstanceProfileArn could be moved to IAMUtils

[evansiroky]

Moved all of this in 5d3a85b

[landonreed]

This has so many changes that I really can't be confident that everything will still work as expected (similarly to the previous PR). The fact that there are no accompanying tests does not give me much comfort. In general, I think it's best to not fix things unless they're broken. I know that some small things were broken with getting credentials, but I'm not sure if the scale of these PRs was really warranted.

Nevertheless, this looks like a good set of changes. Only a couple of comments about improving some very minor things.

[evansiroky]

For now, my method of "testing" is to try deploying something with a custom region and then deploying something that uses roles to get session credentials. If both of those complete successfully then I think it's good enough. It'd probably be a good idea to make some e2e tests or at least mock some of the AWS classes. With this refactor it might be easier to do that. Anyways, I'm reassigning as I believe I have addressed all other comments.

[binh-dam-ibigroup]

Conditional approval: The typos should be fixed IMO. The other code tweaks I mentioned are not blocking.

[binh-dam-ibigroup]

I like the split between EC2, S3, IAM etc.

[landonreed]

Looks good. I had thought that some of the validation methods could be generalized so they could be used without an OtpServer, but that shouldn't be blocking (and could be handled if they're needed later in other contexts).

[landonreed]

🎉 This PR is included in version 3.8.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀