Add caas firewaller #8193

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Owner

wallyworld commented Dec 8, 2017

Description of change

Add caas firewaller worker and facades.
Broken up into a few commits.

QA steps

deploy caas charm
juju expose
-> check ingress
juju unexpose
-> check ingress

mostly looks good, just a couple of things

+ args := entities(appTag)
+
+ var results params.BoolResults
+ if err := c.facade.FacadeCall("GetExposed", args, &results); err != nil {
@axw

axw Dec 11, 2017

Member

s/GetExposed/IsExposed/? or ApplicationExposed? Get is non-idiomatic

@wallyworld

wallyworld Dec 11, 2017

Owner

I was using the same API call name as for the IAAS firewaller for consistency. I can change here.

@axw

axw Dec 11, 2017

Member

Thanks. The IAAS firewaller is kind of an abomination.

+ return errors.Trace(err)
+ }
+
+ exposed, err := aw.applicationGetter.IsExposed(aw.application)
@axw

axw Dec 11, 2017

Member

So you're assuming that the initial state of the model reflects reality? That's not a sound assumption.

Rather than fetching the initial value here, I think a better approach would be:

var previouslyExposed bool
initial := true
for {
    select {
    ...
    case _, ok := <-appWatcher.Changes():
        ...
        exposed, err := aw.applicationGetter.IsExposed(aw.application)
        ...
        if !initial && exposed == previouslyExposed {
            continue
        }
        ...
        initial = false
        previouslyExposed = exposed
    }
}

This way, you will always call ExposeService or UnexposeService once when the firewaller starts up. That's necessary in case of partial handling of a change. This means those methods need to deal with being called twice in a row. Alternatively, you'll need to introduce a third method to check whether or not the service is already exposed.

@wallyworld

wallyworld Dec 11, 2017

Owner

Fair point. This was based on the mode of operation of the IAAS firewaller, so I think we need to consider fixing that one as well. Fixed here.

@axw

axw Dec 11, 2017

Member

The IAAS firewaller does the latter option: it calls an environ/instance method to get the currently-applied ingress rules, and then just applies the delta.

@wallyworld wallyworld referenced this pull request Dec 11, 2017

Merged

Caas firewaller #8202

Owner

wallyworld commented Dec 11, 2017

Replaced by #8202

@wallyworld wallyworld closed this Dec 11, 2017

jujubot added a commit that referenced this pull request Dec 11, 2017

Merge pull request #8202 from wallyworld/caas-firewaller
Caas firewaller

## Description of change

Supersedes #8193

Add caas firewaller worker and facades.
Broken up into a few commits.

## QA steps

deploy caas charm
juju expose
-> check ingress
juju unexpose
-> check ingress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment