ec2test: Create NICs on RunInstances time

ec2test: Create NICs on RunInstances time

Improvements to the ec2test server's RunInstances:
 * When NetworkInterfaces are specified in
   RunInstances params, the interfaces get created
   and attached to the simulated instances.
 * When no network interfaces are specified, a new
   default one is created to simulate what EC2 does
   more closely.

Also, if a default VPC is set in the test server
using SetInitialAttributes(), a VPC and a subnet
will be created in the server for them.

Added live and local tests to confirm the changes.

https://code.launchpad.net/~dimitern/goamz/110-ec2test-improvements/+merge/220257

(do not edit description out of merge proposal)

[dimitern, 2014-05-20 13:32:22]

Please take a look.

[niemeyer, 2014-05-20 14:38:53]

I've provided some feedback below. I don't feel entitled to review the
EC2-specific logic, though. If someone in the team is used to the details of how
EC2 supposedly handles those requests, would be good to have an opinion.

Also, as we discussed online, the tests are quite weak for everything that is
being done in the code. I'll let you decide what's worth testing, though, as
it's fair to say that this is testing the test code. Turtles all the way.

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

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode246
ec2/ec2test/server.go:246: case "defaultForAz", "default-for-az":
Why are there two spellings here? What do the actual docs say, and what's the
real implementation like?

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode247
ec2/ec2test/server.go:247: val, err := strconv.ParseBool(value)
I doubt the boolean parsing logic for EC2 is the same as for Go.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode403
ec2/ec2test/server.go:403: // If default-vpc are being set, create them.
s/are being set/were provided/

