From 373f32e6b2577ee321ba94e9488725637b866dc5 Mon Sep 17 00:00:00 2001 From: harsimran pabla Date: Fri, 10 May 2024 15:34:51 +0000 Subject: [PATCH] bgp: move config mode struct to separate package Moving ConfigMode of bgp controller into its own package. It is done so ConfigMode can be used in various other sub-packages without causing circular dependency. Signed-off-by: harsimran pabla --- pkg/bgpv1/agent/controller.go | 32 +++++++++-------------- pkg/bgpv1/agent/controller_test.go | 42 +++++++++++++++++------------- pkg/bgpv1/agent/mode/mode.go | 41 +++++++++++++++++++++++++++++ pkg/bgpv1/cell.go | 3 ++- 4 files changed, 79 insertions(+), 39 deletions(-) create mode 100644 pkg/bgpv1/agent/mode/mode.go diff --git a/pkg/bgpv1/agent/controller.go b/pkg/bgpv1/agent/controller.go index 7a1126f655cc9..7b6006580952a 100644 --- a/pkg/bgpv1/agent/controller.go +++ b/pkg/bgpv1/agent/controller.go @@ -12,6 +12,7 @@ import ( "github.com/sirupsen/logrus" daemon_k8s "github.com/cilium/cilium/daemon/k8s" + "github.com/cilium/cilium/pkg/bgpv1/agent/mode" "github.com/cilium/cilium/pkg/bgpv1/agent/signaler" "github.com/cilium/cilium/pkg/bgpv1/manager/store" "github.com/cilium/cilium/pkg/hive" @@ -49,17 +50,6 @@ func (plf policyListerFunc) List() ([]*v2alpha1api.CiliumBGPPeeringPolicy, error return plf() } -type ConfigMode int - -const ( - // BGP control plane is not enabled - Disabled ConfigMode = iota - // BGPv1 mode is enabled, BGP configuration of the agent will rely on matching CiliumBGPPeeringPolicy for the node. - BGPv1 - // BGPv2 mode is enabled, BGP configuration of the agent will rely on CiliumBGPNodeConfig, CiliumBGPAdvertisement and CiliumBGPPeerConfig. - BGPv2 -) - // Controller is the agent side BGP Control Plane controller. // // Controller listens for events and drives BGP related sub-systems @@ -91,7 +81,7 @@ type Controller struct { BGPMgr BGPRouterManager // current configuration state - Mode ConfigMode + ConfigMode *mode.ConfigMode } // ControllerParams contains all parameters needed to construct a Controller @@ -103,6 +93,7 @@ type ControllerParams struct { JobGroup job.Group Shutdowner hive.Shutdowner Sig *signaler.BGPCPSignaler + ConfigMode *mode.ConfigMode RouteMgr BGPRouterManager PolicyResource resource.Resource[*v2alpha1api.CiliumBGPPeeringPolicy] BGPNodeConfigStore store.BGPCPResourceStore[*v2alpha1api.CiliumBGPNodeConfig] @@ -127,6 +118,7 @@ func NewController(params ControllerParams) (*Controller, error) { c := &Controller{ Sig: params.Sig, + ConfigMode: params.ConfigMode, BGPMgr: params.RouteMgr, PolicyResource: params.PolicyResource, BGPNodeConfigStore: params.BGPNodeConfigStore, @@ -245,15 +237,15 @@ func (c *Controller) Reconcile(ctx context.Context) error { return err } - switch c.Mode { - case Disabled: + switch c.ConfigMode.Get() { + case mode.Disabled: if bgppExists { err = c.reconcileBGPP(ctx, bgpp) } else if bgpncExists { err = c.reconcileBGPNC(ctx, bgpnc) } - case BGPv1: + case mode.BGPv1: if bgppExists { err = c.reconcileBGPP(ctx, bgpp) } else { @@ -265,7 +257,7 @@ func (c *Controller) Reconcile(ctx context.Context) error { } } - case BGPv2: + case mode.BGPv2: if bgppExists { // delete bgpv2 and apply bgpv1 c.cleanupBGPNC(ctx) @@ -294,7 +286,7 @@ func (c *Controller) reconcileBGPP(ctx context.Context, policy *v2alpha1api.Cili return fmt.Errorf("failed to configure BGP peers, cannot apply BGP peering policy: %w", err) } - c.Mode = BGPv1 + c.ConfigMode.Set(mode.BGPv1) return nil } @@ -305,7 +297,7 @@ func (c *Controller) cleanupBGPP(ctx context.Context) { log.WithError(err).Error("failed to cleanup BGP peering policy peers") } - c.Mode = Disabled + c.ConfigMode.Set(mode.Disabled) } func (c *Controller) reconcileBGPNC(ctx context.Context, bgpnc *v2alpha1api.CiliumBGPNodeConfig) error { @@ -314,7 +306,7 @@ func (c *Controller) reconcileBGPNC(ctx context.Context, bgpnc *v2alpha1api.Cili return fmt.Errorf("failed to reconcile BGPNodeConfig: %w", err) } - c.Mode = BGPv2 + c.ConfigMode.Set(mode.BGPv2) return nil } @@ -324,7 +316,7 @@ func (c *Controller) cleanupBGPNC(ctx context.Context) { log.WithError(err).Error("failed to cleanup BGPNodeConfig") } - c.Mode = Disabled + c.ConfigMode.Set(mode.Disabled) } func (c *Controller) bgppSelection() (*v2alpha1api.CiliumBGPPeeringPolicy, error) { diff --git a/pkg/bgpv1/agent/controller_test.go b/pkg/bgpv1/agent/controller_test.go index 746bea33914f2..44cb12439ad2e 100644 --- a/pkg/bgpv1/agent/controller_test.go +++ b/pkg/bgpv1/agent/controller_test.go @@ -13,6 +13,7 @@ import ( "k8s.io/utils/pointer" "github.com/cilium/cilium/pkg/bgpv1/agent" + "github.com/cilium/cilium/pkg/bgpv1/agent/mode" "github.com/cilium/cilium/pkg/bgpv1/manager/store" "github.com/cilium/cilium/pkg/bgpv1/mock" v2api "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2" @@ -200,6 +201,7 @@ func TestControllerSanity(t *testing.T) { BGPMgr: rtmgr, LocalCiliumNode: node, BGPNodeConfigStore: store.NewMockBGPCPResourceStore[*v2alpha1api.CiliumBGPNodeConfig](), + ConfigMode: mode.NewConfigMode(), } err := c.Reconcile(context.Background()) @@ -265,6 +267,7 @@ func TestDeselection(t *testing.T) { BGPMgr: rtmgr, LocalCiliumNode: node, BGPNodeConfigStore: store.NewMockBGPCPResourceStore[*v2alpha1api.CiliumBGPNodeConfig](), + ConfigMode: mode.NewConfigMode(), } // First, reconcile with the policy selected @@ -556,15 +559,15 @@ func TestPolicySelection(t *testing.T) { func TestBGPModeSelection(t *testing.T) { var table = []struct { name string - initialMode agent.ConfigMode + initialMode mode.Mode ciliumNode *v2api.CiliumNode policy *v2alpha1api.CiliumBGPPeeringPolicy bgpNodeConfig *v2alpha1api.CiliumBGPNodeConfig - expectedMode agent.ConfigMode + expectedMode mode.Mode }{ { name: "Disabled to BGPv1", - initialMode: agent.Disabled, + initialMode: mode.Disabled, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -583,11 +586,11 @@ func TestBGPModeSelection(t *testing.T) { }, }, bgpNodeConfig: nil, - expectedMode: agent.BGPv1, + expectedMode: mode.BGPv1, }, { name: "Disabled to BGPv2", - initialMode: agent.Disabled, + initialMode: mode.Disabled, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -602,11 +605,11 @@ func TestBGPModeSelection(t *testing.T) { Name: "Test Node", }, }, - expectedMode: agent.BGPv2, + expectedMode: mode.BGPv2, }, { name: "BGPv1 to BGPv2", - initialMode: agent.BGPv1, + initialMode: mode.BGPv1, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -621,11 +624,11 @@ func TestBGPModeSelection(t *testing.T) { Name: "Test Node", }, }, - expectedMode: agent.BGPv2, + expectedMode: mode.BGPv2, }, { name: "BGPv2 to BGPv1, BGPNodeConfig present", - initialMode: agent.BGPv2, + initialMode: mode.BGPv2, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -648,11 +651,11 @@ func TestBGPModeSelection(t *testing.T) { Name: "Test Node", }, }, - expectedMode: agent.BGPv1, + expectedMode: mode.BGPv1, }, { name: "BGPv2 to BGPv1, BGPNodeConfig removed", - initialMode: agent.BGPv2, + initialMode: mode.BGPv2, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -671,11 +674,11 @@ func TestBGPModeSelection(t *testing.T) { }, }, bgpNodeConfig: nil, - expectedMode: agent.BGPv1, + expectedMode: mode.BGPv1, }, { name: "BGPv1 to disabled", - initialMode: agent.BGPv1, + initialMode: mode.BGPv1, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -686,11 +689,11 @@ func TestBGPModeSelection(t *testing.T) { }, policy: nil, bgpNodeConfig: nil, - expectedMode: agent.Disabled, + expectedMode: mode.Disabled, }, { name: "BGPv2 to disabled", - initialMode: agent.BGPv2, + initialMode: mode.BGPv2, ciliumNode: &v2api.CiliumNode{ ObjectMeta: metav1.ObjectMeta{ Name: "Test Node", @@ -701,7 +704,7 @@ func TestBGPModeSelection(t *testing.T) { }, policy: nil, bgpNodeConfig: nil, - expectedMode: agent.Disabled, + expectedMode: mode.Disabled, }, } @@ -719,6 +722,9 @@ func TestBGPModeSelection(t *testing.T) { return []*v2alpha1api.CiliumBGPPeeringPolicy{tt.policy}, nil } + cm := mode.NewConfigMode() + cm.Set(tt.initialMode) + c := agent.Controller{ PolicyLister: &agent.MockCiliumBGPPeeringPolicyLister{ List_: policyLister, @@ -733,13 +739,13 @@ func TestBGPModeSelection(t *testing.T) { }, LocalCiliumNode: tt.ciliumNode, BGPNodeConfigStore: mockStore, - Mode: tt.initialMode, + ConfigMode: cm, } err := c.Reconcile(context.Background()) require.NoError(t, err) - require.Equal(t, tt.expectedMode, c.Mode) + require.Equal(t, tt.expectedMode, cm.Get()) }) } } diff --git a/pkg/bgpv1/agent/mode/mode.go b/pkg/bgpv1/agent/mode/mode.go new file mode 100644 index 0000000000000..4145d51c88d76 --- /dev/null +++ b/pkg/bgpv1/agent/mode/mode.go @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Cilium + +package mode + +import ( + "github.com/cilium/cilium/pkg/lock" +) + +// Mode defines the modes in which BGP agent can be configured. +type Mode int + +const ( + // Disabled mode, BGP control plane is not enabled + Disabled Mode = iota + // BGPv1 mode is enabled, BGP configuration of the agent will rely on matching CiliumBGPPeeringPolicy for the node. + BGPv1 + // BGPv2 mode is enabled, BGP configuration of the agent will rely on CiliumBGPNodeConfig, CiliumBGPAdvertisement and CiliumBGPPeerConfig. + BGPv2 +) + +func NewConfigMode() *ConfigMode { + return &ConfigMode{} +} + +type ConfigMode struct { + lock.RWMutex + configMode Mode +} + +func (m *ConfigMode) Get() Mode { + m.RLock() + defer m.RUnlock() + return m.configMode +} + +func (m *ConfigMode) Set(mode Mode) { + m.Lock() + defer m.Unlock() + m.configMode = mode +} diff --git a/pkg/bgpv1/cell.go b/pkg/bgpv1/cell.go index 42849ce489072..14b21458722c1 100644 --- a/pkg/bgpv1/cell.go +++ b/pkg/bgpv1/cell.go @@ -7,6 +7,7 @@ import ( "github.com/cilium/hive/cell" "github.com/cilium/cilium/pkg/bgpv1/agent" + "github.com/cilium/cilium/pkg/bgpv1/agent/mode" "github.com/cilium/cilium/pkg/bgpv1/agent/signaler" "github.com/cilium/cilium/pkg/bgpv1/api" "github.com/cilium/cilium/pkg/bgpv1/manager" @@ -35,7 +36,7 @@ var Cell = cell.Module( "BGP Control Plane", // The Controller which is the entry point of the module - cell.Provide(agent.NewController, signaler.NewBGPCPSignaler), + cell.Provide(agent.NewController, signaler.NewBGPCPSignaler, mode.NewConfigMode), cell.ProvidePrivate( // BGP Peering Policy resource provides the module with a stream of events for the BGPPeeringPolicy resource. newBGPPeeringPolicyResource,