Merge lp:~dimitern/goamz/vpc-api-calls into lp:goamz

[Dimiter Naydenov]

ec2: Add support for AWS VPCs

Added the following new API calls:
- CreateVPC
- DeleteVPC
- VPCs
(and the associated types/responses).

Most new code is in vpc.go and vpc_test.go.

These are needed in order to support VPC-related
operations on EC2, which will come in later follow-ups.
Implementation complexity in ec2test package for the
new calls is minimal.

Some changes were needed in order to support these
and the upcoming API calls. For the new ones, AWS API
version 2013-10-15 (latest) is used, while for the
existing calls use the previous API version, as before
(2011-12-15).

Added tests, updated test doubles and tested live on
EC2.

After running the EC2 live tests numerous times, I
realized some tests are leaking security groups, so
I added a deleteGroups() tests helper that retries
to ensure all groups are deleted. Also improved the
VPC tests to include retrying as well, so the live
tests are more stable and clean up after themselves.

https://codereview.appspot.com/49930045/

[Dimiter Naydenov (dimitern)]

Reviewers: mp+204514_code.launchpad.net,

Message:
Please take a look.

Description:
ec2: Add support for AWS VPCs

Added the following new API calls:
- CreateVpc
- DeleteVpc
- DescribeVpcs
(and the associated types/responses).

These are needed in order to support VPC-related
operations on EC2, which will come in later follow-ups.

Added tests, updated test doubles and tested live on EC2.

https://code.launchpad.net/~dimitern/goamz/vpc-api-calls/+merge/204514

Requires:
https://code.launchpad.net/~dimitern/goamz/lp-1275406-lpcalserversuite-testinstanceinfo/+merge/204469

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/49930045/

Affected files (+404, -13 lines):
   A [revision details]
   M ec2/ec2.go
   M ec2/ec2_test.go
   M ec2/ec2t_test.go
   M ec2/ec2test/server.go
   M ec2/responses_test.go

[Martin Packman (gz)]

LGTM, a couple of comments.