Also, it's good practice to move the comment within the if block, so the reader
doesn't have to read and parse the "if" conditional twice.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode404
ec2/ec2test/server.go:404: // Also create a subnet for each one.
It's easier to say "create the VPC and its subnet".

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode405
ec2/ec2test/server.go:405: if attrName == "default-vpc" {
What about the alternative spelling? One of these is probably not sane.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode553
ec2/ec2test/server.go:553: func (srv *Server) parseRunNetworkInterfaces(req
*http.Request) (ifaces []ec2.RunNetworkInterface, limitToOneInstance bool) {
Both of the results are undocumented. The first one is somewhat implicit, but
the second one is not.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode565
ec2/ec2test/server.go:565: fieldName := fields[2]
fields[2], fields[3] and fields[4] here and below are unchecked in terms of
len(fields). This will panic, probably in a cryptic way to the consumer of the
API. Why it's not a big deal, we might be slightly more forgiving and informing
considering this is a test server oriented towards application tests.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode589
ec2/ec2test/server.go:589: val, err := strconv.ParseBool(vals[0])
Again, what's the parsing rule for booleans on EC2, and what's the rule on Go?
Do they match? I believe they don't.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode596
ec2/ec2test/server.go:596: Address: vals[0], IsPrimary: true,
Either we're breaking the append function call, or we're breaking the struct
fields, and both of these have different indentation conventions than what is
used here. Please separate both fields in their own lines, or put the whole
struct definition in a single line.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode601
ec2/ec2test/server.go:601: ipIndex := atoi(fields[3])
len(fields) is unchecked per comment above.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode611
ec2/ec2test/server.go:611: val, err := strconv.ParseBool(vals[0])
One more of these.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode634
ec2/ec2test/server.go:634: defaultVPCId := "none"
Isn't an empty string a better marker here?

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode656
ec2/ec2test/server.go:656: // We assume the default subnet has a valid CIDR.
Why do we assume that? The comment is one line, a nice panic pointing out the
problem is just three.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode659
ec2/ec2test/server.go:659: if !ipnet.Contains(ip) {
What's going on here? Why is it hardcoding a .5 IP like that, and why does it
assume it's always valid within the CIDR? Also, why does it handle a situation
that should never happen with a sane result?

At the very least, it's missing some good comments explaining all these
assumptions.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode682
ec2/ec2test/server.go:682: macAddress := fmt.Sprintf("%02d:81:60:cb:27:37",
srv.ifaceId)
Shouldn't that be hex?

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode686
ec2/ec2test/server.go:686: macAddress = fmt.Sprintf("%02d:81:60:cb:27:37",
srv.ifaceId)
Same.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode695
ec2/ec2test/server.go:695: primaryIP = ifaceToCreate.PrivateIPs[0].Address
Isn't there a field named IsPrimary? Why is this considering the first one to be
the primary regardless? I've not seen the convention documented or clearly
indicated in the related code above. Feels quite error prone.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode1445
ec2/ec2test/server.go:1445: availIPs, _ := srv.calcSubnetAvailIPs(cidrBlock)
It's a very bad practice to infer in which situations a function that returns an
error is failing. Either check the error, or handle it appropriately. Think
about what happens when the next developer comes about and hacks that method to
return another potential error.

[dimitern, 2014-05-20 15:41:43]

Please take a look.

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

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode246
ec2/ec2test/server.go:246: case "defaultForAz", "default-for-az":
On 2014/05/20 14:38:53, niemeyer wrote:
> Why are there two spellings here? What do the actual docs say, and what's the
> real implementation like?

From AWS API:
http://docs.aws.amazon.com/AWSEC2/latest/APIReference/ApiReference-query-Desc...

defaultForAz

Indicates whether this is the default subnet for the Availability Zone.

You can also use default-for-az as the filter name.

Type: Boolean

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode247
ec2/ec2test/server.go:247: val, err := strconv.ParseBool(value)
This follows the same logic as for other booleans coming from AWS API.
Unfortunately, I couldn't find a clear API doc describing what's considered a
boolean value by AWS. The closest thing I could find is:
http://docs.aws.amazon.com/redshift/latest/dg/r_Boolean_type.html

Which, although is not directly related to AWS API can shed some light on that
matter, and it matches nicely with strconv.ParseBool:
http://golang.org/pkg/strconv/#ParseBool

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode403
ec2/ec2test/server.go:403: // If default-vpc are being set, create them.
On 2014/05/20 14:38:52, niemeyer wrote:
> s/are being set/were provided/
> 
> Also, it's good practice to move the comment within the if block, so the
reader
> doesn't have to read and parse the "if" conditional twice.

Done.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode404
ec2/ec2test/server.go:404: // Also create a subnet for each one.
On 2014/05/20 14:38:53, niemeyer wrote:
> It's easier to say "create the VPC and its subnet".

Done.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode405
ec2/ec2test/server.go:405: if attrName == "default-vpc" {
On 2014/05/20 14:38:52, niemeyer wrote:
> What about the alternative spelling? One of these is probably not sane.

What alternative spelling? This is the only supported attribute name, along with
"supported-platforms", described in AWS API.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode553
ec2/ec2test/server.go:553: func (srv *Server) parseRunNetworkInterfaces(req
*http.Request) (ifaces []ec2.RunNetworkInterface, limitToOneInstance bool) {
On 2014/05/20 14:38:52, niemeyer wrote:
> Both of the results are undocumented. The first one is somewhat implicit, but
> the second one is not.

Updated the doc comment to include that.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode565
ec2/ec2test/server.go:565: fieldName := fields[2]
On 2014/05/20 14:38:52, niemeyer wrote:
> fields[2], fields[3] and fields[4] here and below are unchecked in terms of
> len(fields). This will panic, probably in a cryptic way to the consumer of the
> API. Why it's not a big deal, we might be slightly more forgiving and
informing
> considering this is a test server oriented towards application tests.

Good point, I'll add some more checks.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode589
ec2/ec2test/server.go:589: val, err := strconv.ParseBool(vals[0])
On 2014/05/20 14:38:52, niemeyer wrote:
> Again, what's the parsing rule for booleans on EC2, and what's the rule on Go?
> Do they match? I believe they don't.

As mentioned earlier, if you can point me to an official AWS API documentation
that describes what's considered a boolean value, please do. If you believe
there are substantial differencies, that's good to know and I'll fix it.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode596
ec2/ec2test/server.go:596: Address: vals[0], IsPrimary: true,
On 2014/05/20 14:38:52, niemeyer wrote:
> Either we're breaking the append function call, or we're breaking the struct
> fields, and both of these have different indentation conventions than what is
> used here. Please separate both fields in their own lines, or put the whole
> struct definition in a single line.

Struct literal extracted before the append call.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode601
ec2/ec2test/server.go:601: ipIndex := atoi(fields[3])
On 2014/05/20 14:38:52, niemeyer wrote:
> len(fields) is unchecked per comment above.

Done.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode611
ec2/ec2test/server.go:611: val, err := strconv.ParseBool(vals[0])
On 2014/05/20 14:38:52, niemeyer wrote:
> One more of these.

See previous answers.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode634
ec2/ec2test/server.go:634: defaultVPCId := "none"
On 2014/05/20 14:38:53, niemeyer wrote:
> Isn't an empty string a better marker here?

Done.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode656
ec2/ec2test/server.go:656: // We assume the default subnet has a valid CIDR.
On 2014/05/20 14:38:52, niemeyer wrote:
> Why do we assume that? The comment is one line, a nice panic pointing out the
> problem is just three.

Good point! Added a panic.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode659
ec2/ec2test/server.go:659: if !ipnet.Contains(ip) {
On 2014/05/20 14:38:53, niemeyer wrote:
> What's going on here? Why is it hardcoding a .5 IP like that, and why does it
> assume it's always valid within the CIDR? Also, why does it handle a situation
> that should never happen with a sane result?
> 
> At the very least, it's missing some good comments explaining all these
> assumptions.

OK, so I guess more comments and/or some restructuring is needed to make it
obvious: Pick an IP ending with .5 (any one will do - it's used for testing
anyway, it has to be valid, but not necessarily unique) and make sure it's in
the subnet's CIDR. If not - and perhaps it should panic here (as above with the
invalid CIDR), but instead it skips the NIC creation and returns no error.

I'll improve the comments and panic as needed.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode682
ec2/ec2test/server.go:682: macAddress := fmt.Sprintf("%02d:81:60:cb:27:37",
srv.ifaceId)
On 2014/05/20 14:38:52, niemeyer wrote:
> Shouldn't that be hex?

Done.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode686
ec2/ec2test/server.go:686: macAddress = fmt.Sprintf("%02d:81:60:cb:27:37",
srv.ifaceId)
On 2014/05/20 14:38:52, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode695
ec2/ec2test/server.go:695: primaryIP = ifaceToCreate.PrivateIPs[0].Address
On 2014/05/20 14:38:52, niemeyer wrote:
> Isn't there a field named IsPrimary? Why is this considering the first one to
be
> the primary regardless? I've not seen the convention documented or clearly
> indicated in the related code above. Feels quite error prone.

Due to just how AWS works in practice - when a default NIC gets created on
RunInstances time and there is only 1 private IP, it's considered the primary
one and not included in the list of PrivateIPs slice (it is in fact nil). If on
the other hand, you specify 1 or 2 explicit PrivateIPs when creating a NIC in
RunInstance.NetworkInterfaces, AWS leaves that as you set it.

It's not clearly documented in the API, but it's more of a lesson learned during
live-testing the code.

I'll add some more comments.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode1445
ec2/ec2test/server.go:1445: availIPs, _ := srv.calcSubnetAvailIPs(cidrBlock)
On 2014/05/20 14:38:52, niemeyer wrote:
> It's a very bad practice to infer in which situations a function that returns
an
> error is failing. Either check the error, or handle it appropriately. Think
> about what happens when the next developer comes about and hacks that method
to
> return another potential error.

Good point, done.

[fwereade, 2014-05-21 09:56:00]

As far as I can judge, this looks good, but I'd really like someone who knows
ec2 and/or the testing server *well* to take a look.

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

https://codereview.appspot.com/98430044/diff/20001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:688: return ifaces, limitToOneInstance
I feel like this method could be profitably broken down into 3 (or maybe more).

[dimitern, 2014-05-21 13:54:21]

Please take a look.

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

https://codereview.appspot.com/98430044/diff/20001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:688: return ifaces, limitToOneInstance
On 2014/05/21 09:56:00, fwereade wrote:
> I feel like this method could be profitably broken down into 3 (or maybe
more).

I refactored it a bit and managed to simplify some of the code, also improved
the tests.

[niemeyer, 2014-05-21 15:25:36]

Thanks for the fixes. A few follow ups.

Also, I'd still strongly recommend the review of someone comfortable with the
behavior of the EC2 networking API to review this logic.

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

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode247
ec2/ec2test/server.go:247: val, err := strconv.ParseBool(value)
Yeah, it almost matches, which is perhaps good enough if there's no other
documentation for EC2 proper. I'd not trust RedShift's docs for that, as it's
one of the most recent services, but I cannot find anything for EC2 itself
either. We might use trial and error, but it feels like more work than it's
worth it in this case.

https://codereview.appspot.com/98430044/diff/1/ec2/ec2test/server.go#newcode405
ec2/ec2test/server.go:405: if attrName == "default-vpc" {
Sorry, I mixed it up with default-for-az.

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

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:405: // subnet.
Suggest some rephrasing of the comment. We talked about this live.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:734: // specified there.
This does not explain why the logic is internally inconsistent. There's a case
above that handles PrivateIpAddresses and takes into account whether there's a
Primary setting or not, and this logic completely disregards and dumbly picks
the first one out pretending it's the primary. There's no reason for this in the
comment, in the code, or in the half-an-hour conversation we've had online:

    http://paste.ubuntu.com/7497776/

NOT LGTM without this being fixed.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:1500: fatalf(400, "InvalidParameterValue", "bad subnet
CIDR: %s", cidrBlock)
This continues to infer what the error was, so I'll repeat the previous comment:
It's a very bad practice to infer in which situations a function that returns an
error is failing. Either check the error, or handle it appropriately. Think
about what happens when the next developer comes about and hacks that method to
return another potential error.

[dimitern, 2014-05-21 15:46:48]

Please take a look.

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

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:405: // subnet.
On 2014/05/21 15:25:36, niemeyer wrote:
> Suggest some rephrasing of the comment. We talked about this live.

Done.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:734: // specified there.
On 2014/05/21 15:25:36, niemeyer wrote:
> This does not explain why the logic is internally inconsistent. There's a case
> above that handles PrivateIpAddresses and takes into account whether there's a
> Primary setting or not, and this logic completely disregards and dumbly picks
> the first one out pretending it's the primary. There's no reason for this in
the
> comment, in the code, or in the half-an-hour conversation we've had online:
> 
>     http://paste.ubuntu.com/7497776/
> 
> NOT LGTM without this being fixed.

As agreed on IRC, I'm changing the code to simply scan the PrivateIPs slice to
find the primary private IP marked as such.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:1500: fatalf(400, "InvalidParameterValue", "bad subnet
CIDR: %s", cidrBlock)
On 2014/05/21 15:25:36, niemeyer wrote:
> This continues to infer what the error was, so I'll repeat the previous
comment:
> It's a very bad practice to infer in which situations a function that returns
an
> error is failing. Either check the error, or handle it appropriately. Think
> about what happens when the next developer comes about and hacks that method
to
> return another potential error.

Sorry, I changed the message not to assume the error is known.

[vladislav.klyachin, 2014-05-21 15:59:23]

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

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:428: }
I would create several subnets here with different AZs

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:604: iface.PrivateIPs = append(iface.PrivateIPs,
privateIP)
PrivateIpAddress also limit to one instance, according to documentation

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:714: macAddress := fmt.Sprintf("%02x:81:60:cb:27:37",
srv.ifaceId)
This is often used pattern: fmt.Sprintf("%02x:81:60:cb:27:37", srv.ifaceId)
Because 2 lower bits of MAC first byte have special meaning, I would printf to
the another byte.
This is not related to functionality of test server, but may be accidentally
copy-pasted to production code.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:827: ifacesToCreate, limitToOneInstance :=
srv.parseRunNetworkInterfaces(req)
I think you should add parsing of Placement.AvailabilityZone parameter, because
the instance and all attached NICs and subnets should belong to one AZ.
I don't see AZ related logic in the code.

[hazmat, 2014-05-21 18:37:18]

two points of interest as i think the syntax delta maybe due to struct remapping
of raw values, namely auto assigned addresses on the nics, and explicitly
requesting a public addr when creating vpc instances.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode207
ec2/ec2t_test.go:207: ip2 := validIPForSubnet(c, subnet, 6)
there's also a syntax here for auto assign the private ip addresses to an
interface via  specifying SecondaryPrivateIpAddressCount when creating the
interface. its exclusive to also specifying the addresses.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode208
ec2/ec2t_test.go:208: list, err := s.ec2.RunInstances(&ec2.RunInstances{
we'll typically also be requesting public-ip-address when creating these
instances, else its possible to get instances with no public access in vpc.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode217
ec2/ec2t_test.go:217: PrivateIPs: []ec2.PrivateIP{
per raw response value s/PrivateIps/PrivateIpAddresses

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode218
ec2/ec2t_test.go:218: {Address: ip1, IsPrimary: true},
per raw response on keys would be

s/Address/PrivateIpAddress
s/isPrimary/Primary

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode235
ec2/ec2t_test.go:235: c.Check(iface.Attachment.Id, Matches, "eni-attach-.+")
not sure if this being remapped by the struct but per the raw value from the
response, this should be s/Id/AttachmentId

[dimitern, 2014-05-22 05:58:42]

Please take a look.

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

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:428: }
On 2014/05/21 15:59:22, vladislav.klyachin wrote:
> I would create several subnets here with different AZs

Ok, I'll add another subnet, but several? I don't see the benefit.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:604: iface.PrivateIPs = append(iface.PrivateIPs,
privateIP)
On 2014/05/21 15:59:22, vladislav.klyachin wrote:
> PrivateIpAddress also limit to one instance, according to documentation

Good point, I set the limitToOneInstance = true here as well.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:714: macAddress := fmt.Sprintf("%02x:81:60:cb:27:37",
srv.ifaceId)
On 2014/05/21 15:59:22, vladislav.klyachin wrote:
> This is often used pattern: fmt.Sprintf("%02x:81:60:cb:27:37", srv.ifaceId)
> Because 2 lower bits of MAC first byte have special meaning, I would printf to
> the another byte.
> This is not related to functionality of test server, but may be accidentally
> copy-pasted to production code.

OK, changed to "20:%02x:60:cb:27:37", even though it's only used in tests.

https://codereview.appspot.com/98430044/diff/40001/ec2/ec2test/server.go#newc...
ec2/ec2test/server.go:827: ifacesToCreate, limitToOneInstance :=
srv.parseRunNetworkInterfaces(req)
On 2014/05/21 15:59:22, vladislav.klyachin wrote:
> I think you should add parsing of Placement.AvailabilityZone parameter,
because
> the instance and all attached NICs and subnets should belong to one AZ.
> I don't see AZ related logic in the code.

While your are correct, I'd avoid adding more complex checks until we need them
in juju.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go
File ec2/ec2t_test.go (right):

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode207
ec2/ec2t_test.go:207: ip2 := validIPForSubnet(c, subnet, 6)
On 2014/05/21 18:37:18, hazmat wrote:
> there's also a syntax here for auto assign the private ip addresses to an
> interface via  specifying SecondaryPrivateIpAddressCount when creating the
> interface. its exclusive to also specifying the addresses.

Yes, that's supported as well, although not in the ec2test server, as we don't
need it in juju for now at least.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode208
ec2/ec2t_test.go:208: list, err := s.ec2.RunInstances(&ec2.RunInstances{
On 2014/05/21 18:37:18, hazmat wrote:
> we'll typically also be requesting public-ip-address when creating these
> instances, else its possible to get instances with no public access in vpc.

But the instances in the default VPC get an automatic public IP mapped to the
primary private IP, this is the behavior we'll be using in juju.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode217
ec2/ec2t_test.go:217: PrivateIPs: []ec2.PrivateIP{
On 2014/05/21 18:37:18, hazmat wrote:
> per raw response value s/PrivateIps/PrivateIpAddresses

This is as per a previous review: 
https://codereview.appspot.com/54570048/diff/20001/ec2/networkinterfaces.go#n...

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode218
ec2/ec2t_test.go:218: {Address: ip1, IsPrimary: true},
On 2014/05/21 18:37:18, hazmat wrote:
> per raw response on keys would be
> 
> s/Address/PrivateIpAddress
> s/isPrimary/Primary
> 

It is renamed as many other goamz types' fields.

https://codereview.appspot.com/98430044/diff/60001/ec2/ec2t_test.go#newcode235
ec2/ec2t_test.go:235: c.Check(iface.Attachment.Id, Matches, "eni-attach-.+")
On 2014/05/21 18:37:18, hazmat wrote:
> not sure if this being remapped by the struct but per the raw value from the
> response, this should be s/Id/AttachmentId

It is remapped.

[jameinel, 2014-05-22 06:28:22]

I have a couple of comments, but I think this LGTM. I'd rather see it in and be
able to iterate it from there, as it seems correct even if we find there are
edge cases we are missing.

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

https://codereview.appspot.com/98430044/diff/80001/ec2/ec2t_test.go#newcode121
ec2/ec2t_test.go:121: // its default subnets. If fails if there is no default
VPC or at
It fails if there is no default VPC or if the default VPC contains no subnets.

It seems a little odd that getDefaultVPCIdAndSubnets would be in test code,
rather than being an actual production function that just has some asserts
wrapped around it for testing.

Isn't this the sort of functionality we'll want to be using?

https://codereview.appspot.com/98430044/diff/80001/ec2/ec2t_test.go#newcode198
ec2/ec2t_test.go:198: func (s *ServerTests)
TestRunInstancesVPCCreatesNICsWhenSpecified(c *C) {
Since this is a part of ServerTests it gets run against the real EC2 when run
with '-amazon', right?

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

https://codereview.appspot.com/98430044/diff/80001/ec2/ec2test/server.go#oldc...
ec2/ec2test/server.go:567: 
It seems like this function is getting pretty long, vs breaking it up into
logical chunks of work that get combined into a larger function. (Which at least
makes the context of each chunk of logic more local.)

That doesn't have to happen as part of this patch, but it is certainly something
we should try to keep up with.

[dimitern, 2014-05-22 06:39:10]

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

https://codereview.appspot.com/98430044/diff/80001/ec2/ec2t_test.go#newcode121
ec2/ec2t_test.go:121: // its default subnets. If fails if there is no default
VPC or at
On 2014/05/22 06:28:22, jameinel wrote:
> It fails if there is no default VPC or if the default VPC contains no subnets.
> 
> It seems a little odd that getDefaultVPCIdAndSubnets would be in test code,
> rather than being an actual production function that just has some asserts
> wrapped around it for testing.
> 
> Isn't this the sort of functionality we'll want to be using?

We'll be using something similar in juju when starting a machine, i.e. getting
the default VPC and subnet id for the AZ. But to make things simpler, we'll let
RunInstances to pick AZ for us and create the default NIC, which we can later
get to (from the instance details). Then, once we have the NIC id, we can
allocate additional private IPs for it to use in the containers.

https://codereview.appspot.com/98430044/diff/80001/ec2/ec2t_test.go#newcode198
ec2/ec2t_test.go:198: func (s *ServerTests)
TestRunInstancesVPCCreatesNICsWhenSpecified(c *C) {
On 2014/05/22 06:28:21, jameinel wrote:
> Since this is a part of ServerTests it gets run against the real EC2 when run
> with '-amazon', right?

Yes, that's how I was testing it: -gocheck.v -amazon -gocheck.f RunInstancesVPC.

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

https://codereview.appspot.com/98430044/diff/80001/ec2/ec2test/server.go#oldc...
ec2/ec2test/server.go:567: 
On 2014/05/22 06:28:22, jameinel wrote:
> It seems like this function is getting pretty long, vs breaking it up into
> logical chunks of work that get combined into a larger function. (Which at
least
> makes the context of each chunk of logic more local.)
> 
> That doesn't have to happen as part of this patch, but it is certainly
something
> we should try to keep up with.

I tried to extract logical chunks in helpers as much as reasonable, but some of
the code still belongs in runInstances I think.

[niemeyer, 2014-05-22 15:00:20]

LGTM, superficially. I don't feel able to rule out bugs in this logic, but
you'll probably find it out in practice while exercising the same code against
the real API.

[dimitern, 2014-05-22 15:24:17]

*** Submitted:

ec2test: Create NICs on RunInstances time

Improvements to the ec2test server's RunInstances:
 * When NetworkInterfaces are specified in
   RunInstances params, the interfaces get created
   and attached to the simulated instances.
 * When no network interfaces are specified, a new
   default one is created to simulate what EC2 does
   more closely.

Also, if a default VPC is set in the test server
using SetInitialAttributes(), a VPC and a subnet
will be created in the server for them.

Added live and local tests to confirm the changes.

R=niemeyer, fwereade, vladislav.klyachin, hazmat, jameinel
CC=
https://codereview.appspot.com/98430044