Skip to content

Commit 89042d2

Browse files
dswaffordcwdswaffordcw
committed
review updates
1 parent d88babf commit 89042d2

File tree

4 files changed

+249
-81
lines changed

4 files changed

+249
-81
lines changed

‎pkg/bgpv1/manager/reconcilerv2/service.go‎

Lines changed: 85 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -280,10 +280,11 @@ func (r *ServiceReconciler) getDesiredSvcRoutePolicies(p ReconcileParams, desire
280280
if lbPolicy != nil {
281281
currentLbPolicy := desiredSvcRoutePolicies[lbPolicy.Name]
282282
if currentLbPolicy != nil {
283-
r.handleOverlappingDesiredSvcRoutePolicies(currentLbPolicy, lbPolicy, advert)
284-
} else {
285-
desiredSvcRoutePolicies[lbPolicy.Name] = lbPolicy
283+
if lbPolicy, err = mergeRoutePolicies(currentLbPolicy, lbPolicy); err != nil {
284+
return nil, err
285+
}
286286
}
287+
desiredSvcRoutePolicies[lbPolicy.Name] = lbPolicy
287288
}
288289

289290
// ExternalIP
@@ -294,10 +295,11 @@ func (r *ServiceReconciler) getDesiredSvcRoutePolicies(p ReconcileParams, desire
294295
if extPolicy != nil {
295296
currentExtPolicy := desiredSvcRoutePolicies[extPolicy.Name]
296297
if currentExtPolicy != nil {
297-
r.handleOverlappingDesiredSvcRoutePolicies(currentExtPolicy, extPolicy, advert)
298-
} else {
299-
desiredSvcRoutePolicies[extPolicy.Name] = extPolicy
298+
if extPolicy, err = mergeRoutePolicies(currentExtPolicy, extPolicy); err != nil {
299+
return nil, err
300+
}
300301
}
302+
desiredSvcRoutePolicies[extPolicy.Name] = extPolicy
301303
}
302304

303305
// ClusterIP
@@ -308,10 +310,11 @@ func (r *ServiceReconciler) getDesiredSvcRoutePolicies(p ReconcileParams, desire
308310
if clusterPolicy != nil {
309311
currentClusterPolicy := desiredSvcRoutePolicies[clusterPolicy.Name]
310312
if currentClusterPolicy != nil {
311-
r.handleOverlappingDesiredSvcRoutePolicies(currentClusterPolicy, clusterPolicy, advert)
312-
} else {
313-
desiredSvcRoutePolicies[clusterPolicy.Name] = clusterPolicy
313+
if clusterPolicy, err = mergeRoutePolicies(currentClusterPolicy, clusterPolicy); err != nil {
314+
return nil, err
315+
}
314316
}
317+
desiredSvcRoutePolicies[clusterPolicy.Name] = clusterPolicy
315318
}
316319
}
317320
}
@@ -320,70 +323,6 @@ func (r *ServiceReconciler) getDesiredSvcRoutePolicies(p ReconcileParams, desire
320323
return desiredSvcRoutePolicies, nil
321324
}
322325

323-
// handleOverlappingDesiredSvcRoutePolicies handles the case of a desired BGP advertisement that overlaps a previous
324-
// selector match. Overlaps are handled as additive operations for attributes that can be unioned (standard and large
325-
// communities). If local preference was set, it remains set to the first advertisement matched.
326-
//
327-
// This function modifies existingPolicy to contain the unions described above.
328-
func (r *ServiceReconciler) handleOverlappingDesiredSvcRoutePolicies(
329-
existingPolicy *types.RoutePolicy,
330-
candidatePolicy *types.RoutePolicy,
331-
advert v2alpha1.BGPAdvertisement,
332-
) {
333-
334-
// For tests, we must maintain the original order. Communities will hold
335-
// the unique set of standard communities defined across both policies.
336-
communities := []string{}
337-
communitiesSeen := sets.New[string]()
338-
339-
largeCommunities := []string{}
340-
largeCommunitiesSeen := sets.New[string]()
341-
342-
// Extract the existing policy's communities
343-
for _, statement := range existingPolicy.Statements {
344-
communities = append(communities, statement.Actions.AddCommunities...)
345-
communitiesSeen.Insert(statement.Actions.AddCommunities...)
346-
347-
largeCommunities = append(largeCommunities, statement.Actions.AddLargeCommunities...)
348-
largeCommunitiesSeen.Insert(statement.Actions.AddLargeCommunities...)
349-
}
350-
logMessagePreviousCommunities := fmt.Sprint(communities)
351-
logMessagePreviousLargeCommunities := fmt.Sprint(largeCommunities)
352-
353-
// Extract this policy's communities
354-
for _, statement := range candidatePolicy.Statements {
355-
for _, community := range statement.Actions.AddCommunities {
356-
if communitiesSeen.Has(community) {
357-
continue
358-
}
359-
communities = append(communities, community)
360-
}
361-
362-
for _, largeCommunity := range statement.Actions.AddLargeCommunities {
363-
if largeCommunitiesSeen.Has(largeCommunity) {
364-
continue
365-
}
366-
largeCommunities = append(largeCommunities, largeCommunity)
367-
}
368-
}
369-
370-
// Update the existing policy's communities to be the updated sequence created above
371-
for _, statement := range existingPolicy.Statements {
372-
statement.Actions.AddCommunities = communities
373-
statement.Actions.AddLargeCommunities = largeCommunities
374-
}
375-
376-
r.logger.Debugf(
377-
"Desired bgp advertisement advertisementType=%s, service.addresses=%s, selector=%s "+
378-
"overlaps with previous match. Overlaps are handled as additive operations for "+
379-
"attributes that can be unioned. Previous match sets communities=%s, largeCommunities=%s. "+
380-
"Updated policy sets communities=%s, largeCommunities=%s. If local preference was set, "+
381-
"it remains set to the first advertisement matched.",
382-
advert.AdvertisementType, advert.Service.Addresses, advert.Selector.String(),
383-
logMessagePreviousCommunities, logMessagePreviousLargeCommunities, communities, largeCommunities,
384-
)
385-
}
386-
387326
func (r *ServiceReconciler) reconcilePaths(ctx context.Context, p ReconcileParams, desiredSvcPaths ResourceAFPathsMap) error {
388327
var err error
389328
metadata := r.getMetadata(p.BGPInstance)
@@ -932,6 +871,79 @@ func checkServiceAdvertisement(advert v2alpha1.BGPAdvertisement, advertServiceTy
932871
return true, nil
933872
}
934873

874+
func mergeRoutePolicies(policyA *types.RoutePolicy, policyB *types.RoutePolicy) (*types.RoutePolicy, error) {
875+
if policyA == nil || policyB == nil {
876+
return nil, fmt.Errorf("route policy is nil")
877+
}
878+
if policyA.Name != policyB.Name {
879+
return nil, fmt.Errorf("route policy names do not match")
880+
}
881+
if policyA.Type != policyB.Type {
882+
return nil, fmt.Errorf("route policy types do not match")
883+
}
884+
885+
communities := sets.NewString()
886+
largeCommunities := sets.NewString()
887+
mergedPolicy := &types.RoutePolicy{
888+
Name: policyA.Name,
889+
Type: policyA.Type,
890+
Statements: []*types.RoutePolicyStatement{},
891+
}
892+
routePolicyConditionsSeen := sets.NewString()
893+
894+
// Extract the first policy's statements and communities
895+
for _, statement := range policyA.Statements {
896+
mergedPolicy.Statements = append(mergedPolicy.Statements, &types.RoutePolicyStatement{
897+
// statement.Actions and .Conditions are values, not pointers
898+
Actions: statement.Actions,
899+
Conditions: statement.Conditions,
900+
})
901+
communities.Insert(statement.Actions.AddCommunities...)
902+
largeCommunities.Insert(statement.Actions.AddLargeCommunities...)
903+
routePolicyConditionsSeen.Insert(statement.Conditions.String())
904+
}
905+
906+
// Extract the second policy's communities non-overlapping statements, and communities from all statements
907+
for _, statement := range policyB.Statements {
908+
if !routePolicyConditionsSeen.Has(statement.Conditions.String()) {
909+
mergedPolicy.Statements = append(mergedPolicy.Statements, &types.RoutePolicyStatement{
910+
// statement.Actions and .Conditions are values, not pointers
911+
Actions: statement.Actions,
912+
Conditions: statement.Conditions,
913+
})
914+
}
915+
for _, community := range statement.Actions.AddCommunities {
916+
communities.Insert(community)
917+
}
918+
for _, largeCommunity := range statement.Actions.AddLargeCommunities {
919+
largeCommunities.Insert(largeCommunity)
920+
}
921+
}
922+
923+
bestLocalPreference := int64(0)
924+
localPreferenceDefaultValue := int64(0)
925+
for _, statement := range mergedPolicy.Statements {
926+
if statement.Actions.RouteAction != types.RoutePolicyActionAccept {
927+
continue
928+
}
929+
if len(statement.Actions.AddCommunities) != 0 {
930+
statement.Actions.AddCommunities = communities.List()
931+
}
932+
if len(statement.Actions.AddLargeCommunities) != 0 {
933+
statement.Actions.AddLargeCommunities = largeCommunities.List()
934+
}
935+
// RFC 4271 states "The higher degree of preference MUST be preferred."
936+
localPreference := *statement.Actions.SetLocalPreference
937+
if localPreference != localPreferenceDefaultValue &&
938+
localPreference > bestLocalPreference {
939+
bestLocalPreference = localPreference
940+
} else {
941+
statement.Actions.SetLocalPreference = &bestLocalPreference
942+
}
943+
}
944+
return mergedPolicy, nil
945+
}
946+
935947
func serviceLabelSet(svc *slim_corev1.Service) labels.Labels {
936948
svcLabels := maps.Clone(svc.Labels)
937949
if svcLabels == nil {

‎pkg/bgpv1/manager/reconcilerv2/service_test.go‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1018,7 +1018,7 @@ func Test_ServiceExternalIPReconciler(t *testing.T) {
10181018
Conditions: redPeer65001v4ExtRP.Statements[0].Conditions,
10191019
Actions: types.RoutePolicyActions{
10201020
RouteAction: types.RoutePolicyActionAccept,
1021-
AddCommunities: []string{"101:101", "no-export", "202:202", "303:303"},
1021+
AddCommunities: []string{"101:101", "202:202", "303:303", "no-export"},
10221022
AddLargeCommunities: []string{"1111:1111:1111", "2222:2222:2222", "3333:3333:3333"},
10231023
SetLocalPreference: &localPrefHigh,
10241024
},
@@ -1033,7 +1033,7 @@ func Test_ServiceExternalIPReconciler(t *testing.T) {
10331033
Conditions: redPeer65001v6ExtRP.Statements[0].Conditions,
10341034
Actions: types.RoutePolicyActions{
10351035
RouteAction: types.RoutePolicyActionAccept,
1036-
AddCommunities: []string{"101:101", "no-export", "202:202", "303:303"},
1036+
AddCommunities: []string{"101:101", "202:202", "303:303", "no-export"},
10371037
AddLargeCommunities: []string{"1111:1111:1111", "2222:2222:2222", "3333:3333:3333"},
10381038
SetLocalPreference: &localPrefHigh,
10391039
},
@@ -1318,7 +1318,7 @@ func Test_ServiceClusterIPReconciler(t *testing.T) {
13181318
Conditions: redPeer65001v4ClusterRP.Statements[0].Conditions,
13191319
Actions: types.RoutePolicyActions{
13201320
RouteAction: types.RoutePolicyActionAccept,
1321-
AddCommunities: []string{"101:101", "no-export", "202:202", "303:303"},
1321+
AddCommunities: []string{"101:101", "202:202", "303:303", "no-export"},
13221322
AddLargeCommunities: []string{"1111:1111:1111", "2222:2222:2222", "3333:3333:3333"},
13231323
SetLocalPreference: &localPrefHigh,
13241324
},
@@ -1333,7 +1333,7 @@ func Test_ServiceClusterIPReconciler(t *testing.T) {
13331333
Conditions: redPeer65001v6ClusterRP.Statements[0].Conditions,
13341334
Actions: types.RoutePolicyActions{
13351335
RouteAction: types.RoutePolicyActionAccept,
1336-
AddCommunities: []string{"101:101", "no-export", "202:202", "303:303"},
1336+
AddCommunities: []string{"101:101", "202:202", "303:303", "no-export"},
13371337
AddLargeCommunities: []string{"1111:1111:1111", "2222:2222:2222", "3333:3333:3333"},
13381338
SetLocalPreference: &localPrefHigh,
13391339
},
@@ -1774,8 +1774,6 @@ func Test_ServiceVIPSharing(t *testing.T) {
17741774
upsertEPs []*k8s.Endpoints
17751775
expectedMetadata ServiceReconcilerMetadata
17761776
}{
1777-
// Note: one or more tests below leave artifacts behind that later tests in
1778-
// the same sequence depend on. Order is significant.
17791777
{
17801778
name: "Add service 1 (LoadBalancer) with advertisement",
17811779
upsertAdverts: []*v2alpha1.CiliumBGPAdvertisement{
@@ -2043,7 +2041,7 @@ func Test_ServiceVIPSharing(t *testing.T) {
20432041
Conditions: redPeer65001v4LBRP.Statements[0].Conditions,
20442042
Actions: types.RoutePolicyActions{
20452043
RouteAction: types.RoutePolicyActionAccept,
2046-
AddCommunities: []string{"101:101", "no-export", "202:202"},
2044+
AddCommunities: []string{"101:101", "202:202", "no-export"},
20472045
AddLargeCommunities: []string{"1111:1111:1111", "2222:2222:2222"},
20482046
SetLocalPreference: &localPrefHigh,
20492047
},
@@ -2058,7 +2056,7 @@ func Test_ServiceVIPSharing(t *testing.T) {
20582056
Conditions: redPeer65001v6LBRP.Statements[0].Conditions,
20592057
Actions: types.RoutePolicyActions{
20602058
RouteAction: types.RoutePolicyActionAccept,
2061-
AddCommunities: []string{"101:101", "no-export", "202:202"},
2059+
AddCommunities: []string{"101:101", "202:202", "no-export"},
20622060
AddLargeCommunities: []string{"1111:1111:1111", "2222:2222:2222"},
20632061
SetLocalPreference: &localPrefHigh,
20642062
},
@@ -2141,6 +2139,30 @@ func Test_ServiceVIPSharing(t *testing.T) {
21412139
}
21422140
}
21432141

2142+
func Test_mergeRoutePolicies(t *testing.T) {
2143+
tests := []struct {
2144+
name string
2145+
}{
2146+
{
2147+
name: "nil policies",
2148+
},
2149+
{
2150+
name: "deduplication of communities",
2151+
},
2152+
{
2153+
name: "local preference",
2154+
},
2155+
{
2156+
name: "edge case #1",
2157+
},
2158+
}
2159+
2160+
for _, tt := range tests {
2161+
t.Run(tt.name, func(t *testing.T) {
2162+
})
2163+
}
2164+
}
2165+
21442166
func serviceMetadataEqual(req *require.Assertions, expectedMetadata, runningMetadata ServiceReconcilerMetadata) {
21452167
req.Truef(PeerAdvertisementsEqual(expectedMetadata.ServiceAdvertisements, runningMetadata.ServiceAdvertisements),
21462168
"ServiceAdvertisements mismatch, expected: %v, got: %v", expectedMetadata.ServiceAdvertisements, runningMetadata.ServiceAdvertisements)

‎pkg/bgpv1/types/bgp.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package types
66
import (
77
"context"
88
"net/netip"
9+
"strings"
910

1011
"github.com/osrg/gobgp/v3/pkg/packet/bgp"
1112

@@ -114,6 +115,19 @@ type RoutePolicyConditions struct {
114115
MatchFamilies []Family
115116
}
116117

118+
// String() constructs a string identifier
119+
func (r RoutePolicyConditions) String() string {
120+
values := []string{}
121+
values = append(values, r.MatchNeighbors...)
122+
for _, family := range r.MatchFamilies {
123+
values = append(values, family.String())
124+
}
125+
for _, prefix := range r.MatchPrefixes {
126+
values = append(values, prefix.CIDR.String())
127+
}
128+
return strings.Join(values, "-")
129+
}
130+
117131
// RoutePolicyAction defines the action taken on a route matched by a routing policy.
118132
type RoutePolicyAction int
119133

0 commit comments

Comments
 (0)