https://codereview.appspot.com/49930045/diff/1/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2.go#newcode925
ec2/ec2.go:925: func (ec2 *EC2) CreateVpc(cidrBlock string) (resp
*CreateVpcResp, err error) {
Do want to take and pass through the instanceTenancy argument here even
if we don't need it ourselves?

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode631
ec2/ec2t_test.go:631: resp2, err := s.ec2.CreateVpc("1.2.0.0/18")
Is this a safe cidr to be using?

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode659
ec2/ec2t_test.go:659: _, err = s.ec2.DeleteVpc(id1)
This feels race/eventual consistency prone, perhaps not an issue for
vpcs or delete calls specifically?

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go#newcode217
ec2/ec2test/server.go:217: // Not done: tag, tag-key, tag-value.
Do we want an error not implemented type of thing rather than just not
matching?

https://codereview.appspot.com/49930045/diff/1/ec2/responses_test.go
File ec2/responses_test.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/responses_test.go#newcode594
ec2/responses_test.go:594: <CreateVpcResponse
xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
We probably don't want to mix and match schema versions in our example
output unless we really don't have a feature we need in an older schema.

https://codereview.appspot.com/49930045/

[lp:~dimitern/goamz/vpc-api-calls]

48. By Dimiter Naydenov on 2014-02-04

Changes after review

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/49930045/diff/1/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2.go#newcode925
ec2/ec2.go:925: func (ec2 *EC2) CreateVpc(cidrBlock string) (resp
*CreateVpcResp, err error) {
On 2014/02/03 21:40:54, gz wrote:
> Do want to take and pass through the instanceTenancy argument here
even if we
> don't need it ourselves?

It's not supported as an argument in API version 2011-12-15.

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode631
ec2/ec2t_test.go:631: resp2, err := s.ec2.CreateVpc("1.2.0.0/18")
On 2014/02/03 21:40:54, gz wrote:
> Is this a safe cidr to be using?

Yes, it passes even live tests :)

https://codereview.appspot.com/49930045/diff/1/ec2/ec2t_test.go#newcode659
ec2/ec2t_test.go:659: _, err = s.ec2.DeleteVpc(id1)
On 2014/02/03 21:40:54, gz wrote:
> This feels race/eventual consistency prone, perhaps not an issue for
vpcs or
> delete calls specifically?

I run them multiple times live without issues so far.

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/ec2test/server.go#newcode217
ec2/ec2test/server.go:217: // Not done: tag, tag-key, tag-value.
On 2014/02/03 21:40:54, gz wrote:
> Do we want an error not implemented type of thing rather than just not
matching?

It's technically not an error, but since this is just a test double, I
don't think it's a big deal to error out.

https://codereview.appspot.com/49930045/diff/1/ec2/responses_test.go
File ec2/responses_test.go (right):

https://codereview.appspot.com/49930045/diff/1/ec2/responses_test.go#newcode594
ec2/responses_test.go:594: <CreateVpcResponse
xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
On 2014/02/03 21:40:54, gz wrote:
> We probably don't want to mix and match schema versions in our example
output
> unless we really don't have a feature we need in an older schema.

You're right. I found the API docs for version 2011-12-15 and updated
the example responses, as well as the code to conform to that (i.e. not
IsDefault and no InstanceTenancy fields anymore).

https://codereview.appspot.com/49930045/

[lp:~dimitern/goamz/vpc-api-calls]

49. By Dimiter Naydenov on 2014-02-04

Split VPC calls/tests in a separate file

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/49930045/

[lp:~dimitern/goamz/vpc-api-calls]

50. By Dimiter Naydenov on 2014-02-05

Simplified & refactored a bit; naming fixed, API version updated

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/49930045/

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/49930045/

[Roger Peppe (rogpeppe)]

LGTM with some suggestions, mostly documentation related.

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go#newcode212
ec2/ec2test/server.go:212: return false, fmt.Errorf("tag, tag-key, and
tag-value filters not implemented")
fmt.Errorf("%s filter not implemented", attr)

?

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode122
ec2/ec2.go:122: if params["Version"] == "" {
Given that makeParams now always creates the Version
entry, why bother with this?

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode183
ec2/ec2.go:183: func makeParamsVPC(action string) map[string]string {
I'm not sure this really justifies itself over
just calling makeParamsForVersion(action, VPCAPIVersion)
when necessary.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode187
ec2/ec2.go:187: func makeParamsForVersion(version, action string)
map[string]string {
makeParamsWithVersion(action, version string) map[string]string
?

then makeParamsWithVersion is just makeParams with an extra argument.
(i *think* that With is more appropriate than For in this case).

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go#newcode1095
ec2/ec2test/server.go:1095: func collectIds(form url.Values, prefix
string) map[string]bool {
doc comment?

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode26
ec2/vpc.go:26: // AWS API version used for VPC-related calls.
Do we actually want to export this?

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode53
ec2/vpc.go:53: // The smallest VPC you can create uses a /28 netmask (16
IP
s/you can create/that can be created/

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode57
ec2/vpc.go:57: // The supported tenancy options for instances launched
into the
// The instanceTenancy value holds the tenancy options for
// instances launched into the VPC - either DefaultTenancy
// or DedicatedTenancy.
//
// See http://goo.gl/nkwjvN for more details.

?

I think the details of the tenancy semantics can reasonably
be left to the amazon docs.

We should also document that instanceTenancy may be empty
or (my preference) just leave the description unchanged,
and lose the special case for instanceTenancy == "".

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode79
ec2/vpc.go:79: // DeleteVPC deletes the specified VPC. You must detach
or delete all
I'm not keen on the "you" phrasing (it might not be
the reader that's responsible for doing it).

I'd prefer something like this:

// DeleteVPC deletes the VPC with the specified id.
// All gateways and resources that are associated with
// the VPC must have been previously deleted,
// including instances running in the VPC, and non-default security
// groups a...

[lp:~dimitern/goamz/vpc-api-calls]

51. By Dimiter Naydenov on 2014-02-06

Changes after review; tests improvements

52. By Dimiter Naydenov on 2014-02-06

Changed CIDRs of VPC for easier tracking of leaky tests

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/40001/ec2/ec2test/server.go#newcode212
ec2/ec2test/server.go:212: return false, fmt.Errorf("tag, tag-key, and
tag-value filters not implemented")
On 2014/02/05 14:29:14, rog wrote:
> fmt.Errorf("%s filter not implemented", attr)

> ?

It was changed in later patch sets.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode122
ec2/ec2.go:122: if params["Version"] == "" {
On 2014/02/05 14:29:14, rog wrote:
> Given that makeParams now always creates the Version
> entry, why bother with this?

Just to be on the safe side, but fair enough. Removed.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode183
ec2/ec2.go:183: func makeParamsVPC(action string) map[string]string {
On 2014/02/05 14:29:14, rog wrote:
> I'm not sure this really justifies itself over
> just calling makeParamsForVersion(action, VPCAPIVersion)
> when necessary.

I'd prefer to leave it, because it's shorter to write - it's just a
helper anyway.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2.go#newcode187
ec2/ec2.go:187: func makeParamsForVersion(version, action string)
map[string]string {
On 2014/02/05 14:29:14, rog wrote:
> makeParamsWithVersion(action, version string) map[string]string
> ?

> then makeParamsWithVersion is just makeParams with an extra argument.
> (i *think* that With is more appropriate than For in this case).

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/ec2test/server.go#newcode1095
ec2/ec2test/server.go:1095: func collectIds(form url.Values, prefix
string) map[string]bool {
On 2014/02/05 14:29:14, rog wrote:
> doc comment?

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode26
ec2/vpc.go:26: // AWS API version used for VPC-related calls.
On 2014/02/05 14:29:14, rog wrote:
> Do we actually want to export this?

Not really. Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode53
ec2/vpc.go:53: // The smallest VPC you can create uses a /28 netmask (16
IP
On 2014/02/05 14:29:14, rog wrote:
> s/you can create/that can be created/

Done.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode57
ec2/vpc.go:57: // The supported tenancy options for instances launched
into the
On 2014/02/05 14:29:14, rog wrote:
> // The instanceTenancy value holds the tenancy options for
> // instances launched into the VPC - either DefaultTenancy
> // or DedicatedTenancy.
> //
> // See http://goo.gl/nkwjvN for more details.

> ?

> I think the details of the tenancy semantics can reasonably
> be left to the amazon docs.

> We should also document that instanceTenancy may be empty
> or (my preference) just leave the description unchanged,
> and lose the special case for instanceTenancy == "".

I'll change...

[Roger Peppe (rogpeppe)]

LGTM with a few further suggestions below.

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/80001/ec2/vpc.go#newcode57
ec2/vpc.go:57: // The supported tenancy options for instances launched
into the
On 2014/02/06 11:32:22, dimitern wrote:
> On 2014/02/05 14:29:14, rog wrote:
> > // The instanceTenancy value holds the tenancy options for
> > // instances launched into the VPC - either DefaultTenancy
> > // or DedicatedTenancy.
> > //
> > // See http://goo.gl/nkwjvN for more details.
> >
> > ?
> >
> > I think the details of the tenancy semantics can reasonably
> > be left to the amazon docs.
> >
> > We should also document that instanceTenancy may be empty
> > or (my preference) just leave the description unchanged,
> > and lose the special case for instanceTenancy == "".

> I'll change the comment as suggested, but will change the if below to
set it to
> DefaultTenancy when empty, because we can't pass it as "" to AWS.

If the user passes "" to the function, they deserve any error they get -
the function doesn't document that that's allowed (it actually
explicitly documents that instanceTenancy should be either
DefaultTenancy or DedicatedTenancy)

ec2 doesn't accept "furble" either, but we don't check that, and I don't
think there's a need to.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode656
ec2/ec2t_test.go:656: func (s *ServerTests) errorCode(err error, code
string) bool {
When I've done this function before, I've implemented it as:

func errorCode(err error) string {
     if err, _ := err.(*ec2.Error); err != nil {
         return err.Code
     }
     return ""
}

then the call looks like:

   if errorCode(err) == "InvalidGroup.NotFound" {

I think that reads a little better, and it's more
flexible.

I'd actually suggest doing that, and then a subsequent
CL could change other tests to use it (I count about 5 places).

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go#newcode1096
ec2/ec2test/server.go:1096: // returns a map with the ids as keys for
easier lookup.
s/ for easier lookup././

No particular need for the rationalisation.

https://codereview.appspot.com/49930045/

[Martin Packman (gz)]

LGTM.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go#newcode177
ec2/ec2.go:177: return makeParamsWithVersion(action, "2011-12-15")
I'd stick "2011-12-15" in a const too. Nice adaptation to allow version
passing.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode627
ec2/ec2t_test.go:627: func (s *ServerTests) deleteGroups(c *C, groups
[]ec2.SecurityGroup) {
This is somewhat ick, but I like that there's enough logging going on so
failures that hit the timeout should be clear enough.

https://codereview.appspot.com/49930045/

[lp:~dimitern/goamz/vpc-api-calls]

53. By Dimiter Naydenov on 2014-02-07

Changes after reivew

[Dimiter Naydenov (dimitern)]

Please take a look.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2.go#newcode177
ec2/ec2.go:177: return makeParamsWithVersion(action, "2011-12-15")
On 2014/02/06 16:25:07, gz wrote:
> I'd stick "2011-12-15" in a const too. Nice adaptation to allow
version passing.

Done.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode627
ec2/ec2t_test.go:627: func (s *ServerTests) deleteGroups(c *C, groups
[]ec2.SecurityGroup) {
On 2014/02/06 16:25:07, gz wrote:
> This is somewhat ick, but I like that there's enough logging going on
so
> failures that hit the timeout should be clear enough.

Yeah, it's more resilient than just trying to delete them and not
checking the error :)

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2t_test.go#newcode656
ec2/ec2t_test.go:656: func (s *ServerTests) errorCode(err error, code
string) bool {
On 2014/02/06 14:42:08, rog wrote:
> When I've done this function before, I've implemented it as:

> func errorCode(err error) string {
> if err, _ := err.(*ec2.Error); err != nil {
> return err.Code
> }
> return ""
> }

> then the call looks like:

> if errorCode(err) == "InvalidGroup.NotFound" {

> I think that reads a little better, and it's more
> flexible.

> I'd actually suggest doing that, and then a subsequent
> CL could change other tests to use it (I count about 5 places).

Done.

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go
File ec2/ec2test/server.go (right):

https://codereview.appspot.com/49930045/diff/100001/ec2/ec2test/server.go#newcode1096
ec2/ec2test/server.go:1096: // returns a map with the ids as keys for
easier lookup.
On 2014/02/06 14:42:08, rog wrote:
> s/ for easier lookup././

> No particular need for the rationalisation.

Done.

https://codereview.appspot.com/49930045/

[Gustavo Niemeyer (niemeyer)]

LGTM with trivial points.

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go#newcode31
ec2/ec2.go:31: // VPC-related requests.
Moving the VPC API version here as well would make it more clear. The
constant isn't even used in vpc.go either way.

Also, is there a known reason not to use the new API version across the
board?

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go#newcode657
ec2/ec2t_test.go:657: func (s *ServerTests) errorCode(err error) string
{
This can be a function (s is unnecessary).

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode8
ec2/vpc.go:8: // Written by Gustavo Niemeyer
<email address hidden>
Please drop this line. We should drop it from all the files.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode19
ec2/vpc.go:19: PendingState = "pending"
The names of these constants do not look entirely sane. Although this is
inside the vpc.go file, this is living inside the ec2 package, which
means it's ec2.PendingState, and ec2.DefaultTenancy, which is a lot more
broad than "supported values for VPC state".

I suggest either fixing them to include the fact they are about VPC
(VPCPendingState, for example), or just dropping the constants until we
have a better idea.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode24
ec2/vpc.go:24: DedicatedTenancy = "dedicated"
Same as above.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode30
ec2/vpc.go:30: // VPC describes an Amazon Virtual Private Cloud (VPC).
s/an Amazon Virtual/a Virtual/

We might have other pre-existent instances of that in the documentation,
but we should eventually fix them as well to not refer to Amazon. First,
because every single documentation entry refers to something coming out
of Amazon, and second, because this will work with whoever implements
this API.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc_test.go
File ec2/vpc_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc_test.go#newcode8
ec2/vpc_test.go:8: // Written by Gustavo Niemeyer
<email address hidden>
Please drop this line.

https://codereview.appspot.com/49930045/

[Dimiter Naydenov (dimitern)]

*** Submitted:

ec2: Add support for AWS VPCs

Added the following new API calls:
- CreateVPC
- DeleteVPC
- VPCs
(and the associated types/responses).

Most new code is in vpc.go and vpc_test.go.

These are needed in order to support VPC-related
operations on EC2, which will come in later follow-ups.
Implementation complexity in ec2test package for the
new calls is minimal.

Some changes were needed in order to support these
and the upcoming API calls. For the new ones, AWS API
version 2013-10-15 (latest) is used, while for the
existing calls use the previous API version, as before
(2011-12-15).

Added tests, updated test doubles and tested live on
EC2.

After running the EC2 live tests numerous times, I
realized some tests are leaking security groups, so
I added a deleteGroups() tests helper that retries
to ensure all groups are deleted. Also improved the
VPC tests to include retrying as well, so the live
tests are more stable and clean up after themselves.

R=
CC=
https://codereview.appspot.com/49930045

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go
File ec2/ec2.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2.go#newcode31
ec2/ec2.go:31: // VPC-related requests.
On 2014/02/12 13:23:49, niemeyer wrote:
> Moving the VPC API version here as well would make it more clear. The
constant
> isn't even used in vpc.go either way.

> Also, is there a known reason not to use the new API version across
the board?

I'll move it here. The reason for not using the newer API version is
that some things are changed, possibly in an incompatible way (responses
include different sub sections, more filters, certain arguments have
changed). I didn't want to make a massive change to all current API
calls to make them compatible with 2013-10-15. I could do it in a
follow-up, after the current chain have landed though.

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/ec2t_test.go#newcode657
ec2/ec2t_test.go:657: func (s *ServerTests) errorCode(err error) string
{
On 2014/02/12 13:23:49, niemeyer wrote:
> This can be a function (s is unnecessary).

Done.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go
File ec2/vpc.go (right):

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode8
ec2/vpc.go:8: // Written by Gustavo Niemeyer
<email address hidden>
On 2014/02/12 13:23:49, niemeyer wrote:
> Please drop this line. We should drop it from all the files.

I'll remove it from all files then.

https://codereview.appspot.com/49930045/diff/120001/ec2/vpc.go#newcode19
ec2/vpc.go:19: PendingState = "pending"
On 2014/02/12 13:23:49, niemeyer wrote:
> The names of these constants do not look entirely sane. Although this
is inside
> the vpc.go file, this is living inside the ec2 package, which means
it's
> ec2.PendingState, and ec2.DefaultTenancy, which is a lot more broad
than
> "supported values for VPC state".

> I suggest either fixing them to include the fact they are about VPC
> (VPCPendingState, for example), or just dropping the constants until
we have a
> better idea.

I'll drop ...

[Dimiter Naydenov (dimitern)]

Post-submit approval.

review: Approve

=== modified file 'ec2/ec2.go'
--- ec2/ec2.go	2013-10-04 17:05:49 +0000
+++ ec2/ec2.go	2014-02-07 12:35:18 +0000
@@ -27,6 +27,10 @@
 
 const debug = false
 
+// legacyAPIVersion is the AWS API version used for all but
+// VPC-related requests.
+const legacyAPIVersion = "2011-12-15"
+
 // The EC2 type encapsulates operations with a specific EC2 region.
 type EC2 struct {
 	aws.Auth
@@ -119,7 +123,6 @@
 var timeNow = time.Now
 
 func (ec2 *EC2) query(params map[string]string, resp interface{}) error {
-	params["Version"] = "2011-12-15"
 	params["Timestamp"] = timeNow().In(time.UTC).Format(time.RFC3339)
 	endpoint, err := url.Parse(ec2.Region.EC2Endpoint)
 	if err != nil {
@@ -175,8 +178,17 @@
 }
 
 func makeParams(action string) map[string]string {
+	return makeParamsWithVersion(action, legacyAPIVersion)
+}
+
+func makeParamsVPC(action string) map[string]string {
+	return makeParamsWithVersion(action, vpcAPIVersion)
+}
+
+func makeParamsWithVersion(action, version string) map[string]string {
 	params := make(map[string]string)
 	params["Action"] = action
+	params["Version"] = version
 	return params
 }
 
@@ -803,7 +815,7 @@
 	return resp, nil
 }
 
-// ResourceTag represents key-value metadata used to classify and organize
+// Tag represents key-value metadata used to classify and organize
 // EC2 instances.
 //
 // See http://goo.gl/bncl3 for more details

=== modified file 'ec2/ec2_test.go'
--- ec2/ec2_test.go	2013-10-04 17:05:49 +0000
+++ ec2/ec2_test.go	2014-02-07 12:35:18 +0000
@@ -96,14 +96,14 @@
 		DisableAPITermination: true,
 		ShutdownBehavior:      "terminate",
 		PrivateIPAddress:      "10.0.0.25",
-		BlockDeviceMappings:   []ec2.BlockDeviceMapping{{
-			DeviceName: "device-name",
-			VirtualName: "virtual-name",
-			SnapshotId: "snapshot-id",
-			VolumeType: "volume-type",
-			VolumeSize: 10,
+		BlockDeviceMappings: []ec2.BlockDeviceMapping{{
+			DeviceName:          "device-name",
+			VirtualName:         "virtual-name",
+			SnapshotId:          "snapshot-id",
+			VolumeType:          "volume-type",
+			VolumeSize:          10,
 			DeleteOnTermination: true,
-			IOPS: 1000,
+			IOPS:                1000,
 		}},
 	}
 	resp, err := s.ec2.RunInstances(&options)

=== modified file 'ec2/ec2t_test.go'
--- ec2/ec2t_test.go	2014-02-07 12:35:18 +0000
+++ ec2/ec2t_test.go	2014-02-07 12:35:18 +0000
@@ -5,6 +5,7 @@
 	"net"
 	"regexp"
 	"sort"
+	"time"
 
 	"launchpad.net/goamz/aws"
 	"launchpad.net/goamz/ec2"
@@ -111,7 +112,7 @@
 }
 
 // AmazonServerSuite runs the ec2test server tests against a live EC2 server.
-// It will only be activated if the -all flag is specified.
+// It will only be activated if the -amazon flag is specified.
 type AmazonServerSuite struct {
 	srv AmazonServer
 	ServerTests
@@ -149,7 +150,7 @@
 func (s *ServerTests) makeTestGroup(c *C, name, descr string) ec2.SecurityGroup {
 	// Clean it up if a previous test left it around.
 	_, err := s.ec2.DeleteSecurityGroup(ec2.SecurityGroup{Name: name})
-	if err != nil && err.(*ec2.Error).Code != "InvalidGroup.NotFound" {
+	if err != nil && s.errorCode(err) != "InvalidGroup.NotFound" {
 		c.Fatalf("delete security group: %v", err)
 	}
 
@@ -161,10 +162,8 @@
 
 func (s *ServerTests) TestIPPerms(c *C) {
 	g0 := s.makeTestGroup(c, "goamz-test0", "ec2test group 0")
-	defer s.ec2.DeleteSecurityGroup(g0)
-
 	g1 := s.makeTestGroup(c, "goamz-test1", "ec2test group 1")
-	defer s.ec2.DeleteSecurityGroup(g1)
+	defer s.deleteGroups(c, []ec2.SecurityGroup{g0, g1})
 
 	resp, err := s.ec2.SecurityGroups([]ec2.SecurityGroup{g0, g1}, nil)
 	c.Assert(err, IsNil)
@@ -183,7 +182,7 @@
 		SourceIPs: []string{"z127.0.0.1/24"},
 	}})
 	c.Assert(err, NotNil)
-	c.Check(err.(*ec2.Error).Code, Equals, "InvalidPermission.Malformed")
+	c.Check(s.errorCode(err), Equals, "InvalidPermission.Malformed")
 
 	// Check that AuthorizeSecurityGroup adds the correct authorizations.
 	_, err = s.ec2.AuthorizeSecurityGroup(g0, []ec2.IPPerm{{
@@ -230,7 +229,7 @@
 	// Check that we can't delete g1 (because g0 is using it)
 	_, err = s.ec2.DeleteSecurityGroup(g1)
 	c.Assert(err, NotNil)
-	c.Check(err.(*ec2.Error).Code, Equals, "InvalidGroup.InUse")
+	c.Check(s.errorCode(err), Equals, "InvalidGroup.InUse")
 
 	_, err = s.ec2.RevokeSecurityGroup(g0, []ec2.IPPerm{{
 		Protocol:     "tcp",
@@ -301,7 +300,7 @@
 	c.Assert(err, IsNil)
 
 	_, err = s.ec2.AuthorizeSecurityGroup(ec2.SecurityGroup{Name: name}, perms[0:2])
-	c.Assert(err, ErrorMatches, `.*\(InvalidPermission.Duplicate\)`)
+	c.Assert(s.errorCode(err), Equals, "InvalidPermission.Duplicate")
 }
 
 type filterSpec struct {
@@ -313,12 +312,12 @@
 	groupResp, err := s.ec2.CreateSecurityGroup(sessionName("testgroup1"), "testgroup one description")
 	c.Assert(err, IsNil)
 	group1 := groupResp.SecurityGroup
-	defer s.ec2.DeleteSecurityGroup(group1)
 
 	groupResp, err = s.ec2.CreateSecurityGroup(sessionName("testgroup2"), "testgroup two description")
 	c.Assert(err, IsNil)
 	group2 := groupResp.SecurityGroup
-	defer s.ec2.DeleteSecurityGroup(group2)
+
+	defer s.deleteGroups(c, []ec2.SecurityGroup{group1, group2})
 
 	insts := make([]*ec2.Instance, 3)
 	inst, err := s.ec2.RunInstances(&ec2.RunInstances{
@@ -484,8 +483,8 @@
 		c.Assert(err, IsNil)
 		g[i] = resp.SecurityGroup
 		c.Logf("group %d: %v", i, g[i])
-		defer s.ec2.DeleteSecurityGroup(g[i])
 	}
+	defer s.deleteGroups(c, g)
 
 	perms := [][]ec2.IPPerm{
 		{{
@@ -621,3 +620,43 @@
 		}
 	}
 }
+
+// deleteGroups ensures the given groups are deleted, by retrying
+// until a timeout or all groups cannot be found anymore.
+// This should be used to make sure tests leave no groups around.
+func (s *ServerTests) deleteGroups(c *C, groups []ec2.SecurityGroup) {
+	testAttempt := aws.AttemptStrategy{
+		Total: 2 * time.Minute,
+		Delay: 5 * time.Second,
+	}
+	for a := testAttempt.Start(); a.Next(); {
+		deleted := 0
+		c.Logf("deleting groups %v", groups)
+		for _, group := range groups {
+			_, err := s.ec2.DeleteSecurityGroup(group)
+			if err == nil || s.errorCode(err) == "InvalidGroup.NotFound" {
+				c.Logf("group %v deleted", group)
+				deleted++
+				continue
+			}
+			if err != nil {
+				c.Logf("retrying; DeleteSecurityGroup returned: %v", err)
+			}
+		}
+		if deleted == len(groups) {
+			c.Logf("all groups deleted")
+			return
+		}
+	}
+	c.Fatalf("timeout while waiting %v groups to get deleted!", groups)
+}
+
+// errorCode returns the code of the given error, assuming it's not
+// nil and it's an instance of *ec2.Error. It returns an empty string
+// otherwise.
+func (s *ServerTests) errorCode(err error) string {
+	if err, _ := err.(*ec2.Error); err != nil {
+		return err.Code
+	}
+	return ""
+}

=== modified file 'ec2/ec2test/server.go'
--- ec2/ec2test/server.go	2013-12-18 14:22:35 +0000
+++ ec2/ec2test/server.go	2014-02-07 12:35:18 +0000
@@ -50,10 +50,13 @@
 	instances            map[string]*Instance      // id -> instance
 	reservations         map[string]*reservation   // id -> reservation
 	groups               map[string]*securityGroup // id -> group
+	vpcs                 map[string]*vpc           // id -> vpc
 	maxId                counter
 	reqId                counter
 	reservationId        counter
 	groupId              counter
+	vpcId                counter
+	dhcpOptsId           counter
 	initialInstanceState ec2.InstanceState
 }
 
@@ -191,6 +194,24 @@
 	return
 }
 
+type vpc struct {
+	ec2.VPC
+}
+
+func (v *vpc) matchAttr(attr, value string) (ok bool, err error) {
+	switch attr {
+	case "cidr":
+		return v.CIDRBlock == value, nil
+	case "state":
+		return v.State == value, nil
+	case "vpc-id":
+		return v.Id == value, nil
+	case "tag", "tag-key", "tag-value", "dhcp-options-id", "isDefault":
+		return false, fmt.Errorf("%q filter is not implemented", attr)
+	}
+	return false, fmt.Errorf("unknown attribute %q", attr)
+}
+
 var actions = map[string]func(*Server, http.ResponseWriter, *http.Request, string) interface{}{
 	"RunInstances":                  (*Server).runInstances,
 	"TerminateInstances":            (*Server).terminateInstances,
@@ -200,6 +221,9 @@
 	"DeleteSecurityGroup":           (*Server).deleteSecurityGroup,
 	"AuthorizeSecurityGroupIngress": (*Server).authorizeSecurityGroupIngress,
 	"RevokeSecurityGroupIngress":    (*Server).revokeSecurityGroupIngress,
+	"CreateVpc":                     (*Server).createVpc,
+	"DeleteVpc":                     (*Server).deleteVpc,
+	"DescribeVpcs":                  (*Server).describeVpcs,
 }
 
 const ownerId = "9876"
@@ -220,6 +244,7 @@
 	srv := &Server{
 		instances:            make(map[string]*Instance),
 		groups:               make(map[string]*securityGroup),
+		vpcs:                 make(map[string]*vpc),
 		reservations:         make(map[string]*reservation),
 		initialInstanceState: Pending,
 	}
@@ -798,9 +823,11 @@
 	}
 }
 
-var secGroupPat = regexp.MustCompile(`^sg-[a-z0-9]+$`)
-var ipPat = regexp.MustCompile(`^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/[0-9]+$`)
-var ownerIdPat = regexp.MustCompile(`^[0-9]+$`)
+var (
+	secGroupPat = regexp.MustCompile(`^sg-[a-z0-9]+$`)
+	cidrIpPat   = regexp.MustCompile(`^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+/([0-9]+)$`)
+	ownerIdPat  = regexp.MustCompile(`^[0-9]+$`)
+)
 
 // parsePerms returns a slice of permKey values extracted
 // from the permission fields in req.
@@ -884,7 +911,7 @@
 			}
 			switch rest {
 			case "CidrIp":
-				if !ipPat.MatchString(val) {
+				if !cidrIpPat.MatchString(val) {
 					fatalf(400, "InvalidPermission.Malformed", "Invalid IP range: %q", val)
 				}
 				ec2p.SourceIPs = append(ec2p.SourceIPs, val)
@@ -978,6 +1005,61 @@
 	}
 }
 
+func (srv *Server) createVpc(w http.ResponseWriter, req *http.Request, reqId string) interface{} {
+	cidrBlock := parseCidr(req.Form.Get("CidrBlock"))
+	tenancy := req.Form.Get("InstanceTenancy")
+	if tenancy == "" {
+		tenancy = ec2.DefaultTenancy
+	}
+
+	srv.mu.Lock()
+	defer srv.mu.Unlock()
+	v := &vpc{ec2.VPC{
+		Id:              fmt.Sprintf("vpc-%d", srv.vpcId.next()),
+		State:           ec2.AvailableState,
+		CIDRBlock:       cidrBlock,
+		DHCPOptionsId:   fmt.Sprintf("dopt-%d", srv.dhcpOptsId.next()),
+		InstanceTenancy: tenancy,
+	}}
+	srv.vpcs[v.Id] = v
+	r := &ec2.CreateVPCResp{
+		RequestId: reqId,
+		VPC:       v.VPC,
+	}
+	return r
+}
+
+func (srv *Server) deleteVpc(w http.ResponseWriter, req *http.Request, reqId string) interface{} {
+	v := srv.vpc(req.Form.Get("VpcId"))
+	srv.mu.Lock()
+	defer srv.mu.Unlock()
+
+	delete(srv.vpcs, v.Id)
+	return &ec2.SimpleResp{
+		XMLName:   xml.Name{"", "DeleteVpcResponse"},
+		RequestId: reqId,
+	}
+}
+
+func (srv *Server) describeVpcs(w http.ResponseWriter, req *http.Request, reqId string) interface{} {
+	srv.mu.Lock()
+	defer srv.mu.Unlock()
+
+	idMap := collectIds(req.Form, "VpcId.")
+	f := newFilter(req.Form)
+	var resp ec2.VPCsResp
+	resp.RequestId = reqId
+	for _, v := range srv.vpcs {
+		ok, err := f.ok(v)
+		if ok && (len(idMap) == 0 || idMap[v.Id]) {
+			resp.VPCs = append(resp.VPCs, v.VPC)
+		} else if err != nil {
+			fatalf(400, "InvalidParameterValue", "describe VPCs: %v", err)
+		}
+	}
+	return &resp
+}
+
 func (r *reservation) hasRunningMachine() bool {
 	for _, inst := range r.instances {
 		if inst.state.Code != ShuttingDown.Code && inst.state.Code != Terminated.Code {
@@ -987,6 +1069,41 @@
 	return false
 }
 
+func parseCidr(val string) string {
+	if val == "" {
+		fatalf(400, "MissingParameter", "missing cidrBlock")
+	}
+	if _, _, err := net.ParseCIDR(val); err != nil {
+		fatalf(400, "InvalidParameterValue", "bad CIDR %q: %v", val, err)
+	}
+	return val
+}
+
+func (srv *Server) vpc(id string) *vpc {
+	if id == "" {
+		fatalf(400, "MissingParameter", "missing vpcId")
+	}
+	srv.mu.Lock()
+	defer srv.mu.Unlock()
+	v, found := srv.vpcs[id]
+	if !found {
+		fatalf(400, "InvalidVpcID.NotFound", "VPC %s not found", id)
+	}
+	return v
+}
+
+// collectIds takes all values with the given prefix from form and
+// returns a map with the ids as keys.
+func collectIds(form url.Values, prefix string) map[string]bool {
+	idMap := make(map[string]bool)
+	for name, vals := range form {
+		if strings.HasPrefix(name, prefix) {
+			idMap[vals[0]] = true
+		}
+	}
+	return idMap
+}
+
 type counter int
 
 func (c *counter) next() (i int) {

=== modified file 'ec2/responses_test.go'
--- ec2/responses_test.go	2013-10-02 19:10:36 +0000
+++ ec2/responses_test.go	2014-02-07 12:35:18 +0000
@@ -584,7 +584,48 @@
 // http://goo.gl/baoUf
 var RebootInstancesExample = `
 <RebootInstancesResponse xmlns="http://ec2.amazonaws.com/doc/2011-12-15/">
-  <requestId>59dbff89-35bd-4eac-99ed-be587EXAMPLE</requestId> 
+  <requestId>59dbff89-35bd-4eac-99ed-be587EXAMPLE</requestId>
   <return>true</return>
 </RebootInstancesResponse>
 `
+
+// http://goo.gl/nkwjv
+var CreateVpcExample = `
+<CreateVpcResponse xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
+   <requestId>7a62c49f-347e-4fc4-9331-6e8eEXAMPLE</requestId>
+   <vpc>
+      <vpcId>vpc-1a2b3c4d</vpcId>
+      <state>pending</state>
+      <cidrBlock>10.0.0.0/16</cidrBlock>
+      <dhcpOptionsId>dopt-1a2b3c4d2</dhcpOptionsId>
+      <instanceTenancy>default</instanceTenancy>
+      <tagSet/>
+   </vpc>
+</CreateVpcResponse>
+`
+
+// http://goo.gl/bcxtbf
+var DeleteVpcExample = `
+<DeleteVpcResponse xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
+   <requestId>7a62c49f-347e-4fc4-9331-6e8eEXAMPLE</requestId>
+   <return>true</return>
+</DeleteVpcResponse>
+`
+
+// http://goo.gl/Y5kHqG
+var DescribeVpcsExample = `
+<DescribeVpcsResponse xmlns="http://ec2.amazonaws.com/doc/2013-10-15/">
+  <requestId>7a62c49f-347e-4fc4-9331-6e8eEXAMPLE</requestId>
+  <vpcSet>
+    <item>
+      <vpcId>vpc-1a2b3c4d</vpcId>
+      <state>available</state>
+      <cidrBlock>10.0.0.0/23</cidrBlock>
+      <dhcpOptionsId>dopt-7a8b9c2d</dhcpOptionsId>
+      <instanceTenancy>default</instanceTenancy>
+      <isDefault>false</isDefault>
+      <tagSet/>
+    </item>
+  </vpcSet>
+</DescribeVpcsResponse>
+`

=== added file 'ec2/vpc.go'
--- ec2/vpc.go	1970-01-01 00:00:00 +0000
+++ ec2/vpc.go	2014-02-07 12:35:18 +0000
@@ -0,0 +1,120 @@
+//
+// goamz - Go packages to interact with the Amazon Web Services.
+//
+//   https://wiki.ubuntu.com/goamz
+//
+// Copyright (c) 2014 Canonical Ltd.
+//
+// Written by Gustavo Niemeyer <[e-mail]>
+//
+
+package ec2
+
+import (
+	"strconv"
+)
+
+const (
+	// Supported values for VPC state.
+	PendingState   = "pending"
+	AvailableState = "available"
+
+	// Supported values for InstanceTenancy.
+	DefaultTenancy   = "default"
+	DedicatedTenancy = "dedicated"
+
+	// AWS API version used for VPC-related calls.
+	vpcAPIVersion = "2013-10-15"
+)
+
+// VPC describes an Amazon Virtual Private Cloud (VPC).
+//
+// See http://goo.gl/Uy6ZLL for more details.
+type VPC struct {
+	Id              string `xml:"vpcId"`
+	State           string `xml:"state"`
+	CIDRBlock       string `xml:"cidrBlock"`
+	DHCPOptionsId   string `xml:"dhcpOptionsId"`
+	Tags            []Tag  `xml:"tagSet>item"`
+	InstanceTenancy string `xml:"instanceTenancy"`
+	IsDefault       bool   `xml:"isDefault"`
+}
+
+// CreateVPCResp is the response to a CreateVPC request.
+//
+// See http://goo.gl/nkwjvN for more details.
+type CreateVPCResp struct {
+	RequestId string `xml:"requestId"`
+	VPC       VPC    `xml:"vpc"`
+}
+
+// CreateVPC creates a VPC with the specified CIDR block.
+//
+// The smallest VPC that can be created uses a /28 netmask (16 IP
+// addresses), and the largest uses a /16 netmask (65,536 IP
+// addresses).
+//
+// The instanceTenancy value holds the tenancy options for instances
+// launched into the VPC - either DefaultTenancy or DedicatedTenancy.
+//
+// See http://goo.gl/nkwjvN for more details.
+func (ec2 *EC2) CreateVPC(CIDRBlock, instanceTenancy string) (resp *CreateVPCResp, err error) {
+	params := makeParamsVPC("CreateVpc")
+	params["CidrBlock"] = CIDRBlock
+	if instanceTenancy == "" {
+		instanceTenancy = DefaultTenancy
+	}
+	params["InstanceTenancy"] = instanceTenancy
+	resp = &CreateVPCResp{}
+	err = ec2.query(params, resp)
+	if err != nil {
+		return nil, err
+	}
+	return resp, nil
+}
+
+// DeleteVPC deletes the VPC with the specified id. All gateways and
+// resources that are associated with the VPC must have been
+// previously deleted, including instances running in the VPC, and
+// non-default security groups and route tables associated with the
+// VPC.
+//
+// See http://goo.gl/bcxtbf for more details.
+func (ec2 *EC2) DeleteVPC(id string) (resp *SimpleResp, err error) {
+	params := makeParamsVPC("DeleteVpc")
+	params["VpcId"] = id
+	resp = &SimpleResp{}
+	err = ec2.query(params, resp)
+	if err != nil {
+		return nil, err
+	}
+	return resp, nil
+}
+
+// VPCsResp is the response to a VPCs request.
+//
+// See http://goo.gl/Y5kHqG for more details.
+type VPCsResp struct {
+	RequestId string `xml:"requestId"`
+	VPCs      []VPC  `xml:"vpcSet>item"`
+}
+
+// VPCs describes one or more VPCs. Both parameters are optional, and
+// if specified will limit the returned VPCs to the matching ids or
+// filtering rules.
+//
+// See http://goo.gl/Y5kHqG for more details.
+func (ec2 *EC2) VPCs(ids []string, filter *Filter) (resp *VPCsResp, err error) {
+	params := makeParamsVPC("DescribeVpcs")
+	for i, id := range ids {
+		params["VpcId."+strconv.Itoa(i+1)] = id
+	}
+	filter.addParams(params)
+
+	resp = &VPCsResp{}
+	err = ec2.query(params, resp)
+	if err != nil {
+		return nil, err
+	}
+	return resp, nil
+}

=== added file 'ec2/vpc_test.go'
--- ec2/vpc_test.go	1970-01-01 00:00:00 +0000
+++ ec2/vpc_test.go	2014-02-07 12:35:18 +0000
@@ -0,0 +1,172 @@
+//
+// goamz - Go packages to interact with the Amazon Web Services.
+//
+//   https://wiki.ubuntu.com/goamz
+//
+// Copyright (c) 2014 Canonical Ltd.
+//
+// Written by Gustavo Niemeyer <[e-mail]>
+//
+
+package ec2_test
+
+import (
+	"launchpad.net/goamz/aws"
+	"launchpad.net/goamz/ec2"
+	. "launchpad.net/gocheck"
+	"time"
+)
+
+// VPC tests with example responses
+
+func (s *S) TestCreateVPCExample(c *C) {
+	testServer.Response(200, nil, CreateVpcExample)
+
+	resp, err := s.ec2.CreateVPC("10.0.0.0/16", ec2.DefaultTenancy)
+	req := testServer.WaitRequest()
+
+	c.Assert(req.Form["Action"], DeepEquals, []string{"CreateVpc"})
+	c.Assert(req.Form["CidrBlock"], DeepEquals, []string{"10.0.0.0/16"})
+	c.Assert(req.Form["InstanceTenancy"], DeepEquals, []string{ec2.DefaultTenancy})
+
+	c.Assert(err, IsNil)
+	c.Assert(resp.RequestId, Equals, "7a62c49f-347e-4fc4-9331-6e8eEXAMPLE")
+	vpc := resp.VPC
+	c.Check(vpc.Id, Equals, "vpc-1a2b3c4d")
+	c.Check(vpc.State, Equals, ec2.PendingState)
+	c.Check(vpc.CIDRBlock, Equals, "10.0.0.0/16")
+	c.Check(vpc.DHCPOptionsId, Equals, "dopt-1a2b3c4d2")
+	c.Check(vpc.Tags, HasLen, 0)
+	c.Check(vpc.IsDefault, Equals, false)
+	c.Check(vpc.InstanceTenancy, Equals, ec2.DefaultTenancy)
+}
+
+func (s *S) TestDeleteVPCExample(c *C) {
+	testServer.Response(200, nil, DeleteVpcExample)
+
+	resp, err := s.ec2.DeleteVPC("vpc-id")
+	req := testServer.WaitRequest()
+
+	c.Assert(req.Form["Action"], DeepEquals, []string{"DeleteVpc"})
+	c.Assert(req.Form["VpcId"], DeepEquals, []string{"vpc-id"})
+
+	c.Assert(err, IsNil)
+	c.Assert(resp.RequestId, Equals, "7a62c49f-347e-4fc4-9331-6e8eEXAMPLE")
+}
+
+func (s *S) TestVPCsExample(c *C) {
+	testServer.Response(200, nil, DescribeVpcsExample)
+
+	resp, err := s.ec2.VPCs([]string{"vpc-1a2b3c4d"}, nil)
+	req := testServer.WaitRequest()
+
+	c.Assert(req.Form["Action"], DeepEquals, []string{"DescribeVpcs"})
+	c.Assert(req.Form["VpcId.1"], DeepEquals, []string{"vpc-1a2b3c4d"})
+
+	c.Assert(err, IsNil)
+	c.Assert(resp.RequestId, Equals, "7a62c49f-347e-4fc4-9331-6e8eEXAMPLE")
+	c.Assert(resp.VPCs, HasLen, 1)
+	vpc := resp.VPCs[0]
+	c.Check(vpc.Id, Equals, "vpc-1a2b3c4d")
+	c.Check(vpc.State, Equals, ec2.AvailableState)
+	c.Check(vpc.CIDRBlock, Equals, "10.0.0.0/23")
+	c.Check(vpc.DHCPOptionsId, Equals, "dopt-7a8b9c2d")
+	c.Check(vpc.Tags, HasLen, 0)
+	c.Check(vpc.IsDefault, Equals, false)
+	c.Check(vpc.InstanceTenancy, Equals, ec2.DefaultTenancy)
+}
+
+// VPC tests to run against either a local test server or live on EC2.
+
+func (s *ServerTests) TestVPCs(c *C) {
+	resp1, err := s.ec2.CreateVPC("10.0.0.0/16", "")
+	c.Assert(err, IsNil)
+	assertVPC(c, resp1.VPC, "", "10.0.0.0/16")
+	id1 := resp1.VPC.Id
+
+	resp2, err := s.ec2.CreateVPC("10.1.0.0/16", ec2.DefaultTenancy)
+	c.Assert(err, IsNil)
+	assertVPC(c, resp2.VPC, "", "10.1.0.0/16")
+	id2 := resp2.VPC.Id
+
+	// We only check for the VPCs we just created, because the user
+	// might have others in his account (when testing against the EC2
+	// servers). In some cases it takes a short while until both VPCs
+	// are created, so we need to retry a few times to make sure.
+	var list *ec2.VPCsResp
+	done := false
+	testAttempt := aws.AttemptStrategy{
+		Total: 2 * time.Minute,
+		Delay: 5 * time.Second,
+	}
+	for a := testAttempt.Start(); a.Next(); {
+		c.Logf("waiting for %v to be created", []string{id1, id2})
+		list, err = s.ec2.VPCs(nil, nil)
+		if err != nil {
+			c.Logf("retrying; VPCs returned: %v", err)
+			continue
+		}
+		found := 0
+		for _, vpc := range list.VPCs {
+			c.Logf("found VPC %v", vpc)
+			switch vpc.Id {
+			case id1:
+				assertVPC(c, vpc, id1, resp1.VPC.CIDRBlock)
+				found++
+			case id2:
+				assertVPC(c, vpc, id2, resp2.VPC.CIDRBlock)
+				found++
+			}
+			if found == 2 {
+				done = true
+				break
+			}
+		}
+		if done {
+			c.Logf("all VPCs were created")
+			break
+		}
+	}
+	if !done {
+		c.Fatalf("timeout while waiting for VPCs %v", []string{id1, id2})
+	}
+
+	list, err = s.ec2.VPCs([]string{id1}, nil)
+	c.Assert(err, IsNil)
+	c.Assert(list.VPCs, HasLen, 1)
+	assertVPC(c, list.VPCs[0], id1, resp1.VPC.CIDRBlock)
+
+	f := ec2.NewFilter()
+	f.Add("cidr", resp2.VPC.CIDRBlock)
+	list, err = s.ec2.VPCs(nil, f)
+	c.Assert(err, IsNil)
+	c.Assert(list.VPCs, HasLen, 1)
+	assertVPC(c, list.VPCs[0], id2, resp2.VPC.CIDRBlock)
+
+	_, err = s.ec2.DeleteVPC(id1)
+	c.Assert(err, IsNil)
+	_, err = s.ec2.DeleteVPC(id2)
+	c.Assert(err, IsNil)
+}
+
+func assertVPC(c *C, obtained ec2.VPC, expectId, expectCidr string) {
+	if expectId != "" {
+		c.Check(obtained.Id, Equals, expectId)
+	} else {
+		c.Check(obtained.Id, Matches, `^vpc-[0-9a-f]+$`)
+	}
+	c.Check(obtained.State, Matches, "("+ec2.AvailableState+"|"+ec2.PendingState+")")
+	if expectCidr != "" {
+		c.Check(obtained.CIDRBlock, Equals, expectCidr)
+	} else {
+		c.Check(obtained.CIDRBlock, Matches, `^\d+\.\d+\.\d+\.\d+/\d+$`)
+	}
+	c.Check(obtained.DHCPOptionsId, Matches, `^dopt-[0-9a-f]+$`)
+	c.Check(obtained.IsDefault, Equals, false)
+	c.Check(obtained.Tags, HasLen, 0)
+	c.Check(
+		obtained.InstanceTenancy,
+		Matches,
+		"("+ec2.DefaultTenancy+"|"+ec2.DedicatedTenancy+")",
+	)
+}