/juju

provider/gce: implement Subnets and NetworkInterfaces #7126

Merged
merged 9 commits into from Mar 23, 2017

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@babbageclunk @jujubot @jameinel
@babbageclunk
Member

babbageclunk commented Mar 20, 2017

Description of change

Implement NetworkingEnviron for the GCE provider. No support for spaces, space discovery or container addressing, just listing subnets and network interfaces at the moment. This enables more fine-grained management of ingress rules for cross-model relations.

QA steps

Bootstrap a controller in GCE, in a project that has some subnets (this will require creating an additional network if the project is using a legacy default network). The controller should bootstrap successfully and running juju subnets should list all of the subnets in the relevant region.
@babbageclunk
Member

babbageclunk commented Mar 20, 2017

!!build!!
@babbageclunk
Member

babbageclunk commented Mar 20, 2017

!!build!!
@jameinel

jameinel requested changes Mar 20, 2017
View changes

I think the general shape is OK. I'd like to double check what we should be using as a ProviderId as the full URL doesn't quite feel right.
https://cloud.google.com/compute/docs/instances/create-start-instance#creating_an_instance_in_a_custom_subnet_network

seems to say the 'providerID' should be just the tail:
"subnetwork": "regions/[REGION]/subnetworks/[SUBNET_NAME]",

