loader: Remove duplicated Loader interface

There was no pressing need for having two separate Loader interfaces. Simplify things by just keeping the datapath types version. Refactor DetachXDP() to take an interface name to avoid leaking netlink.Link in the export methods. It anyway did a link lookup again anyway, so there was no benefit to reusing the passed in Link. Signed-off-by: Jussi Maki <jussi@isovalent.com>
ovidiutirla · May 31, 2024 · 0a02d60 · 0a02d60
1 parent 7209275
commit 0a02d60
Show file tree

Hide file tree

Showing 13 changed files with 27 additions and 52 deletions.
diff --git a/cilium-dbg/cmd/post_uninstall_cleanup.go b/cilium-dbg/cmd/post_uninstall_cleanup.go
@@ -555,7 +555,7 @@ func removeXDPAttachments(links []netlink.Link) error {
 	})
 
 	for _, link := range links {
-		if err := loader.DetachXDP(link, bpf.CiliumPath(), "cil_xdp_entry"); err != nil {
+		if err := loader.DetachXDP(link.Attrs().Name, bpf.CiliumPath(), "cil_xdp_entry"); err != nil {
 			return err
 		}
 		fmt.Printf("removed cilium xdp of %s\n", link.Attrs().Name)

diff --git a/pkg/datapath/cells.go b/pkg/datapath/cells.go
@@ -26,7 +26,6 @@ import (
 	"github.com/cilium/cilium/pkg/datapath/linux/sysctl"
 	"github.com/cilium/cilium/pkg/datapath/linux/utime"
 	"github.com/cilium/cilium/pkg/datapath/loader"
-	loaderTypes "github.com/cilium/cilium/pkg/datapath/loader/types"
 	"github.com/cilium/cilium/pkg/datapath/orchestrator"
 	"github.com/cilium/cilium/pkg/datapath/prefilter"
 	"github.com/cilium/cilium/pkg/datapath/tables"
@@ -218,7 +217,7 @@ type datapathParams struct {
 
 	TunnelConfig tunnel.Config
 
-	Loader loaderTypes.Loader
+	Loader types.Loader
 
 	NodeManager nodeManager.NodeManager
 

diff --git a/pkg/datapath/fake/cells.go b/pkg/datapath/fake/cells.go
@@ -16,6 +16,7 @@ import (
 	"github.com/cilium/cilium/pkg/datapath/iptables/ipset"
 	"github.com/cilium/cilium/pkg/datapath/linux/bigtcp"
 	"github.com/cilium/cilium/pkg/datapath/linux/sysctl"
+	"github.com/cilium/cilium/pkg/datapath/loader"
 	"github.com/cilium/cilium/pkg/datapath/tables"
 	"github.com/cilium/cilium/pkg/datapath/tunnel"
 	"github.com/cilium/cilium/pkg/datapath/types"
@@ -54,6 +55,7 @@ var Cell = cell.Module(
 		func() mtu.MTU { return &fakeTypes.MTU{} },
 		func() *wg.Agent { return nil },
 		func() types.Loader { return &fakeTypes.FakeLoader{} },
+		loader.NewCompilationLock,
 		func() sysctl.Sysctl { return &Sysctl{} },
 
 		tables.NewDeviceTable,

diff --git a/pkg/datapath/fake/types/datapath.go b/pkg/datapath/fake/types/datapath.go
@@ -166,6 +166,10 @@ func (f *FakeLoader) RestoreTemplates(stateDir string) error {
 	return nil
 }
 
+func (f *FakeLoader) DetachXDP(ifaceName string, bpffsBase, progName string) error {
+	return nil
+}
+
 type FakeOrchestrator struct{}
 
 func (f *FakeOrchestrator) Reinitialize(ctx context.Context) error {

diff --git a/pkg/datapath/linux/datapath.go b/pkg/datapath/linux/datapath.go
@@ -6,7 +6,6 @@ package linux
 import (
 	"github.com/cilium/statedb"
 
-	loader "github.com/cilium/cilium/pkg/datapath/loader/types"
 	"github.com/cilium/cilium/pkg/datapath/tables"
 	datapath "github.com/cilium/cilium/pkg/datapath/types"
 	"github.com/cilium/cilium/pkg/maps/lbmap"
@@ -31,7 +30,7 @@ type linuxDatapath struct {
 	nodeIDHandler  datapath.NodeIDHandler
 	nodeNeighbors  datapath.NodeNeighbors
 	nodeAddressing datapath.NodeAddressing
-	loader         loader.Loader
+	loader         datapath.Loader
 	wgAgent        datapath.WireguardAgent
 	lbmap          datapath.LBMap
 	bwmgr          datapath.BandwidthManager
@@ -46,7 +45,7 @@ type DatapathParams struct {
 	BWManager      datapath.BandwidthManager
 	NodeAddressing datapath.NodeAddressing
 	MTU            datapath.MTUConfiguration
-	Loader         loader.Loader
+	Loader         datapath.Loader
 	NodeManager    manager.NodeManager
 	DB             *statedb.DB
 	Devices        statedb.Table[*tables.Device]

diff --git a/pkg/datapath/loader/cell.go b/pkg/datapath/loader/cell.go
@@ -7,7 +7,7 @@ import (
 	"github.com/cilium/hive/cell"
 	"github.com/spf13/pflag"
 
-	loaderTypes "github.com/cilium/cilium/pkg/datapath/loader/types"
+	datapath "github.com/cilium/cilium/pkg/datapath/types"
 )
 
 var Cell = cell.Module(
@@ -16,11 +16,11 @@ var Cell = cell.Module(
 
 	cell.Config(DefaultConfig),
 	cell.Provide(NewLoader),
-	cell.Provide(newCompilationLock),
+	cell.Provide(NewCompilationLock),
 )
 
 // NewLoader returns a new loader.
-func NewLoader(p Params) loaderTypes.Loader {
+func NewLoader(p Params) datapath.Loader {
 	return newLoader(p)
 }
 

diff --git a/pkg/datapath/loader/compile.go b/pkg/datapath/loader/compile.go
@@ -394,6 +394,6 @@ type compilationLock struct {
 	lock.RWMutex
 }
 
-func newCompilationLock() types.CompilationLock {
+func NewCompilationLock() types.CompilationLock {
 	return &compilationLock{}
 }
diff --git a/pkg/datapath/loader/types/types.go b/pkg/datapath/loader/types/types.go
diff --git a/pkg/datapath/loader/xdp.go b/pkg/datapath/loader/xdp.go
@@ -86,7 +86,7 @@ func (l *loader) maybeUnloadObsoleteXDPPrograms(xdpDevs []string, xdpMode, bpffs
 			}
 		}
 		if !used {
-			if err := l.DetachXDP(link, bpffsBase, symbolFromHostNetdevXDP); err != nil {
+			if err := l.DetachXDP(link.Attrs().Name, bpffsBase, symbolFromHostNetdevXDP); err != nil {
 				log.WithError(err).Warn("Failed to detach obsolete XDP program")
 			}
 		}
@@ -276,9 +276,14 @@ func attachXDPProgram(iface netlink.Link, prog *ebpf.Program, progName, bpffsDir
 //
 // bpffsBase is typically /sys/fs/bpf/cilium, but can be overridden to a tempdir
 // during tests.
-func (l *loader) DetachXDP(iface netlink.Link, bpffsBase, progName string) error {
+func (l *loader) DetachXDP(ifaceName string, bpffsBase, progName string) error {
+	iface, err := netlink.LinkByName(ifaceName)
+	if err != nil {
+		return fmt.Errorf("getting link '%s' by name: %w", ifaceName, err)
+	}
+
 	pin := filepath.Join(bpffsDeviceLinksDir(bpffsBase, iface), progName)
-	err := bpf.UnpinLink(pin)
+	err = bpf.UnpinLink(pin)
 	if err == nil {
 		return nil
 	}
@@ -287,12 +292,6 @@ func (l *loader) DetachXDP(iface netlink.Link, bpffsBase, progName string) error
 		return fmt.Errorf("unpinning XDP program using bpf_link: %w", err)
 	}
 
-	// Refresh the link info since the caller may have constructed iface manually.
-	iface, err = netlink.LinkByName(iface.Attrs().Name)
-	if err != nil {
-		return fmt.Errorf("getting link '%s' by name: %w", iface.Attrs().Name, err)
-	}
-
 	xdp := iface.Attrs().Xdp
 	if xdp == nil || !xdp.Attached {
 		return nil

diff --git a/pkg/datapath/loader/xdp_test.go b/pkg/datapath/loader/xdp_test.go
@@ -183,7 +183,7 @@ func TestAttachXDPWithExistingLink(t *testing.T) {
 		require.NoError(t, err)
 
 		// Detach the program.
-		err = newTestLoader(t).DetachXDP(veth, basePath, "test")
+		err = newTestLoader(t).DetachXDP(veth.Attrs().Name, basePath, "test")
 		require.NoError(t, err)
 
 		err = netlink.LinkDel(veth)
@@ -214,11 +214,11 @@ func TestDetachXDPWithPreviousAttach(t *testing.T) {
 		require.True(t, getLink(t, veth).Attrs().Xdp.Attached)
 
 		// Detach with the wrong name, leaving the program attached.
-		err = newTestLoader(t).DetachXDP(veth, basePath, "foo")
+		err = newTestLoader(t).DetachXDP(veth.Attrs().Name, basePath, "foo")
 		require.NoError(t, err)
 		require.True(t, getLink(t, veth).Attrs().Xdp.Attached)
 
-		err = newTestLoader(t).DetachXDP(veth, basePath, "test")
+		err = newTestLoader(t).DetachXDP(veth.Attrs().Name, basePath, "test")
 		require.NoError(t, err)
 		require.False(t, getLink(t, veth).Attrs().Xdp.Attached)
 

diff --git a/pkg/datapath/orchestrator/orchestrator.go b/pkg/datapath/orchestrator/orchestrator.go
@@ -9,8 +9,8 @@ import (
 	"github.com/cilium/hive/cell"
 
 	"github.com/cilium/cilium/pkg/datapath/iptables"
-	"github.com/cilium/cilium/pkg/datapath/loader/types"
 	"github.com/cilium/cilium/pkg/datapath/tunnel"
+	"github.com/cilium/cilium/pkg/datapath/types"
 	"github.com/cilium/cilium/pkg/mtu"
 	"github.com/cilium/cilium/pkg/proxy"
 )

diff --git a/pkg/datapath/types/loader.go b/pkg/datapath/types/loader.go
@@ -24,6 +24,7 @@ type Loader interface {
 	Reinitialize(ctx context.Context, tunnelConfig tunnel.Config, deviceMTU int, iptMgr IptablesManager, p Proxy) error
 	HostDatapathInitialized() <-chan struct{}
 	RestoreTemplates(stateDir string) error
+	DetachXDP(iface string, bpffsBase, progName string) error
 }
 
 // PreFilter an interface for an XDP pre-filter.

diff --git a/test/controlplane/suite/agent.go b/test/controlplane/suite/agent.go
@@ -18,7 +18,6 @@ import (
 	fakecni "github.com/cilium/cilium/daemon/cmd/cni/fake"
 	fakeDatapath "github.com/cilium/cilium/pkg/datapath/fake"
 	fakeTypes "github.com/cilium/cilium/pkg/datapath/fake/types"
-	"github.com/cilium/cilium/pkg/datapath/loader"
 	"github.com/cilium/cilium/pkg/datapath/prefilter"
 	datapathTables "github.com/cilium/cilium/pkg/datapath/tables"
 	fqdnproxy "github.com/cilium/cilium/pkg/fqdn/proxy"
@@ -78,7 +77,6 @@ func (h *agentHandle) setupCiliumAgentHive(clientset k8sClient.Clientset, extraC
 		),
 		fakeDatapath.Cell,
 		prefilter.Cell,
-		loader.Cell,
 		monitorAgent.Cell,
 		metrics.Cell,
 		store.Cell,