/gomaasapi

Expose the StaticRoutes API #63

Merged
merged 4 commits into from Feb 27, 2017

Conversation

Reviewers

@reedobrien reedobrien

@howbazaar howbazaar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@jameinel @jujubot @reedobrien @howbazaar
@jameinel
Owner

jameinel commented Feb 23, 2017 •
Edited 1 time

This implements support for listing StaticRoutes.
https://docs.ubuntu.com/maas/2.0/en/api#static-route

The only thing it supports is reading all of them. It doesn't support creating them, or reading just a single one of them. However, that's all we concretely need for Juju's use case. (MaaS doesn't let you look up static routes by source/destination subnets, etc. So we always have to read all of them and then find the ones we care about from there.)

This is necessary to implement Static Route support for containers in Juju:
https://bugs.launchpad.net/juju/+bug/1653708

jameinel added some commits Feb 23, 2017

@jameinel
Start working on StaticRoutes. 
Start by implementing Static Route handler in the test service, in
preparation for updating the actual code base to interact with the
test service.
bdc9408
@jameinel
Finish wiring up all of the static route objects. 
Controller.StaticRoutes() now works, and we have some
direct tests of parsing the values from a canned MAAS response,
and a test service that can also be populated with routes.
92b7ec2
@jameinel
go fmt
51e6b37
@reedobrien

reedobrien approved these changes Feb 23, 2017
View changes

Seems a lot of type checking in code rather than letting the compiler do it. Mostly nits and questions, but seems good to me.

I am not too familiar with maas api and it is >500 line change so you should get another +1.

Also there's no QA steps to verify.

interfaces.go
+ // inside Source should use GatewayIP to reach Destination addresses.)
+ Source() Subnet
+ // Destination is the subnet that a machine wants to send packets to. We
+ // want to configure a route to that subnet via GatewayIP
@reedobrien

reedobrien Feb 23, 2017

full stop?

@jameinel

jameinel Feb 23, 2017

Owner

👍

interfaces.go
+ // Destination is the subnet that a machine wants to send packets to. We
+ // want to configure a route to that subnet via GatewayIP
+ Destination() Subnet
+ // GatewayIP is the IPAddress of the
@reedobrien

reedobrien Feb 23, 2017

of the ...

@jameinel

jameinel Feb 23, 2017

Owner

👍

interfaces.go
+ // want to configure a route to that subnet via GatewayIP
+ Destination() Subnet
+ // GatewayIP is the IPAddress of the
+ GatewayIP() string
@reedobrien

reedobrien Feb 23, 2017

why not a net.IPAddr? (after reading through I think I get why we arent' using net.IPAddr, net.IPNet, and friends.) Alas...

interfaces.go
+ // Metric is the routing metric that determines whether this route will
+ // take precedence over similar routes (there may be a route for 10/8, but
+ // also a more concrete route for 10.0/16 that should take precedence if it
+ // applies.) Metric should be a non-negative integer.
@reedobrien

reedobrien Feb 23, 2017

why isn't it a uint if it should not be negative?

@jameinel

jameinel Feb 23, 2017

Owner

IMO, 2 reasons

  1. config.Schema doesn't support Uint
  2. It allows you to have an "invalid" value inline. I don't know if that will actually be used, but "ID" is also actually a uint on the MAAS side, but is always presented as an 'int', so I followed suit.
staticroute.go
+)
+
+type staticRoute struct {
+ // Add the controller in when we need to do things with the staticRoute.
@reedobrien

reedobrien Feb 23, 2017

TODO?

@jameinel

jameinel Feb 23, 2017

Owner

Copied from Spaces, I can just remove it.

staticroute.go
+ checker := schema.List(schema.StringMap(schema.Any()))
+ coerced, err := checker.Coerce(source, nil)
+ if err != nil {
+ return nil, errors.Annotatef(err, "static-route base schema check failed")
@reedobrien

reedobrien Feb 23, 2017

Is there a non base schema check? Why not just a schema check failure? Or just make source a string map and let the compiler type check... I am sure there's reasons... I just don't know the history.

@jameinel

jameinel Feb 23, 2017

Owner

This is checking that we have a list of objects (eg [{}, {}, {}], which is supposed to be true regardless of what version of the API we are talking to.

I think this was a deliberate choice from the people implementing the maas parser that you break apart parsing a list of objects into a top level 'parse it into a list' and then intermediate 'parse it into version-specific objects'.

staticroute.go
+ if err != nil {
+ return nil, errors.Annotatef(err, "static-route base schema check failed")
+ }
+ valid := coerced.([]interface{})
@reedobrien

reedobrien Feb 23, 2017

So we convert []interface to a stringmap and then convert it back to []interface? If source is valid, why not just use it?

@jameinel

jameinel Feb 23, 2017

Owner

Coerce returns an interface{}, what we have is a list of interfaces []interface{}. We don't have a StringMap, because that's not how Golang works.

@reedobrien

reedobrien Feb 23, 2017

I see. My brain ignored the [] in the type assertion.

staticroute.go
+ return readStaticRouteList(valid, readFunc)
+}
+
+// readStaticRouteList expects the values of the sourceList to be string maps.
@reedobrien

reedobrien Feb 23, 2017

Again wondering why sourceList isn't a a slice of string maps (or an interface that has a StringMaps method which returns one). I'm sure for historical reasons.

staticroute.go
+ }
+ valid := coerced.(map[string]interface{})
+ // From here we know that the map returned from the schema coercion
+ // contains fields of the right type.
@reedobrien

reedobrien Feb 23, 2017

Smells like typechecking:)