provider/gce/environ_network.go
+ googleInst := envInst.base
+ // According to GCE docs, an instance can only have one nic.
+ // https://cloud.google.com/compute/docs/instances/#instances_and_networks
+ if ifaces := len(googleInst.NetworkInterfaces()); ifaces != 1 {
@jameinel

jameinel Mar 20, 2017

Owner

Why force it to be just one? It seems like all of this can just iterate over a loop on these objects and build up a list and perhaps only at the end assert that there is 1.
Given the function calls are already returning lists, it seems silly to start failing the moment the list can be >1.

@babbageclunk

babbageclunk Mar 20, 2017

Member

My thinking was that it would be a bit of fiddly code that we don't need now - we don't want to do one Subnets call for each interface, so it would be something like:

  • collect the subnet ids
  • make the call and pull the results into a map
  • match each interface up with its subnet
  • handle and test for the cases where not all the subnets come back.

But I wasn't super comfortable with that decision - like you say, the source is a list so probably there'll be more things in there at some point. I'll expand it out.

provider/gce/environ_network.go
+}
+
+// AllocateContainerAddresses implements environs.NetworkingEnviron.
+func (e *environ) AllocateContainerAddresses(instance.Id, names.MachineTag, []network.InterfaceInfo) ([]network.InterfaceInfo, error) {
@jameinel

jameinel Mar 20, 2017

Owner

All of this is fine, with container addresses being disabled, etc.

provider/gce/environ_network.go
+ return errors.NotSupportedf("container addresses")
+}
+
+func makeSubnetInfo(subnetId network.Id, cidr string, zones []string) network.SubnetInfo {
@jameinel

jameinel Mar 20, 2017

Owner

This seems perfectly fine. You wouldn't be using SpaceProviderID or VLAN as they aren't relevant for these (and should be commented as optional).

Often I've found the Go practice of omitting fields that are 'implicitly' set to the Zero value to not be amazing, because it isn't always clear that the caller knew that they were there and explicitly wanted the zero value, vs didn't know that they were there and thus they accidentally inherited the default value.

so I'd personally be fine with adding an explicit:
VLAN: 0, // No VLANs in GCE
SpaceProviderID: "", // No Spaces defined by GCE

or something to that effect, but I wouldn't mandate it, either.

@babbageclunk

babbageclunk Mar 20, 2017

Member

Yeah, I think you're right - I've done that for default-value fields in InterfaceInfo, for example. I'll add those in.

provider/gce/environ_network.go
+ return network.SubnetInfo{
+ ProviderId: subnetId,
+ CIDR: cidr,
+ AvailabilityZones: zonesCopy,
@jameinel

jameinel Mar 20, 2017

Owner

Reading through the GCE docs, I'm pretty sure we need to start modeling "networks" sooner rather than later. Can you add a field to SubnetInfo which is NetworkProviderId and populate that field here? Existing Subnets are probably ok, because MAAS doesn't really have the concept yet of disjoint subnets. We should start populating VPC-ID for AWS subnets as well.
But given GCE can have 1 project with multiple disjoint Networks, such that you end up with overlapping Subnet ranges, we really need to start tracking network ids.

If it ends up a huge yak with many layers of tests that start breaking, you can punt, but it is pretty clearly useful to get there, and feels better to start adding them now if we can.

provider/gce/environ_network_test.go
- c.Check(s.FakeConn.Calls[0].FirewallName, gc.Equals, fwname)
- c.Check(s.FakeConn.Calls[0].Rules, jc.DeepEquals, s.Rules)
+ c.Assert(subnets, gc.DeepEquals, []network.SubnetInfo{{
+ ProviderId: "https://www.googleapis.com/compute/v1/projects/sonic-youth/regions/asia-east1/subnetworks/go-team",
@jameinel

jameinel Mar 20, 2017

Owner

Is there really no contextual shorter ID for a subnet? Is this the value that you're supposed to pass back to something like StartInstance to get an instance that is using that subnetwork?
It looks more like the ProviderID should be something like "go-team" and the rest of it is inferred, but I could be wrong.

@babbageclunk

babbageclunk Mar 20, 2017

Member

Subnetworks have an integer ID which seems like it might be the right thing to use for Provider ID, but a network interface's Subnet field is the link rather than the ID (which I guess makes sense from a REST perspective).
My thinking was that if I use the ID as the subnet ProviderId, then I won't be able to construct a SubnetInfo directly from the NetworkInterface in the instance version of Subnets - I'd need to do an extra call to get the subnets for an instance so I can get the ID.

But as I typed that it sounded wrong, and looking at the code I think it is - if I make the subnet ProviderId the integer ID everything should still work the same. I'll do that. I'd rather use the ID than trying to unpack components out of the URL.

provider/gce/google/instance.go
+// NetworkInterfaces returns the details of the network connection for
+// this instance.
+func (gi Instance) NetworkInterfaces() []*compute.NetworkInterface {
+ return gi.InstanceSummary.NetworkInterfaces
@jameinel

jameinel Mar 20, 2017

Owner

should this be something that we copy() just in case someone tries to mutate the result value?

@babbageclunk

babbageclunk Mar 20, 2017

Member

Yeah, good call - I'll change that.

babbageclunk added some commits Mar 15, 2017

@babbageclunk
Update Google API dependency to 20170224 revision 
This is needed to use the Subnetworks service. Handle
compute.MetadataItems.Value changing from string to *string, and the Do
method for various operations now taking ...googleapi.CallOption.
a044945
@babbageclunk
Add ListSubnetworks to the raw GCE connection
32cfe4b
@babbageclunk
Move environ_network to environ_firewall 
Since it's implementing the Firewaller interface (and I'm about to
implement NetworkingEnviron).
2ae8272
@babbageclunk
Implement NetworkingEnviron for the gce provider 
GCE doesn't support spaces or container addresses, so this only has
non-trivial implementations for Subnets and NetworkInterfaces methods.
f11cc3e
@babbageclunk
Review changes 
Handle multiple network interfaces on GCE instances, and use the subnet
name as the provider id (rather than the URL).
e9249fc
@babbageclunk
Record the network on subnets and interface info 
Since a GCE account can have multiple networks such that subnets in
different networks might have overlapping ranges, it's useful to record
the network they belong to.
df60a69
@babbageclunk
Member

babbageclunk commented Mar 22, 2017

!!build!!
@babbageclunk
Member

babbageclunk commented Mar 22, 2017

!!chittychitty!!
@babbageclunk
Member

babbageclunk commented Mar 22, 2017

!!build!!
@jameinel

jameinel approved these changes Mar 22, 2017
View changes

This feels like its good enough that we can use it for now, and evolve it in the future if we find it needs tweaks.
Ideally we'd make sure that we could provision an instance with the Subnet information that we're storing (can we recreate the URL needed to pass into the instance from the information we are recording).

provider/gce/environ_network.go
+ CIDR: subnet.CIDR,
+ // The network interface has no id in GCE so it's
+ // identified by the machine's id + its position.
+ ProviderId: network.Id(fmt.Sprintf("%s/%d", instId, i)),
@jameinel

jameinel Mar 22, 2017

Owner

https://cloud.google.com/compute/docs/reference/latest/instances says that network interfaces have a "Name" field. Is that better than index? (if you are ever able to add/remove them it feels slightly better)

@babbageclunk

babbageclunk Mar 22, 2017

Member

D'oh, yes, that would be a better thing to use here. Updating it now.

provider/gce/environ_network.go
+ if err != nil {
+ return nil, errors.Trace(err)
+ }
+ // We don't want to get all subnets if no urls are passed.
@jameinel

jameinel Mar 22, 2017

Owner

You have this comment, but don't seem to be doing:
if len(urls) == 0 {
return nil, nil
}

@babbageclunk

babbageclunk Mar 22, 2017

Member

It was meant to explain why I'm not calling something that could return an includeAny, but you're right, doing an early return here makes more sense.

provider/gce/environ_network.go
+ subnet.IpCidrRange,
+ zones,
+ )
+ }
@jameinel

jameinel Mar 22, 2017

Owner

Should we do some sort of checking if we got the same SelfLink 2 times? I guess urlSet doesn't let us determine whether it not in the set because we saw it, or whether it is not in the set because we never asked for it.

@babbageclunk

babbageclunk Mar 22, 2017

Member

Do you mean a sanity check to make sure GCE aren't breaking their own rules? I could see that being useful if an interface was marked with the wrong CIDR because two subnets had the same URL. But I'm not sure it's worth restructuring the code to check for. What about a sanity check that the ip address is contained in the CIDR higher up?

provider/gce/environ_network_test.go
jc "github.com/juju/testing/checkers"
+ // "github.com/juju/version"
@jameinel

jameinel Mar 22, 2017

Owner

we certainly shouldn't leave in a commented out import

@babbageclunk

babbageclunk Mar 22, 2017

Member

No we should not!

@babbageclunk
Member

babbageclunk commented Mar 22, 2017

!!build!!
@babbageclunk
Member

babbageclunk commented Mar 22, 2017

At the moment the GCE provider doesn't support subnet placement - I'm not going to add that in this PR, but I'll add a card to the board to do it. I'm a bit wary about constructing the URL directly from the project + region + subnet-name though - it means we'll break if they change their format. Rather than constructing the URL we'd find the subnet by name and get the URL from it, maybe?
@babbageclunk
Member

babbageclunk commented Mar 22, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Mar 22, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 22, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10527
@babbageclunk
Member

babbageclunk commented Mar 22, 2017

Seems like a spurious lxd failure, retrying.

$$merge$$
@jujubot
Contributor

jujubot commented Mar 22, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju
@jujubot
Contributor

jujubot commented Mar 23, 2017

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/10530
@babbageclunk
Member

babbageclunk commented Mar 23, 2017

Seems like another different intermittent failure (StateSuite.TestNoModelDocs).

$$retry$$
@jujubot
Contributor

jujubot commented Mar 23, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 565ce69 into juju:develop Mar 23, 2017
1 check failed

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@babbageclunk babbageclunk deleted the babbageclunk:subnets-gce branch Mar 23, 2017

@jameinel
Owner

jameinel commented Mar 23, 2017

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