From 0a02d60fb467ea41ed7ad28d486dfa5afed78f33 Mon Sep 17 00:00:00 2001 From: Jussi Maki Date: Fri, 24 May 2024 14:24:23 +0200 Subject: [PATCH] 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 --- cilium-dbg/cmd/post_uninstall_cleanup.go | 2 +- pkg/datapath/cells.go | 3 +-- pkg/datapath/fake/cells.go | 2 ++ pkg/datapath/fake/types/datapath.go | 4 ++++ pkg/datapath/linux/datapath.go | 5 ++--- pkg/datapath/loader/cell.go | 6 ++--- pkg/datapath/loader/compile.go | 2 +- pkg/datapath/loader/types/types.go | 27 ----------------------- pkg/datapath/loader/xdp.go | 17 +++++++------- pkg/datapath/loader/xdp_test.go | 6 ++--- pkg/datapath/orchestrator/orchestrator.go | 2 +- pkg/datapath/types/loader.go | 1 + test/controlplane/suite/agent.go | 2 -- 13 files changed, 27 insertions(+), 52 deletions(-) delete mode 100644 pkg/datapath/loader/types/types.go diff --git a/cilium-dbg/cmd/post_uninstall_cleanup.go b/cilium-dbg/cmd/post_uninstall_cleanup.go index b18318725c85e..975787741f9be 100644 --- 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 index f6a9c1bd0b463..b2933a1c138bb 100644 --- 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 index 0c01c1f3b16f5..3ae8bcbe3e302 100644 --- 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 index 9a371bf0e4ce1..63be677ceae7c 100644 --- 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 index d7150449e5978..30ef257a1a5f8 100644 --- 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 index a5482ae632291..eb1ff5c9d91a6 100644 --- 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 index 28c7074e44a50..4c5d857c0461e 100644 --- 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 deleted file mode 100644 index 5437f14b2f687..0000000000000 --- a/pkg/datapath/loader/types/types.go +++ /dev/null @@ -1,27 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright Authors of Cilium - -package types - -import ( - "context" - - "github.com/vishvananda/netlink" - - "github.com/cilium/cilium/pkg/datapath/loader/metrics" - "github.com/cilium/cilium/pkg/datapath/tunnel" - "github.com/cilium/cilium/pkg/datapath/types" -) - -type Loader interface { - CallsMapPath(id uint16) string - CustomCallsMapPath(id uint16) string - DetachXDP(iface netlink.Link, bpffsBase, progName string) error - EndpointHash(cfg types.EndpointConfiguration) (string, error) - HostDatapathInitialized() <-chan struct{} - Reinitialize(ctx context.Context, tunnelConfig tunnel.Config, deviceMTU int, iptMgr types.IptablesManager, p types.Proxy) error - ReinitializeXDP(ctx context.Context, extraCArgs []string) error - ReloadDatapath(ctx context.Context, ep types.Endpoint, stats *metrics.SpanStat) (err error) - RestoreTemplates(stateDir string) error - Unload(ep types.Endpoint) -} diff --git a/pkg/datapath/loader/xdp.go b/pkg/datapath/loader/xdp.go index 443e89325d6f6..a262428f0b219 100644 --- 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 index 29cab00c8d103..cd3a1c88d0c90 100644 --- 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 index e0cb72762bcc0..003ca058531b9 100644 --- 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 index 69f4d026897d6..5283c8f881fe1 100644 --- 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 index ea09f92e14456..509a9e6b8f040 100644 --- 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,