Cluster UI: Blocklists management, locked config-file items, read-only upstreams; honour allow_config_write

The console now works as the cluster's editor, without the monitoring pages.

Web: Blocklists gets Add/Edit/Remove for BlockList objects (and shows ones the
resolver has not compiled yet); Clients marks cluster groups and locks config-file
groups and the default policy; Upstreams becomes a read-only health view when the
config cannot be written; Zones and Domains lose the managed-directory and list
copy; the Local zones page and its API calls are removed; nav is Status,
Blocklists, Domains, Lookup, Address redirects, Zones, Clients, Upstreams, Settings.

Console: a change to a group the resolver's config file defines is refused with the
reason instead of being skipped silently, and /api/clients says what the console
can and cannot edit.

Server: isFileWritable called a read-only existing file (a mounted ConfigMap)
writable, and api.allow_config_write was not reflected in the writable flag nor
checked by the upstream/ACL edit path. Both fixed; tests pin them.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

internal/api/api.go

@@ -615,23 +615,32 @@ func queryTime(r *http.Request, key string) time.Time {
 	return time.Time{}
 }
 
+// isFileWritable reports whether the config file at path can be rewritten. A file
+// that exists and refuses a write (a ConfigMap mounted into a pod, a read-only
+// bind mount) is not writable: only a file that does not exist yet counts as
+// writable, and then only when its directory accepts a new file.
 func isFileWritable(path string) bool {
 	if path == "" {
 		return false
 	}
 	f, err := os.OpenFile(path, os.O_WRONLY, 0)
-	if err != nil {
-		// A missing file in a writable directory is still writable.
-		dir := path
-		if i := strings.LastIndexByte(dir, '/'); i > 0 {
-			dir = dir[:i]
-		}
-		d, derr := os.Stat(dir)
-		if derr != nil {
-			return false
-		}
-		return d.IsDir()
+	if err == nil {
+		_ = f.Close()
+		return true
 	}
-	_ = f.Close()
+	if !os.IsNotExist(err) {
+		return false
+	}
+	dir := "."
+	if i := strings.LastIndexByte(path, '/'); i > 0 {
+		dir = path[:i]
+	}
+	probe, err := os.CreateTemp(dir, ".fancy-dns-writable-*")
+	if err != nil {
+		return false
+	}
+	name := probe.Name()
+	_ = probe.Close()
+	_ = os.Remove(name)
 	return true
 }

internal/api/handlers.go

@@ -48,7 +48,7 @@ func (s *Server) handleStatus(w http.ResponseWriter, _ *http.Request) {
 			"generation":   s.hooks.Store.Generation(),
 			"path":         s.hooks.Store.Path(),
 			"last_error":   s.hooks.Store.LastError(),
-			"writable":     isFileWritable(s.hooks.Store.Path()),
+			"writable":     s.configWritable(),
 			"reload_ready": s.hooks.Store.Path() != "",
 		},
 		"components": s.hooks.Status(),
@@ -460,7 +460,7 @@ func (s *Server) handleSettingsGet(w http.ResponseWriter, _ *http.Request) {
 		"defaults_yaml":    string(defaults),
 		"last_error":       s.hooks.Store.LastError(),
 		"reload_available": path != "",
-		"writable":         isFileWritable(path),
+		"writable":         s.configWritable(),
 	})
 }
 
@@ -555,12 +555,22 @@ func (s *Server) handleSettingsReload(w http.ResponseWriter, _ *http.Request) {
 	writeJSON(w, http.StatusOK, map[string]any{"ok": true, "version": s.hooks.Store.Get().Version})
 }
 
