Skip to content

Commit

Permalink
Remove orphan deletion. (#32)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Gerrit91
Gerrit91 authored Apr 12, 2023
1 parent 0c2ab05 commit fa2d09f
Show file tree
Hide file tree
Showing 4 changed files with 8 additions and 153 deletions.
61 changes: 0 additions & 61 deletions controllers/set/delete.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,7 @@ import (

v2 "github.com/metal-stack/firewall-controller-manager/api/v2"
"github.com/metal-stack/firewall-controller-manager/controllers"
"github.com/metal-stack/metal-go/api/client/firewall"
"github.com/metal-stack/metal-go/api/client/machine"
"github.com/metal-stack/metal-go/api/models"
"github.com/metal-stack/metal-lib/pkg/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"
)

func (c *controller) Delete(r *controllers.Ctx[*v2.FirewallSet]) error {
Expand Down Expand Up @@ -76,60 +72,3 @@ func (c *controller) deleteAfterTimeout(r *controllers.Ctx[*v2.FirewallSet], fws

return result, nil
}

// deletePhysicalOrphans checks in the backend if there are firewall entities that belong to the controller
// event though there is no corresponding firewall resource managed by this controller.
//
// such firewalls will be deleted in the backend.
func (c *controller) deletePhysicalOrphans(r *controllers.Ctx[*v2.FirewallSet]) error {
resp, err := c.c.GetMetal().Firewall().FindFirewalls(firewall.NewFindFirewallsParams().WithBody(&models.V1FirewallFindRequest{
AllocationProject: r.Target.Spec.Template.Spec.Project,
Tags: []string{c.c.GetClusterTag(), v2.FirewallSetTag(r.Target.Name)},
}).WithContext(r.Ctx), nil)
if err != nil {
r.Log.Error(err, "unable to retrieve firewalls for orphan checking, backing off...")
return controllers.RequeueAfter(10*time.Second, "backing off")
}

if len(resp.Payload) == 0 {
return nil
}

fws := &v2.FirewallList{}
err = c.c.GetSeedClient().List(r.Ctx, fws, client.InNamespace(c.c.GetSeedNamespace()))
if err != nil {
return err
}

ownedFirewalls, _, err := controllers.GetOwnedResources(r.Ctx, c.c.GetSeedClient(), r.Target.Spec.Selector, r.Target, &v2.FirewallList{}, func(fl *v2.FirewallList) []*v2.Firewall {
return fl.GetItems()
})
if err != nil {
return fmt.Errorf("unable to get owned firewalls: %w", err)
}

existingNames := map[string]bool{}
for _, fw := range ownedFirewalls {
existingNames[fw.Name] = true
}

for _, fw := range resp.Payload {
if fw.Allocation == nil || fw.Allocation.Name == nil {
continue
}
if _, ok := existingNames[*fw.Allocation.Name]; ok {
continue
}

r.Log.Info("found physical orphan firewall, deleting", "firewall-name", *fw.Allocation.Name, "id", *fw.ID, "non-orphans", existingNames)

_, err = c.c.GetMetal().Machine().FreeMachine(machine.NewFreeMachineParams().WithID(*fw.ID), nil)
if err != nil {
return fmt.Errorf("error deleting orphaned firewall: %w", err)
}

c.recorder.Eventf(r.Target, "Normal", "Delete", "deleted orphaned firewall %s id %s", *fw.Allocation.Name, *fw.ID)
}

return nil
}
2 changes: 1 addition & 1 deletion controllers/set/reconcile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (c *controller) Reconcile(r *controllers.Ctx[*v2.FirewallSet]) error {
return err
}

return c.deletePhysicalOrphans(r)
return nil
}

func pop[E any](slice []E) (E, []E) {
Expand Down
10 changes: 1 addition & 9 deletions controllers/set/suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,10 @@ import (
"github.com/metal-stack/firewall-controller-manager/controllers"
"github.com/metal-stack/firewall-controller-manager/controllers/firewall"
"github.com/metal-stack/firewall-controller-manager/controllers/set"
metalfirewall "github.com/metal-stack/metal-go/api/client/firewall"
"github.com/metal-stack/metal-go/api/models"
metalclient "github.com/metal-stack/metal-go/test/client"
"github.com/metal-stack/metal-lib/pkg/tag"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/stretchr/testify/mock"

"k8s.io/client-go/kubernetes/scheme"
ctrl "sigs.k8s.io/controller-runtime"
Expand Down Expand Up @@ -90,12 +87,7 @@ var _ = BeforeSuite(func() {
})
Expect(err).ToNot(HaveOccurred())

_, metalClient := metalclient.NewMetalMockClient(testingT, &metalclient.MetalMockFns{
Firewall: func(m *mock.Mock) {
// muting the orphan controller
m.On("FindFirewalls", mock.Anything, mock.Anything).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}, nil)
},
})
_, metalClient := metalclient.NewMetalMockClient(testingT, &metalclient.MetalMockFns{})

cc, err := controllerconfig.New(&controllerconfig.NewControllerConfig{
SeedClient: k8sClient,
Expand Down
88 changes: 6 additions & 82 deletions integration/integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (

testcommon "github.com/metal-stack/firewall-controller-manager/integration/common"

"github.com/metal-stack/metal-go/api/client/firewall"
metalfirewall "github.com/metal-stack/metal-go/api/client/firewall"
"github.com/metal-stack/metal-go/api/client/machine"
"github.com/metal-stack/metal-go/api/client/network"
Expand Down Expand Up @@ -159,20 +158,7 @@ var _ = Context("integration test", Ordered, func() {
swapMetalClient(&metalclient.MetalMockFns{
Firewall: func(m *mock.Mock) {
m.On("AllocateFirewall", mock.Anything, nil).Return(&metalfirewall.AllocateFirewallOK{Payload: firewall1}, nil).Maybe()

// we need to filter the orphan controller as it would delete the firewall
call := m.On("FindFirewalls", mock.Anything, mock.Anything).Maybe()
call.Run(func(args mock.Arguments) {
resp := &metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}
params, ok := args.Get(0).(*firewall.FindFirewallsParams)
if !ok {
panic(fmt.Sprintf("unexpected type: %T", args.Get(0)))
}
if params.Body.AllocationName != "" {
resp.Payload = append(resp.Payload, firewall1)
}
call.ReturnArguments = mock.Arguments{resp, nil}
})
m.On("FindFirewalls", mock.Anything, nil).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{firewall1}}, nil).Maybe()
},
Network: func(m *mock.Mock) {
m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe()
Expand All @@ -193,20 +179,7 @@ var _ = Context("integration test", Ordered, func() {
swapMetalClient(&metalclient.MetalMockFns{
Firewall: func(m *mock.Mock) {
m.On("AllocateFirewall", mock.Anything, nil).Return(&metalfirewall.AllocateFirewallOK{Payload: firewall1}, nil).Maybe()

// we need to filter the orphan controller as it would delete the firewall
call := m.On("FindFirewalls", mock.Anything, mock.Anything).Maybe()
call.Run(func(args mock.Arguments) {
resp := &metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}
params, ok := args.Get(0).(*firewall.FindFirewallsParams)
if !ok {
panic(fmt.Sprintf("unexpected type: %T", args.Get(0)))
}
if params.Body.AllocationName != "" {
resp.Payload = append(resp.Payload, firewall1)
}
call.ReturnArguments = mock.Arguments{resp, nil}
})
m.On("FindFirewalls", mock.Anything, nil).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{firewall1}}, nil).Maybe()
},
Network: func(m *mock.Mock) {
m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe()
Expand Down Expand Up @@ -454,20 +427,7 @@ var _ = Context("integration test", Ordered, func() {
swapMetalClient(&metalclient.MetalMockFns{
Firewall: func(m *mock.Mock) {
m.On("AllocateFirewall", mock.Anything, nil).Return(&metalfirewall.AllocateFirewallOK{Payload: installingFirewall}, nil).Maybe()

// we need to filter the orphan controller as it would delete the firewall
call := m.On("FindFirewalls", mock.Anything, mock.Anything).Maybe()
call.Run(func(args mock.Arguments) {
resp := &metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}
params, ok := args.Get(0).(*firewall.FindFirewallsParams)
if !ok {
panic(fmt.Sprintf("unexpected type: %T", args.Get(0)))
}
if params.Body.AllocationName != "" {
resp.Payload = append(resp.Payload, installingFirewall)
}
call.ReturnArguments = mock.Arguments{resp, nil}
})
m.On("FindFirewalls", mock.Anything, nil).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{installingFirewall}}, nil).Maybe()
},
Network: func(m *mock.Mock) {
m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe()
Expand Down Expand Up @@ -732,19 +692,7 @@ var _ = Context("integration test", Ordered, func() {
m.On("UpdateMachine", mock.Anything, nil).Return(&machine.UpdateMachineOK{Payload: &models.V1MachineResponse{}}, nil).Maybe()
},
Firewall: func(m *mock.Mock) {
// we need to filter the orphan controller as it would delete the firewall
call := m.On("FindFirewalls", mock.Anything, mock.Anything).Maybe()
call.Run(func(args mock.Arguments) {
resp := &metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}
params, ok := args.Get(0).(*firewall.FindFirewallsParams)
if !ok {
panic(fmt.Sprintf("unexpected type: %T", args.Get(0)))
}
if params.Body.AllocationName != "" {
resp.Payload = append(resp.Payload, readyFirewall)
}
call.ReturnArguments = mock.Arguments{resp, nil}
})
m.On("FindFirewalls", mock.Anything, nil).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{readyFirewall}}, nil).Maybe()
},
Network: func(m *mock.Mock) {
m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe()
Expand Down Expand Up @@ -885,19 +833,7 @@ var _ = Context("migration path", Ordered, func() {
DeferCleanup(func() {
swapMetalClient(&metalclient.MetalMockFns{
Firewall: func(m *mock.Mock) {
// we need to filter the orphan controller as it would delete the firewall
call := m.On("FindFirewalls", mock.Anything, mock.Anything).Maybe()
call.Run(func(args mock.Arguments) {
resp := &metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}
params, ok := args.Get(0).(*firewall.FindFirewallsParams)
if !ok {
panic(fmt.Sprintf("unexpected type: %T", args.Get(0)))
}
if params.Body.AllocationName != "" {
resp.Payload = append(resp.Payload, firewall1)
}
call.ReturnArguments = mock.Arguments{resp, nil}
})
m.On("FindFirewalls", mock.Anything, nil).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{firewall1}}, nil).Maybe()
},
Network: func(m *mock.Mock) {
m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe()
Expand All @@ -917,19 +853,7 @@ var _ = Context("migration path", Ordered, func() {
Describe("the migration starts", Ordered, func() {
swapMetalClient(&metalclient.MetalMockFns{
Firewall: func(m *mock.Mock) {
// we need to filter the orphan controller as it would delete the firewall
call := m.On("FindFirewalls", mock.Anything, mock.Anything).Maybe()
call.Run(func(args mock.Arguments) {
resp := &metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{}}
params, ok := args.Get(0).(*firewall.FindFirewallsParams)
if !ok {
panic(fmt.Sprintf("unexpected type: %T", args.Get(0)))
}
if params.Body.AllocationName != "" {
resp.Payload = append(resp.Payload, firewall1)
}
call.ReturnArguments = mock.Arguments{resp, nil}
})
m.On("FindFirewalls", mock.Anything, nil).Return(&metalfirewall.FindFirewallsOK{Payload: []*models.V1FirewallResponse{firewall1}}, nil).Maybe()
},
Network: func(m *mock.Mock) {
m.On("FindNetwork", mock.Anything, nil).Return(&network.FindNetworkOK{Payload: network1}, nil).Maybe()
Expand Down

0 comments on commit fa2d09f

Please sign in to comment.