Remove bastion security group when disabling the bastion

We reconcile the security groups before the bastion, because the bastion
needs its security group to be created first when managed security groups are enabled.
When the bastion is disabled, we will try to delete the security group if it exists.
In the first attempt, the security group will still be in-use by the bastion instance
but then the bastion instance will be deleted in the next reconcile loop.
We do that here because we don't want to manage the bastion security group from
elsewhere, that could cause infinite loops between ReconCileSecurityGroups and ReconcileBastion.
Therefore we try to delete the bastion security group as a best effort here
and also when the cluster is deleted so we're sure it will be deleted at some point.

Also, we're trying to remove it when the cluster is deleted in case it
wasn't done before. This doesn't trigger an error if the security group
didn't exist.

Adding e2e tests to cover the scenarios:
* Disabling the bastion should reduce the total number of managed SGs to
  2.
* Re-enabling it should make it to 3 SGs.

pkg/cloud/services/networking/securitygroups.go

@@ -56,9 +56,24 @@ func (s *Service) ReconcileSecurityGroups(openStackCluster *infrav1.OpenStackClu
 		workerSuffix:       secWorkerGroupName,
 	}
 
+	secBastionGroupName := getSecBastionGroupName(clusterResourceName)
 	if bastionEnabled {
-		secBastionGroupName := getSecBastionGroupName(clusterResourceName)
 		suffixToNameMap[bastionSuffix] = secBastionGroupName
+	} else {
+		// We reconcile the security groups before the bastion, because the bastion
+		// needs its security group to be created first when managed security groups are enabled.
+		// When the bastion is disabled, we will try to delete the security group if it exists.
+		// In the first attempt, the security group will still be in-use by the bastion instance
+		// but then the bastion instance will be deleted in the next reconcile loop.
+		// We do that here because we don't want to manage the bastion security group from
+		// elsewhere, that could cause infinite loops between ReconCileSecurityGroups and ReconcileBastion.
+		// Therefore we try to delete the bastion security group as a best effort here
+		// and also when the cluster is deleted so we're sure it will be deleted at some point.
+		// https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/2113
+		if err := s.deleteSecurityGroup(openStackCluster, secBastionGroupName); err != nil {
+			s.scope.Logger().Info("Non-fatal error when deleting the bastion security group", "name", secBastionGroupName, "error", err)
+			return nil
+		}
 	}
 
 	// create security groups first, because desired rules use group ids.
@@ -351,10 +366,10 @@ func (s *Service) DeleteSecurityGroups(openStackCluster *infrav1.OpenStackCluste
 	secGroupNames := []string{
 		getSecControlPlaneGroupName(clusterResourceName),
 		getSecWorkerGroupName(clusterResourceName),
-	}
-
-	if openStackCluster.Spec.Bastion.IsEnabled() {
-		secGroupNames = append(secGroupNames, getSecBastionGroupName(clusterResourceName))
+		// Even if the bastion might be disabled, we still try to delete the security group in case
+		// we had a bastion before and for some reason we didn't delete its security group.
+		// https://github.com/kubernetes-sigs/cluster-api-provider-openstack/issues/2113
+		getSecBastionGroupName(clusterResourceName),
 	}
 
 	for _, secGroupName := range secGroupNames {

pkg/cloud/services/networking/securitygroups_test.go

@@ -568,6 +568,7 @@ func TestService_ReconcileSecurityGroups(t *testing.T) {
 					Return([]groups.SecGroup{{ID: "0", Name: controlPlaneSGName}}, nil)
 				m.ListSecGroup(groups.ListOpts{Name: workerSGName}).
 					Return([]groups.SecGroup{{ID: "1", Name: workerSGName}}, nil)
+				m.ListSecGroup(groups.ListOpts{Name: bastionSGName}).Return(nil, nil)
 
 				// We expect a total of 12 rules to be created.
 				// Nothing actually looks at the generated

test/e2e/suites/e2e/e2e_test.go

@@ -198,6 +198,9 @@ var _ = Describe("e2e tests [PR-Blocking]", func() {
 					return false, errors.New("Bastion was not removed in OpenStackCluster.Status")
 				}, e2eCtx.E2EConfig.GetIntervals(specName, "wait-bastion")...,
 			).Should(BeTrue())
+			securityGroupsList, err = shared.DumpOpenStackSecurityGroups(e2eCtx, groups.ListOpts{Tags: clusterName})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(securityGroupsList).To(HaveLen(2))
 
 			shared.Logf("Delete the bastion")
 			openStackCluster, err = shared.ClusterForSpec(ctx, e2eCtx, namespace)
@@ -242,6 +245,9 @@ var _ = Describe("e2e tests [PR-Blocking]", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(openStackCluster.Spec.Bastion).To(Equal(openStackClusterWithNewBastionFlavor.Spec.Bastion))
 			Expect(openStackCluster.Status.Bastion).NotTo(BeNil(), "OpenStackCluster.Status.Bastion with new flavor has not been populated")
+			securityGroupsList, err = shared.DumpOpenStackSecurityGroups(e2eCtx, groups.ListOpts{Tags: clusterName})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(securityGroupsList).To(HaveLen(3))
 		})
 	})