From 499034f30afdcfd9e3e31ae235067368eb4e0bea Mon Sep 17 00:00:00 2001 From: Ivan Pokhabov Date: Wed, 12 Aug 2026 14:40:53 +0300 Subject: [PATCH] fix(server): apply SetPolicies policy assignments --- internal/pkg/table/policy.go | 40 ++++--- internal/pkg/table/policy_test.go | 74 ++++++++++++ pkg/server/server.go | 51 +++++++++ pkg/server/server_test.go | 181 +++++++++++++++++++++++++++++- 4 files changed, 325 insertions(+), 21 deletions(-) diff --git a/internal/pkg/table/policy.go b/internal/pkg/table/policy.go index 422f58907..032b1119d 100644 --- a/internal/pkg/table/policy.go +++ b/internal/pkg/table/policy.go @@ -4725,27 +4725,27 @@ func (r *RoutingPolicy) Initialize() error { return nil } -func (r *RoutingPolicy) setPeerPolicy(id string, c oc.ApplyPolicy) { - for _, dir := range []PolicyDirection{POLICY_DIRECTION_IMPORT, POLICY_DIRECTION_EXPORT} { - ps, def, err := r.getAssignmentFromConfig(dir, c) - if err != nil { - r.logger.Error("failed to get policy info", - slog.String("Topic", "Policy"), - slog.String("Dir", dir.String()), - slog.String("Error", err.Error())) - continue - } - r.setDefaultPolicy(id, dir, def) - r.setPolicy(id, dir, ps) +func (r *RoutingPolicy) setPeerPolicy(id string, c oc.ApplyPolicy) error { + importPolicies, defaultImport, err := r.getAssignmentFromConfig(POLICY_DIRECTION_IMPORT, c) + if err != nil { + return fmt.Errorf("failed to get %s policy info for %s: %w", POLICY_DIRECTION_IMPORT, id, err) } + exportPolicies, defaultExport, err := r.getAssignmentFromConfig(POLICY_DIRECTION_EXPORT, c) + if err != nil { + return fmt.Errorf("failed to get %s policy info for %s: %w", POLICY_DIRECTION_EXPORT, id, err) + } + r.setDefaultPolicy(id, POLICY_DIRECTION_IMPORT, defaultImport) + r.setPolicy(id, POLICY_DIRECTION_IMPORT, importPolicies) + r.setDefaultPolicy(id, POLICY_DIRECTION_EXPORT, defaultExport) + r.setPolicy(id, POLICY_DIRECTION_EXPORT, exportPolicies) + return nil } func (r *RoutingPolicy) SetPeerPolicy(peerId string, c oc.ApplyPolicy) error { r.mu.Lock() defer r.mu.Unlock() - r.setPeerPolicy(peerId, c) - return nil + return r.setPeerPolicy(peerId, c) } // DeletePeerPolicy drops the policy assignment of a peer that is gone. Nothing @@ -4766,7 +4766,9 @@ func (r *RoutingPolicy) Reset(rp *oc.RoutingPolicy, ap map[string]oc.ApplyPolicy r.mu.Lock() defer r.mu.Unlock() - if err := r.reload(*rp); err != nil { + // Build the complete replacement before changing the active policy state. + next := NewRoutingPolicy(r.logger) + if err := next.reload(*rp); err != nil { r.logger.Error("failed to create routing policy", slog.String("Topic", "Policy"), slog.String("Error", err.Error())) @@ -4774,8 +4776,14 @@ func (r *RoutingPolicy) Reset(rp *oc.RoutingPolicy, ap map[string]oc.ApplyPolicy } for id, c := range ap { - r.setPeerPolicy(id, c) + if err := next.setPeerPolicy(id, c); err != nil { + return err + } } + r.definedSetMap = next.definedSetMap + r.policyMap = next.policyMap + r.statementMap = next.statementMap + r.assignmentMap = next.assignmentMap return nil } diff --git a/internal/pkg/table/policy_test.go b/internal/pkg/table/policy_test.go index 34dc76c6a..99c09b5d9 100644 --- a/internal/pkg/table/policy_test.go +++ b/internal/pkg/table/policy_test.go @@ -32,6 +32,80 @@ import ( "github.com/stretchr/testify/require" ) +func TestSetPeerPolicyAtomic(t *testing.T) { + for _, names := range [][]string{{"missing"}, {"new", "new"}} { + t.Run(strings.Join(names, ","), func(t *testing.T) { + r := NewRoutingPolicy(logger) + r.policyMap["old"] = &Policy{Name: "old"} + r.policyMap["new"] = &Policy{Name: "new"} + initial := oc.ApplyPolicy{Config: oc.ApplyPolicyConfig{ + ImportPolicyList: []string{"old"}, ExportPolicyList: []string{"old"}, + DefaultImportPolicy: oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, + DefaultExportPolicy: oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, + }} + require.NoError(t, r.SetPeerPolicy("peer", initial)) + before := *r.assignmentMap["peer"] + invalid := oc.ApplyPolicy{Config: oc.ApplyPolicyConfig{ + ImportPolicyList: []string{"new"}, ExportPolicyList: names, + }} + require.Error(t, r.SetPeerPolicy("peer", invalid)) + require.Equal(t, before, *r.assignmentMap["peer"]) + require.Error(t, r.SetPeerPolicy("new-peer", invalid)) + require.NotContains(t, r.assignmentMap, "new-peer") + + invalid.Config.ExportPolicyList = []string{"new"} + require.NoError(t, r.SetPeerPolicy("peer", invalid)) + for _, dir := range []PolicyDirection{POLICY_DIRECTION_IMPORT, POLICY_DIRECTION_EXPORT} { + def, ps, err := r.GetPolicyAssignment("peer", dir) + require.NoError(t, err) + require.Equal(t, ROUTE_TYPE_ACCEPT, def) + require.Equal(t, []*Policy{r.policyMap["new"]}, ps) + } + }) + } +} + +func TestRoutingPolicyResetAtomic(t *testing.T) { + r := NewRoutingPolicy(logger) + initial := &oc.RoutingPolicy{PolicyDefinitions: []oc.PolicyDefinition{{Name: "old"}}} + assignment := oc.ApplyPolicy{Config: oc.ApplyPolicyConfig{ + ImportPolicyList: []string{"old"}, ExportPolicyList: []string{"old"}, + DefaultImportPolicy: oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, + DefaultExportPolicy: oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, + }} + ap := map[string]oc.ApplyPolicy{GLOBAL_RIB_NAME: assignment, "peer1": assignment, "peer2": assignment} + require.NoError(t, r.Reset(initial, ap)) + oldSets, oldPolicies, oldStatements, oldAssignments := r.definedSetMap, r.policyMap, r.statementMap, r.assignmentMap + next := &oc.RoutingPolicy{PolicyDefinitions: []oc.PolicyDefinition{{Name: "new"}}} + valid := oc.ApplyPolicy{Config: oc.ApplyPolicyConfig{ + ImportPolicyList: []string{"new"}, ExportPolicyList: []string{"new"}, + }} + for _, names := range [][]string{{"missing"}, {"new", "new"}} { + invalid := valid + invalid.Config.ExportPolicyList = names + err := r.Reset(next, map[string]oc.ApplyPolicy{ + GLOBAL_RIB_NAME: valid, "peer1": valid, "peer2": invalid, + }) + require.Error(t, err) + require.Equal(t, oldSets, r.definedSetMap) + require.Equal(t, oldPolicies, r.policyMap) + require.Equal(t, oldStatements, r.statementMap) + require.Equal(t, oldAssignments, r.assignmentMap) + } + require.NoError(t, r.Reset(next, map[string]oc.ApplyPolicy{GLOBAL_RIB_NAME: valid, "peer1": valid})) + require.NotContains(t, r.policyMap, "old") + require.NotContains(t, r.assignmentMap, "peer2") + for _, id := range []string{GLOBAL_RIB_NAME, "peer1"} { + for _, dir := range []PolicyDirection{POLICY_DIRECTION_IMPORT, POLICY_DIRECTION_EXPORT} { + def, ps, err := r.GetPolicyAssignment(id, dir) + require.NoError(t, err) + require.Equal(t, ROUTE_TYPE_ACCEPT, def) + require.Len(t, ps, 1) + require.Same(t, r.policyMap["new"], ps[0]) + } + } +} + func TestGetStatement(t *testing.T) { r := NewRoutingPolicy(logger) r.statementMap["statement1"] = &Statement{Name: "statement1"} diff --git a/pkg/server/server.go b/pkg/server/server.go index 8674aa494..f10058d0d 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -2297,6 +2297,41 @@ func (s *BgpServer) SetPolicies(ctx context.Context, r *api.SetPoliciesRequest) c.Config.DefaultExportPolicy = rt return c, nil } + applyAssignment := func(ap map[string]oc.ApplyPolicy, assignment *api.PolicyAssignment) error { + if assignment == nil { + return fmt.Errorf("nil policy assignment") + } + id, dir, err := s.toPolicyInfo(assignment.Name, assignment.Direction) + if err != nil { + return err + } + + policies := make([]string, 0, len(assignment.Policies)) + for _, policy := range assignment.Policies { + if policy == nil { + return fmt.Errorf("nil policy in assignment %q", assignment.Name) + } + policies = append(policies, policy.Name) + } + + c := ap[id] + switch dir { + case table.POLICY_DIRECTION_IMPORT: + c.Config.ImportPolicyList = policies + if defaultPolicy, ok := defaultPolicyTypeFromRouteAction(assignment.DefaultAction); ok { + c.Config.DefaultImportPolicy = defaultPolicy + } + case table.POLICY_DIRECTION_EXPORT: + c.Config.ExportPolicyList = policies + if defaultPolicy, ok := defaultPolicyTypeFromRouteAction(assignment.DefaultAction); ok { + c.Config.DefaultExportPolicy = defaultPolicy + } + default: + return fmt.Errorf("invalid policy direction") + } + ap[id] = c + return nil + } return s.mgmtOperation(func() error { ap := make(map[string]oc.ApplyPolicy, len(s.neighborMap)+1) @@ -2317,10 +2352,26 @@ func (s *BgpServer) SetPolicies(ctx context.Context, r *api.SetPoliciesRequest) } ap[peer.ID()] = *a } + for _, assignment := range r.Assignments { + if err := applyAssignment(ap, assignment); err != nil { + return err + } + } return s.policy.Reset(rp, ap) }, false) } +func defaultPolicyTypeFromRouteAction(action api.RouteAction) (oc.DefaultPolicyType, bool) { + switch action { + case api.RouteAction_ROUTE_ACTION_ACCEPT: + return oc.DEFAULT_POLICY_TYPE_ACCEPT_ROUTE, true + case api.RouteAction_ROUTE_ACTION_REJECT: + return oc.DEFAULT_POLICY_TYPE_REJECT_ROUTE, true + default: + return "", false + } +} + // EVPN MAC MOBILITY HANDLING // // We don't have multihoming function now, so ignore diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index ff3f0c17b..b654f7e0a 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -496,6 +496,171 @@ func TestModPolicyAssign(t *testing.T) { assert.Equal(len(ps), 2) } +func TestSetPoliciesAssignments(t *testing.T) { + policy := &api.Policy{Name: "p1"} + validAssignments := []*api.PolicyAssignment{ + { + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, + Policies: []*api.Policy{{Name: policy.Name}}, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }, + { + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_EXPORT, + Policies: []*api.Policy{{Name: policy.Name}}, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }, + } + tests := []struct { + name string + request *api.SetPoliciesRequest + nextRequest *api.SetPoliciesRequest + err string + expectAssignments bool + }{ + { + name: "applies assignments", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: validAssignments, + }, + expectAssignments: true, + }, + { + name: "missing policy", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: []*api.PolicyAssignment{{ + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, + Policies: []*api.Policy{{Name: "missing"}}, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }}, + }, + err: "not found policy missing", + }, + { + name: "duplicate policy", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: []*api.PolicyAssignment{{ + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, + Policies: []*api.Policy{{Name: policy.Name}, {Name: policy.Name}}, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }}, + }, + err: "duplicated policy p1", + }, + { + name: "nil assignment", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: []*api.PolicyAssignment{nil}, + }, + err: "nil policy assignment", + }, + { + name: "nil policy", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: []*api.PolicyAssignment{{ + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, + Policies: []*api.Policy{nil}, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }}, + }, + err: "nil policy in assignment", + }, + { + name: "without assignments preserves existing assignments", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: validAssignments, + }, + nextRequest: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + }, + expectAssignments: true, + }, + { + name: "empty assignment policies clears existing policies", + request: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: validAssignments, + }, + nextRequest: &api.SetPoliciesRequest{ + Policies: []*api.Policy{policy}, + Assignments: []*api.PolicyAssignment{ + { + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_IMPORT, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }, + { + Name: table.GLOBAL_RIB_NAME, + Direction: api.PolicyDirection_POLICY_DIRECTION_EXPORT, + DefaultAction: api.RouteAction_ROUTE_ACTION_REJECT, + }, + }, + }, + expectAssignments: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + + s := NewBgpServer() + go s.Serve() + err := s.StartBgp(context.Background(), &api.StartBgpRequest{ + Global: &api.Global{ + Asn: 1, + RouterId: "1.1.1.1", + ListenPort: -1, + }, + }) + require.NoError(err) + defer s.StopBgp(context.Background(), &api.StopBgpRequest{}) + + err = s.SetPolicies(context.Background(), tt.request) + if tt.err != "" { + require.ErrorContains(err, tt.err) + return + } + require.NoError(err) + + if tt.nextRequest != nil { + err = s.SetPolicies(context.Background(), tt.nextRequest) + require.NoError(err) + } + + for _, direction := range []api.PolicyDirection{ + api.PolicyDirection_POLICY_DIRECTION_IMPORT, + api.PolicyDirection_POLICY_DIRECTION_EXPORT, + } { + assignments := []*api.PolicyAssignment{} + err = s.ListPolicyAssignment(context.Background(), &api.ListPolicyAssignmentRequest{ + Name: table.GLOBAL_RIB_NAME, + Direction: direction, + }, func(p *api.PolicyAssignment) { assignments = append(assignments, p) }) + require.NoError(err) + require.Len(assignments, 1) + require.Equal(api.RouteAction_ROUTE_ACTION_REJECT, assignments[0].DefaultAction) + if tt.expectAssignments { + require.Len(assignments[0].Policies, 1) + require.Equal(policy.Name, assignments[0].Policies[0].Name) + } else { + require.Empty(assignments[0].Policies) + } + } + }) + } +} + func TestBMPMonitoringPolicyFromAPI(t *testing.T) { t.Parallel() @@ -4932,11 +5097,17 @@ func TestDeletePeerDropsPolicyAssignment(t *testing.T) { err = s.DeletePeer(context.Background(), &api.DeletePeerRequest{Address: "127.0.0.1"}) assert.NoError(err) - // Adding the peer back with an apply-policy that does not resolve leaves - // the assignment unset. The peer must not inherit what the deleted peer - // had. - assert.NoError(addPeer("not-defined")) - assert.Empty(assignedPolicies()) + // Deleting the peer must remove both its policies and default actions. + for _, dir := range []table.PolicyDirection{table.POLICY_DIRECTION_IMPORT, table.POLICY_DIRECTION_EXPORT} { + def, policies, err := s.policy.GetPolicyAssignment("127.0.0.1", dir) + require.NoError(t, err) + assert.Equal(table.ROUTE_TYPE_NONE, def) + assert.Empty(policies) + } + + // An unresolved policy now rejects the peer instead of silently leaving + // it without an assignment. + require.ErrorContains(t, addPeer("not-defined"), "not found policy not-defined") } // startServerWithPassivePeer starts a BgpServer without a TCP listener and