Model firewall rules in state #7823

Merged
merged 1 commit into from Sep 5, 2017

Conversation

Projects
None yet
3 participants
Owner

wallyworld commented Sep 4, 2017

Description of change

Firewall rules are modelled to allow restrictions to be placed on incoming connections to a Juju model. The restrictions may be applied to a well known internet service like SSH, or a Juju defined service like connecting to a controller or to a remote offer. The currently supported services are:

  • ssh
  • juju controller connections
  • connections to workloads defined by juju application offers

Note that the rules are globally applied to a given service - there's no scope to have fine grained rules on a per offer basis for example; rules are applied equally to all application offers.

QA steps

No QA at this stage - just infrastructure.

haven't looked enough to +1, but looks good

state/firewallrules.go
+ ops = []txn.Op{{
+ C: firewallRulesC,
+ Id: existing.Id(),
+ Assert: txn.DocExists,
@axw

axw Sep 4, 2017

Member

check that they haven't changed, rather than just exist?

@wallyworld

wallyworld Sep 4, 2017

Owner

The thinking here is last one wins, so just checking for existence is ok I think. It's the pattern we tend to use everywhere else.

@axw

axw Sep 5, 2017

Member

fair enough

axw approved these changes Sep 5, 2017

LGTM, but I'd prefer if FirewallRule was a struct. It doesn't have behaviour, doesn't need to be mocked.

state/firewallrules.go
+// - ssh
+// - juju-controller
+// - juju-application-offer
+type FirewallRule interface {
@axw

axw Sep 5, 2017

Member

Why not just pass around struct values? I don't see any benefit to making this an interface.

@wallyworld

wallyworld Sep 5, 2017

Owner

Ok, changed to structs.

Owner

wallyworld commented Sep 5, 2017

$$merge$$

Contributor

jujubot commented Sep 5, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

@jujubot jujubot merged commit 666d1ca into juju:develop Sep 5, 2017

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment