Fix AdoptResources for nova networking

[babbageclunk]

Description of change

legacyNovaFirewaller.UpdateGroupController wasn't filtering the groups
to those that matched its regex, so it was failing in the replace
step. This would prevent migrations from finishing on Canonistack or any
other Openstack using nova networking - the model would be running fine
on the target controller (and the machine and unit agents would be talking to
the model on the target), but would not be removed from the source.

QA steps

• Bootstrap 2 controllers A and B on an Openstack using Nova networking.
• Add a model m to A.
• Migrate it to B.
• The model is removed from A.

Bug reference

Fixes https://bugs.launchpad.net/juju/+bug/1717860

[jameinel]

can we mock up tests that we do the right thing?

[babbageclunk]

The test ended up being kind of bonkers, but... yes, this now tests that legacyFirewaller handles unknown groups correctly.

[jameinel]

Clearly someone incorporated logic about Juju into goose, which shouldn't be there. "goose" is supposed to just be about bringing Openstack ideas into Go.
However, you didn't make that mistake and I think your filtering is correct.
A bit of a comment to help understand what, specifically, the test is testing for.
Also, did you make sure the test fails without your patch?

[jameinel]

One thing that this clearly indicates to me, is that we messed up our layering, but it isn't your fault.
goose should never have a constant with 'juju' in the name.

[babbageclunk]

Oh, it's not called out (I'll add a comment) but those juju- groups are added by the bootstrapEnv call - nothing in goose is creating them.

[jameinel]

so is the idea that without the regex we used to fail to UpgradeGroup, or that we were modifying the old groups somehow? (I'm guessing the former).

maybe a comment in the test to clarify the specific reasons for this test.
// ensure that when Juju updates the security groups when we don't have
// Neutron networking, that we don't get confused by security groups
// that are not part of this model.

I also wonder if it would be useful to mix in Juju-* security groups which aren't the model we are updating. To make sure we don't touch those either (eg, the rule isn't just juju-.*)

[babbageclunk]

The former, yes - I'll add the comment. Also, other juju- groups for a different model is a good call, I'll add those too.

[babbageclunk]

Thanks for the thoughtful review (as always)! Yes, I checked that the test fails in the expected way if the filtering is removed.

[babbageclunk]

$$merge$$

[jujubot]

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