refactor: one way to change a stack

cmd/fjordd/http_error.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"errors"
+	"io/fs"
+	"net/http"
+)
+
+// httpError is an error that says which status answers it, so a change made
+// inside manager.Update can refuse with a 400 while a failed write stays 500.
+type httpError struct {
+	code int
+	msg  string
+}
+
+func (e httpError) Error() string { return e.msg }
+
+// replyErr answers err: an httpError with its own status, a stack that is not
+// there with 404, anything else with 500.
+func replyErr(w http.ResponseWriter, err error) {
+	var he httpError
+	switch {
+	case errors.As(err, &he):
+		http.Error(w, he.msg, he.code)
+	case errors.Is(err, fs.ErrNotExist):
+		http.Error(w, "Stack not found", 404)
+	default:
+		http.Error(w, err.Error(), 500)
+	}
+}

cmd/fjordd/keep_address.go

@@ -124,11 +124,14 @@ func (s *server) keepAddresses(ctx context.Context, st *stack.Stack) (kept, skip
 		}
 		return compose
 	}
-	out, err := s.manager.UpdateCompose(st.Name, pin)
+	saved, err := s.manager.Update(st.Name, func(fresh *stack.Stack) error {
+		fresh.Compose = pin(fresh.Compose)
+		return nil
+	})
 	if err != nil {
 		return []string{"could not keep addresses: " + err.Error()}, skipped
 	}
-	st.Compose = out
+	st.Compose = saved.Compose
 	if len(kept) == 0 {
 		return nil, skipped
 	}

cmd/fjordd/restart_policy.go

@@ -148,19 +148,21 @@ func (s *server) stackRestart(w http.ResponseWriter, r *http.Request, name strin
 		json.NewEncoder(w).Encode(map[string]any{"status": "saved"})
 		return
 	}
-	compose, err := composepkg.SetRestart(st.Compose, req)
+	var old string
+	st, err = s.manager.Update(name, func(st *stack.Stack) error {
+		compose, err := composepkg.SetRestart(st.Compose, req)
+		if err != nil {
+			return httpError{400, err.Error()}
+		}
+		old, st.Compose = st.Compose, compose
+		return nil
+	})
 	if err != nil {
-		http.Error(w, err.Error(), 400)
-		return
-	}
-	old := st.Compose
-	st.Compose = compose
-	if err := s.manager.Save(st); err != nil {
-		http.Error(w, err.Error(), 500)
+		replyErr(w, err)
 		return
 	}
 	env := st.EnvMap()
-	s.recordPending(name, composepkg.ChangedServices(old, env, compose, env))
+	s.recordPending(name, composepkg.ChangedServices(old, env, st.Compose, env))
 	log.Printf("%s: restart policy %s", name, req)
 	w.Header().Set("Content-Type", "application/json")
 	json.NewEncoder(w).Encode(map[string]any{"status": "saved", "composeHash": composeHash(st)})

cmd/fjordd/rollback.go

@@ -81,29 +81,23 @@ func (s *server) stackRollback(w http.ResponseWriter, r *http.Request, name stri
 		http.Error(w, `send {"services": [...]}`, 400)
 		return
 	}