+// configWritable is whether an edit to the config can land: the operator allows
+// it (api.allow_config_write) and the file accepts a write. The console uses it
+// to decide between an editor and a read-only view.
+func (s *Server) configWritable() bool {
+	return s.hooks.Store.Get().API.AllowConfigWrite && isFileWritable(s.hooks.Store.Path())
+}
+
 // commit writes the config to disk, swaps it in and notifies the cluster.
 func (s *Server) commit(next *config.Config) error {
 	path := s.hooks.Store.Path()
 	if path == "" {
 		return fmt.Errorf("cannot persist: no config file path is known")
 	}
+	if !s.hooks.Store.Get().API.AllowConfigWrite {
+		return fmt.Errorf("cannot persist: config writes are disabled (api.allow_config_write is false)")
+	}
 	if !isFileWritable(path) {
 		return fmt.Errorf("cannot persist: %s is not writable", path)
 	}

internal/api/writable_test.go

@@ -0,0 +1,51 @@
+package api
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// A config file that exists but refuses a write is not writable. This is what a
+// ConfigMap mounted into a pod looks like, and calling it writable made the
+// console offer an editor whose every save failed.
+func TestIsFileWritable(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root can write a read-only file")
+	}
+	dir := t.TempDir()
+
+	rw := filepath.Join(dir, "rw.yaml")
+	if err := os.WriteFile(rw, []byte("x"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	ro := filepath.Join(dir, "ro.yaml")
+	if err := os.WriteFile(ro, []byte("x"), 0o444); err != nil {
+		t.Fatal(err)
+	}
+	cases := map[string]struct {
+		path string
+		want bool
+	}{
+		"writable file":          {rw, true},
+		"read-only file":         {ro, false},
+		"missing file, open dir": {filepath.Join(dir, "new.yaml"), true},
+		"missing file, no dir":   {filepath.Join(dir, "nodir", "x.yaml"), false},
+		"no path":                {"", false},
+	}
+	for name, tc := range cases {
+		if got := isFileWritable(tc.path); got != tc.want {
+			t.Errorf("%s: isFileWritable(%q) = %v, want %v", name, tc.path, got, tc.want)
+		}
+	}
+
+	// A directory that refuses new files makes a missing file unwritable too.
+	locked := filepath.Join(dir, "locked")
+	if err := os.Mkdir(locked, 0o555); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(locked, 0o755) })
+	if isFileWritable(filepath.Join(locked, "x.yaml")) {
+		t.Error("a file in a directory that refuses new files was reported writable")
+	}
+}

internal/app/console_e2e_test.go

@@ -1,6 +1,7 @@
 package app
 
 import (
+	"context"
 	"encoding/json"
 	"io"
 	"net/http"
@@ -10,10 +11,13 @@ import (
 	"time"
 
 	"github.com/miekg/dns"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/dynamic"
 
 	"git.k8s-git-k6n.net/mats/fancy-dns/internal/config"
 	"git.k8s-git-k6n.net/mats/fancy-dns/internal/console"
 	"git.k8s-git-k6n.net/mats/fancy-dns/internal/kubeedit"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/kubesource"
 )
 
 // TestConsoleEditsReachTheResolver runs both halves: a console with its own
@@ -22,34 +26,15 @@ import (
 // which is the whole deployment contract.
 func TestConsoleEditsReachTheResolver(t *testing.T) {
 	cluster := kubeClient()
-	h := newHarness(t, nil, func(o *Options) {
+	h := newHarness(t, func(c *config.Config) {
+		// A group the resolver's own config file defines, which the console must
+		// show but not rewrite.
+		c.ACL.Groups = []config.Group{{Name: "from-file", Policy: "allow", Match: config.Match{CIDRs: []string{"198.51.100.0/24"}}}}
+	}, func(o *Options) {
 		o.Kube = KubeOptions{Enabled: true, Namespace: "dns", Client: cluster, SyncTimeout: 5 * time.Second}
 	})
 
-	rt, err := console.NewRuntime("http://"+h.api, h.key)
-	if err != nil {
-		t.Fatal(err)
-	}
-	cfg := config.Default()
-	cfg.API.Auth.Mode = "apikey"
-	cfg.API.Auth.APIKeys = []string{"console-key"}
-	cfg.API.Auth.TrustLoopback = false
-	cfg.API.UI = false
-	c, err := console.New(console.Options{
-		Config:  config.NewStore(cfg, ""),
-		Runtime: rt,
-		Editor:  kubeedit.New(cluster, "dns"),
-		Version: "test",
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	handler, err := c.Handler()
-	if err != nil {
-		t.Fatal(err)
-	}
-	ts := httptest.NewServer(handler)
-	t.Cleanup(ts.Close)
+	ts := startConsole(t, h, cluster)
 
 	do := func(method, path, body, key string) (int, string) {
 		t.Helper()
@@ -88,14 +73,40 @@ func TestConsoleEditsReachTheResolver(t *testing.T) {
 		t.Fatalf("proxied status = %v (%v)", status, err)
 	}
 
+	// The config file's group is listed, marked as not editable here, and a
+	// change to it is refused with the reason rather than quietly ignored.
+	var pre struct {
+		Groups  []struct{ Name, Source string } `json:"groups"`
+		Default *bool                           `json:"default_policy_editable"`
+		Files   *bool                           `json:"file_groups_editable"`
+	}
+	if err := json.Unmarshal([]byte(ok("GET", "/api/clients", "")), &pre); err != nil || len(pre.Groups) != 1 || pre.Groups[0].Name != "from-file" || pre.Groups[0].Source != "" {
+		t.Fatalf("clients before any edit = %+v (%v)", pre, err)
+	}
+	if pre.Default == nil || *pre.Default || pre.Files == nil || *pre.Files {
+		t.Errorf("the console must say what it cannot edit: %+v", pre)
+	}
+	if code, out := do("PUT", "/api/clients", `{"groups":[{"name":"from-file","policy":"deny","match":{"cidrs":["198.51.100.0/24"]}}]}`, "console-key"); code != http.StatusBadRequest || !strings.Contains(out, "config file") {
+		t.Errorf("changing a config-file group = %d %s, want 400 naming the config file", code, out)
+	}
+	if code, out := do("PUT", "/api/clients", `{"groups":[],"default_policy":"deny"}`, "console-key"); code != http.StatusBadRequest || !strings.Contains(out, "default_policy") {
+		t.Errorf("changing the default policy = %d %s, want 400", code, out)
+	}
+	// Sending the file's group back unchanged, as the page does, is fine and
+	// writes nothing for it.
+	ok("PUT", "/api/clients", `{"groups":[{"name":"from-file","policy":"allow","match":{"cidrs":["198.51.100.0/24"]}}]}`)
+	if l, _ := cluster.Resource(kubesource.GroupGVR).Namespace("dns").List(context.Background(), metav1.ListOptions{}); len(l.Items) != 0 {
+		t.Fatalf("echoing the file's group created %d objects", len(l.Items))
+	}
+
 	// A group saved in the console becomes a ClientGroup, then policy.
-	ok("PUT", "/api/clients", `{"groups":[{"name":"no-loopback","policy":"deny","match":{"cidrs":["127.0.0.0/8"]}}]}`)
+	ok("PUT", "/api/clients", `{"groups":[{"name":"from-file","policy":"allow","match":{"cidrs":["198.51.100.0/24"]}},{"name":"no-loopback","policy":"deny","match":{"cidrs":["127.0.0.0/8"]}}]}`)
 	type clients struct {
 		Groups []struct{ Name, Source string } `json:"groups"`
 	}
 	// Read back at once, before the resolver can have seen it.
 	var view clients
-	if err := json.Unmarshal([]byte(ok("GET", "/api/clients", "")), &view); err != nil || len(view.Groups) != 1 || view.Groups[0].Name != "no-loopback" {
+	if err := json.Unmarshal([]byte(ok("GET", "/api/clients", "")), &view); err != nil || len(view.Groups) != 2 || view.Groups[1].Name != "no-loopback" {
 		t.Fatalf("clients right after save = %+v (%v)", view, err)
 	}
 	eventually(t, "the resolver to apply the saved group", func() bool {
@@ -105,10 +116,7 @@ func TestConsoleEditsReachTheResolver(t *testing.T) {
 	ok("PUT", "/api/clients", `{"groups":[]}`)
 	view = clients{}
 	raw := ok("GET", "/api/clients", "")
-	if !strings.Contains(raw, `"groups":[]`) {
-		t.Errorf("an empty group list must be [] and not null: %s", raw)
-	}
-	if err := json.Unmarshal([]byte(raw), &view); err != nil || len(view.Groups) != 0 {
+	if err := json.Unmarshal([]byte(raw), &view); err != nil || len(view.Groups) != 1 || view.Groups[0].Name != "from-file" {
 		t.Fatalf("clients after delete = %+v (%v)", view, err)
 	}
 	eventually(t, "the resolver to drop the deleted group", func() bool {
@@ -158,6 +166,47 @@ func TestConsoleEditsReachTheResolver(t *testing.T) {
 		return !m.Authoritative
 	})
 
+	// Blocklist sources: BlockList objects the Blocklists page manages.
+	ok("PUT", "/api/blocklist-sources/test-list", `{"url":"https://example.invalid/hosts","format":"hosts","refresh":"12h","categories":["ads"]}`)
+	var srcs struct {
+		Sources []struct {
+			Name string `json:"name"`
+			Spec struct {
+				URL      string   `json:"url"`
+				Refresh  string   `json:"refresh"`
+				Disabled bool     `json:"disabled"`
+				Cats     []string `json:"categories"`
+			} `json:"spec"`
+		} `json:"sources"`
+	}
+	if err := json.Unmarshal([]byte(ok("GET", "/api/blocklist-sources", "")), &srcs); err != nil || len(srcs.Sources) != 1 ||
+		srcs.Sources[0].Name != "test-list" || srcs.Sources[0].Spec.Refresh != "12h" {
+		t.Fatalf("sources right after save = %+v (%v)", srcs, err)
+	}
+	// The toggle on the page turns the object off and back on.
+	ok("POST", "/api/blocklists/toggle", `{"name":"test-list","enabled":false}`)
+	srcs.Sources = nil
+	_ = json.Unmarshal([]byte(ok("GET", "/api/blocklist-sources", "")), &srcs)
+	if len(srcs.Sources) != 1 || !srcs.Sources[0].Spec.Disabled {
+		t.Errorf("the toggle did not disable the BlockList: %+v", srcs)
+	}
+	for body, why := range map[string]string{
+		`{"url":"file:///etc/passwd"}`:           "a file URL",
+		`{"url":"https://x/y","format":"yaml"}`:  "an unknown format",
+		`{"url":"https://x/y","refresh":"soon"}`: "an unparsable interval",
+	} {
+		if code, out := do("PUT", "/api/blocklist-sources/bad-list", body, "console-key"); code != http.StatusBadRequest {
+			t.Errorf("%s was accepted: %d %s", why, code, out)
+		}
+	}
+	if code, _ := do("PUT", "/api/blocklist-sources/Not_A_Name", `{"url":"https://x/y"}`, "console-key"); code != http.StatusBadRequest {
+		t.Errorf("a name that is not a Kubernetes object name = %d, want 400", code)
+	}
+	ok("DELETE", "/api/blocklist-sources/test-list", "")
+	if code, _ := do("DELETE", "/api/blocklist-sources/test-list", "", "console-key"); code != http.StatusNotFound {
+		t.Errorf("deleting a list twice = %d, want 404", code)
+	}
+
 	// Address redirects: the page that used to rewrite a ConfigMap.
 	ok("PUT", "/api/address-overrides", `{"domain":"pin.test","address":"10.7.7.1"}`)
 	var ov struct {
@@ -209,3 +258,54 @@ func dnsFirstA(m *dns.Msg) (string, bool) {
 	}
 	return a.A.String(), true
 }
+
+// startConsole runs a console, with its own authentication, in front of the
+// resolver in h and writing to cluster. Its API key is "console-key".
+func startConsole(t *testing.T, h *harness, cluster dynamic.Interface) *httptest.Server {
+	t.Helper()
+	rt, err := console.NewRuntime("http://"+h.api, h.key)
+	if err != nil {
+		t.Fatal(err)
+	}
+	cfg := config.Default()
+	cfg.API.Auth.Mode = "apikey"
+	cfg.API.Auth.APIKeys = []string{"console-key"}
+	cfg.API.Auth.TrustLoopback = false
+	cfg.API.UI = false
+	c, err := console.New(console.Options{
+		Config:  config.NewStore(cfg, ""),
+		Runtime: rt,
+		Editor:  kubeedit.New(cluster, "dns"),
+		Version: "test",
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	handler, err := c.Handler()
+	if err != nil {
+		t.Fatal(err)
+	}
+	ts := httptest.NewServer(handler)
+	t.Cleanup(ts.Close)
+	return ts
+}
+
+// An empty list is [] on the wire, not null: the page counts what it is given.
+func TestConsoleEmptyGroupListIsAnArray(t *testing.T) {
+	cluster := kubeClient()
+	h := newHarness(t, nil, func(o *Options) {
+		o.Kube = KubeOptions{Enabled: true, Namespace: "dns", Client: cluster, SyncTimeout: 5 * time.Second}
+	})
+	ts := startConsole(t, h, cluster)
+	req, _ := http.NewRequest("GET", ts.URL+"/api/clients", nil)
+	req.Header.Set("X-API-Key", "console-key")
+	res, err := http.DefaultClient.Do(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+	body, _ := io.ReadAll(res.Body)
+	if !strings.Contains(string(body), `"groups":[]`) {
+		t.Errorf("an empty group list must be [] and not null: %s", body)
+	}
+}

internal/app/integration_test.go

@@ -4,6 +4,8 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -789,3 +791,44 @@ func TestConcurrentQueries(t *testing.T) {
 		}
 	}
 }
+
+// TestConfigWritesFollowAllowConfigWrite: with api.allow_config_write off, which
+// is how the cluster deployment runs, nothing may rewrite the config file, and
+// the console is told so (config.writable) instead of offering editors whose
+// every save fails. The upstream editor used to ignore the setting.
+func TestConfigWritesFollowAllowConfigWrite(t *testing.T) {
+	h := newHarness(t, func(c *config.Config) { c.API.AllowConfigWrite = false })
+	before := h.OriginalConfig()
+
+	_, body := h.get("/api/status")
+	var st struct {
+		Config struct {
+			Writable bool `json:"writable"`
+		} `json:"config"`
+	}
+	if err := json.Unmarshal(body, &st); err != nil || st.Config.Writable {
+		t.Errorf("status config.writable = %v (%v), want false", st.Config.Writable, err)
+	}
+	_, body = h.get("/api/settings")
+	var settings struct {
+		Writable bool `json:"writable"`
+	}
+	if err := json.Unmarshal(body, &settings); err != nil || settings.Writable {
+		t.Errorf("settings writable = %v (%v), want false", settings.Writable, err)
+	}
+
+	up := fmt.Sprintf(`{"policy":"failover","servers":[{"addr":%q,"proto":"udp","weight":1}],"max_attempts":1,"breaker_threshold":5,"healthcheck_interval_s":0}`, h.up.addr)
+	if code, out := h.do("PUT", "/api/upstreams", up); code != http.StatusBadRequest || !strings.Contains(string(out), "allow_config_write") {
+		t.Errorf("PUT /api/upstreams = %d %s, want a 400 naming the setting", code, out)
+	}
+	if code, _ := h.do("PUT", "/api/clients", `{"groups":[]}`); code != http.StatusBadRequest {
+		t.Errorf("PUT /api/clients = %d, want 400", code)
+	}
+	if code, _ := h.do("PUT", "/api/settings", `{"config_yaml":"x: 1"}`); code != http.StatusForbidden {
+		t.Errorf("PUT /api/settings = %d, want 403", code)
+	}
+	raw, err := os.ReadFile(filepath.Join(h.dir, "config.yaml"))
+	if err != nil || string(raw) != before {
+		t.Errorf("the config file changed (%v)", err)
+	}
+}

internal/console/hooks.go

@@ -2,6 +2,7 @@ package console
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"net/http"
@@ -52,12 +53,27 @@ func (c *Console) health() any {
 // reads.
 type clientsView struct {
 	Groups []struct {
-		Name   string `json:"name"`
+		config.Group
 		Source string `json:"source"`
 	} `json:"groups"`
 	DefaultPolicy string `json:"default_policy"`
 }
 
+// sameGroup compares two groups by what they configure, so an empty list and a
+// missing one are the same.
+func sameGroup(a, b config.Group) bool {
+	norm := func(g config.Group) string {
+		for _, p := range []*[]string{&g.Match.CIDRs, &g.Match.PTRSuffixes, &g.Match.Tags, &g.AllowDomains, &g.DenyDomains, &g.Upstreams} {
+			if *p == nil {
+				*p = []string{}
+			}
+		}
+		out, _ := json.Marshal(g)
+		return string(out)
+	}
+	return norm(a) == norm(b)
+}
+
 func (c *Console) putACL(ctx context.Context, groups []config.Group, def string) (int64, error) {
 	var view clientsView
 	if err := c.opt.Runtime.Get(ctx, "/api/clients", &view); err != nil {
@@ -69,13 +85,21 @@ func (c *Console) putACL(ctx context.Context, groups []config.Group, def string)
 	if def != "" && def != view.DefaultPolicy {
 		return 0, fmt.Errorf("default_policy is resolver configuration (currently %q), not a ClientGroup; change it in the resolver's config", view.DefaultPolicy)
 	}
-	inFile := map[string]bool{}
+	inFile := map[string]config.Group{}
 	for _, g := range view.Groups {
 		if g.Source == "" {
-			inFile[g.Name] = true
+			inFile[g.Name] = g.Group
 		}
 	}
-	res, err := c.opt.Editor.SyncGroups(ctx, groups, func(n string) bool { return inFile[n] }, actor(ctx))
+	// The console writes cluster objects. A group the resolver's config file
+	// defines is left alone, and a change to it is refused, because skipping it
+	// quietly would make the save look as if it worked.
+	for _, g := range groups {
+		if have, ok := inFile[g.Name]; ok && !sameGroup(have, g) {
+			return 0, fmt.Errorf("group %q is defined in the resolver's config file; change it there", g.Name)
+		}
+	}
+	res, err := c.opt.Editor.SyncGroups(ctx, groups, func(n string) bool { _, ok := inFile[n]; return ok }, actor(ctx))
 	if err != nil {
 		return 0, err
 	}
@@ -209,6 +233,9 @@ func (c *Console) handleClients(w http.ResponseWriter, r *http.Request) {
 		merged = append(merged, groupJSON(o))
 	}
 	view["groups"] = merged
+	// What this editor can and cannot change, so the page can say so.
+	view["default_policy_editable"] = false
+	view["file_groups_editable"] = false
 	api.WriteJSON(w, http.StatusOK, view)
 }