Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add remote firewaller facade #7021
Conversation
axw
requested changes
Feb 23, 2017
A few things. Most importantly: the location/dependency on worker/api, and the name of the new facade.
| @@ -71,6 +71,7 @@ var facadeVersions = map[string]int{ | ||
| "ProxyUpdater": 1, | ||
| "Reboot": 2, | ||
| "RelationUnitsWatcher": 1, | ||
| + "RemoteFirewaller": 1, |
axw
Feb 23, 2017
Member
I'm not so sure about the name for this one. It's not so much about firewalling, as it is about networking in general is it? I mean, you connect to the remote controller to learn about its networks, so you can configure your own firewall; not to do any firewalling in the remote controller.
| +// NewRemoteFirewallerAPI creates a new server-side RemoteFirewallerAPI facade. | ||
| +func NewRemoteFirewallerAPI( | ||
| + st State, | ||
| + pool StatePool, |
wallyworld
Feb 23, 2017
Owner
No, i was using it in an earlier iteration when model tags were passed in to the WatchSubnets() API. But I think the method of creating an API facade per model is the right way to go as it's how hosted juju works (albeit using redirection in the controller proxy).
I left it there for future use but perhaps it's YAGNI
| +// Copyright 2017 Canonical Ltd. | ||
| +// Licensed under the AGPLv3, see LICENCE file for details. | ||
| + | ||
| +package worker |
axw
Feb 23, 2017
Member
I don't think this belongs here, it has nothing to do with workers. Maybe in the "juju/juju/juju" package?
I think rather than having APIOpen in the manifold configs, you should pass in a function that returns an api.Connection given a model tag.
wallyworld
Feb 23, 2017
•
Edited 1 time
-
wallyworld
Feb 23, 2017
Owner
-
Feb 23, 2017
moved to juju/juju/api
added specific func to manifold
| +// defined on the environs.Environ interface. | ||
| +type Environ interface { | ||
| + environs.Firewaller | ||
| + Instances(ids []instance.Id) ([]instance.Instance, error) |
axw
Feb 23, 2017
Member
I think it would be better for this worker to accept two things: an environs.Firewaller, and an Instanceser (ha ha - real name TBD), an each of those just-so-happen to be satisfied by the same environs.Environ.
| + | ||
| +// Config defines the operation of a Worker. | ||
| +type Config struct { | ||
| + ModelUUID string |
wallyworld
Feb 23, 2017
Owner
Not used in this PR. I expect to use it in the next one. If not, I'll remove it there.
| - remoteRelationsApi *remoterelations.Client, | ||
| -) (worker.Worker, error) { | ||
| +// NewFirewaller returns a new Firewaller. | ||
| +func NewFirewaller(cfg Config) (worker.Worker, error) { | ||
| fw := &Firewaller{ |
| @@ -270,6 +270,25 @@ func open( | ||
| return st, nil | ||
| } | ||
| +type ConnectionForModelFunc func(*Info) (func(string) (Connection, error), error) | ||
| + | ||
| +// APIConnForModelFunc provides a function which returns a model API |
axw
Feb 23, 2017
Member
also, maybe "NewConnectionForModelFunc" would be more appropriate? ConnectionForModel does not itself return a Connection.
| + "github.com/juju/juju/state" | ||
| +) | ||
| + | ||
| +// StatePool provides the subset of a state pool required by the |
| + WatchSubnets() state.StringsWatcher | ||
| +} | ||
| + | ||
| +type statePoolShim struct { |
| + *state.State | ||
| +} | ||
| + | ||
| +func (st stateShim) WatchSubnets() state.StringsWatcher { |
axw
Feb 23, 2017
Member
this is unnecessary, since state.State already has the WatchSubnets method with the required signature
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
wallyworld commentedFeb 23, 2017
•
Edited 1 time
-
wallyworld
Feb 23, 2017
Description of change
This PR adds a new remote firewaller facade. The facade will be used by the firewaller worker and provides firewall/network related functionality against a different (remote) model, where a cross model application is hosted and a relation established.
The only method currently is WatchSubnets() which notifies of changes to subnets in a remote model. The next PR will move the IngressSubnetsForRelation() method off the remote relation facade and onto this new remote firewaller facade.
The worker uses a helper method to construct a remote firewaller facade to access a given model - the remote relations worker uses the same methodology so some code to manage the underlying API connection itself was extracted to a helper. At the moment, the result is a facade pointing to a model on the same controller, but in future will be used to allow cross controller interactions transparent to the worker.
The PR includes all the components of the facade - api, apiserver, state functionality. The firewaller worker manifold was restructured to allow tests to be updated later to plug in mocks etc (as part of the work to plug in the remote firewaller facade).
QA steps
bootstrap aws
create 2 models and deploy an app to each
relate the apps across models
expose the app
check the ports