/amz

Discovery of Internet Gateways, RouteTables; ec2test improvements #60

Merged
merged 15 commits into from Aug 10, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@dimitern @voidspace @bz2
@dimitern
Member

dimitern commented Aug 9, 2015

Building on the previous PR, this one improves ec2test server and adds tests for some of its features (more in a follow-up). Added support to pre-configure ec2test servers with:

  • SetInitialAttributes() got renamed to SetAccountAttributes(), and the behavior of treating "default-vpc" specially has been dropped in favor of AddDefaultVPCAndSubnets() below. In order not to break existing tests code using ec2test (if any), SetInitialAttributes() is an alias of SetAccountAttributes(), but to simulate a default VPC in ec2test such tests need to start calling AddDefaultVPCAndSubnets() instead.
  • VPCs: AddVPC(), UpdateVPC(), RemoveVPC()
  • AddDefaultVPCAndSubnets() - a convenient helper to add a default VPC, one default subnet per AZ (with MapPublicIPOnLaunch set), an Internet Gateway (IGW) connected to the VPC, a main route table for the VPC with routes for local VPC traffic and a default route to the IGW, finally calling SetAccountAttributes() to announce the default VPC and make it discoverable.
  • Subnets: AddSubnet(), UpdateSubnet(), RemoveSubnet()
  • Internet Gateways: AddInternetGateway(), UpdateInternetGateway(), RemoveInternetGateway()
  • Route Tables: AddRouteTable(), UpdateRouteTable(), RemoveRouteTable()
  • SetAvailabilityZones() now removes any subnets which have the zone in their AvailZone field.
  • Reset() method added to ec2test, which takes a withoutZonesOrGroups flag - when true everything stored in the test server is reset, including counters, but the listener is kept. A value of false will add default AZ and security groups after resetting.

In the ec2 package, added support for the DescribeInternetGateways and DescribeRouteTables AWS APIs, with more or less complete filtering support except for tags.

Finally, this PR includes a number of fixes around live tests to make them more robust and reliable.
Tested with different combinations of: go test -check.v -test.timeout=1200m -amazon -race -coverprofile=cover -covermode=atomic
ec2/ec2test/availability_zones.go
+ defer srv.mu.Unlock()
+ oldZones := srv.zones
+ srv.zones = make(map[string]availabilityZone)
+ for _, z := range zones {
@voidspace

voidspace Aug 10, 2015

What this loop does is:

iterate over all the new zones removing all subnets associated with each new zone

Did you mean to iterate over oldZones instead?

(In the end, with the current loop, I believe that the only subnets left will be ones associated with zones from oldZones but not in new zones. I think the effect you're after is exactly the opposite.)

@dimitern

dimitern Aug 10, 2015

Member

Good catch! You're correct, I'll change this.

ec2/ec2test/internet_gateways.go
+ return fmt.Errorf("internet gateway %q not found", igwId)
+}
+
+type internetGateway struct {
@voidspace

voidspace Aug 10, 2015

Why do we create a private type that exactly matches the public type - why not just use the public type?

@dimitern

dimitern Aug 10, 2015

Member

internetGateway (and similar ec2test types) embed the public type to provide access to it, but the unexported type has extra (internal) methods (e.g. matchAttr called while filtering).

ec2/ec2test/internet_gateways_test.go
+}
+
+func (s *S) TestUpdateInternetGateway(c *C) {
+ // AddDefaultVPCAndSubnets() creates a IGW as well.
@voidspace

voidspace Aug 10, 2015

"an IGW"

@voidspace

voidspace Aug 10, 2015

but you're not calling AddDefaultVPCAndSubnets are you? So either the comment is outdated or there is some code missing.

@dimitern

dimitern Aug 10, 2015

Member

I'll fix the typo. AddDefaultVPCAndSubnets is called in SetUpTest of the S suite.

@voidspace

voidspace Aug 10, 2015

Oh, I looked for a SetUp but didn't see one. I obviously didn't look hard enough! Thanks.

ec2/ec2test/route_tables_test.go
+ }},
+ PropagatingVGWIds: []string{"foo", "bar"},
+ }
+ added, err = s.srv.AddRouteTable(toAdd)
@voidspace

voidspace Aug 10, 2015

Missing an assert about err.

@dimitern

dimitern Aug 10, 2015

Member

Thanks, will fix this.

ec2/ec2test/subnets_test.go
+ subnet, err := s.srv.AddSubnet(ec2.Subnet{})
+ c.Assert(err, ErrorMatches, "empty VPCId field")
+ c.Assert(subnet, DeepEquals, ec2.Subnet{})
+ s.assertVPCsExist(c, true)
@voidspace

voidspace Aug 10, 2015

It's not clear why this assert is called here. If it's a precondition that should be true before AddSubnet and true after, then please add an additional call to assertVPCsExist before calling AddSubnet. If the call to AddSubnet changes something about VPCs, please add a corresponding assert before AddSubnet.

@dimitern

dimitern Aug 10, 2015

Member

Fair enough, will add a comment and move the assert (which just verifies AddDefaultVPCAndSubnets was called in SetUpTest) before AddSubnet.

ec2/internet_gateways_test.go
+ c.Check(igw.Tags, HasLen, 0)
+}
+
+// Internet Gateway tests that run either agsints the local test
@voidspace

voidspace Aug 10, 2015

"against" not agsints

@dimitern

dimitern Aug 10, 2015

Member

Thanks, fill fix.

@voidspace

voidspace commented Aug 10, 2015

Other than a few minor points, LGTM
ec2/ec2test/network_interfaces.go
@@ -404,6 +416,10 @@ func (srv *Server) createIFace(w http.ResponseWriter, req *http.Request, reqId s
return resp
}
+func (srv *Server) dnsNameFromPrivateIP(privateIP string) string {
+ return fmt.Sprintf("ip-%s.ec2.internal", strings.Replace(privateIP, ".", "-", -1))
@bz2

bz2 Aug 10, 2015

Member

Using .internal may actually resolve to something when running tests, whereas .invalid is reserved, see rfc 2606. The tests should not be trying to do dns lookups on these faked names, but I like using .invalid or .test even so.

@dimitern

dimitern Aug 10, 2015

Member

The only reason for this change was to make both the local-live and live tests pass consistently (AWS does not populate DNSName sometimes).

ec2/internet_gateways.go
+
+// InternetGateways describes one or more Internet Gateways (IGWs).
+// Both parameters are optional, and if specified will limit the
+// returned IGWs to the matching ids or filterring rules.
@bz2

bz2 Aug 10, 2015

Member

Tyop, 'filterring'.

@dimitern

dimitern Aug 10, 2015

Member

Fixed.

@bz2
Member

bz2 commented Aug 10, 2015

LGTM, though without going in detail over all the new test code. As mentioned on irc, have some version compat concern with the ec2test changes but I'm not sure how widely that's used.
@dimitern
Member

dimitern commented Aug 10, 2015

Thanks for the reviews!
$$merge$$

dimitern pushed a commit that referenced this pull request Aug 10, 2015

Merge pull request #60 from dimitern/describe-igw-and-routes-take-2 
Discovery of Internet Gateways, RouteTables; ec2test improvements
c48fcea

@dimitern dimitern merged commit c48fcea into go-amz:v3 Aug 10, 2015
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dimitern dimitern deleted the dimitern:describe-igw-and-routes-take-2 branch Aug 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment