Skip to content

Commit

Permalink
bgpv1: Add support for setting BGP timer params in CiliumBGPNeighbor CRD
Browse files Browse the repository at this point in the history 
Extends the CiliumBGPNeighbor CRD with 3 new configuration options:
ConnectRetryTime, HoldTime and KeepAliveTime. These can be used to
fine-tune BGP peering, e.g. to achieve faster failover times.
If not set, the default values for the affected timers remain
the same as before this change.

This also introduces a new UpdateNeighbor API for bgpv1,
to support changes of an existing peering. During the update
we first dump existing peer configuration from GoBGP and then
perform the update on the dumped value. The reason for that is that
many peer config fields are defaulted internally in GoBGP and would
cause peer reset if not provided on update.

Timer values are included in the state API of the BgpPeer. Since the
applied values of HoldTime and KeepAliveTime may be different
from the configured values (they also depend on negotiation during
the session setup), the state API differentiates between
"configured" and "applied" values of these timer intervals.

Signed-off-by: Rastislav Szabo <rastislav.szabo@isovalent.com>
  • Loading branch information
@rastislavs
rastislavs committed May 24, 2023
1 parent 847e8ad commit b3debfe
Show file tree
Hide file tree
Showing 15 changed files with 713 additions and 107 deletions.
23 changes: 23 additions & 0 deletions api/v1/models/bgp_peer.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

23 changes: 23 additions & 0 deletions api/v1/openapi.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3506,6 +3506,29 @@ definitions:
type: array
items:
"$ref": "#/definitions/BgpPeerFamilies"
connect-retry-time-seconds:
description: Initial value for the BGP ConnectRetryTimer (RFC 4271, Section 8) in seconds
type: integer
configured-hold-time-seconds:
description: |
Configured initial value for the BGP HoldTimer (RFC 4271, Section 4.2) in seconds.
The configured value will be used for negotiation with the peer during the BGP session establishment.
type: integer
applied-hold-time-seconds:
description: |
Applied initial value for the BGP HoldTimer (RFC 4271, Section 4.2) in seconds.
The applied value holds the value that is in effect on the current BGP session.
type: integer
configured-keep-alive-time-seconds:
description: |
Configured initial value for the BGP KeepaliveTimer (RFC 4271, Section 8) in seconds.
The applied value may be different than the configured value, as it depends on the negotiated hold time interval.
type: integer
applied-keep-alive-time-seconds:
description: |
Applied initial value for the BGP KeepaliveTimer (RFC 4271, Section 8) in seconds.
The applied value holds the value that is in effect on the current BGP session.
type: integer
BgpPeerFamilies:
description: |-
BGP AFI SAFI state of the peer
Expand Down
40 changes: 40 additions & 0 deletions api/v1/server/embedded_spec.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

