From 033a5b0a61741578ba7c21ff51aee2eeefec91c0 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Fri, 23 Oct 2026 21:39:41 +1100 Subject: [PATCH] fix: map unknown role/environment to 422, set environment in one UPDATE --- internal/cli/cli_test.go | 9 +++++++ internal/database/database_test.go | 38 +++++++++++++++++++++++++--- internal/database/nodes.go | 40 +++++++++++++++++++++++++++++- internal/server/auth_test.go | 27 ++++++++++++++++++++ internal/server/handlers.go | 11 ++++---- internal/server/server.go | 1 + internal/server/server_test.go | 16 ++++++++++++ 7 files changed, 131 insertions(+), 11 deletions(-) diff --git a/internal/cli/cli_test.go b/internal/cli/cli_test.go index 5e82e66..4f7835a 100644 --- a/internal/cli/cli_test.go +++ b/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 { diff --git a/internal/database/database_test.go b/internal/database/database_test.go index fe694f5..07074e6 100644 --- a/internal/database/database_test.go +++ b/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) + } +} diff --git a/internal/database/nodes.go b/internal/database/nodes.go index 8591408..1583737 100644 --- a/internal/database/nodes.go +++ b/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, ¶ms) + 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 ( diff --git a/internal/server/auth_test.go b/internal/server/auth_test.go index caee576..249607c 100644 --- a/internal/server/auth_test.go +++ b/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) + } + } +} diff --git a/internal/server/handlers.go b/internal/server/handlers.go index 6483b5d..04d772f 100644 --- a/internal/server/handlers.go +++ b/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) } diff --git a/internal/server/server.go b/internal/server/server.go index f68e779..f5eb476 100644 --- a/internal/server/server.go +++ b/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 } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 3cdd3c8..0d5a702 100644 --- a/internal/server/server_test.go +++ b/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 {