@jameinel

jameinel Feb 23, 2017

Owner

so we have a "map[string]interface{}", but we know the type of all the contents that the schema has defined (valid["resource_uri"] must be a 'string' because it was defined in the schema, and Coerce would have given an error if it was the wrong type.)

testservice_staticroutes.go
+ err = json.NewEncoder(w).Encode(server.staticRoutes[ID])
+ }
+ checkError(err)
+ case "POST":
@reedobrien

reedobrien Feb 23, 2017

These imply there's something to test for PUT and POST. If there isn't they should prolly just be handled by default -- or be a TODO.

@jameinel

jameinel Feb 23, 2017

Owner

I was copying this, but I went ahead and put in at least http.StatusNotImplemented.

testservice_staticroutes.go
+ return postedStaticRoute
+}
+
+// NewStaticRoute creates a Static Route in the test server
@reedobrien

reedobrien Feb 23, 2017

Full stop

@jameinel

jameinel Feb 23, 2017

Owner

👍

@jameinel

jameinel reviewed Feb 23, 2017
View changes

For the overall comments-

We are converting from a JSON string into a typed object. I believe we are choosing to do so via schema.Coerce instead of json.Unmarshal because Unmarshal is very loose in its interpretation of your content. I believe that schema.Coerce enforces that fields that are flagged as 'must be there' will cause an error if it isn't there, rather than silently using the zero value, etc.

At the very least, I wouldn't change the mechanism for deserializing json strings for one type vs using what all the other types did.

interfaces.go
+ // inside Source should use GatewayIP to reach Destination addresses.)
+ Source() Subnet
+ // Destination is the subnet that a machine wants to send packets to. We
+ // want to configure a route to that subnet via GatewayIP
@reedobrien

reedobrien Feb 23, 2017

full stop?

@jameinel

jameinel Feb 23, 2017

Owner

👍

interfaces.go
+ // Destination is the subnet that a machine wants to send packets to. We
+ // want to configure a route to that subnet via GatewayIP
+ Destination() Subnet
+ // GatewayIP is the IPAddress of the
@reedobrien

reedobrien Feb 23, 2017

of the ...

@jameinel

jameinel Feb 23, 2017

Owner

👍

interfaces.go
+ // Metric is the routing metric that determines whether this route will
+ // take precedence over similar routes (there may be a route for 10/8, but
+ // also a more concrete route for 10.0/16 that should take precedence if it
+ // applies.) Metric should be a non-negative integer.
@reedobrien

reedobrien Feb 23, 2017

why isn't it a uint if it should not be negative?

@jameinel

jameinel Feb 23, 2017

Owner

IMO, 2 reasons

  1. config.Schema doesn't support Uint
  2. It allows you to have an "invalid" value inline. I don't know if that will actually be used, but "ID" is also actually a uint on the MAAS side, but is always presented as an 'int', so I followed suit.