57 changes: 57 additions & 0 deletions pkg/bgpv1/agent/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (

"github.com/cilium/workerpool"

"github.com/cilium/cilium/pkg/bgpv1/types"
"github.com/cilium/cilium/pkg/hive"
"github.com/cilium/cilium/pkg/hive/cell"
"github.com/cilium/cilium/pkg/ip"
Expand All @@ -26,6 +27,9 @@ import (
"github.com/cilium/cilium/pkg/option"
)

// minHoldTime represents the minimal BGP hold time duration
const minHoldTime = 3 * time.Second

var (
log = logging.DefaultLogger.WithField(logfields.LogSubsys, "bgp-control-plane")
)
Expand Down Expand Up @@ -368,6 +372,14 @@ func (c *Controller) Reconcile(ctx context.Context) error {
return nil
}

// apply policy defaults to have consistent default config across sub-systems
policy = c.applyPolicyDefaults(policy)

err = c.validatePolicy(policy)
if err != nil {
return fmt.Errorf("invalid BGP peering policy %s: %w", policy.Name, err)
}

// parse any virtual router specific attributes defined on this node via
// kubernetes annotations
//
Expand Down Expand Up @@ -413,3 +425,48 @@ func (c *Controller) Reconcile(ctx context.Context) error {
func (c *Controller) FullWithdrawal(ctx context.Context) {
_ = c.BGPMgr.ConfigurePeers(ctx, nil, nil) // cannot fail, no need for error handling
}

// applyPolicyDefaults applies default values on the CiliumBGPPeeringPolicy.
func (c *Controller) applyPolicyDefaults(policy *v2alpha1api.CiliumBGPPeeringPolicy) *v2alpha1api.CiliumBGPPeeringPolicy {
p := policy.DeepCopy() // deepcopy to not modify the policy object in store
for _, r := range p.Spec.VirtualRouters {
for j := range r.Neighbors {
n := &r.Neighbors[j]
if n.ConnectRetryTime.Duration == 0 {
n.ConnectRetryTime.Duration = types.DefaultBGPConnectRetryTime
}
if n.HoldTime.Duration == 0 {
// RFC4271 Sec 4.4 says that hold time can be 0 and has a special meaning that disables keepalive.
// However, as GoBGP defaults the hold time for 0 value, it cannot be 0 in our case.
n.HoldTime.Duration = types.DefaultBGPHoldTime
}
if n.KeepAliveTime.Duration == 0 {
n.KeepAliveTime.Duration = n.HoldTime.Duration / 3
}
}
}
return p
}

// validatePolicy validates the CiliumBGPPeeringPolicy.
func (c *Controller) validatePolicy(policy *v2alpha1api.CiliumBGPPeeringPolicy) error {
for _, r := range policy.Spec.VirtualRouters {
for _, n := range r.Neighbors {
if n.ConnectRetryTime.Duration < 0 {
return fmt.Errorf("connectRetryTime is negative for peer ASN %d, IP %s", n.PeerASN, n.PeerAddress)
}
if n.HoldTime.Duration < minHoldTime {
// RFC4271 Sec 4.2 says that the hold time MUST be zero or at least 3 seconds.
// However, as GoBGP defaults the hold time for 0 value, it cannot be 0 in our case.
return fmt.Errorf("holdTime is lower than %v for peer ASN %d, IP %s", minHoldTime, n.PeerASN, n.PeerAddress)
}
if n.KeepAliveTime.Duration < 0 {
return fmt.Errorf("keepAliveTime is negative for peer ASN %d, IP %s", n.PeerASN, n.PeerAddress)
}
if n.KeepAliveTime.Duration > n.HoldTime.Duration {
return fmt.Errorf("keepAliveTime time larger than holdTime for peer ASN %d, IP %s", n.PeerASN, n.PeerAddress)
}
}
}
return nil
}
119 changes: 117 additions & 2 deletions pkg/bgpv1/agent/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ import (
"errors"
"net/netip"
"testing"
"time"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/cilium/cilium/pkg/bgpv1/agent"
"github.com/cilium/cilium/pkg/bgpv1/mock"
Expand Down Expand Up @@ -88,8 +91,7 @@ func TestControllerSanity(t *testing.T) {
return []*v2alpha1api.CiliumBGPPeeringPolicy{wantPolicy}, nil
},
configurePeers: func(_ context.Context, p *v2alpha1api.CiliumBGPPeeringPolicy, c *agent.ControlPlaneState) error {
// pointer check, not deep equal
if p != wantPolicy {
if !p.DeepEqual(wantPolicy) {
t.Fatalf("got: %+v, want: %+v", p, wantPolicy)
}
if c.IPv4 != nodeIPv4 {
Expand All @@ -99,6 +101,51 @@ func TestControllerSanity(t *testing.T) {
},
err: nil,
},
// test policy defaulting
{
name: "policy defaulting on successful reconcile",
labels: func() (map[string]string, error) {
return map[string]string{
"bgp-policy": "a",
}, nil
},
annotations: func() (map[string]string, error) {
return map[string]string{}, nil
},
podCIDRs: func() ([]string, error) {
return []string{}, nil
},
plist: func() ([]*v2alpha1api.CiliumBGPPeeringPolicy, error) {
p := wantPolicy.DeepCopy()
p.Spec.VirtualRouters = []v2alpha1api.CiliumBGPVirtualRouter{
{
LocalASN: 65001,
Neighbors: []v2alpha1api.CiliumBGPNeighbor{
{
PeerASN: 65000,
PeerAddress: "172.0.0.1/32",
},
},
},
}
return []*v2alpha1api.CiliumBGPPeeringPolicy{p}, nil
},
configurePeers: func(_ context.Context, p *v2alpha1api.CiliumBGPPeeringPolicy, c *agent.ControlPlaneState) error {
defaulted := false
for _, r := range p.Spec.VirtualRouters {
for _, n := range r.Neighbors {
if n.ConnectRetryTime.Duration != 0 && n.HoldTime.Duration != 0 && n.KeepAliveTime.Duration != 0 {
defaulted = true
}
}
}
if !defaulted {
t.Fatalf("policy: %v not defaulted properly", p)
}
return nil
},
err: nil,
},
// follow tests demonstrate proper error handling when dependencies
// return errors.
//
Expand Down Expand Up @@ -179,6 +226,74 @@ func TestControllerSanity(t *testing.T) {
},
err: errors.New(""),
},
{
name: "connect retry time validation error",
plist: func() ([]*v2alpha1api.CiliumBGPPeeringPolicy, error) {
p := wantPolicy.DeepCopy()
p.Spec.VirtualRouters = []v2alpha1api.CiliumBGPVirtualRouter{
{
LocalASN: 65001,
Neighbors: []v2alpha1api.CiliumBGPNeighbor{
{
PeerASN: 65000,
PeerAddress: "172.0.0.1/32",
ConnectRetryTime: metav1.Duration{Duration: -1 * time.Second},
},
},
},
}
return []*v2alpha1api.CiliumBGPPeeringPolicy{p}, nil
},
labels: func() (map[string]string, error) {
return map[string]string{
"bgp-policy": "a",
}, nil
},
annotations: func() (map[string]string, error) {
return map[string]string{}, nil
},
podCIDRs: func() ([]string, error) {
return []string{}, nil
},
configurePeers: func(_ context.Context, p *v2alpha1api.CiliumBGPPeeringPolicy, c *agent.ControlPlaneState) error {
return nil
},
err: errors.New(""),
},
{
name: "hold time validation error",
plist: func() ([]*v2alpha1api.CiliumBGPPeeringPolicy, error) {
p := wantPolicy.DeepCopy()
p.Spec.VirtualRouters = []v2alpha1api.CiliumBGPVirtualRouter{
{
LocalASN: 65001,
Neighbors: []v2alpha1api.CiliumBGPNeighbor{
{
PeerASN: 65000,
PeerAddress: "172.0.0.1/32",
HoldTime: metav1.Duration{Duration: 1 * time.Second},
},
},
},
}
return []*v2alpha1api.CiliumBGPPeeringPolicy{p}, nil
},
labels: func() (map[string]string, error) {
return map[string]string{
"bgp-policy": "a",
}, nil
},
annotations: func() (map[string]string, error) {
return map[string]string{}, nil
},
podCIDRs: func() ([]string, error) {
return []string{}, nil
},
configurePeers: func(_ context.Context, p *v2alpha1api.CiliumBGPPeeringPolicy, c *agent.ControlPlaneState) error {
return nil
},
err: errors.New(""),
},
}
for _, tt := range table {
t.Run(tt.name, func(t *testing.T) {
Expand Down

0 comments on commit b3debfe

Please sign in to comment.