From 1cec6cc39352c96e59999f707308027e56bd1ad3 Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 12 Oct 2026 20:06:18 +0100 Subject: [PATCH] Fix "update PTR": add a missing PTR, also in linked views Ticking update PTR alone did nothing: the web UI didn't send it, and the engine only acted when the name or address changed. Alone it now adds a missing PTR record (or enables it); for linked zones every linked view's reverse zone counts. Co-Authored-By: Claude Opus 5.5 --- docs/cli.md | 4 +- internal/engine/expand.go | 82 ++++++++++++++++++++---- internal/engine/inputs.go | 4 +- internal/engine/ptr_update_test.go | 53 +++++++++++++++ internal/web/static/assets/pages/zone.js | 6 +- scripts/e2e.sh | 9 +++ 6 files changed, 141 insertions(+), 17 deletions(-) create mode 100644 internal/engine/ptr_update_test.go diff --git a/docs/cli.md b/docs/cli.md index 59fc7b2..5c7a1db 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -307,7 +307,9 @@ adds the quotes and splits long values into 255-byte strings. In reverse zones t host name (a bare `nas` is refused, since it would mean `nas.0.10.10.in-addr.arpa.`). `--ptr` keeps the PTR record in the reverse zone in step (when that zone is managed -by Jarl). Every record type BIND supports can be used. Jarl knows the +by Jarl); `jarl record update ID --ptr` with nothing else adds the PTR record +if it is missing, e.g. after creating the reverse zone. In linked zones, the +reverse zones of every linked view count. Every record type BIND supports can be used. Jarl knows the syntax of the common ones and checks them strictly, and the rest are validated by parsing. diff --git a/internal/engine/expand.go b/internal/engine/expand.go index 63ae2fe..924da2d 100644 --- a/internal/engine/expand.go +++ b/internal/engine/expand.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "net/netip" + "slices" "strings" "time" @@ -642,11 +643,42 @@ func (x *expander) ptrCreate(z *model.Zone, owner string, p *rr.Parsed) ([]model if !ok { return nil, errf(KindBadRequest, "ptr only applies to A and AAAA records") } - rz, ptrName := reverseZoneFor(x.st, z.ViewID, addr) - if rz == nil { - x.warnings = append(x.warnings, fmt.Sprintf("no reverse zone for %s in this view, so no PTR record was added", addr)) - return nil, nil + var ops []model.Op + found := false + for _, vid := range ptrViews(x.st, z) { + rz, ptrName := reverseZoneFor(x.st, vid, addr) + if rz == nil { + continue + } + found = true + more, err := x.ptrCreateIn(rz, ptrName, owner) + if err != nil { + return nil, err + } + ops = append(ops, more...) } + if !found { + x.warnings = append(x.warnings, fmt.Sprintf("no reverse zone for %s in this view, so no PTR record was added (create one: jarl zone create %s/%d)", addr, addr, addr.BitLen())) + } + return ops, nil +} + +// ptrViews are the views whose reverse zones get the PTR records of zone +// z's address records: z's own, and, for a linked zone, the other views +// of the link (the record is in all of them). +func ptrViews(st *model.State, z *model.Zone) []int64 { + out := []int64{z.ViewID} + for _, o := range st.LinkedWith(z) { + if !slices.Contains(out, o.ViewID) { + out = append(out, o.ViewID) + } + } + return out +} + +// ptrCreateIn returns operations creating the PTR record ptrName -> owner +// in reverse zone rz (none when it is there; enabling it when disabled). +func (x *expander) ptrCreateIn(rz *model.Zone, ptrName, owner string) ([]model.Op, error) { for _, r := range x.st.ZoneRecords(rz.ID) { if r.Name == ptrName && r.Type == "PTR" { if strings.EqualFold(r.Data, owner) { @@ -674,10 +706,18 @@ func (x *expander) ptrSetDisabled(z *model.Zone, owner string, p *rr.Parsed, dis if !ok { return nil } - rz, ptrName := reverseZoneFor(x.st, z.ViewID, addr) - if rz == nil { - return nil + var ops []model.Op + for _, vid := range ptrViews(x.st, z) { + rz, ptrName := reverseZoneFor(x.st, vid, addr) + if rz == nil { + continue + } + ops = append(ops, x.ptrSetDisabledIn(rz, ptrName, owner, disabled)...) } + return ops +} + +func (x *expander) ptrSetDisabledIn(rz *model.Zone, ptrName, owner string, disabled bool) []model.Op { var ops []model.Op for _, r := range x.st.ZoneRecords(rz.ID) { if r.Name == ptrName && r.Type == "PTR" && strings.EqualFold(r.Data, owner) && r.Disabled != disabled { @@ -700,12 +740,16 @@ func (x *expander) ptrDelete(z *model.Zone, owner string, p *rr.Parsed) []model. if !ok { return nil } - rz, ptrName := reverseZoneFor(x.st, z.ViewID, addr) - if rz == nil { - return nil + var ops []model.Op + for _, vid := range ptrViews(x.st, z) { + rz, ptrName := reverseZoneFor(x.st, vid, addr) + if rz == nil { + continue + } + in := RecordInput{ZoneID: rz.ID, Name: model.Ptr(ptrName), Type: model.Ptr("PTR"), Data: model.Ptr(owner), MissingOK: true} + ops = append(ops, newOp(OpRecordDelete, 0, 0, in, fmt.Sprintf("delete %s IN PTR %s (if present)", ptrName, owner))) } - in := RecordInput{ZoneID: rz.ID, Name: model.Ptr(ptrName), Type: model.Ptr("PTR"), Data: model.Ptr(owner), MissingOK: true} - return []model.Op{newOp(OpRecordDelete, 0, 0, in, fmt.Sprintf("delete %s IN PTR %s (if present)", ptrName, owner))} + return ops } // parseWarnings keeps what parsing noticed (e.g. a name read as absolute). @@ -885,7 +929,10 @@ func (x *expander) recordUpdate(req Request) ([]model.Op, error) { if len(pre) > 0 { base = 0 // the record was just updated by pre } - ops := append(pre, newOp(OpRecordUpdate, r.ID, base, in, summary)) + ops := pre + if string(mustJSON(in)) != "{}" || !ptr { + ops = append(ops, newOp(OpRecordUpdate, r.ID, base, in, summary)) + } disabled := r.Disabled if in.Disabled != nil { disabled = *in.Disabled @@ -909,6 +956,15 @@ func (x *expander) recordUpdate(req Request) ([]model.Op, error) { } ops = append(ops, x.ptrSetDisabled(z, owner, p, disabled)...) } + if ptr && !moved && !toggled && !disabled { + // Nothing else changes: make sure the PTR record is there (the + // reverse zone may be newer than the record). + more, err := x.ptrCreate(z, owner, p) + if err != nil { + return nil, err + } + ops = append(ops, more...) + } return ops, nil } diff --git a/internal/engine/inputs.go b/internal/engine/inputs.go index ac99c83..54b12a2 100644 --- a/internal/engine/inputs.go +++ b/internal/engine/inputs.go @@ -174,7 +174,9 @@ type RecordInput struct { // Disabled keeps the record in Jarl but out of the zone. Disabled *bool `json:"disabled,omitempty"` // PTR also creates (or deletes, disables or enables) the matching PTR - // record in the reverse zone, for A and AAAA records. + // record in the reverse zone, for A and AAAA records. On an update that + // changes nothing else it adds the PTR record if it is missing. In a + // linked zone, every linked view's reverse zone counts. PTR bool `json:"ptr,omitempty"` // MissingOK makes a delete-by-match succeed when nothing matches. MissingOK bool `json:"missing_ok,omitempty"` diff --git a/internal/engine/ptr_update_test.go b/internal/engine/ptr_update_test.go new file mode 100644 index 0000000..d2cbf17 --- /dev/null +++ b/internal/engine/ptr_update_test.go @@ -0,0 +1,53 @@ +package engine + +import ( + "slices" + "strings" + "testing" + + "jarl/internal/model" +) + +// Ticking "update PTR" on an unchanged address record adds its PTR when +// the reverse zone is there (it was created after the record). +func TestUpdatePTRAddsMissing(t *testing.T) { + env := newEnv(t) + env.submit(env.admin, NewRequest(OpSettingsUpdate, 0, SettingsInput{Nameservers: &[]string{"ns1.example.net."}})) + env.submit(env.admin, NewRequest(OpZoneCreate, 0, ZoneInput{Name: model.Ptr("example.com")})) + env.submit(env.admin, rec("example.com", "ldn", "A", "139.144.144.154")) + env.submit(env.admin, NewRequest(OpZoneCreate, 0, ZoneInput{Name: model.Ptr("139.144.144.154/32")})) + var id int64 + for _, r := range env.state().Records { + if r.Name == "ldn.example.com." { + id = r.ID + } + } + env.submit(env.admin, NewRequest(OpRecordUpdate, id, RecordInput{PTR: true})) + got := env.records("154.144.144.139.in-addr.arpa.") + if !slices.ContainsFunc(got, func(s string) bool { return strings.Contains(s, "PTR ldn.example.com.") }) { + t.Errorf("reverse zone = %q", got) + } +} + +// In linked zones the PTR goes to the reverse zone of whichever linked +// view has one. +func TestUpdatePTRLinked(t *testing.T) { + env := linkedEnv(t) + env.submit(env.admin, NewRequest(OpZoneCreate, 0, ZoneInput{Name: model.Ptr("example.com"), View: model.Ptr("external"), LinkWith: "internal"})) + env.submit(env.admin, recIn("internal", "ldn", "A", "139.144.144.154")) + env.submit(env.admin, NewRequest(OpZoneCreate, 0, ZoneInput{Name: model.Ptr("139.144.144.154/32"), View: model.Ptr("external")})) + res := env.submit(env.admin, NewRequest(OpRecordUpdate, env.recordID("internal", "ldn", "A"), RecordInput{PTR: true})) + st := env.state() + z := st.FindZone("154.144.144.139.in-addr.arpa.", st.ViewByName("external").ID) + ok := false + for _, r := range st.ZoneRecords(z.ID) { + ok = ok || r.Type == "PTR" && r.Data == "ldn.example.com." + } + if !ok { + t.Errorf("no PTR in external's reverse zone (warnings %q)", res.Warnings) + } + // Again: already there, nothing to do. + if e := env.submitErr(env.admin, NewRequest(OpRecordUpdate, env.recordID("internal", "ldn", "A"), RecordInput{PTR: true})); !strings.Contains(e.Message, "nothing to do") { + t.Errorf("second time: %v", e) + } +} diff --git a/internal/web/static/assets/pages/zone.js b/internal/web/static/assets/pages/zone.js index 31d189e..3885dd0 100644 --- a/internal/web/static/assets/pages/zone.js +++ b/internal/web/static/assets/pages/zone.js @@ -625,7 +625,7 @@ async function recordsTab(el, zone, query) { const eTTL = input({ type: "number", value: r.ttl ?? "", placeholder: "default", style: { width: "90px" } }); const eData = input({ value: r.type === "TXT" && r.text !== undefined ? r.text : r.data, class: "mono", style: { width: "100%" } }); const eComment = input({ value: r.comment || "", style: { width: "100%" } }); - const ePTR = checkbox("update PTR", false); + const ePTR = checkbox("update PTR", false, { title: "Keep the PTR record in the reverse zone in step: moved with a new name or address, or added if it is missing" }); const eErr = h("div"); const save = async () => { const body = {}; @@ -638,7 +638,9 @@ async function recordsTab(el, zone, query) { else if (eTTL.value !== "" && Number(eTTL.value) !== r.ttl) body.ttl = Number(eTTL.value); if (eComment.value !== (r.comment || "")) body.comment = eComment.value; if (ePTR.input.checked) body.ptr = true; - if (Object.keys(body).length === 0 || (Object.keys(body).length === 1 && body.ptr)) { editing = null; draw(); return; } + // "update PTR" alone adds a missing PTR record (the reverse zone may + // be newer than the record). + if (Object.keys(body).length === 0) { editing = null; draw(); return; } try { const res = await change("PATCH", "/records/" + r.id, body); toast(describeResult(res)); diff --git a/scripts/e2e.sh b/scripts/e2e.sh index e74f0d8..7b5e203 100755 --- a/scripts/e2e.sh +++ b/scripts/e2e.sh @@ -377,6 +377,15 @@ run "user delete" 0 $J user delete certbot -y run "audit" 0 $J audit --limit 10 contains "token.revoke" +# A reverse zone created after the record: "update PTR" alone adds it. +run "an address record" 0 $J record add adbraun.test ldn A 192.0.2.154 +run "its /32 reverse zone, afterwards" 0 $J zone create 192.0.2.154/32 +LDN=$($J record list adbraun.test --name ldn -o id) +run "record update --ptr adds the missing PTR" 0 $J record update $LDN --ptr +expect_dig "the PTR is served" "ldn.adbraun.test." -x 192.0.2.154 +run "remove the reverse zone again" 0 $J zone delete 154.2.0.192.in-addr.arpa -y +run "and the record" 0 $J record delete adbraun.test ldn A -y + # Views run "view create (adopts zones)" 0 $J view create internal --match-clients homelab --adopt-zones run "view create external" 0 $J view create external --match-clients any