v1.14 Backports 2024-02-27

daemon/cmd/endpoint.go

@@ -373,6 +373,16 @@ func (d *Daemon) createEndpoint(ctx context.Context, owner regeneration.Owner, e
 		"sync-build":            epTemplate.SyncBuildEndpoint,
 	}).Info("Create endpoint request")
 
+	// We don't need to create the endpoint with the labels. This might cause
+	// the endpoint regeneration to not be triggered further down, with the
+	// ep.UpdateLabels or the ep.RunMetadataResolver, because the regeneration
+	// is only triggered in case the labels are changed, which they might not
+	// change because NewEndpointFromChangeModel would contain the
+	// epTemplate.Labels, the same labels we would be calling ep.UpdateLabels or
+	// the ep.RunMetadataResolver.
+	apiLabels := labels.NewLabelsFromModel(epTemplate.Labels)
+	epTemplate.Labels = nil
+
 	ep, err := endpoint.NewEndpointFromChangeModel(d.ctx, owner, d, d.ipcache, d.l7Proxy, d.identityAllocator, epTemplate)
 	if err != nil {
 		return invalidDataError(ep, fmt.Errorf("unable to parse endpoint parameters: %s", err))
@@ -411,7 +421,6 @@ func (d *Daemon) createEndpoint(ctx context.Context, owner regeneration.Owner, e
 		return invalidDataError(ep, err)
 	}
 
-	apiLabels := labels.NewLabelsFromModel(epTemplate.Labels)
 	infoLabels := labels.NewLabelsFromModel([]string{})
 
 	if len(apiLabels) > 0 {

install/kubernetes/cilium/templates/cilium-envoy/daemonset.yaml

@@ -82,7 +82,7 @@ spec:
         {{- if semverCompare ">=1.20-0" .Capabilities.KubeVersion.Version }}
         startupProbe:
           httpGet:
-            host: "localhost"
+            host: {{ .Values.ipv4.enabled | ternary "127.0.0.1" "::1" | quote }}
             path: /healthz
             port: {{ .Values.envoy.healthPort }}
             scheme: HTTP
@@ -92,7 +92,7 @@ spec:
         {{- end }}
         livenessProbe:
           httpGet:
-            host: "localhost"
+            host: {{ .Values.ipv4.enabled | ternary "127.0.0.1" "::1" | quote }}
             path: /healthz
             port: {{ .Values.envoy.healthPort }}
             scheme: HTTP
@@ -110,7 +110,7 @@ spec:
           timeoutSeconds: 5
         readinessProbe:
           httpGet:
-            host: "localhost"
+            host: {{ .Values.ipv4.enabled | ternary "127.0.0.1" "::1" | quote }}
             path: /healthz
             port: {{ .Values.envoy.healthPort }}
             scheme: HTTP

operator/pkg/lbipam/lbipam.go

@@ -1121,6 +1121,7 @@ func (ipam *LBIPAM) handlePoolModified(ctx context.Context, pool *cilium_api_v2a
 	}
 
 	existingRanges, _ := ipam.rangesStore.GetRangesForPool(pool.GetName())
+	existingRanges = slices.Clone(existingRanges)
 
 	// Remove existing ranges that no longer exist
 	for _, extRange := range existingRanges {

pkg/bgpv1/agent/controller.go

@@ -144,6 +144,9 @@ type Controller struct {
 	BGPMgr BGPRouterManager
 
 	workerpool *workerpool.WorkerPool
+
+	// PolicyApplied indicates whether the policy is applied to the node or not
+	policyApplied bool
 }
 
 // ControllerParams contains all parameters needed to construct a Controller
@@ -262,7 +265,6 @@ func (c *Controller) Run(ctx context.Context) {
 			l.Info("Cilium BGP Control Plane Controller shut down")
 			return
 		case <-c.Sig.Sig:
-			l.Info("Cilium BGP Control Plane Controller woken for reconciliation")
 			if err := c.Reconcile(ctx); err != nil {
 				l.WithError(err).Error("Encountered error during reconciliation")
 			} else {
@@ -355,7 +357,6 @@ func (c *Controller) Reconcile(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("failed to list CiliumBGPPeeringPolicies")
 	}
-	l.WithField("count", len(policies)).Debug("Successfully listed CiliumBGPPeeringPolicies")
 
 	// perform policy selection based on node.
 	labels, err := c.NodeSpec.Labels()
@@ -369,10 +370,11 @@ func (c *Controller) Reconcile(ctx context.Context) error {
 		return err
 	}
 	if policy == nil {
-		// no policy was discovered, tell router manager to withdrawal peers if
-		// they are configured.
-		l.Debug("No BGP peering policy applies to this node, any existing BGP sessions will be removed.")
-		c.FullWithdrawal(ctx)
+		if c.policyApplied {
+			l.Info("No CiliumBGPPeeringPolicy applies to this node anymore. Removing all virtual router instances.")
+			c.FullWithdrawal(ctx)
+			c.policyApplied = false
+		}
 		return nil
 	}
 
@@ -423,11 +425,12 @@ func (c *Controller) Reconcile(ctx context.Context) error {
 	}
 
 	// call bgp sub-systems required to apply this policy's BGP topology.
-	l.Debug("Asking configured BGPRouterManager to configure peering")
 	if err := c.BGPMgr.ConfigurePeers(ctx, policy, state); err != nil {
 		return fmt.Errorf("failed to configure BGP peers, cannot apply BGP peering policy: %w", err)
 	}
 
+	c.policyApplied = true
+
 	return nil
 }
 

pkg/bgpv1/agent/controller_test.go

@@ -9,10 +9,12 @@ import (
 	"net/netip"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"k8s.io/utils/pointer"
 
 	"github.com/cilium/cilium/pkg/bgpv1/agent"
 	"github.com/cilium/cilium/pkg/bgpv1/mock"
+	"github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
 	v2alpha1api "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
 	v1 "github.com/cilium/cilium/pkg/k8s/slim/k8s/apis/meta/v1"
 	nodeaddr "github.com/cilium/cilium/pkg/node"
@@ -300,6 +302,93 @@ func TestControllerSanity(t *testing.T) {
 	}
 }
 
+// TestDeselection ensures that the deselection of a policy causes a full withdrawal
+func TestDeselection(t *testing.T) {
+	var policy = &v2alpha1api.CiliumBGPPeeringPolicy{
+		Spec: v2alpha1api.CiliumBGPPeeringPolicySpec{
+			NodeSelector: &v1.LabelSelector{
+				MatchLabels: map[string]string{
+					"bgp-policy": "a",
+				},
+			},
+		},
+	}
+
+	withPolicy := func() ([]*v2alpha1api.CiliumBGPPeeringPolicy, error) {
+		return []*v2alpha1api.CiliumBGPPeeringPolicy{policy}, nil
+	}
+
+	withoutPolicy := func() ([]*v2alpha1api.CiliumBGPPeeringPolicy, error) {
+		return []*v2alpha1api.CiliumBGPPeeringPolicy{}, nil
+	}
+
+	// Start from empty policy list
+	policyLister := &agent.MockCiliumBGPPeeringPolicyLister{
+		List_: withoutPolicy,
+	}
+
+	fullWithdrawalObserved := false
+	rtmgr := &mock.MockBGPRouterManager{
+		ConfigurePeers_: func(ctx context.Context, p *v2alpha1.CiliumBGPPeeringPolicy, cs *agent.ControlPlaneState) error {
+			if p == nil && cs == nil {
+				fullWithdrawalObserved = true
+			}
+			return nil
+		},
+	}
+
+	// create test cilium node
+	nodeSpecer := fakeNodeSpecer{
+		PodCIDRs_: func() ([]string, error) { return nil, nil },
+		Labels_: func() (map[string]string, error) {
+			return map[string]string{
+				"bgp-policy": "a",
+			}, nil
+		},
+		Annotations_: func() (map[string]string, error) {
+			return nil, nil
+		},
+	}
+
+	c := agent.Controller{
+		NodeSpec:     &nodeSpecer,
+		PolicyLister: policyLister,
+		BGPMgr:       rtmgr,
+	}
+
+	// Initialize LocalNodeStore
+	nodeaddr.SetTestLocalNodeStore()
+	t.Cleanup(func() {
+		nodeaddr.UnsetTestLocalNodeStore()
+	})
+
+	// First, reconcile with the policy selected
+	err := c.Reconcile(context.Background())
+	require.NoError(t, err)
+
+	// At this point, we shouldn't see any full withdrawal because
+	// there is no previous policy.
+	require.False(t, fullWithdrawalObserved)
+
+	// Now, reconcile with the policy selected
+	policyLister.List_ = withPolicy
+	err = c.Reconcile(context.Background())
+	require.NoError(t, err)
+
+	// At this point, we shouldn't see any full withdrawal because
+	// the policy is still selected.
+	require.False(t, fullWithdrawalObserved)
+
+	// Now, reconcile with the policy deselected
+	policyLister.List_ = withoutPolicy
+	err = c.Reconcile(context.Background())
+	require.NoError(t, err)
+
+	// At this point, we should see a full withdrawal because
+	// the policy is no longer selected.
+	require.True(t, fullWithdrawalObserved)
+}
+
 // TestPolicySelection ensure the selection of a policy is performed correctly
 // and enforces the rule set documented by the PolicySelection function.
 func TestPolicySelection(t *testing.T) {

pkg/bgpv1/gobgp/server.go

@@ -87,7 +87,7 @@ func NewGoBGPServerWithConfig(ctx context.Context, log *logrus.Entry, params typ
 	}
 	err := s.WatchEvent(ctx, watchRequest, func(r *gobgp.WatchEventResponse) {
 		if p := r.GetPeer(); p != nil && p.Type == gobgp.WatchEventResponse_PeerEvent_STATE {
-			logger.l.Info(p)
+			logger.l.Debug(p)
 		}
 	})
 	if err != nil {

pkg/bgpv1/manager/reconcile.go

@@ -222,7 +222,6 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, params ReconcilePara
 		curNeigh []*v2alpha1api.CiliumBGPNeighbor = nil
 	)
 	newNeigh := newc.Neighbors
-	l.Debugf("Begin reconciling peers for virtual router with local ASN %v", newc.LocalASN)
 
 	metaMap := r.getMetadata(params.Server)
 	if len(metaMap) > 0 {
@@ -289,12 +288,6 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, params ReconcilePara
 		}
 	}
 
-	if len(toCreate) > 0 || len(toRemove) > 0 || len(toUpdate) > 0 {
-		l.Infof("Reconciling peers for virtual router with local ASN %v", newc.LocalASN)
-	} else {
-		l.Debugf("No peer changes necessary for virtual router with local ASN %v", newc.LocalASN)
-	}
-
 	// create new neighbors
 	for _, n := range toCreate {
 		l.Infof("Adding peer %v %v to local ASN %v", n.PeerAddress, n.PeerASN, newc.LocalASN)
@@ -322,7 +315,6 @@ func (r *NeighborReconciler) Reconcile(ctx context.Context, params ReconcilePara
 		r.deleteMetadata(sc, n)
 	}
 
-	l.Infof("Done reconciling peers for virtual router with local ASN %v", newc.LocalASN)
 	return nil
 }
 

pkg/slices/slices_test.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"math"
 	"math/rand"
+	"slices"
 	"strconv"
 	"testing"
 	"time"
@@ -63,10 +64,11 @@ func TestUnique(t *testing.T) {
 func TestUniqueFunc(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			input := slices.Clone(tc.input)
 			got := UniqueFunc(
-				tc.input,
+				input,
 				func(i int) int {
-					return tc.input[i]
+					return input[i]
 				},
 			)
 			assert.ElementsMatch(t, tc.expected, got)
@@ -77,7 +79,8 @@ func TestUniqueFunc(t *testing.T) {
 func TestSortedUnique(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			got := SortedUnique(tc.input)
+			input := slices.Clone(tc.input)
+			got := SortedUnique(input)
 			assert.ElementsMatch(t, tc.expected, got)
 		})
 	}
@@ -86,10 +89,11 @@ func TestSortedUnique(t *testing.T) {
 func TestSortedUniqueFunc(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
+			input := slices.Clone(tc.input)
 			got := SortedUniqueFunc(
-				tc.input,
+				input,
 				func(i, j int) bool {
-					return tc.input[i] < tc.input[j]
+					return input[i] < input[j]
 				},
 				func(a, b int) bool {
 					return a == b