staticroute.go
+)
+
+type staticRoute struct {
+ // Add the controller in when we need to do things with the staticRoute.
@reedobrien

reedobrien Feb 23, 2017

TODO?

@jameinel

jameinel Feb 23, 2017

Owner

Copied from Spaces, I can just remove it.

staticroute.go
+ checker := schema.List(schema.StringMap(schema.Any()))
+ coerced, err := checker.Coerce(source, nil)
+ if err != nil {
+ return nil, errors.Annotatef(err, "static-route base schema check failed")
@reedobrien

reedobrien Feb 23, 2017

Is there a non base schema check? Why not just a schema check failure? Or just make source a string map and let the compiler type check... I am sure there's reasons... I just don't know the history.

@jameinel

jameinel Feb 23, 2017

Owner

This is checking that we have a list of objects (eg [{}, {}, {}], which is supposed to be true regardless of what version of the API we are talking to.

I think this was a deliberate choice from the people implementing the maas parser that you break apart parsing a list of objects into a top level 'parse it into a list' and then intermediate 'parse it into version-specific objects'.

staticroute.go
+ if err != nil {
+ return nil, errors.Annotatef(err, "static-route base schema check failed")
+ }
+ valid := coerced.([]interface{})
@reedobrien

reedobrien Feb 23, 2017

So we convert []interface to a stringmap and then convert it back to []interface? If source is valid, why not just use it?

@jameinel

jameinel Feb 23, 2017

Owner

Coerce returns an interface{}, what we have is a list of interfaces []interface{}. We don't have a StringMap, because that's not how Golang works.

@reedobrien

reedobrien Feb 23, 2017

I see. My brain ignored the [] in the type assertion.

staticroute.go
+ }
+ valid := coerced.(map[string]interface{})
+ // From here we know that the map returned from the schema coercion
+ // contains fields of the right type.
@reedobrien

reedobrien Feb 23, 2017

Smells like typechecking:)

@jameinel

jameinel Feb 23, 2017

Owner

so we have a "map[string]interface{}", but we know the type of all the contents that the schema has defined (valid["resource_uri"] must be a 'string' because it was defined in the schema, and Coerce would have given an error if it was the wrong type.)

testservice_staticroutes.go
+ err = json.NewEncoder(w).Encode(server.staticRoutes[ID])
+ }
+ checkError(err)
+ case "POST":
@reedobrien

reedobrien Feb 23, 2017

These imply there's something to test for PUT and POST. If there isn't they should prolly just be handled by default -- or be a TODO.

@jameinel

jameinel Feb 23, 2017

Owner

I was copying this, but I went ahead and put in at least http.StatusNotImplemented.

testservice_staticroutes.go
+ return postedStaticRoute
+}
+
+// NewStaticRoute creates a Static Route in the test server
@reedobrien

reedobrien Feb 23, 2017

Full stop

@jameinel

jameinel Feb 23, 2017

Owner

👍

@jameinel jameinel referenced this pull request in juju/juju Feb 24, 2017

Merged

2.1.1 static routes for containers #7034

staticroute.go
+ "source": schema.StringMap(schema.Any()),
+ "destination": schema.StringMap(schema.Any()),
+ "gateway_ip": schema.String(),
+ "metric": schema.ForceInt(),
@howbazaar

howbazaar Feb 27, 2017

Owner

There is a ForceUint now.

@jameinel
Owner

jameinel commented Feb 27, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Feb 27, 2017

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

@jujubot jujubot merged commit cfbc096 into juju:master Feb 27, 2017

jujubot added a commit to juju/juju that referenced this pull request Feb 27, 2017

@jujubot
Merge pull request #7034 from jameinel/2.1.1-static-routes-1653708 
2.1.1 static routes for containers

## Description of change

This change allows Juju to propagate Static Route information that MAAS declares into containers. It depends on this patch to gomaasapi: juju/gomaasapi#63. (I updated dependencies.tsv for my version of the code, but we should update dependencies to match the final merge.) The other changes to dependencies.tsv are just reordering because of how the file was generated.

We use the Static Routes API https://docs.ubuntu.com/maas/2.0/en/api#static-route to find what routes are needed, and apply them based on their Source subnet.
We add a post-up and a pre-down rule that uses 'ip route add'. This differs slightly from MAAS and Curtin that use "route add", but it means we can use CIDR instead of netmask.
http://bazaar.launchpad.net/~curtin-dev/curtin/trunk/view/head:/curtin/net/__init__.py#L380

## QA steps

Using MAAS create a static route for one of your subnets (you can do so via the web ui by looking at the source subnet). You'll need to create destination subnets for it to route to. Afterwards, deploy a node, and you should see that "ip route show" lists that static route. You should then be able to deploy a container onto that machine using the same space. It should have the same route. Without this patch (stock juju 2.1.0) it should not.

## Documentation changes

Potentially we should document static routes with containers, but it feels a bit like people using static routes just expect it to work.

## Bug reference

[lp:1653708](https://bugs.launchpad.net/juju/+bug/1653708)
5a819de
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment