-
-
Notifications
You must be signed in to change notification settings - Fork 353
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
Add GovCloud region name to validation set. #175
Conversation
Add GovCloud region name to validation set.
Add GovCloud region name to validation set.
Thanks! |
I was curious why you were validating input on the client side. It seems to me that you're liable to get into trouble when AWS inevitably changes allowed values (like adding regions, for example). Is it not workable to pass unvalidated calls to AWS and let the AWS API response error inform the user that they passed invalid values? What was the motivation for the engineyard pull request that added this client side validation? |
much better to validate up front. |
I see. The immediate failure mode that comes to mind (and which is the one that affected us) is that not every user of this library is a direct developer with access to the fog AWS object's configuration. So modifying the list of known regions isn't necessarily easy or even possible, and so the default behavior becomes much more important. For example, the vagrant-aws vagrant plugin takes configuration values out of a Vagrantfile and passes them on to fog to instantiate EC2 instances. If the vagrant user specifies a legitimate AWS region that's unknown to fog (like the govcloud region was), the tool fails. And because fog was a "hidden" dependency from the point of view of the end user, it's not clear to the user why the failure happened or what to do about it. In general, it's brittle practice to validate a client library's allowed values in lockstep with a 3rd-party API. Inevitable changes in behavior on the API's end will manifest as sudden breakages for deployments which haven't incorporated the latest library code. And the client development team must maintain constant vigilance for even minor changes in the API. Even the swiftest response on the part of the development team will still leave users in a state of breakage for an unknown period of time. If it's a hard requirement that invalid regions be detected on object creation, then it seems to me that the correct behavior would be a call out to AWS to fetch the set of allowed regions, with the input validated against that list. I'm unfamiliar with the details of the AWS API, but perhaps this would provide that information? http://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DescribeRegions.html In any case, thank you again for your quick turn around on our pull request. |
@triplepoint you make some excellent points. 100% understand the issues with hardcoding the regions, exacerbated by fog not being a direct dependency. I will pitch this to the other owners. Might be worth adding an option to disable validation. |
An easy compromise might be to change this from an error to a warning. It wouldn't "just work" for govcloud as a region, but I think it would allow you to explicitly pass that as the region name as well as the endpoint host/path and (probably) get it to work? Might be worth a try anyway. What do you think? |
It looks like the GovCloud region was left off the enumerated set of region names.
It's a bit difficult to parse Amazon's documentation, but we believe 'us-gov-west-1' is the only GovCloud region:
http://docs.aws.amazon.com/govcloud-us/latest/UserGuide/using-govcloud-arns.html
We discovered this through mitchellh/vagrant-aws, which uses this library to validate Vagrant parameters when building ec2 instances.