Skip to content

Commit

Permalink
bgp: move config mode struct to separate package
Browse files Browse the repository at this point in the history 
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 <hpabla@isovalent.com>
  • Loading branch information
@harsimran-pabla @julianwiedmann
harsimran-pabla authored and julianwiedmann committed May 15, 2024
1 parent 7a930fa commit 373f32e
Show file tree
Hide file tree
Showing 4 changed files with 79 additions and 39 deletions.
32 changes: 12 additions & 20 deletions pkg/bgpv1/agent/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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]
Expand All @@ -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,
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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
}

Expand All @@ -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 {
Expand All @@ -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
}

Expand All @@ -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) {
Expand Down
42 changes: 24 additions & 18 deletions pkg/bgpv1/agent/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -701,7 +704,7 @@ func TestBGPModeSelection(t *testing.T) {
},
policy: nil,
bgpNodeConfig: nil,
expectedMode: agent.Disabled,
expectedMode: mode.Disabled,
},
}

Expand All @@ -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,
Expand All @@ -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())
})
}
}
41 changes: 41 additions & 0 deletions pkg/bgpv1/agent/mode/mode.go
View file
Original file line number Diff line number Diff line change
@@ -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
}
3 changes: 2 additions & 1 deletion pkg/bgpv1/cell.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down

0 comments on commit 373f32e

Please sign in to comment.