-	st, err := s.manager.Get(name)
-	if err != nil {
-		http.Error(w, "Stack not found", 404)
-		return
-	}
-	state, err := s.manager.LoadState(name)
-	if err != nil || state == nil {
-		http.Error(w, "no state for "+name, 500)
-		return
-	}
-	for _, svc := range req.Services {
-		rb, ok := state.Rollback[svc]
-		if !ok {
-			http.Error(w, fmt.Sprintf("%s has no earlier image recorded for %s", name, svc), 400)
-			return
+	if _, err := s.manager.Update(name, func(st *stack.Stack) error {
+		state, err := s.manager.LoadState(name)
+		if err != nil || state == nil {
+			return httpError{500, "no state for " + name}
 		}
-		if err := setServiceImage(st, svc, withoutDigest(rb.Ref)+"@"+rb.Digest); err != nil {
-			http.Error(w, err.Error(), 400)
-			return
+		for _, svc := range req.Services {
+			rb, ok := state.Rollback[svc]
+			if !ok {
+				return httpError{400, fmt.Sprintf("%s has no earlier image recorded for %s", name, svc)}
+			}
+			if err := setServiceImage(st, svc, withoutDigest(rb.Ref)+"@"+rb.Digest); err != nil {
+				return httpError{400, err.Error()}
+			}
 		}
-	}
-	if err := s.manager.Save(st); err != nil {
-		http.Error(w, err.Error(), 500)
+		return nil
+	}); err != nil {
+		replyErr(w, err)
 		return
 	}
 	body, _ := json.Marshal(map[string][]string{"services": req.Services})
@@ -127,29 +121,24 @@ func (s *server) stackUnpin(w http.ResponseWriter, r *http.Request, name string)
 		http.Error(w, `send {"service": "..."}`, 400)
 		return
 	}
-	st, err := s.manager.Get(name)
-	if err != nil {
-		http.Error(w, "Stack not found", 404)
-		return
-	}
-	cur := serviceImage(st, req.Service)
-	if !strings.Contains(cur, "@") {
-		http.Error(w, req.Service+" is not pinned", 400)
-		return
-	}
-	restore := withoutDigest(cur)
-	if state, _ := s.manager.LoadState(name); state != nil {
-		if rb, ok := state.Rollback[req.Service]; ok && rb.Compose != "" &&
-			composepkg.ExpandEnv(rb.Compose, st.EnvMap()) == restore {
-			restore = rb.Compose
+	if _, err := s.manager.Update(name, func(st *stack.Stack) error {
+		cur := serviceImage(st, req.Service)
+		if !strings.Contains(cur, "@") {
+			return httpError{400, req.Service + " is not pinned"}
 		}
-	}
-	if err := setServiceImage(st, req.Service, restore); err != nil {
-		http.Error(w, err.Error(), 400)
-		return
-	}
-	if err := s.manager.Save(st); err != nil {
-		http.Error(w, err.Error(), 500)
+		restore := withoutDigest(cur)
+		if state, _ := s.manager.LoadState(name); state != nil {
+			if rb, ok := state.Rollback[req.Service]; ok && rb.Compose != "" &&
+				composepkg.ExpandEnv(rb.Compose, st.EnvMap()) == restore {
+				restore = rb.Compose
+			}
+		}
+		if err := setServiceImage(st, req.Service, restore); err != nil {
+			return httpError{400, err.Error()}
+		}
+		return nil
+	}); err != nil {
+		replyErr(w, err)
 		return
 	}
 	s.fleet.forget(name)

cmd/fjordd/stack_handlers.go

@@ -886,15 +886,19 @@ func (s *server) stackSetTag(w http.ResponseWriter, r *http.Request, name string
 				return
 			}
 		}
-		st.Compose, st.Env = newCompose, newEnv
-		// And where appjail reads it: the service's from:, or the Makejail's
-		// tag argument, pin included.
-		if err := directorFollowsCompose(st, []string{body.Service}); err != nil {
-			http.Error(w, "director: "+err.Error(), 400)
-			return
-		}
-		if err := s.manager.Save(st); err != nil {
-			http.Error(w, err.Error(), 500)
+		if _, err := s.manager.Update(name, func(fresh *stack.Stack) error {
+			if err := unchangedSince(fresh, st); err != nil {
+				return err
+			}
+			fresh.Compose, fresh.Env = newCompose, newEnv
+			// And where appjail reads it: the service's from:, or the
+			// Makejail's tag argument, pin included.
+			if err := directorFollowsCompose(fresh, []string{body.Service}); err != nil {
+				return httpError{400, "director: " + err.Error()}
+			}
+			return nil
+		}); err != nil {
+			replyErr(w, err)
 			return
 		}
 		w.Header().Set("Content-Type", "application/json")
@@ -929,17 +933,21 @@ func (s *server) stackSetTag(w http.ResponseWriter, r *http.Request, name string
 			return
 		}
 	}
-	st.Compose, st.Env = newCompose, newEnv
-	// And where appjail will read it. Setting only the compose left the jail
-	// on whatever ${tag} defaults to, so the version on screen and the version
-	// running were different numbers. A pin goes along: the digest, not just
-	// the tag.
-	if err := directorFollowsCompose(st, nil); err != nil {
-		http.Error(w, "director: "+err.Error(), 400)
-		return
-	}
-	if err := s.manager.Save(st); err != nil {
-		http.Error(w, err.Error(), 500)
+	if _, err := s.manager.Update(name, func(fresh *stack.Stack) error {
+		if err := unchangedSince(fresh, st); err != nil {
+			return err
+		}
+		fresh.Compose, fresh.Env = newCompose, newEnv
+		// And where appjail will read it. Setting only the compose left the
+		// jail on whatever ${tag} defaults to, so the version on screen and
+		// the version running were different numbers. A pin goes along: the
+		// digest, not just the tag.
+		if err := directorFollowsCompose(fresh, nil); err != nil {
+			return httpError{400, "director: " + err.Error()}
+		}
+		return nil
+	}); err != nil {
+		replyErr(w, err)
 		return
 	}
 	w.Header().Set("Content-Type", "application/json")
@@ -1438,18 +1446,32 @@ func (s *server) directorSetTag(ctx context.Context, w http.ResponseWriter, st *
 		}
 		img += "@" + digest
 	}
-	if err := setServiceImage(st, service, img); err != nil {
-		http.Error(w, err.Error(), 400)
-		return
-	}
-	if err := s.manager.Save(st); err != nil {
-		http.Error(w, err.Error(), 500)
+	if _, err := s.manager.Update(st.Name, func(fresh *stack.Stack) error {
+		if err := unchangedSince(fresh, st); err != nil {
+			return err
+		}
+		if err := setServiceImage(fresh, service, img); err != nil {
+			return httpError{400, err.Error()}
+		}
+		return nil
+	}); err != nil {
+		replyErr(w, err)
 		return
 	}
 	w.Header().Set("Content-Type", "application/json")
 	w.Write([]byte(`{"status":"ok"}`))
 }
 
+// unchangedSince refuses a change worked out from snap when the stack has
+// been saved since. Set tag asks the registry first (up to 30s, outside the
+// lock); writing its answer over a save made meanwhile would undo that save.
+func unchangedSince(fresh, snap *stack.Stack) error {
+	if fresh.Compose != snap.Compose || fresh.Env != snap.Env || fresh.Director != snap.Director {
+		return httpError{http.StatusConflict, fresh.Name + " was changed while fjord looked up that version; try again"}
+	}
+	return nil
+}
+
 // stackDismissFailure forgets the stack's last failure. Without it Dismiss
 // lasted only as long as the page: a failure no later action clears (a slow
 // inspect read as "gone") came back on every reload.

cmd/fjordd/stack_update_test.go

@@ -0,0 +1,65 @@
+package main
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"github.com/daemonless/fjord/pkg/stack"
+)
+
+func unpinFixture(t *testing.T, compose string) *server {
+	t.Helper()
+	s := &server{manager: stack.NewManager(t.TempDir())}
+	if err := s.manager.Save(&stack.Stack{Name: "app", Compose: compose, Env: "TAG=2.0\n"}); err != nil {
+		t.Fatal(err)
+	}
+	return s
+}
+
+func postUnpin(s *server, name, body string) *httptest.ResponseRecorder {
+	rec := httptest.NewRecorder()
+	s.stackUnpin(rec, httptest.NewRequest(http.MethodPost, "/api/stacks/"+name+"/unpin", strings.NewReader(body)), name)
+	return rec
+}
+
+// Unpin drops the digest, puts back what the operator wrote when the
+// rollback record still matches it, and leaves the .env as it was.
+func TestStackUnpin(t *testing.T) {
+	s := unpinFixture(t, "services:\n  web:\n    image: repo/web:2.0@sha256:abc\n")
+	s.manager.UpdateState("app", func(st *stack.State) *stack.State {
+		return &stack.State{SchemaVersion: 1, Rollback: map[string]stack.RollbackImage{"web": {Compose: "repo/web:${TAG}"}}}
+	})
+	if rec := postUnpin(s, "app", `{"service":"web"}`); rec.Code != http.StatusNoContent {
+		t.Fatalf("%d %s", rec.Code, rec.Body)
+	}
+	st, _ := s.manager.Get("app")
+	if !strings.Contains(st.Compose, "image: repo/web:${TAG}") || st.Env != "TAG=2.0\n" {
+		t.Errorf("compose:\n%s\nenv %q", st.Compose, st.Env)
+	}
+}
+
+// What unpin refuses, with the status each gets.
+func TestStackUnpinRefuses(t *testing.T) {
+	s := unpinFixture(t, "services:\n  web:\n    image: repo/web:2.0\n")
+	if rec := postUnpin(s, "app", `{"service":"web"}`); rec.Code != 400 || !strings.Contains(rec.Body.String(), "web is not pinned") {
+		t.Errorf("not pinned: %d %s", rec.Code, rec.Body)
+	}
+	if rec := postUnpin(s, "gone", `{"service":"web"}`); rec.Code != 404 {
+		t.Errorf("no such stack: %d %s", rec.Code, rec.Body)
+	}
+}
+
+// Set tag looks the version up before it writes; a save that lands in
+// between is not written over.
+func TestUnchangedSince(t *testing.T) {
+	snap := &stack.Stack{Name: "app", Compose: "a", Env: "e"}
+	if err := unchangedSince(&stack.Stack{Name: "app", Compose: "a", Env: "e"}, snap); err != nil {
+		t.Errorf("unchanged refused: %v", err)
+	}
+	err := unchangedSince(&stack.Stack{Name: "app", Compose: "b", Env: "e"}, snap)
+	if he, ok := err.(httpError); !ok || he.code != http.StatusConflict {
+		t.Errorf("changed: %v", err)
+	}
+}