From 68626446364a223241c98372b5f0f9d410ad03c3 Mon Sep 17 00:00:00 2001 From: Jack Shaw Date: Fri, 21 Jun 2024 11:38:18 +0100 Subject: [PATCH 1/3] fix(openstack): fix model sec group misconfiguration When a machine fails to provision, the openstack provider attempted to delete all security groups attached to that instance. Including any model-scoped security groups (and potentially the openstack default sec group!) This can lead to errors. The firewaller configrues the model sec group only when: 1) Once when the first machine is provioned 2) when a change to ssh-allow is detected This means if the first machine you attempt to provision fails, the model sec group will be deleted after it's configured. So it won't recievd it's initial config Attempting to deleting all secgroups attached to a machine could also lead to countless other unexpected behaviours. Fix this by only deleting the machine sec group when a machine fails to provision i.e. the sec group unique to the relevant machine. This means we will no longer attempt to delete sec groups potentially attached to other instances. --- provider/openstack/firewaller.go | 11 +++++++++++ provider/openstack/local_test.go | 20 +++++++++++++------- provider/openstack/provider.go | 15 +-------------- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/provider/openstack/firewaller.go b/provider/openstack/firewaller.go index 60656a22877..956ad451b67 100644 --- a/provider/openstack/firewaller.go +++ b/provider/openstack/firewaller.go @@ -73,6 +73,13 @@ type Firewaller interface { // If the model security group doesn't exist, return a NotFound error ModelIngressRules(ctx context.ProviderCallContext) (firewall.IngressRules, error) + // DeleteMachineGroup delete's the security group specific to the provided machine. + // When in 'instance' firewall mode, each instance in a model is assigned its own + // security group, with a lifecycle matching that of the instance itself. + // In 'global' mode, all security groups are model scoped, and have lifecycles + // matching the model, so this method will remove no groups. + DeleteMachineGroup(ctx context.ProviderCallContext, machineId string) error + // DeleteAllModelGroups deletes all security groups for the // model. DeleteAllModelGroups(ctx context.ProviderCallContext) error @@ -463,6 +470,10 @@ func (c *neutronFirewaller) DeleteAllModelGroups(ctx context.ProviderCallContext return deleteSecurityGroupsMatchingName(ctx, c.deleteSecurityGroups, c.jujuGroupPrefixRegexp()) } +func (c *neutronFirewaller) DeleteMachineGroup(ctx context.ProviderCallContext, machineID string) error { + return deleteSecurityGroupsMatchingName(ctx, c.deleteSecurityGroups, c.machineGroupRegexp(machineID)) +} + // UpdateGroupController implements Firewaller interface. func (c *neutronFirewaller) UpdateGroupController(ctx context.ProviderCallContext, controllerUUID string) error { neutronClient := c.environ.neutron() diff --git a/provider/openstack/local_test.go b/provider/openstack/local_test.go index d08a66cf3f4..d284713ccf0 100644 --- a/provider/openstack/local_test.go +++ b/provider/openstack/local_test.go @@ -849,7 +849,7 @@ func (s *localServerSuite) TestStartInstanceWaitForActiveDetails(c *gc.C) { c.Assert(insts, gc.HasLen, 0, gc.Commentf("expected launched instance to be terminated if stuck in BUILD state")) } -func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnInstanceCreateFailure(c *gc.C) { +func (s *localServerSuite) TestStartInstanceDeletesMachineSecurityGroupOnInstanceCreateFailure(c *gc.C) { env := s.openEnviron(c, coretesting.Attrs{"firewall-mode": config.FwInstance}) // Force an error in waitForActiveServerDetails @@ -864,7 +864,10 @@ func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnInstanceCreat c.Check(inst, gc.IsNil) c.Assert(err, gc.NotNil) - assertSecurityGroups(c, env, []string{"default"}) + assertSecurityGroups(c, env, []string{ + fmt.Sprintf("juju-%s-%s", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()), + "default", + }) } func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnFailure(c *gc.C) { @@ -881,7 +884,10 @@ func (s *localServerSuite) TestStartInstanceDeletesSecurityGroupsOnFailure(c *gc _, _, _, err := testing.StartInstance(env, s.callCtx, s.ControllerUUID, "100") c.Assert(err, gc.NotNil) - assertSecurityGroups(c, env, []string{"default"}) + assertSecurityGroups(c, env, []string{ + fmt.Sprintf("juju-%s-%s", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()), + "default", + }) } func assertSecurityGroups(c *gc.C, env environs.Environ, expected []string) { @@ -3062,8 +3068,8 @@ func (s *localServerSuite) TestUpdateGroupController(c *gc.C) { c.Assert(err, jc.ErrorIsNil) groupNamesBefore := set.NewStrings(groupNames...) c.Assert(groupNamesBefore, gc.DeepEquals, set.NewStrings( - "juju-deadbeef-1bad-500d-9000-4b1d0d06f00d-deadbeef-0bad-400d-8000-4b1d0d06f00d", - "juju-deadbeef-1bad-500d-9000-4b1d0d06f00d-deadbeef-0bad-400d-8000-4b1d0d06f00d-0", + fmt.Sprintf("juju-%s-%s", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()), + fmt.Sprintf("juju-%s-%s-0", coretesting.ControllerTag.Id(), coretesting.ModelTag.Id()), )) firewaller := openstack.GetFirewaller(s.env) @@ -3074,8 +3080,8 @@ func (s *localServerSuite) TestUpdateGroupController(c *gc.C) { c.Assert(err, jc.ErrorIsNil) groupNamesAfter := set.NewStrings(groupNames...) c.Assert(groupNamesAfter, gc.DeepEquals, set.NewStrings( - "juju-aabbccdd-eeee-ffff-0000-0123456789ab-deadbeef-0bad-400d-8000-4b1d0d06f00d", - "juju-aabbccdd-eeee-ffff-0000-0123456789ab-deadbeef-0bad-400d-8000-4b1d0d06f00d-0", + fmt.Sprintf("juju-aabbccdd-eeee-ffff-0000-0123456789ab-%s", coretesting.ModelTag.Id()), + fmt.Sprintf("juju-aabbccdd-eeee-ffff-0000-0123456789ab-%s-0", coretesting.ModelTag.Id()), )) } diff --git a/provider/openstack/provider.go b/provider/openstack/provider.go index 169a2005e9b..cb6fb136692 100644 --- a/provider/openstack/provider.go +++ b/provider/openstack/provider.go @@ -1273,7 +1273,7 @@ func (e *Environ) startInstance( server, err := tryStartNovaInstance(shortAttempt, e.nova(), opts) if err != nil || server == nil { // Attempt to clean up any security groups we created. - if err := e.cleanupGroups(ctx, e.nova(), novaGroupNames); err != nil { + if err := e.firewaller.DeleteMachineGroup(ctx, args.InstanceConfig.MachineId); err != nil { // If we failed to clean up the security groups, we need the user // to manually clean them up. logger.Errorf("cannot cleanup security groups: %v", err) @@ -1351,19 +1351,6 @@ func (e *Environ) startInstance( }, nil } -// Clean up any groups that we have created if we fail to start the instance. -func (e *Environ) cleanupGroups( - ctx context.ProviderCallContext, - client *nova.Client, - groups []nova.SecurityGroupName, -) error { - names := make([]string, len(groups)) - for i, group := range groups { - names[i] = group.Name - } - return e.firewaller.DeleteGroups(ctx, names...) -} - func (e *Environ) userFriendlyInvalidNetworkError(err error) error { msg := fmt.Sprintf("%s\n\t%s\n\t%s", err.Error(), "This error was caused by juju attempting to create an OpenStack instance with no network defined.", From e4b46adb5d6cf5bd2aa04c80af763d6905915724 Mon Sep 17 00:00:00 2001 From: Alastair Flynn Date: Mon, 1 Jul 2024 09:50:25 +0200 Subject: [PATCH 2/3] chore: update PR template with info about conventional commits --- PULL_REQUEST_TEMPLATE.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/PULL_REQUEST_TEMPLATE.md b/PULL_REQUEST_TEMPLATE.md index ea38f35d808..d61bb3a9e7e 100644 --- a/PULL_REQUEST_TEMPLATE.md +++ b/PULL_REQUEST_TEMPLATE.md @@ -1,3 +1,10 @@ + + ## Checklist From 11cb1ae2e3daa49b027f9f9b88c4ef32a9c6da39 Mon Sep 17 00:00:00 2001 From: Jordan Barrett <90195985+barrettj12@users.noreply.github.com.> Date: Thu, 4 Jul 2024 15:26:51 +1200 Subject: [PATCH 3/3] chore(ci): update GitHub actions dependencies - actions/checkout: v3 -> v4 - actions/setup-go: v4 -> v5 - dorny/paths-filter: v2 -> v3 --- .github/workflows/build.yml | 2 +- .github/workflows/client-tests.yml | 2 +- .github/workflows/docs.yml | 2 +- .github/workflows/gen.yml | 2 +- .github/workflows/license.yml | 2 +- .github/workflows/merge.yml | 2 +- .github/workflows/microk8s-tests.yml | 2 +- .github/workflows/migrate.yml | 2 +- .github/workflows/smoke.yml | 2 +- .github/workflows/snap.yml | 2 +- .github/workflows/static-analysis.yml | 12 ++++++------ .github/workflows/terraform-smoke.yml | 4 ++-- .github/workflows/upgrade.yml | 4 ++-- 13 files changed, 20 insertions(+), 20 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c3256cf3e87..a8863d5a295 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -35,7 +35,7 @@ jobs: steps: - name: "Checkout" - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: "Set up Go" uses: actions/setup-go@v5 diff --git a/.github/workflows/client-tests.yml b/.github/workflows/client-tests.yml index 76b82349b85..6ffdedc4736 100644 --- a/.github/workflows/client-tests.yml +++ b/.github/workflows/client-tests.yml @@ -29,7 +29,7 @@ jobs: steps: - name: "Checkout" - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: "Set up Go" uses: actions/setup-go@v5 diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 64ca1588f49..e5392f7188b 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -17,7 +17,7 @@ jobs: steps: - name: Checkout code - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Go uses: actions/setup-go@v5 diff --git a/.github/workflows/gen.yml b/.github/workflows/gen.yml index 8007eb16e24..77ab76adfcf 100644 --- a/.github/workflows/gen.yml +++ b/.github/workflows/gen.yml @@ -23,7 +23,7 @@ jobs: steps: - name: "Checkout" - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: "Set up Go" uses: actions/setup-go@v5 diff --git a/.github/workflows/license.yml b/.github/workflows/license.yml index 663b90dd3f6..278176d369e 100644 --- a/.github/workflows/license.yml +++ b/.github/workflows/license.yml @@ -20,7 +20,7 @@ jobs: if: github.event.pull_request.draft == false steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Go uses: actions/setup-go@v5 with: diff --git a/.github/workflows/merge.yml b/.github/workflows/merge.yml index 67ba1cdc353..62add5219a1 100644 --- a/.github/workflows/merge.yml +++ b/.github/workflows/merge.yml @@ -25,7 +25,7 @@ jobs: echo "target=$TARGET_BRANCH" >> "$GITHUB_OUTPUT" - name: Checkout repository - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: fetch-depth: 0 ref: ${{ steps.branch.outputs.source }} diff --git a/.github/workflows/microk8s-tests.yml b/.github/workflows/microk8s-tests.yml index b236cd5b1ac..b926c114038 100644 --- a/.github/workflows/microk8s-tests.yml +++ b/.github/workflows/microk8s-tests.yml @@ -23,7 +23,7 @@ jobs: steps: - name: Checking out repo - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Go uses: actions/setup-go@v5 diff --git a/.github/workflows/migrate.yml b/.github/workflows/migrate.yml index ab582fef6bb..5a9b838a243 100644 --- a/.github/workflows/migrate.yml +++ b/.github/workflows/migrate.yml @@ -33,7 +33,7 @@ jobs: steps: - name: Checkout code - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Go uses: actions/setup-go@v5 diff --git a/.github/workflows/smoke.yml b/.github/workflows/smoke.yml index 5bdabeb49c3..8ad2f0395f4 100644 --- a/.github/workflows/smoke.yml +++ b/.github/workflows/smoke.yml @@ -29,7 +29,7 @@ jobs: sudo DEBIAN_FRONTEND=noninteractive apt install -y expect - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup LXD if: matrix.cloud == 'localhost' diff --git a/.github/workflows/snap.yml b/.github/workflows/snap.yml index bf5ca4a2058..0091a6cc041 100644 --- a/.github/workflows/snap.yml +++ b/.github/workflows/snap.yml @@ -33,7 +33,7 @@ jobs: echo "/snap/bin" >> $GITHUB_PATH - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup LXD uses: canonical/setup-lxd@4e959f8e0d9c5feb27d44c5e4d9a330a782edee0 diff --git a/.github/workflows/static-analysis.yml b/.github/workflows/static-analysis.yml index c8577715aa3..17fdc25799e 100644 --- a/.github/workflows/static-analysis.yml +++ b/.github/workflows/static-analysis.yml @@ -18,10 +18,10 @@ jobs: if: github.event.pull_request.draft == false steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Determine which tests to run - uses: dorny/paths-filter@v2 + uses: dorny/paths-filter@v3 id: filter with: filters: | @@ -101,10 +101,10 @@ jobs: if: github.event.pull_request.draft == false steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Check if there is anything to test - uses: dorny/paths-filter@v2 + uses: dorny/paths-filter@v3 id: filter with: filters: | @@ -160,10 +160,10 @@ jobs: binary: ["jujud", "juju"] steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Set up Go - uses: actions/setup-go@v4 + uses: actions/setup-go@v5 with: go-version-file: 'go.mod' cache: true diff --git a/.github/workflows/terraform-smoke.yml b/.github/workflows/terraform-smoke.yml index 6576336a7d5..096292e95af 100644 --- a/.github/workflows/terraform-smoke.yml +++ b/.github/workflows/terraform-smoke.yml @@ -29,7 +29,7 @@ jobs: sudo DEBIAN_FRONTEND=noninteractive apt install -y expect - name: Checkout juju - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup LXD uses: canonical/setup-lxd@4e959f8e0d9c5feb27d44c5e4d9a330a782edee0 @@ -61,7 +61,7 @@ jobs: juju version - name: Find terraform provider for juju latest release - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: repository: 'juju/terraform-provider-juju' #path: terraform-provider-juju diff --git a/.github/workflows/upgrade.yml b/.github/workflows/upgrade.yml index c6b0f6d721a..403ca4a8d9b 100644 --- a/.github/workflows/upgrade.yml +++ b/.github/workflows/upgrade.yml @@ -29,7 +29,7 @@ jobs: channel: ${{ steps.check.outputs.channel }} steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Check id: check run: | @@ -62,7 +62,7 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v3 + uses: actions/checkout@v4 - name: Setup LXD if: matrix.cloud == 'localhost'