From 786f992ef36b07960ed2e5aaa1491b9d2ea1e473 Mon Sep 17 00:00:00 2001 From: Blagovest Petrov Date: Tue, 10 Nov 2026 10:18:14 +0200 Subject: [PATCH] fix(import): handle bindings that already exist on re-import Importing a bundle a second time failed on every PolicyBinding document with a duplicate key error on (policy_id, group_id): only policies were compared with existing ones, bindings were always inserted. The dry run had the same blind spot and reported "would create". Existing bindings are now matched by policy and node group and follow the conflict mode: - new-version: set the priority from the file when it differs, report "updated"; report "skipped" when it is already the same. The enabled or disabled state is never changed. - skip: report "skipped". - error: fail during validation, so nothing is written. The dry run reports the same outcome as the import. This works for bindings that reference the policy by in-file name and by display name on the server. Also: - reject a bundle that binds the same policy to the same node group twice, which hit the same constraint - updating a binding through import needs binding:toggle on the group, like the bindings API - resync the group when the priority of an enabled binding changes - describe the binding behaviour in the user guide Fixes #209 Signed-off-by: Blagovest Petrov --- doc/user-guide/policies.md | 13 +++ server/internal/api/export.go | 41 ++++++++- server/internal/export/export_test.go | 115 +++++++++++++++++++++++++- server/internal/export/import.go | 86 ++++++++++++++++++- server/pkg/app/app.go | 3 + 5 files changed, 252 insertions(+), 6 deletions(-) diff --git a/doc/user-guide/policies.md b/doc/user-guide/policies.md index 762ef004..f70231de 100644 --- a/doc/user-guide/policies.md +++ b/doc/user-guide/policies.md @@ -302,6 +302,16 @@ The policy state is not exported. | **Skip that policy** | The existing policy is kept unchanged. | | **Update the existing draft** | The existing policy's content is replaced, but only if it is a draft; a released or archived policy is an error. | + The same option applies to a binding in the file whose policy and + node group are already bound on the server, which is the usual case + when a file is imported a second time: + + | Option | Effect on an existing binding | + |---|---| + | **Fail the import (recommended)** | The document is an error and nothing is imported. | + | **Skip that policy** | The existing binding is kept unchanged. | + | **Update the existing draft** | The priority is set to the one in the file. A binding that already has that priority is reported as `skipped`. | + Changing the option re-runs the preview. 4. If the preview is valid, select **Import N resources**. Otherwise fix the file; if any document has an error, nothing is imported. @@ -312,6 +322,9 @@ Rules: yourself. - Imported bindings are always created **disabled**. Enable them on [Policy Bindings](bindings.md) after releasing the policy. +- An import never enables or disables a binding that already exists; + only its priority can change. +- A file may bind a policy to a node group only once. - A binding's `spec.policy` refers to a `metadata.name` in the same file or to the name of a policy that already exists on the server. Its `spec.group` must be the name of an existing node group; node groups are diff --git a/server/internal/api/export.go b/server/internal/api/export.go index c693431c..4b0d7710 100644 --- a/server/internal/api/export.go +++ b/server/internal/api/export.go @@ -30,6 +30,9 @@ type ExportHandler struct { // az backs the systemd:exec check on import; nil refuses every // command-running Systemd policy in a bundle (fail closed). az authz.Authorizer + // OnBindingChange is called after an import changes the priority of an + // enabled binding, so the nodes of that group pick up the new order. + OnBindingChange func(b *models.PolicyBinding) } // NewExportHandler wires the export/import endpoints to their services. @@ -334,12 +337,48 @@ func (s scopedPolicyStore) UpdatePolicy(ctx context.Context, id string, req *mod } // scopedBindingStore only creates bindings to in-scope groups, for policies -// the caller can see. +// the caller can see. Existing bindings are matched among the ones the caller +// can view, and changing one needs the same reach as the bindings API. type scopedBindingStore struct { h *ExportHandler r *http.Request } +func (s scopedBindingStore) ListBindings(ctx context.Context) ([]*models.PolicyBindingWithDetails, error) { + all, err := s.h.bindingSvc.ListBindings(ctx) + if err != nil { + return nil, err + } + scope := requestScope(s.r, "binding", "view") + visible := make([]*models.PolicyBindingWithDetails, 0, len(all)) + for _, b := range all { + if scope.Allows(b.GroupID) { + visible = append(visible, b) + } + } + return visible, nil +} + +func (s scopedBindingStore) UpdateBinding(ctx context.Context, id string, req *models.UpdatePolicyBindingRequest) (*models.PolicyBinding, error) { + existing, err := s.h.bindingSvc.GetBinding(ctx, id) + if err != nil || existing == nil || !requestScope(s.r, "binding", "view").Allows(existing.GroupID) { + return nil, fmt.Errorf("policy binding not found") + } + if !requestScope(s.r, "binding", "toggle").Allows(existing.GroupID) { + return nil, errOutOfScope + } + updated, err := s.h.bindingSvc.UpdateBinding(ctx, id, req) + if err != nil { + return nil, err + } + // A disabled binding delivers nothing, so its priority is not yet + // visible to any node. + if s.h.OnBindingChange != nil && updated != nil && updated.State == models.BindingStateEnabled { + s.h.OnBindingChange(updated) + } + return updated, nil +} + func (s scopedBindingStore) CreateBinding(ctx context.Context, req *models.CreatePolicyBindingRequest) (*models.PolicyBinding, error) { if !requestScope(s.r, "binding", "create").Allows(req.GroupID) { return nil, errOutOfScope diff --git a/server/internal/export/export_test.go b/server/internal/export/export_test.go index ae0c5f3d..25970c15 100644 --- a/server/internal/export/export_test.go +++ b/server/internal/export/export_test.go @@ -188,7 +188,20 @@ func (f *fakePolicies) UpdatePolicy(_ context.Context, id string, req *models.Up } type fakeBindings struct { - created []*models.CreatePolicyBindingRequest + existing []*models.PolicyBindingWithDetails + created []*models.CreatePolicyBindingRequest + updated map[string]*models.UpdatePolicyBindingRequest +} + +func (f *fakeBindings) ListBindings(_ context.Context) ([]*models.PolicyBindingWithDetails, error) { + return f.existing, nil +} +func (f *fakeBindings) UpdateBinding(_ context.Context, id string, req *models.UpdatePolicyBindingRequest) (*models.PolicyBinding, error) { + if f.updated == nil { + f.updated = map[string]*models.UpdatePolicyBindingRequest{} + } + f.updated[id] = req + return &models.PolicyBinding{ID: id}, nil } func (f *fakeBindings) CreateBinding(_ context.Context, req *models.CreatePolicyBindingRequest) (*models.PolicyBinding, error) { @@ -307,6 +320,106 @@ func TestImportConflictModes(t *testing.T) { } } +// A bundle that is imported again meets the bindings it created the first +// time; they follow the conflict mode instead of being inserted twice. +func TestImportExistingBinding(t *testing.T) { + nameRefBundle := `apiVersion: bor.dev/v1 +kind: PolicyBinding +metadata: + name: firefox-eng +spec: + policy: "Firefox Baseline" + group: "Engineering" + priority: 10 +` + tests := []struct { + name string + bundle string + onConflict string + priority int // priority of the binding already on the server + wantOk bool + wantStatus string // status of the PolicyBinding document + wantMessage string // substring of its message + wantPriority int // priority written by UpdateBinding; 0 = no update + }{ + {name: "new-version updates a different priority", bundle: importBundle, onConflict: OnConflictNewVersion, priority: 50, wantOk: true, wantStatus: "updated", wantMessage: "from 50 to 10", wantPriority: 10}, + {name: "new-version leaves an equal priority", bundle: importBundle, onConflict: OnConflictNewVersion, priority: 10, wantOk: true, wantStatus: "skipped", wantMessage: "same priority"}, + {name: "skip leaves the binding", bundle: importBundle, onConflict: OnConflictSkip, priority: 50, wantOk: true, wantStatus: "skipped", wantMessage: "binding exists"}, + {name: "error rejects the bundle", bundle: nameRefBundle, onConflict: OnConflictError, priority: 50, wantOk: false, wantStatus: "error", wantMessage: "already exists"}, + {name: "new-version by policy name", bundle: nameRefBundle, onConflict: OnConflictNewVersion, priority: 50, wantOk: true, wantStatus: "updated", wantMessage: "from 50 to 10", wantPriority: 10}, + {name: "skip by policy name", bundle: nameRefBundle, onConflict: OnConflictSkip, priority: 50, wantOk: true, wantStatus: "skipped", wantMessage: "binding exists"}, + } + for _, tc := range tests { + for _, dryRun := range []bool{true, false} { + t.Run(fmt.Sprintf("%s/dry_run=%t", tc.name, dryRun), func(t *testing.T) { + fp, fb, fg := importFixture() + fp.existing = []*models.Policy{{ID: "p1", Name: "Firefox Baseline", Type: "Firefox", State: "draft"}} + fb.existing = []*models.PolicyBindingWithDetails{ + {PolicyBinding: models.PolicyBinding{ID: "b1", PolicyID: "p1", GroupID: "g1", State: models.BindingStateEnabled, Priority: tc.priority}}, + // Same group, other policy: must not be mistaken for a match. + {PolicyBinding: models.PolicyBinding{ID: "b2", PolicyID: "p2", GroupID: "g1", Priority: 1}}, + } + report, err := Import(context.Background(), fp, fb, fg, nil, []byte(tc.bundle), Options{DryRun: dryRun, OnConflict: tc.onConflict}) + if err != nil { + t.Fatal(err) + } + if report.Ok != tc.wantOk { + t.Fatalf("ok = %t, want %t: %+v", report.Ok, tc.wantOk, report) + } + var got *DocResult + for i := range report.Results { + if report.Results[i].Kind == "PolicyBinding" { + got = &report.Results[i] + } + } + if got == nil || got.Status != tc.wantStatus || !strings.Contains(got.Message, tc.wantMessage) { + t.Fatalf("binding result = %+v, want status %q with %q", got, tc.wantStatus, tc.wantMessage) + } + if len(fb.created) != 0 { + t.Fatalf("existing binding inserted again: %+v", fb.created) + } + wantUpdates := 0 + if tc.wantPriority != 0 && !dryRun { + wantUpdates = 1 + } + if len(fb.updated) != wantUpdates { + t.Fatalf("got %d binding updates, want %d", len(fb.updated), wantUpdates) + } + if wantUpdates == 1 { + req := fb.updated["b1"] + if req == nil || req.Priority == nil || *req.Priority != tc.wantPriority { + t.Fatalf("priority not updated on b1: %+v", fb.updated) + } + if req.State != nil { + t.Fatalf("import must not change the binding state: %v", *req.State) + } + } + }) + } + } +} + +func TestImportDuplicateBindingInBundle(t *testing.T) { + bundle := importBundle + `--- +apiVersion: bor.dev/v1 +kind: PolicyBinding +metadata: + name: firefox-eng-again +spec: + policy: firefox-baseline + group: "Engineering" + priority: 20 +` + fp, fb, fg := importFixture() + report, err := Import(context.Background(), fp, fb, fg, nil, []byte(bundle), Options{}) + if err != nil { + t.Fatal(err) + } + if report.Ok || report.Errors != 1 || len(fp.created) != 0 || len(fb.created) != 0 { + t.Fatalf("duplicate binding must reject the bundle: %+v", report) + } +} + func TestImportMissingGroupRejectsBundle(t *testing.T) { fp, fb, fg := importFixture() fg.groups = nil diff --git a/server/internal/export/import.go b/server/internal/export/import.go index e622db83..694e5459 100644 --- a/server/internal/export/import.go +++ b/server/internal/export/import.go @@ -23,6 +23,8 @@ type PolicyStore interface { // BindingStore is the slice of PolicyBindingService the importer needs. type BindingStore interface { + ListBindings(ctx context.Context) ([]*models.PolicyBindingWithDetails, error) + UpdateBinding(ctx context.Context, id string, req *models.UpdatePolicyBindingRequest) (*models.PolicyBinding, error) CreateBinding(ctx context.Context, req *models.CreatePolicyBindingRequest) (*models.PolicyBinding, error) } @@ -31,7 +33,8 @@ type GroupStore interface { ListNodeGroups(ctx context.Context) ([]*models.NodeGroup, error) } -// Conflict handling for policies whose display name already exists. +// Conflict handling for policies whose display name already exists and for +// bindings whose policy and node group are already bound. const ( OnConflictError = "error" OnConflictSkip = "skip" @@ -88,8 +91,14 @@ type pendingBinding struct { priority int policySlug bool // ref resolved against an in-file slug policyID string + existing *models.PolicyBinding // binding already on the server for this policy and group + skip bool // leave the existing binding untouched + skipReason string } +// bindingKey identifies a binding by the pair the database keeps unique. +type bindingKey struct{ policyID, groupID string } + // Import validates the whole bundle first and creates resources only when // every document is valid ("validate all, then execute"): nothing is written // for a bundle containing any invalid document. Execution-stage failures @@ -131,11 +140,20 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr for _, g := range groupList { groupByName[g.Name] = g.ID } + bindingList, err := bindings.ListBindings(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list policy bindings: %w", err) + } + bindingByKey := make(map[bindingKey]*models.PolicyBinding, len(bindingList)) + for _, eb := range bindingList { + bindingByKey[bindingKey{eb.PolicyID, eb.GroupID}] = &eb.PolicyBinding + } // ── Pass 1: parse + validate every document ── var pendingPolicies []*pendingPolicy var pendingBindings []*pendingBinding slugs := make(map[string]*pendingPolicy) + bindingsInFile := make(map[bindingKey]bool) namesInFile := make(map[string]bool) for i, doc := range docs { @@ -238,14 +256,47 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr continue } b.groupID = gid - if _, ok := slugs[b.policyRef]; ok { + // existingPolicyID is the server-side policy the binding will + // attach to, when there is one already: a policy that is updated + // or skipped keeps its ID, a policy the bundle creates has none. + existingPolicyID := "" + if p, ok := slugs[b.policyRef]; ok { b.policySlug = true + if prev, exists := byName[p.name]; exists { + existingPolicyID = prev.ID + } } else if prev, ok := byName[b.policyRef]; ok { b.policyID = prev.ID + existingPolicyID = prev.ID } else { fail(n, res.Kind, name, fmt.Sprintf("policy %q not found in bundle or on this server", b.policyRef)) continue } + fileKey := bindingKey{existingPolicyID, gid} + if existingPolicyID == "" { + fileKey.policyID = "slug:" + b.policyRef + } + if bindingsInFile[fileKey] { + fail(n, res.Kind, name, "duplicate binding for this policy and node group in bundle") + continue + } + bindingsInFile[fileKey] = true + if prev, ok := bindingByKey[bindingKey{existingPolicyID, gid}]; ok && existingPolicyID != "" { + switch opts.OnConflict { + case OnConflictError: + fail(n, res.Kind, name, "a binding for this policy and node group already exists (on_conflict=error)") + continue + case OnConflictSkip: + b.skip = true + b.skipReason = "binding exists" + case OnConflictNewVersion: + b.existing = prev + if prev.Priority == b.priority { + b.skip = true + b.skipReason = "binding exists with the same priority" + } + } + } pendingBindings = append(pendingBindings, b) } } @@ -272,8 +323,18 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr } } for _, b := range pendingBindings { - report.Created++ - report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "created", Message: "would create"}) + switch { + case b.skip: + report.Skipped++ + report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "skipped", Message: b.skipReason}) + case b.existing != nil: + report.Updated++ + report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "updated", + Message: fmt.Sprintf("would change priority from %d to %d", b.existing.Priority, b.priority)}) + default: + report.Created++ + report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "created", Message: "would create"}) + } } report.Ok = true return report, nil @@ -324,6 +385,23 @@ func Import(ctx context.Context, policies PolicyStore, bindings BindingStore, gr } } for _, b := range pendingBindings { + if b.skip { + report.Skipped++ + report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "skipped", Message: b.skipReason}) + continue + } + if b.existing != nil { + // Only the priority follows the bundle; whether the binding is + // enabled stays an explicit decision made on the server. + if _, err := bindings.UpdateBinding(ctx, b.existing.ID, &models.UpdatePolicyBindingRequest{Priority: &b.priority}); err != nil { + fail(b.doc, "PolicyBinding", b.name, err.Error()) + continue + } + report.Updated++ + report.Results = append(report.Results, DocResult{Doc: b.doc, Kind: "PolicyBinding", Name: b.name, Status: "updated", + Message: fmt.Sprintf("priority changed from %d to %d", b.existing.Priority, b.priority)}) + continue + } id := b.policyID if b.policySlug { id = slugToID[b.policyRef] diff --git a/server/pkg/app/app.go b/server/pkg/app/app.go index cf9710fd..47875ed6 100644 --- a/server/pkg/app/app.go +++ b/server/pkg/app/app.go @@ -585,6 +585,9 @@ func Run(opts Options) { // import creates drafts and needs create. Both are audit-logged by the // handler itself (exports are GETs, which the audit middleware skips). exportHandler := api.NewExportHandler(policySvc, policyBindingSvc, nodeGroupSvc, auditSvc).WithAuthorizer(az) + exportHandler.OnBindingChange = func(b *models.PolicyBinding) { + policyHub.PublishResync(b.GroupID) + } mux.Handle("/api/v1/policies/export", authMiddleware(gate.RequireScoped("policy", "view")(http.HandlerFunc(exportHandler.Export)))) mux.Handle("/api/v1/policies/import", authMiddleware(gate.RequireScoped("policy", "create")(http.HandlerFunc(exportHandler.Import))))