fix: map unknown role/environment to 422, set environment in one UPDATE

internal/cli/cli_test.go

@@ -90,6 +90,15 @@ func (m *memStore) GetNode(_ context.Context, c string) (*models.Node, error) {
 	}
 	return &n, nil
 }
+func (m *memStore) SetNodeEnvironment(_ context.Context, c, env string) (*models.Node, error) {
+	n, ok := m.nodes[c]
+	if !ok {
+		return nil, database.ErrNotFound
+	}
+	n.Environment = env
+	m.nodes[c] = n
+	return &n, nil
+}
 func (m *memStore) ListNodes(context.Context) ([]models.Node, error) {
 	out := []models.Node{}
 	for _, n := range m.nodes {

internal/database/database_test.go

@@ -127,13 +127,13 @@ func TestNodeCRUDAndForeignKeys(t *testing.T) {
 
 	// node referencing an unknown role must fail the FK
 	badRole := &models.Node{Certname: "h1", Role: "roles::ghost", Environment: "testing"}
-	if err := testDB.UpsertNode(ctx, badRole); err == nil {
-		t.Error("expected FK violation for unknown role")
+	if err := testDB.UpsertNode(ctx, badRole); !errors.Is(err, ErrInvalidReference) {
+		t.Errorf("UpsertNode unknown role = %v, want ErrInvalidReference", err)
 	}
 	// node referencing an unknown environment must fail the FK
 	badEnv := &models.Node{Certname: "h1", Role: "roles::base", Environment: "ghost"}
-	if err := testDB.UpsertNode(ctx, badEnv); err == nil {
-		t.Error("expected FK violation for unknown environment")
+	if err := testDB.UpsertNode(ctx, badEnv); !errors.Is(err, ErrInvalidReference) {
+		t.Errorf("UpsertNode unknown environment = %v, want ErrInvalidReference", err)
 	}
 
 	n := &models.Node{Certname: "h1", Role: "roles::base", Environment: "testing", Params: map[string]any{"x": "y"}}
@@ -168,3 +168,33 @@ func TestNodeCRUDAndForeignKeys(t *testing.T) {
 		t.Errorf("DeleteNode missing = %v, want ErrNotFound", err)
 	}
 }
+
+func TestSetNodeEnvironment(t *testing.T) {
+	requireDB(t)
+	clean(t)
+	seed(t)
+	ctx := context.Background()
+	if err := testDB.UpsertStatus(ctx, &models.Status{Name: "production"}); err != nil {
+		t.Fatal(err)
+	}
+	n := &models.Node{Certname: "h1", Role: "roles::base", Environment: "production", Params: map[string]any{"x": "y"}}
+	if err := testDB.UpsertNode(ctx, n); err != nil {
+		t.Fatal(err)
+	}
+
+	got, err := testDB.SetNodeEnvironment(ctx, "h1", "testing")
+	if err != nil || got.Environment != "testing" || got.Role != "roles::base" || got.Params["x"] != "y" {
+		t.Fatalf("SetNodeEnvironment = %+v, %v", got, err)
+	}
+	stored, _ := testDB.GetNode(ctx, "h1")
+	if stored.Environment != "testing" || stored.Params["x"] != "y" {
+		t.Errorf("stored node = %+v", stored)
+	}
+
+	if _, err := testDB.SetNodeEnvironment(ctx, "h1", "ghost"); !errors.Is(err, ErrInvalidReference) {
+		t.Errorf("unknown environment = %v, want ErrInvalidReference", err)
+	}
+	if _, err := testDB.SetNodeEnvironment(ctx, "missing", "testing"); !errors.Is(err, ErrNotFound) {
+		t.Errorf("unknown node = %v, want ErrNotFound", err)
+	}
+}

internal/database/nodes.go

@@ -6,10 +6,24 @@ import (
 	"fmt"
 
 	"github.com/jackc/pgx/v5"
+	"github.com/jackc/pgx/v5/pgconn"
 
 	"git.unkin.net/unkin/encapi/pkg/models"
 )
 
+// ErrInvalidReference is returned when a node names a role or environment that
+// does not exist.
+var ErrInvalidReference = errors.New("unknown role or environment")
+
+// fkErr maps a foreign-key violation to ErrInvalidReference.
+func fkErr(err error) error {
+	var pgErr *pgconn.PgError
+	if errors.As(err, &pgErr) && pgErr.Code == "23503" {
+		return ErrInvalidReference
+	}
+	return err
+}
+
 // UpsertNode creates or updates a host-to-role assignment. The referenced role
 // and environment must already exist (enforced by foreign keys).
 func (db *DB) UpsertNode(ctx context.Context, n *models.Node) error {
@@ -27,11 +41,35 @@ func (db *DB) UpsertNode(ctx context.Context, n *models.Node) error {
 		    updated_at = NOW()
 	`, n.Certname, n.Role, n.Environment, params)
 	if err != nil {
-		return fmt.Errorf("upsert node %q: %w", n.Certname, err)
+		return fmt.Errorf("upsert node %q: %w", n.Certname, fkErr(err))
 	}
 	return nil
 }
 
+// SetNodeEnvironment changes only a node's environment in a single statement and
+// returns the updated node, ErrNotFound, or ErrInvalidReference.
+func (db *DB) SetNodeEnvironment(ctx context.Context, certname, environment string) (*models.Node, error) {
+	var (
+		n      models.Node
+		params []byte
+	)
+	err := db.Pool.QueryRow(ctx, `
+		UPDATE nodes SET environment = $2, updated_at = NOW()
+		WHERE certname = $1
+		RETURNING certname, role, environment, params
+	`, certname, environment).Scan(&n.Certname, &n.Role, &n.Environment, &params)
+	if errors.Is(err, pgx.ErrNoRows) {
+		return nil, ErrNotFound
+	}
+	if err != nil {
+		return nil, fmt.Errorf("set environment for node %q: %w", certname, fkErr(err))
+	}
+	if n.Params, err = unmarshalParams(params); err != nil {
+		return nil, fmt.Errorf("decode params for node %q: %w", certname, err)
+	}
+	return &n, nil
+}
+
 // GetNode returns a single node or ErrNotFound.
 func (db *DB) GetNode(ctx context.Context, certname string) (*models.Node, error) {
 	var (

internal/server/auth_test.go

@@ -1,6 +1,8 @@
 package server
 
 import (
+	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -8,6 +10,7 @@ import (
 	"time"
 
 	"git.unkin.net/unkin/encapi/internal/auth"
+	"git.unkin.net/unkin/encapi/internal/database"
 	"git.unkin.net/unkin/encapi/internal/testsupport"
 	"git.unkin.net/unkin/encapi/pkg/models"
 )
@@ -111,3 +114,27 @@ func TestOIDCIssuerDown503(t *testing.T) {
 		t.Errorf("static token with issuer down: status %d, want 200", got)
 	}
 }
+
+func TestUnknownReferenceIs422(t *testing.T) {
+	ts, f, _ := oidcServer(t)
+	f.nodeErr = fmt.Errorf("upsert node: %w", database.ErrInvalidReference)
+	for _, c := range []struct{ url, body string }{
+		{ts.URL + "/api/v1/nodes/h1.example/environment", `{"environment":"ghost"}`},
+		{ts.URL + "/api/v1/nodes/h2", `{"role":"roles::base","environment":"ghost"}`},
+	} {
+		req, _ := http.NewRequest(http.MethodPut, c.url, strings.NewReader(c.body))
+		req.Header.Set("Authorization", "Bearer s3cret")
+		resp, err := http.DefaultClient.Do(req)
+		if err != nil {
+			t.Fatal(err)
+		}
+		body, _ := io.ReadAll(resp.Body)
+		resp.Body.Close()
+		if resp.StatusCode != http.StatusUnprocessableEntity {
+			t.Errorf("PUT %s: status %d, want 422", c.url, resp.StatusCode)
+		}
+		if strings.Contains(string(body), "upsert node") {
+			t.Errorf("PUT %s leaks internal error: %s", c.url, body)
+		}
+	}
+}

internal/server/handlers.go

@@ -34,6 +34,10 @@ func mapErr(w http.ResponseWriter, err error) {
 		writeError(w, http.StatusNotFound, "not found")
 		return
 	}
+	if errors.Is(err, database.ErrInvalidReference) {
+		writeError(w, http.StatusUnprocessableEntity, database.ErrInvalidReference.Error())
+		return
+	}
 	writeError(w, http.StatusInternalServerError, err.Error())
 }
 
@@ -232,16 +236,11 @@ func (s *Server) putNodeEnvironment(w http.ResponseWriter, r *http.Request) {
 		writeError(w, http.StatusForbidden, "token may only set environment to testing")
 		return
 	}
-	node, err := s.store.GetNode(r.Context(), chi.URLParam(r, "certname"))
+	node, err := s.store.SetNodeEnvironment(r.Context(), chi.URLParam(r, "certname"), body.Environment)
 	if err != nil {
 		mapErr(w, err)
 		return
 	}
-	node.Environment = body.Environment
-	if err := s.store.UpsertNode(r.Context(), node); err != nil {
-		mapErr(w, err)
-		return
-	}
 	writeJSON(w, http.StatusOK, node)
 }
 

internal/server/server.go

@@ -31,6 +31,7 @@ type Store interface {
 
 	UpsertNode(ctx context.Context, n *models.Node) error
 	GetNode(ctx context.Context, certname string) (*models.Node, error)
+	SetNodeEnvironment(ctx context.Context, certname, environment string) (*models.Node, error)
 	ListNodes(ctx context.Context) ([]models.Node, error)
 	DeleteNode(ctx context.Context, certname string) error
 }

internal/server/server_test.go

@@ -19,6 +19,7 @@ type fakeStore struct {
 	roles    map[string]models.Role
 	statuses map[string]models.Status
 	nodes    map[string]models.Node
+	nodeErr  error // returned by node writes when set
 }
 
 func newFake() *fakeStore {
@@ -80,9 +81,24 @@ func (f *fakeStore) DeleteStatus(_ context.Context, name string) error {
 	return nil
 }
 func (f *fakeStore) UpsertNode(_ context.Context, n *models.Node) error {
+	if f.nodeErr != nil {
+		return f.nodeErr
+	}
 	f.nodes[n.Certname] = *n
 	return nil
 }
+func (f *fakeStore) SetNodeEnvironment(_ context.Context, certname, env string) (*models.Node, error) {
+	if f.nodeErr != nil {
+		return nil, f.nodeErr
+	}
+	n, ok := f.nodes[certname]
+	if !ok {
+		return nil, database.ErrNotFound
+	}
+	n.Environment = env
+	f.nodes[certname] = n
+	return &n, nil
+}
 func (f *fakeStore) GetNode(_ context.Context, certname string) (*models.Node, error) {
 	n, ok := f.nodes[certname]
 	if !ok {