From 05c811bddcf27abaa85a6b04f8d153efecf29dc0 Mon Sep 17 00:00:00 2001 From: abrown38 Date: Tue, 20 Oct 2026 21:18:37 +0100 Subject: [PATCH] Rules: placed in the same change they are created in hc rule add --before/--after/--first created the rule at the end of its chain and then moved it: two history entries, two probations, the rule briefly in force in the wrong place, and in a changeset the place was lost. POST /rules (and the other ordered kinds) now takes ?before=ID, ?after=ID or ?first=1, kept with the operation when staged. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++++ cmd/hc/rules.go | 32 +++++++++++------------ docs/api.md | 2 +- internal/model/changeset.go | 5 +++- internal/server/h_config.go | 15 ++++++++++- internal/server/ops.go | 12 ++++++++- internal/server/server_test.go | 46 ++++++++++++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9c03eb..33079b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,11 @@ All notable changes to Housecarl are documented here. The format follows ### Fixed +- `hc rule add --before/--after/--first` made two changes (create at the + end, then move), so two history entries, two probations, and the rule + briefly in force at the end of its chain; staged in a changeset, the + place was lost. The create now takes the place (`POST + /rules?before=ID`, `after`, `first`) and makes one change. - `hc matrix` in a terminal: the colours' escape codes threw the columns out of line. - `hc counters` (`GET /counters`) kept listing the objects of a diff --git a/cmd/hc/rules.go b/cmd/hc/rules.go index 8be50a1..860ff21 100644 --- a/cmd/hc/rules.go +++ b/cmd/hc/rules.go @@ -3,6 +3,7 @@ package main import ( "fmt" "io" + "net/url" "regexp" "strconv" "strings" @@ -340,25 +341,20 @@ output (from it) or early (before connection tracking: cheap drops). if err != nil { return err } - var resp api.MutationResponse - if err := c.Do(ctx(), "POST", "/rules", nil, r, &resp); err != nil { - return err + // Placed in the same change: one history entry, and never in + // force at the end of the chain. + q := url.Values{} + switch { + case rf.first: + q.Set("first", "1") + case rf.before != 0: + q.Set("before", fmt.Sprint(rf.before)) + case rf.after != 0: + q.Set("after", fmt.Sprint(rf.after)) } - // Then place it. - if resp.Applied && (rf.first || rf.before != 0 || rf.after != 0) { - var created model.Rule - if err := jsonUnmarshal(resp.Object, &created); err == nil { - mv := api.MoveInput{Before: rf.before, After: rf.after} - if rf.first { - mv.To = "start" - } - var moved api.MutationResponse - if err := c.Do(ctx(), "POST", objPath("rules", fmt.Sprint(created.ID))+"/move", nil, mv, &moved); err != nil { - return err - } - resp.Changes = append(resp.Changes, moved.Changes...) - resp.ConfirmBy = moved.ConfirmBy - } + var resp api.MutationResponse + if err := c.Do(ctx(), "POST", "/rules", q, r, &resp); err != nil { + return err } return report(&resp) }, diff --git a/docs/api.md b/docs/api.md index 33c6d49..9e2490a 100644 --- a/docs/api.md +++ b/docs/api.md @@ -114,7 +114,7 @@ share one pattern, `/{kind}` with kind `zones`, `sets`, `services`, | | | |---|---| | `GET /{kind}` | all of them, in order; `GET /rules?chain=input` for one chain; `GET /services` includes the built-in ones | -| `POST /{kind}` | create; rules, forwards and NAT rules go last unless given a `position` | +| `POST /{kind}[?before=ID\|after=ID\|first=1]` | create; rules, forwards and NAT rules go last unless given a `position`, or placed before or after another of their chain, or first, in the same change (one history entry; staged in a changeset, the place goes with it) | | `GET /{kind}/{ref}` | by ID, or name for zones, sets, services and forwards | | `PATCH /{kind}/{ref}` | change the fields given (a JSON merge: `null` clears) | | `DELETE /{kind}/{ref}` | refused while something uses it | diff --git a/internal/model/changeset.go b/internal/model/changeset.go index 7632734..0f0b059 100644 --- a/internal/model/changeset.go +++ b/internal/model/changeset.go @@ -53,7 +53,10 @@ type Op struct { ID int64 `json:"id,omitempty"` // Data is the object (create), the fields to change (update, // settings) or where to move it (move). - Data json.RawMessage `json:"data,omitempty"` + Data json.RawMessage `json:"data,omitempty"` + // Place is where a create puts the object in its order (a move's + // data); none: at the end. + Place json.RawMessage `json:"place,omitempty"` Summary string `json:"summary"` AddedAt time.Time `json:"added_at"` } diff --git a/internal/server/h_config.go b/internal/server/h_config.go index 1abfcb5..b92a2c0 100644 --- a/internal/server/h_config.go +++ b/internal/server/h_config.go @@ -6,6 +6,7 @@ import ( "net/http" "net/netip" "sort" + "strconv" "strings" udiff "github.com/aymanbagabas/go-udiff" @@ -310,7 +311,19 @@ func (s *Server) createObject(w http.ResponseWriter, r *http.Request, p *auth.Pr if _, err := decodeObject(kind, raw); err != nil { return err } - return s.change(w, r, p, model.Op{Op: model.OpCreate, Kind: kind, Data: raw}) + op := model.Op{Op: model.OpCreate, Kind: kind, Data: raw} + // ?before=ID, ?after=ID or ?first=1: where it goes in its order. + q := r.URL.Query() + var place api.MoveInput + place.Before, _ = strconv.ParseInt(q.Get("before"), 10, 64) + place.After, _ = strconv.ParseInt(q.Get("after"), 10, 64) + if queryBool(r, "first") { + place.To = "start" + } + if place.Before != 0 || place.After != 0 || place.To != "" { + op.Place, _ = json.Marshal(place) + } + return s.change(w, r, p, op) } func (s *Server) updateObject(w http.ResponseWriter, r *http.Request, p *auth.Principal) error { diff --git a/internal/server/ops.go b/internal/server/ops.go index 2770344..687180c 100644 --- a/internal/server/ops.go +++ b/internal/server/ops.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "sort" + "strconv" "strings" "housecarl/internal/api" @@ -63,7 +64,16 @@ func createOp(st *model.State, op *model.Op) (int64, error) { *pos = lastPosition(st, op.Kind, chainOf(o)) } data, _ := json.Marshal(o) - return op.ID, st.Put(op.Kind, op.ID, data) + if err := st.Put(op.Kind, op.ID, data); err != nil { + return 0, err + } + // Placed in the same change, so it is never in force anywhere else. + if len(op.Place) > 0 { + if _, err := moveOp(st, &model.Op{Op: model.OpMove, Kind: op.Kind, Ref: strconv.FormatInt(op.ID, 10), Data: op.Place}); err != nil { + return 0, err + } + } + return op.ID, nil } func updateOp(st *model.State, op *model.Op) (int64, error) { diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 669492f..eda5033 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1184,3 +1184,49 @@ func (e *env) text(path string) string { b, _ := io.ReadAll(resp.Body) return string(b) } + +// TestCreatePlaced: a rule created ?before= another is placed in the same +// change (one history entry), also when staged in a changeset. +func TestCreatePlaced(t *testing.T) { + e := newEnv(t) + var a, b, c api.MutationResponse + e.call(e.token, "POST", "/rules", model.Rule{Chain: "input", Match: model.Match{Services: []string{"ssh"}}}, &a) + e.call(e.token, "POST", "/rules", model.Rule{Chain: "input", Match: model.Match{Services: []string{"dns"}}}, &b) + var first model.Rule + json.Unmarshal(a.Object, &first) + if code := e.call(e.token, "POST", "/rules?before="+itoa(first.ID), model.Rule{Chain: "input", Match: model.Match{Services: []string{"http"}}, Comment: "placed"}, &c); code != 201 { + t.Fatalf("create placed: %d %+v", code, c) + } + order := func() string { + var out []string + for _, r := range e.eng.State().Rules { + out = append(out, strings.Join(r.Services, "")) + } + return strings.Join(out, ",") + } + if o := order(); o != "http,ssh,dns" { + t.Errorf("order: %s", o) + } + var hs []model.HistoryEntry + e.call(e.token, "GET", "/history", nil, &hs) + if len(hs) != 3 || len(hs[0].Changes) != 1 || hs[0].Changes[0].Action() != "create" { + t.Errorf("history: %d entries, newest %+v", len(hs), hs[0]) + } + // Staged, the place goes with it. + var cs model.Changeset + e.call(e.token, "POST", "/changesets", nil, &cs) + e.call(e.token, "POST", "/rules?first=1&changeset="+itoa(cs.ID), model.Rule{Chain: "input", Match: model.Match{Services: []string{"ntp"}}}, nil) + if code := e.call(e.token, "POST", "/changesets/"+itoa(cs.ID)+"/apply", nil, nil); code != 200 { + t.Fatalf("apply: %d", code) + } + if o := order(); o != "ntp,http,ssh,dns" { + t.Errorf("order after the changeset: %s", o) + } + // A bad place: refused, nothing created. + if code := e.call(e.token, "POST", "/rules?before=99999", model.Rule{Chain: "input"}, nil); code != 400 { + t.Errorf("before a rule that isn't there: %d", code) + } + if n := len(e.eng.State().Rules); n != 4 { + t.Errorf("%d rules", n) + } +}