Check firewall rules when updating cmr network ingress #7839

Merged
merged 1 commit into from Sep 8, 2017

Conversation

Projects
None yet
3 participants
Owner

wallyworld commented Sep 8, 2017

Description of change

The firewaller facade will now check whether the requested ingress satisfies any defined firewall rules in the offering model. If there's an issue, the consuming side will set an error status on the relation. The ingress check will fail if:

  • any of the requested ingress subnets are inside the blacklist subnets
  • all of the requested ingress subnets are not inside the whitelist subnets

QA steps

Deploy mysql, make offer
$ juju set-firewall-rule juju-application-offer --whitelist 1.2.3.4/32

On consuming model, deploy mediawiki and attempt to relate
$ juju relate mediawiki:db somemodel.mysql

Check that status shows the mediawiki-mysql relation is in error with a firewall message

Owner

wallyworld commented Sep 8, 2017

!!build!!

axw approved these changes Sep 8, 2017

+ if err != nil && !errors.IsNotFound(err) {
+ return errors.Trace(err)
+ }
+ if err == nil {
@axw

axw Sep 8, 2017

Member

return nil if not found, drop the indentation

+
+// firstLastAddresses returns the first and last addresses of the subnet.
+func firstLastAddresses(subnet *net.IPNet) (net.IP, net.IP) {
+ firstIP := subnet.IP
@axw

axw Sep 8, 2017

Member

simpler (set all of the trailing bits):

first, last := subnet.IP, subnet.IP
for i, b := range last {
        last[i] = b ^ (^subnet.Mask[i])
}
+
+ // If the requested ingress is not permitted on the offering side,
+ // mark the relation as in error. It's not an error that requires a
+ // worker restart though.
@axw

axw Sep 8, 2017

Member

say the offerer adjusts their firewall rules, how do you fix this?

@wallyworld

wallyworld Sep 8, 2017

Owner

To be thrashed out, but I'm thinking the relation would be marked as in error and the consumer would need to add-relation again with adjusted ingress.

+ // mark the relation as in error. It's not an error that requires a
+ // worker restart though.
+ if params.IsCodeForbidden(err) {
+ return fw.firewallerApi.SetRelationStatus(relData.tag.Id(), relation.Error, err.Error())
@axw

axw Sep 8, 2017

Member

it makes me a little uneasy having the firewaller set status on the relation, but I'm not sure what the alternative is. what else sets the status? how do they coordinate?

@wallyworld

wallyworld Sep 8, 2017

Owner

Ostensibly, the other way a relation status can be set is if the user suspends the relation, but in that case, this firewall logic wouldn't be applied. Ingress is only requested when the relation is joined (again). So it hangs together as things stand. Juju sets status to joined, error, or broken. A user can set to suspended (but only if current status is joined, although that's not checked yet). There's a few loose ends to tidy up once we get feedback on this MVP.

Owner

wallyworld commented Sep 8, 2017

$$merge$$

Contributor

jujubot commented Sep 8, 2017

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

@jujubot jujubot merged commit 40489e2 into juju:develop Sep 8, 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