diff --git a/internal/admin/router.go b/internal/admin/router.go index 43411e8..9447f73 100644 --- a/internal/admin/router.go +++ b/internal/admin/router.go @@ -3,6 +3,7 @@ package admin import ( + "encoding/json" "errors" "net/http" "strconv" @@ -104,18 +105,75 @@ func updateCluster(repo *storage.ClusterRepo, auditRepo *audit.Repo) gin.Handler return } - var cl cluster.Cluster - if err := c.ShouldBindJSON(&cl); err != nil { + // Parse the body as a generic map first so we can tell which fields + // the client actually sent. A struct + ShouldBindJSON can't tell + // "absent" from "zero value" for bools, ints, and empty strings — + // which silently flips true→false and 1→0 on PATCH-style updates. + rawBody := make(map[string]json.RawMessage) + if err := c.ShouldBindJSON(&rawBody); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } - cl.ID = id - if cl.AuthPassword == "" { - cl.AuthPassword = old.AuthPassword + merged := *old + merged.ID = id + + // Strings: absent = keep old; empty string = keep old (no way to + // distinguish "set to empty" from "not set" in JSON; treat them + // the same — clients should send the new value or omit). + if v, ok := rawBody["name"]; ok { + merged.Name = decodeString(v) + } + if v, ok := rawBody["rm_url"]; ok { + merged.RMURL = decodeString(v) + } + if v, ok := rawBody["shs_url"]; ok { + merged.SHSURL = decodeString(v) + } + if v, ok := rawBody["spark_submit_execute_bin"]; ok { + merged.SparkSubmitExecuteBin = decodeString(v) + } + if v, ok := rawBody["auth_type"]; ok { + merged.AuthType = cluster.AuthType(decodeString(v)) + } + if v, ok := rawBody["auth_username"]; ok { + merged.AuthUsername = decodeString(v) + } + if v, ok := rawBody["auth_password"]; ok { + // Field is json:"-" so it never round-trips through admin JSON, + // but a future dedicated password endpoint could set it here. + merged.AuthPassword = decodeString(v) + } + if v, ok := rawBody["ssl_ca_bundle"]; ok { + merged.SSLCABundle = decodeString(v) + } + // Ints. + if v, ok := rawBody["rate_limit_per_min"]; ok { + merged.RateLimitPerMin = decodeInt(v) + } + // Bools. + if v, ok := rawBody["is_active"]; ok { + merged.IsActive = decodeBool(v) + } + if v, ok := rawBody["ssl_verify"]; ok { + merged.SSLVerify = decodeBool(v) + } + // Slices: null = keep old; [] = replace with empty. + if v, ok := rawBody["url_allowlist"]; ok { + merged.URLAllowlist = decodeStringSlice(v) + } + if v, ok := rawBody["default_submit_args"]; ok { + merged.DefaultSubmitArgs = decodeStringSlice(v) } - if err := repo.Update(ctx, &cl); err != nil { + // Re-validate after merging so a missing auth_type on a fresh + // cluster (defaulted by DB to "none") doesn't get clobbered. + if err := validateClusterUpdate(&merged); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } + + if err := repo.Update(ctx, &merged); err != nil { if errors.Is(err, storage.ErrNotFound) { c.JSON(http.StatusNotFound, gin.H{"error": "cluster not found"}) return @@ -126,21 +184,47 @@ func updateCluster(repo *storage.ClusterRepo, auditRepo *audit.Repo) gin.Handler details, err := audit.MarshalDetails(map[string]any{ "before": clusterFields(old), - "after": clusterFields(&cl), + "after": clusterFields(&merged), }) if err == nil { _ = auditRepo.Insert(ctx, &audit.Entry{ Actor: actor(c), Action: audit.ActionClusterUpdate, - ClusterID: cl.ID, + ClusterID: merged.ID, Details: details, }) } - c.JSON(http.StatusOK, &cl) + c.JSON(http.StatusOK, &merged) } } +func decodeString(raw json.RawMessage) string { + var s string + _ = json.Unmarshal(raw, &s) + return s +} + +func decodeInt(raw json.RawMessage) int { + var n int + _ = json.Unmarshal(raw, &n) + return n +} + +func decodeBool(raw json.RawMessage) bool { + var b bool + _ = json.Unmarshal(raw, &b) + return b +} + +func decodeStringSlice(raw json.RawMessage) []string { + // Handle both "key": null (clear to nil — caller treats as keep-old + // since they would have skipped this field) and "key": [...] + var s []string + _ = json.Unmarshal(raw, &s) + return s +} + func deleteCluster(repo *storage.ClusterRepo, auditRepo *audit.Repo) gin.HandlerFunc { return func(c *gin.Context) { id := c.Param("id") @@ -236,6 +320,24 @@ func validateClusterCreate(c *cluster.Cluster) error { return nil } +// validateClusterUpdate is the lighter validation for PATCH semantics: only +// fields that constrain cluster behavior are checked, since the existing row +// already supplies the rest. +func validateClusterUpdate(c *cluster.Cluster) error { + switch c.AuthType { + case cluster.AuthNone, cluster.AuthSimple, cluster.AuthBasic: + // ok + case "": + // Should not happen — defaults to "none" in DB. + default: + return errors.New("auth_type must be one of none, simple, basic") + } + if c.RateLimitPerMin < 0 { + return errors.New("rate_limit_per_min must be >= 0") + } + return nil +} + func clusterFields(c *cluster.Cluster) map[string]any { return map[string]any{ "id": c.ID, diff --git a/internal/admin/router_test.go b/internal/admin/router_test.go index 6511cb3..d58294d 100644 --- a/internal/admin/router_test.go +++ b/internal/admin/router_test.go @@ -13,6 +13,7 @@ import ( "github.com/gin-gonic/gin" "spark-mcp-go/internal/audit" + "spark-mcp-go/internal/cluster" "spark-mcp-go/internal/storage" ) @@ -335,6 +336,85 @@ func TestUpdateCluster_PreservesEmptyPassword(t *testing.T) { } } +// TestUpdateCluster_PartialUpdatePreservesOtherFields verifies that PUT only +// touches fields present in the request body. Sends a full cluster via POST, +// then PUTs a body with a single field ("is_active": false) and checks every +// other field still matches the original. +func TestUpdateCluster_PartialUpdatePreservesOtherFields(t *testing.T) { + r, repo, _ := newTestAdmin(t) + original := fullClusterMap("c1") + original["is_active"] = true + original["rate_limit_per_min"] = 25 + original["url_allowlist"] = []string{"rm", "shs"} + original["default_submit_args"] = []string{"--conf", "spark.foo=bar"} + if w := doReq(t, r, "POST", "/admin/clusters", "good-token", original); w.Code != http.StatusCreated { + t.Fatalf("seed POST: %d %s", w.Code, w.Body.String()) + } + + patch := map[string]any{"is_active": false} + w := doReq(t, r, "PUT", "/admin/clusters/c1", "good-token", patch) + if w.Code != http.StatusOK { + t.Fatalf("PUT: %d %s", w.Code, w.Body.String()) + } + + got, err := repo.Get(context.Background(), "c1") + if err != nil { + t.Fatalf("get after PUT: %v", err) + } + if got.IsActive != false { + t.Errorf("is_active: got %v, want false", got.IsActive) + } + // Every other field should equal the original. + if got.Name != original["name"] { + t.Errorf("name: got %q, want %q", got.Name, original["name"]) + } + if got.RMURL != original["rm_url"] { + t.Errorf("rm_url: got %q, want %q", got.RMURL, original["rm_url"]) + } + if got.SHSURL != original["shs_url"] { + t.Errorf("shs_url: got %q, want %q", got.SHSURL, original["shs_url"]) + } + if got.SparkSubmitExecuteBin != original["spark_submit_execute_bin"] { + t.Errorf("spark_submit_execute_bin: got %q, want %q", got.SparkSubmitExecuteBin, original["spark_submit_execute_bin"]) + } + if got.AuthType != cluster.AuthBasic { + t.Errorf("auth_type: got %q, want basic (preserved from seed)", got.AuthType) + } + if got.RateLimitPerMin != 25 { + t.Errorf("rate_limit_per_min: got %d, want 25 (preserved from seed)", got.RateLimitPerMin) + } + if len(got.URLAllowlist) != 2 || got.URLAllowlist[0] != "rm" || got.URLAllowlist[1] != "shs" { + t.Errorf("url_allowlist: got %v, want [rm shs] (preserved)", got.URLAllowlist) + } + if len(got.DefaultSubmitArgs) != 2 || got.DefaultSubmitArgs[0] != "--conf" { + t.Errorf("default_submit_args: got %v, want [--conf ...] (preserved)", got.DefaultSubmitArgs) + } +} + +// TestUpdateCluster_SliceReplacement checks that an explicit non-nil slice +// in the body *replaces* the old slice (rather than appending or being +// ignored as a zero value). +func TestUpdateCluster_SliceReplacement(t *testing.T) { + r, repo, _ := newTestAdmin(t) + if w := doReq(t, r, "POST", "/admin/clusters", "good-token", fullClusterMap("c1")); w.Code != http.StatusCreated { + t.Fatalf("seed POST: %d %s", w.Code, w.Body.String()) + } + seed, _ := repo.Get(context.Background(), "c1") + if len(seed.URLAllowlist) == 0 { + t.Fatal("seed should have url_allowlist from fullClusterMap") + } + + // Replace with a new single-entry list. + patch := map[string]any{"url_allowlist": []string{"only-this"}} + if w := doReq(t, r, "PUT", "/admin/clusters/c1", "good-token", patch); w.Code != http.StatusOK { + t.Fatalf("PUT: %d %s", w.Code, w.Body.String()) + } + got, _ := repo.Get(context.Background(), "c1") + if len(got.URLAllowlist) != 1 || got.URLAllowlist[0] != "only-this" { + t.Errorf("url_allowlist: got %v, want [only-this]", got.URLAllowlist) + } +} + func TestDeleteCluster(t *testing.T) { r, _, auditRepo := newTestAdmin(t) doReq(t, r, "POST", "/admin/clusters", "good-token", fullClusterMap("c1"))