diff --git a/README.md b/README.md index cd2768b..ad59643 100644 --- a/README.md +++ b/README.md @@ -202,6 +202,18 @@ rossoctl agents import --deployment-type sandbox from-image \ # over --envVarsURL, whichever order the flags appear in. rossoctl agents import from-image --name orders --containerImage ghcr.io/x/y:latest \ --envVar LOG_LEVEL=debug --envVar 'TAGS=a,b,c' +# --additionalParameterJSON sends request fields the CLI has no flag for. Its value +# is a JSON dict, or the name of a file containing one — a value starting with '{' +# is the document itself, anything else is a filename. +rossoctl agents import from-image --name orders --containerImage ghcr.io/x/y:latest \ + --additionalParameterJSON '{"serviceAccount":"orders-sa"}' +# Repeatable: the dicts are merged, a later one winning a key they share, and the +# result is overlaid onto the request body. Merging is by top-level key, so a +# repeated key is replaced whole rather than combined with what it had. Keys that +# name a field the other flags set — containerImage here — override them. +rossoctl agents import from-image --name orders --containerImage ghcr.io/x/y:latest \ + --additionalParameterJSON ./base.json \ + --additionalParameterJSON '{"containerImage":"ghcr.io/x/y:pinned"}' # `agents --namespace` overrides the context's namespace for any agents subcommand rossoctl agents --namespace team2 get orders # -> GET /agents/team2/orders @@ -231,6 +243,10 @@ rossoctl tools import from-image --name weather-mcp --containerImage ghcr.io/x/y --envVar LOG_LEVEL=debug --envVar 'TAGS=a,b,c' # --ports sets service ports as name:port:targetPort[:protocol] (default http:9090:9090:TCP); a bare "port" = http:port:port:TCP rossoctl tools import from-image --name weather-mcp --containerImage ghcr.io/x/y:latest --ports grpc:9000:9001:TCP,8080 +# --additionalParameterJSON works as it does for agents: a JSON dict or a file +# containing one, repeatable, merged by top-level key, and winning over the flags above +rossoctl tools import from-image --name weather-mcp --containerImage ghcr.io/x/y:latest \ + --additionalParameterJSON '{"serviceAccount":"mcp-sa"}' --additionalParameterJSON ./extra.json # A tool built from source reports "Building" until its build finishes, which is # often longer than the 60s default, so allow for it. A failed build reports diff --git a/cmd/additionalparams.go b/cmd/additionalparams.go new file mode 100644 index 0000000..2969742 --- /dev/null +++ b/cmd/additionalparams.go @@ -0,0 +1,141 @@ +package cmd + +import ( + "bytes" + "encoding/json" + "fmt" + "maps" + "os" + "strings" +) + +// additionalParameterFlagName is the flag whose values loadAdditionalParameters +// interprets. Named once so the errors below and the flag registrations in +// agents_import.go and tools_import.go cannot drift apart. +const additionalParameterFlagName = "additionalParameterJSON" + +// loadAdditionalParameters interprets repeated --additionalParameterJSON values +// and merges them into the single dict that is overlaid onto a create request. +// +// Each value is either inline JSON or the name of a file containing JSON; see +// readAdditionalParameter. Every value must decode to a JSON object, because the +// result is merged into the request body by name — an array or a scalar has no +// names to merge. +// +// Merging is shallow and last-wins: a key present in two values takes the later +// value whole, rather than the two objects underneath being combined. So +// '{"resources":{"limits":{"cpu":"1"}}}' followed by +// '{"resources":{"requests":{"cpu":"1"}}}' sends only the requests, not both. +// The rule is the one mergeEnvVars applies to a repeated variable name, and it is +// the rule that lets a caller *replace* a nested structure the CLI or an earlier +// file already set; a deep merge could only ever add to it. +// +// Returns nil, not an empty map, when no values are given: the marshaler treats +// an empty overlay as no overlay, and nil says the same thing without allocating. +func loadAdditionalParameters(values []string) (map[string]any, error) { + var merged map[string]any + for _, v := range values { + obj, err := readAdditionalParameter(v) + if err != nil { + return nil, err + } + if merged == nil { + merged = make(map[string]any, len(obj)) + } + maps.Copy(merged, obj) + } + return merged, nil +} + +// readAdditionalParameter interprets one --additionalParameterJSON value as +// either inline JSON or a filename, and decodes it as a JSON object. +// +// The two are told apart by the first non-whitespace byte: '{' means the value +// is itself the document, anything else means it names a file. Sniffing the +// leading character rather than probing the filesystem keeps the meaning of a +// value a property of what the user typed, so a command does not change behavior +// because a file named '{"a":1}' happens to exist, and a misspelled filename +// reports that the file is missing instead of being parsed as JSON and reported +// as a syntax error. +// +// A value that is neither — a bare word, an array, a quoted string — is a +// missing file, and is reported as one. +func readAdditionalParameter(value string) (map[string]any, error) { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return nil, fmt.Errorf("--%s must not be empty", additionalParameterFlagName) + } + + data := []byte(trimmed) + source := "inline JSON" + if !strings.HasPrefix(trimmed, "{") { + contents, err := os.ReadFile(trimmed) + if err != nil { + return nil, fmt.Errorf("--%s %q: %w", additionalParameterFlagName, value, err) + } + data = contents + source = fmt.Sprintf("file %q", trimmed) + } + + obj, err := decodeJSONObject(data) + if err != nil { + return nil, fmt.Errorf("--%s: %s: %w", additionalParameterFlagName, source, err) + } + return obj, nil +} + +// decodeJSONObject decodes data as a JSON object, rejecting a duplicate key and +// anything that is not an object. +// +// json.Unmarshal into a map would accept both: a duplicate name silently keeps +// the last value, and there would be no way to tell an object from any other +// value until the type assertion. A token-driven decode is used instead so +// '{"a":1,"a":2}' is refused rather than quietly becoming one of the two — a file +// naming the same parameter twice is a mistake worth reporting, and unlike a +// repeated *flag* (whose last-wins is a deliberate layering rule) nothing about +// one document expresses an ordering intent. +// +// UseNumber keeps numeric literals as json.Number, so a value passes through to +// the request body as written instead of being rendered from a float64 — 1 stays +// 1 rather than becoming 1e+00, and a large int64 keeps its digits. +func decodeJSONObject(data []byte) (map[string]any, error) { + dec := json.NewDecoder(bytes.NewReader(data)) + dec.UseNumber() + + tok, err := dec.Token() + if err != nil { + return nil, fmt.Errorf("invalid JSON: %w", err) + } + if delim, ok := tok.(json.Delim); !ok || delim != '{' { + return nil, fmt.Errorf("expected a JSON object") + } + + out := make(map[string]any) + for dec.More() { + keyTok, err := dec.Token() + if err != nil { + return nil, fmt.Errorf("invalid JSON: %w", err) + } + key, ok := keyTok.(string) + if !ok { + return nil, fmt.Errorf("invalid JSON: expected an object key") + } + if _, dup := out[key]; dup { + return nil, fmt.Errorf("duplicate key %q", key) + } + var val any + if err := dec.Decode(&val); err != nil { + return nil, fmt.Errorf("invalid JSON for key %q: %w", key, err) + } + out[key] = val + } + // Consumes the closing '}' and confirms nothing follows it, so trailing + // content after a complete object is an error rather than being ignored. + if _, err := dec.Token(); err != nil { + return nil, fmt.Errorf("invalid JSON: %w", err) + } + if dec.More() { + return nil, fmt.Errorf("unexpected trailing content after the JSON object") + } + return out, nil +} diff --git a/cmd/additionalparams_test.go b/cmd/additionalparams_test.go new file mode 100644 index 0000000..d8c5366 --- /dev/null +++ b/cmd/additionalparams_test.go @@ -0,0 +1,211 @@ +package cmd + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// renderJSON encodes a value canonically so a test can compare a decoded dict +// against an expected shape in one string comparison. Go's encoder sorts map +// keys, which makes the result stable. +func renderJSON(t *testing.T, v any) string { + t.Helper() + data, err := json.Marshal(v) + if err != nil { + t.Fatalf("marshaling %#v: %v", v, err) + } + return string(data) +} + +func TestLoadAdditionalParametersNone(t *testing.T) { + // Nil rather than an empty map: the marshaler treats an empty overlay as no + // overlay, and nil is how "no values given" is said without allocating. + got, err := loadAdditionalParameters(nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != nil { + t.Errorf("got %#v, want nil for no flag values", got) + } +} + +func TestLoadAdditionalParametersInline(t *testing.T) { + got, err := loadAdditionalParameters([]string{`{"a":1,"b":"two"}`}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if s := renderJSON(t, got); s != `{"a":1,"b":"two"}` { + t.Errorf("got %s, want the dict as written", s) + } +} + +// TestLoadAdditionalParametersNumbersAreVerbatim verifies a numeric literal +// survives unchanged. +// +// The substance is the large integer: decoding into an `any` without UseNumber +// yields a float64, which cannot hold this value exactly and re-renders in +// exponent form. A caller passing an ID or a byte count would see it altered. +func TestLoadAdditionalParametersNumbersAreVerbatim(t *testing.T) { + got, err := loadAdditionalParameters([]string{`{"big":123456789012345678,"one":1,"frac":1.50}`}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if s := renderJSON(t, got); s != `{"big":123456789012345678,"frac":1.50,"one":1}` { + t.Errorf("got %s; numeric literals must pass through as written", s) + } +} + +func TestLoadAdditionalParametersFromFile(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "extra.json") + if err := os.WriteFile(path, []byte(`{"resources":{"limits":{"cpu":"2"}}}`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + got, err := loadAdditionalParameters([]string{path}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if s := renderJSON(t, got); s != `{"resources":{"limits":{"cpu":"2"}}}` { + t.Errorf("got %s, want the file's contents", s) + } +} + +// TestLoadAdditionalParametersMergesLastWins verifies repeated values merge, and +// that a shared key takes the later value whole. +// +// The "nested" key is the assertion that matters: a deep merge would produce +// {"a":1,"b":2} there. Shallow replacement is what lets a caller *replace* a +// structure an earlier file set, rather than only add to it. +func TestLoadAdditionalParametersMergesLastWins(t *testing.T) { + got, err := loadAdditionalParameters([]string{ + `{"first":1,"shared":"early","nested":{"a":1}}`, + `{"second":2,"shared":"late","nested":{"b":2}}`, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + const want = `{"first":1,"nested":{"b":2},"second":2,"shared":"late"}` + if s := renderJSON(t, got); s != want { + t.Errorf("got %s, want %s", s, want) + } +} + +// TestLoadAdditionalParametersMixesInlineAndFile verifies the two forms merge +// with each other under the same last-wins rule. +func TestLoadAdditionalParametersMixesInlineAndFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "base.json") + if err := os.WriteFile(path, []byte(`{"fromFile":true,"shared":"file"}`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + got, err := loadAdditionalParameters([]string{path, `{"shared":"inline"}`}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if s := renderJSON(t, got); s != `{"fromFile":true,"shared":"inline"}` { + t.Errorf("got %s, want the inline value to win over the file's", s) + } +} + +// TestLoadAdditionalParametersLeadingBraceIsInline verifies classification is by +// the leading character and not by what exists on disk. +// +// A file whose *name* is a JSON document is created here deliberately. Probing +// the filesystem first would read it and the assertion below would see +// "fromFile"; sniffing '{' means the value the user typed is the document, +// whatever the directory happens to contain. +func TestLoadAdditionalParametersLeadingBraceIsInline(t *testing.T) { + dir := t.TempDir() + t.Chdir(dir) + + const name = `{"a":1}` + if err := os.WriteFile(filepath.Join(dir, name), []byte(`{"fromFile":true}`), 0o600); err != nil { + t.Skipf("this platform cannot create a file named %q: %v", name, err) + } + + got, err := loadAdditionalParameters([]string{name}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if s := renderJSON(t, got); s != `{"a":1}` { + t.Errorf("got %s; a value starting with '{' is inline JSON, not a filename", s) + } +} + +// TestLoadAdditionalParametersLeadingWhitespace verifies an inline dict is +// recognized despite surrounding whitespace, which a shell heredoc or a +// copy-paste readily introduces. +func TestLoadAdditionalParametersLeadingWhitespace(t *testing.T) { + got, err := loadAdditionalParameters([]string{" \n\t{\"a\":1}\n "}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if s := renderJSON(t, got); s != `{"a":1}` { + t.Errorf("got %s, want the surrounding whitespace ignored", s) + } +} + +func TestLoadAdditionalParametersErrors(t *testing.T) { + missing := filepath.Join(t.TempDir(), "nope.json") + badJSONFile := filepath.Join(t.TempDir(), "bad.json") + if err := os.WriteFile(badJSONFile, []byte(`{"a":`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + arrayFile := filepath.Join(t.TempDir(), "array.json") + if err := os.WriteFile(arrayFile, []byte(`[1,2]`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + for _, tc := range []struct { + name string + value string + want string // substring the error must contain + }{ + // A bare word is a filename, so the error names the missing file rather + // than reporting a JSON syntax problem in text the user meant as a path. + {"missing file", missing, "nope.json"}, + {"bare word", "notjson", "notjson"}, + {"empty value", "", "must not be empty"}, + {"whitespace only", " ", "must not be empty"}, + {"truncated inline", `{"a":`, "invalid JSON"}, + {"truncated file", badJSONFile, "invalid JSON"}, + // An array has no names to merge by, so it is refused rather than dropped. + {"array file", arrayFile, "expected a JSON object"}, + // A duplicate key in one document is a mistake, not a layering intent. + {"duplicate key", `{"a":1,"a":2}`, `duplicate key "a"`}, + // Trailing content means the value was not the single dict it appeared to + // be; ignoring it would silently drop the second half. + {"trailing content", `{"a":1} {"b":2}`, "trailing"}, + } { + t.Run(tc.name, func(t *testing.T) { + _, err := loadAdditionalParameters([]string{tc.value}) + if err == nil { + t.Fatalf("expected an error for %q", tc.value) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %v, want it to mention %q", err, tc.want) + } + // Every error identifies the flag, so a user with several flags on the + // command line knows which one to fix. + if !strings.Contains(err.Error(), additionalParameterFlagName) { + t.Errorf("error = %v, want it to name --%s", err, additionalParameterFlagName) + } + }) + } +} + +// TestLoadAdditionalParametersErrorStopsAtFirstBadValue verifies a later bad +// value fails the whole call rather than the good values being used. +func TestLoadAdditionalParametersErrorStopsAtFirstBadValue(t *testing.T) { + got, err := loadAdditionalParameters([]string{`{"a":1}`, `{"b":`}) + if err == nil { + t.Fatal("expected an error for the second value") + } + if got != nil { + t.Errorf("got %#v, want nil; a partial merge must not be returned", got) + } +} diff --git a/cmd/agents_import.go b/cmd/agents_import.go index b31ca4b..c6ab971 100644 --- a/cmd/agents_import.go +++ b/cmd/agents_import.go @@ -23,6 +23,19 @@ var importDeploymentType string // reason behind it. var importCreateHTTPRoute bool +// importAdditionalParameterJSON backs the persistent, repeatable +// --additionalParameterJSON flag on the import group. Each value is a JSON dict +// or the name of a file containing one; all of them are merged and overlaid onto +// the request body. See loadAdditionalParameters. +// +// Persistent, like the two flags above, because reaching a backend field the CLI +// has no flag for is not specific to how the agent was built. +// +// A StringArray with a nil default, for the reasons given for --envVar below: a +// JSON document routinely contains commas and quotes, which a StringSlice would +// split or reject outright, and a non-nil slice default leaks between tests. +var importAdditionalParameterJSON []string + // newAgentsImportCmd builds the `agents import` command and its two // subcommands, `from-image` and `from-source`. // @@ -36,6 +49,8 @@ func newAgentsImportCmd() *cobra.Command { "workload type for the agent: deployment|statefulset|job|sandbox") importCmd.PersistentFlags().BoolVar(&importCreateHTTPRoute, "createHttpRoute", false, "create an HTTPRoute exposing the agent") + importCmd.PersistentFlags().StringArrayVar(&importAdditionalParameterJSON, additionalParameterFlagName, nil, + "JSON dict, or a file containing one, merged into the request body (repeatable; later values and these keys win)") importCmd.AddCommand( newAgentsImportFromImageCmd(), @@ -64,7 +79,13 @@ current context's namespace. --deployment-type selects the workload type. Env vars come from --envVarsURL (a document of newline-separated key=value pairs) and from --envVar key=value, which may be repeated. When both name the -same variable, --envVar wins, whatever order the flags appear in.`, +same variable, --envVar wins, whatever order the flags appear in. + +--additionalParameterJSON sends request fields this command has no flag for. Its +value is either a JSON dict or the name of a file containing one, and the flag +may be repeated; all of the dicts are merged, a later one winning for a key they +share, and the result is overlaid onto the request body. A key that names a field +the flags above already set replaces it.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { if name == "" { @@ -94,6 +115,13 @@ same variable, --envVar wins, whatever order the flags appear in.`, // fetched document for the same name. envVars := mergeEnvVars(docEnvVars, flagEnvVars) + // Before newClient, like the env-var parsing above: a malformed dict or + // an unreadable file must fail without having created the agent. + additional, err := loadAdditionalParameters(importAdditionalParameterJSON) + if err != nil { + return err + } + client, err := newClient(cmd) if err != nil { return err @@ -107,6 +135,11 @@ same variable, --envVar wins, whatever order the flags appear in.`, ImagePullSecret: imagePullSecret, EnvVars: envVars, CreateHTTPRoute: importCreateHTTPRoute, + + // Set last, but applied last as well: the overlay happens when the + // request is marshaled, so it wins over every field above — including + // PersistentStorage, assigned after this literal. + AdditionalParameters: additional, } if storageSize != "" { request.PersistentStorage = &apiclient.PersistentStorageConfig{ diff --git a/cmd/agents_import_test.go b/cmd/agents_import_test.go index d330b95..1f62725 100644 --- a/cmd/agents_import_test.go +++ b/cmd/agents_import_test.go @@ -5,6 +5,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" ) @@ -547,6 +549,270 @@ func TestAgentsImportCreateHTTPRouteExplicitFalse(t *testing.T) { } } +// TestAgentsImportAdditionalParameterJSON verifies an inline dict reaches the +// request body alongside the fields the flags set. +func TestAgentsImportAdditionalParameterJSON(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", `{"serviceAccount":"orders-sa","replicas":3}`); err != nil { + t.Fatalf("import: %v", err) + } + + if body["serviceAccount"] != "orders-sa" { + t.Errorf("serviceAccount = %v, want orders-sa", body["serviceAccount"]) + } + // JSON numbers decode to float64 on this side of the wire; the value is what + // matters, not the Go type the test's decoder chose. + if body["replicas"] != float64(3) { + t.Errorf("replicas = %#v, want 3", body["replicas"]) + } + // The flags' own fields must survive the overlay. + if body["name"] != "orders" || body["containerImage"] != "img" { + t.Errorf("flag-set fields lost: %+v", body) + } +} + +// TestAgentsImportAdditionalParameterJSONFromFile verifies a value naming a file +// sends that file's contents. +func TestAgentsImportAdditionalParameterJSONFromFile(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + path := filepath.Join(t.TempDir(), "extra.json") + if err := os.WriteFile(path, []byte(`{"nodeSelector":{"disktype":"ssd"}}`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", path); err != nil { + t.Fatalf("import: %v", err) + } + + selector, ok := body["nodeSelector"].(map[string]any) + if !ok { + t.Fatalf("nodeSelector = %#v, want an object", body["nodeSelector"]) + } + if selector["disktype"] != "ssd" { + t.Errorf("nodeSelector = %#v, want disktype ssd", selector) + } +} + +// TestAgentsImportAdditionalParameterJSONRepeatedMerges verifies repeating the +// flag merges the dicts, with a later value winning a shared key. +func TestAgentsImportAdditionalParameterJSONRepeatedMerges(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + path := filepath.Join(t.TempDir(), "base.json") + if err := os.WriteFile(path, []byte(`{"fromFile":true,"shared":"file"}`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", path, + "--additionalParameterJSON", `{"shared":"inline","fromFlag":true}`); err != nil { + t.Fatalf("import: %v", err) + } + + if body["fromFile"] != true || body["fromFlag"] != true { + t.Errorf("both dicts should contribute: %+v", body) + } + if body["shared"] != "inline" { + t.Errorf("shared = %v, want inline (the later value wins)", body["shared"]) + } +} + +// TestAgentsImportAdditionalParameterJSONOverridesFlags verifies a key naming a +// field the CLI already sets replaces it. +// +// The createHttpRoute case is the one worth pinning: it is a bool with no +// omitempty, so it is always present in the encoded request. Overriding it proves +// the overlay replaces a member that is already there rather than only filling in +// absent ones. +func TestAgentsImportAdditionalParameterJSONOverridesFlags(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", + `{"containerImage":"override:1","workloadType":"job","createHttpRoute":true}`); err != nil { + t.Fatalf("import: %v", err) + } + + if body["containerImage"] != "override:1" { + t.Errorf("containerImage = %v, want the additional JSON to win", body["containerImage"]) + } + if body["workloadType"] != "job" { + t.Errorf("workloadType = %v, want job", body["workloadType"]) + } + if body["createHttpRoute"] != true { + t.Errorf("createHttpRoute = %v, want true from the additional JSON", body["createHttpRoute"]) + } +} + +// TestAgentsImportAdditionalParameterJSONOverridesPersistentStorage verifies the +// overlay also beats a field assigned after the request literal is built. +// +// --storage-size is set on the struct in a later statement than +// AdditionalParameters, so a naive implementation that merged at construction +// time would have this one field escape the overlay. +func TestAgentsImportAdditionalParameterJSONOverridesPersistentStorage(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + if _, err := execute(t, "agents", "import", "--deployment-type", "statefulset", "from-image", + "--name", "orders", "--containerImage", "img", "--storage-size", "5Gi", + "--additionalParameterJSON", `{"persistentStorage":{"enabled":true,"size":"20Gi"}}`); err != nil { + t.Fatalf("import: %v", err) + } + + storage, ok := body["persistentStorage"].(map[string]any) + if !ok { + t.Fatalf("persistentStorage = %#v, want an object", body["persistentStorage"]) + } + if storage["size"] != "20Gi" { + t.Errorf("persistentStorage size = %v, want 20Gi from the additional JSON", storage["size"]) + } +} + +// TestAgentsImportAdditionalParameterJSONInvalid verifies a malformed value fails +// the command and sends nothing. +// +// The no-request assertion is the substance, exactly as for --envVar: parsing +// after the client call would still return an error while having already created +// the agent. +func TestAgentsImportAdditionalParameterJSONInvalid(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", `{"a":`) + if err == nil { + t.Fatal("expected an error for a truncated dict") + } + if !strings.Contains(err.Error(), "additionalParameterJSON") { + t.Errorf("error should name the flag: %v", err) + } + if body != nil { + t.Errorf("no agent should have been created, but the server received %+v", body) + } +} + +// TestAgentsImportAdditionalParameterJSONAbsent verifies the request body is +// unchanged when the flag is not used. +// +// The unknown-key check is the point: an implementation that always merged +// through a map could introduce a stray key (an empty overlay object, say), and a +// server rejecting unknown fields would start failing imports that never asked +// for this feature. +func TestAgentsImportAdditionalParameterJSONAbsent(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img"); err != nil { + t.Fatalf("import: %v", err) + } + + known := map[string]bool{ + "name": true, "namespace": true, "deploymentMethod": true, "workloadType": true, + "envVars": true, "persistentStorage": true, "containerImage": true, + "imagePullSecret": true, "gitUrl": true, "gitPath": true, "gitBranch": true, + "createHttpRoute": true, + } + for k := range body { + if !known[k] { + t.Errorf("unexpected key %q in a request with no --additionalParameterJSON: %+v", k, body) + } + } +} + +// TestAgentsImportAdditionalParameterJSONFlagSurface verifies both subcommands +// document the flag and that it is a string array. +// +// The type assertion matters here more than for --envVar: a StringSlice would +// split an inline dict on its commas, so '{"a":1,"b":2}' would arrive as the two +// fragments '{"a":1' and 'b":2}', neither of which is JSON. +func TestAgentsImportAdditionalParameterJSONFlagSurface(t *testing.T) { + isolateHome(t) + for _, sub := range []string{"from-image", "from-source"} { + out, err := execute(t, "agents", "import", sub, "--help") + if err != nil { + t.Errorf("%s --help: %v", sub, err) + continue + } + if !strings.Contains(out, "--additionalParameterJSON") { + t.Errorf("%s --help does not document --additionalParameterJSON:\n%s", sub, out) + } + + cmd, _, err := rootCmd.Find([]string{"agents", "import", sub}) + if err != nil { + t.Fatalf("could not find %s: %v", sub, err) + } + f := cmd.InheritedFlags().Lookup("additionalParameterJSON") + if f == nil { + t.Fatalf("%s does not inherit --additionalParameterJSON", sub) + } + if f.Value.Type() != "stringArray" { + t.Errorf("%s --additionalParameterJSON is a %s; it must be a stringArray, or inline dicts are CSV-split", + sub, f.Value.Type()) + } + } +} + +// TestAgentsImportAdditionalParameterJSONAcrossRuns verifies flag state does not +// leak between two runs in one process, as TestAgentsImportEnvVarIsRepeatableAcrossRuns +// does for --envVar; the same nil-default reasoning applies. +func TestAgentsImportAdditionalParameterJSONAcrossRuns(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newImportServer(t, &body) + setupImportContext(t, srv, "team1") + + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", `{"first":true}`); err != nil { + t.Fatalf("first import: %v", err) + } + if body["first"] != true { + t.Fatalf("first run did not send its dict: %+v", body) + } + + body = nil + if _, err := execute(t, "agents", "import", "from-image", + "--name", "orders", "--containerImage", "img", + "--additionalParameterJSON", `{"second":true}`); err != nil { + t.Fatalf("second import: %v", err) + } + if _, leaked := body["first"]; leaked { + t.Errorf("the first run's dict leaked into the second: %+v", body) + } + if body["second"] != true { + t.Errorf("second run did not send its dict: %+v", body) + } +} + // TestAgentsImportCreateHTTPRouteIsPersistent verifies the flag is accepted on // the group and by both subcommands, like --deployment-type. func TestAgentsImportCreateHTTPRouteIsPersistent(t *testing.T) { diff --git a/cmd/tools_import.go b/cmd/tools_import.go index b2ce05e..ae2e6a3 100644 --- a/cmd/tools_import.go +++ b/cmd/tools_import.go @@ -25,6 +25,12 @@ var toolsImportDeploymentType string // reason behind it. var toolsImportCreateHTTPRoute bool +// toolsImportAdditionalParameterJSON backs the persistent, repeatable +// --additionalParameterJSON flag on the tools import group, mirroring the agents +// one; see importAdditionalParameterJSON in agents_import.go for why it is +// persistent and a nil-defaulted StringArray. +var toolsImportAdditionalParameterJSON []string + // newToolsImportCmd builds the `tools import` command and its two subcommands, // `from-image` and `from-source`, mirroring `agents import`. // @@ -39,6 +45,8 @@ func newToolsImportCmd() *cobra.Command { "workload type for the tool: deployment|statefulset") importCmd.PersistentFlags().BoolVar(&toolsImportCreateHTTPRoute, "createHttpRoute", false, "create an HTTPRoute exposing the tool") + importCmd.PersistentFlags().StringArrayVar(&toolsImportAdditionalParameterJSON, additionalParameterFlagName, nil, + "JSON dict, or a file containing one, merged into the request body (repeatable; later values and these keys win)") importCmd.AddCommand( newToolsImportFromImageCmd(), @@ -70,7 +78,13 @@ current context's namespace. --deployment-type selects the workload type. Env vars come from --envVarsURL (a document of newline-separated key=value pairs) and from --envVar key=value, which may be repeated. When both name the -same variable, --envVar wins, whatever order the flags appear in.`, +same variable, --envVar wins, whatever order the flags appear in. + +--additionalParameterJSON sends request fields this command has no flag for. Its +value is either a JSON dict or the name of a file containing one, and the flag +may be repeated; all of the dicts are merged, a later one winning for a key they +share, and the result is overlaid onto the request body. A key that names a field +the flags above already set replaces it.`, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, _ []string) error { if name == "" { @@ -102,6 +116,13 @@ same variable, --envVar wins, whatever order the flags appear in.`, return err } + // Before newClient, like the parsing above: a malformed dict or an + // unreadable file must fail without having created the tool. + additional, err := loadAdditionalParameters(toolsImportAdditionalParameterJSON) + if err != nil { + return err + } + client, err := newClient(cmd) if err != nil { return err @@ -116,6 +137,10 @@ same variable, --envVar wins, whatever order the flags appear in.`, EnvVars: envVars, ServicePorts: servicePorts, CreateHTTPRoute: toolsImportCreateHTTPRoute, + + // Applied when the request is marshaled, so it wins over every field + // above. + AdditionalParameters: additional, }) if err != nil { return err diff --git a/cmd/tools_import_test.go b/cmd/tools_import_test.go index e6acc78..b0a6edc 100644 --- a/cmd/tools_import_test.go +++ b/cmd/tools_import_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" ) @@ -457,3 +459,123 @@ func TestToolsImportCreateHTTPRouteIsPersistent(t *testing.T) { } } } + +// TestToolsImportAdditionalParameterJSON verifies inline and file values merge +// into the request body, with a later value winning a shared key. +// +// The merge rules themselves are covered once, over the shared helper, in +// additionalparams_test.go; what this pins is that `tools import` is wired to that +// helper at all, and that the tool's own fields survive the overlay. +func TestToolsImportAdditionalParameterJSON(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newToolsImportServer(t, &body) + setupToolsImportContext(t, srv, "team1") + + path := filepath.Join(t.TempDir(), "extra.json") + if err := os.WriteFile(path, []byte(`{"fromFile":true,"shared":"file"}`), 0o600); err != nil { + t.Fatalf("writing fixture: %v", err) + } + + if _, err := execute(t, "tools", "import", "from-image", + "--name", "weather-mcp", "--containerImage", "img", + "--additionalParameterJSON", path, + "--additionalParameterJSON", `{"shared":"inline","serviceAccount":"mcp-sa"}`); err != nil { + t.Fatalf("import: %v", err) + } + + if body["fromFile"] != true || body["serviceAccount"] != "mcp-sa" { + t.Errorf("both dicts should contribute: %+v", body) + } + if body["shared"] != "inline" { + t.Errorf("shared = %v, want inline (the later value wins)", body["shared"]) + } + if body["name"] != "weather-mcp" || body["containerImage"] != "img" { + t.Errorf("flag-set fields lost: %+v", body) + } +} + +// TestToolsImportAdditionalParameterJSONOverridesServicePorts verifies the +// overlay replaces servicePorts, the field --ports builds. +// +// Worth its own test because --ports is the one create field unique to tools, and +// because it has a non-empty default: the encoded request always contains it, so +// overriding it exercises replacement of a present member rather than the +// insertion of an absent one. +func TestToolsImportAdditionalParameterJSONOverridesServicePorts(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newToolsImportServer(t, &body) + setupToolsImportContext(t, srv, "team1") + + if _, err := execute(t, "tools", "import", "from-image", + "--name", "weather-mcp", "--containerImage", "img", + "--additionalParameterJSON", + `{"servicePorts":[{"name":"grpc","port":50051,"targetPort":50051,"protocol":"TCP"}]}`); err != nil { + t.Fatalf("import: %v", err) + } + + ports, ok := body["servicePorts"].([]any) + if !ok || len(ports) != 1 { + t.Fatalf("servicePorts = %#v, want the single overridden entry", body["servicePorts"]) + } + first, ok := ports[0].(map[string]any) + if !ok { + t.Fatalf("servicePorts[0] = %#v, want an object", ports[0]) + } + if first["name"] != "grpc" || first["port"] != float64(50051) { + t.Errorf("servicePorts[0] = %#v, want the grpc entry from the additional JSON", first) + } +} + +// TestToolsImportAdditionalParameterJSONInvalid verifies a malformed value fails +// the command and sends nothing. +func TestToolsImportAdditionalParameterJSONInvalid(t *testing.T) { + isolateHome(t) + var body map[string]any + srv := newToolsImportServer(t, &body) + setupToolsImportContext(t, srv, "team1") + + _, err := execute(t, "tools", "import", "from-image", + "--name", "weather-mcp", "--containerImage", "img", + "--additionalParameterJSON", "no-such-file.json") + if err == nil { + t.Fatal("expected an error for a missing file") + } + if !strings.Contains(err.Error(), "additionalParameterJSON") { + t.Errorf("error should name the flag: %v", err) + } + if body != nil { + t.Errorf("no tool should have been created, but the server received %+v", body) + } +} + +// TestToolsImportAdditionalParameterJSONFlagSurface verifies both subcommands +// inherit the flag and that it is a string array; see the agents counterpart for +// why the type matters. +func TestToolsImportAdditionalParameterJSONFlagSurface(t *testing.T) { + isolateHome(t) + for _, sub := range []string{"from-image", "from-source"} { + out, err := execute(t, "tools", "import", sub, "--help") + if err != nil { + t.Errorf("%s --help: %v", sub, err) + continue + } + if !strings.Contains(out, "--additionalParameterJSON") { + t.Errorf("%s --help does not document --additionalParameterJSON:\n%s", sub, out) + } + + cmd, _, err := rootCmd.Find([]string{"tools", "import", sub}) + if err != nil { + t.Fatalf("could not find %s: %v", sub, err) + } + f := cmd.InheritedFlags().Lookup("additionalParameterJSON") + if f == nil { + t.Fatalf("%s does not inherit --additionalParameterJSON", sub) + } + if f.Value.Type() != "stringArray" { + t.Errorf("%s --additionalParameterJSON is a %s; it must be a stringArray, or inline dicts are CSV-split", + sub, f.Value.Type()) + } + } +} diff --git a/internal/apiclient/apiclient.go b/internal/apiclient/apiclient.go index 0d66a8a..e68f9bb 100644 --- a/internal/apiclient/apiclient.go +++ b/internal/apiclient/apiclient.go @@ -565,6 +565,46 @@ type PersistentStorageConfig struct { Size string `json:"size"` } +// marshalWithAdditional encodes req and overlays additional onto the resulting +// JSON object, with additional winning for a name they share. +// +// Shared by the create-agent and create-tool requests, whose --additionalParameterJSON +// support differs only in which struct is being encoded. +// +// With no additional parameters the encoding of req is returned untouched, so a +// request that does not use the feature is byte-identical to one from before it +// existed — no key reordering, and no empty object where a nil map was. +// +// A req that does not encode to a JSON object would be a programming error here +// (both callers pass a struct), but it is reported rather than assumed: silently +// discarding an overlay the caller asked for is the worse failure. +func marshalWithAdditional(req any, additional map[string]any) ([]byte, error) { + encoded, err := json.Marshal(req) + if err != nil { + return nil, err + } + if len(additional) == 0 { + return encoded, nil + } + + // Decoded into map[string]json.RawMessage rather than map[string]any so the + // fields this struct did populate keep their exact encoding: a round trip + // through any would turn every number into a float64 and re-render it, so an + // int64 too large for a float would come back changed. + var merged map[string]json.RawMessage + if err := json.Unmarshal(encoded, &merged); err != nil { + return nil, fmt.Errorf("request body is not a JSON object: %w", err) + } + for k, v := range additional { + raw, err := json.Marshal(v) + if err != nil { + return nil, fmt.Errorf("encoding additional parameter %q: %w", k, err) + } + merged[k] = raw + } + return json.Marshal(merged) +} + // CreateAgentRequest is the subset of the backend's CreateAgentRequest that // the CLI populates. Fields the server defaults are omitted; only what we set // is sent. deploymentMethod selects image vs source; workloadType selects @@ -595,6 +635,31 @@ type CreateAgentRequest struct { // default agrees today, but sending what the caller asked for should not // depend on the two staying in agreement. CreateHTTPRoute bool `json:"createHttpRoute"` + + // AdditionalParameters are extra top-level members overlaid onto the request + // body, letting a caller reach a backend field this struct has no name for. + // See MarshalJSON, which applies them; the `json:"-"` keeps the map itself + // from being sent as a nested object under some field name. + AdditionalParameters map[string]any `json:"-"` +} + +// MarshalJSON encodes the request and then overlays AdditionalParameters onto +// the resulting object, so an entry named after one of the fields above replaces +// it and an entry naming anything else is added. +// +// The overlay is on the encoded form rather than the struct because that is the +// only place the JSON names exist: a caller supplies "containerImage", not +// ContainerImage. It is also what makes the omitempty fields overridable — a key +// this struct omitted is simply absent from the object, so setting it is the +// same operation as replacing one that is present. +// +// The alias type is what keeps this from recursing: CreateAgentRequest's method +// set includes MarshalJSON, so marshaling the receiver directly would call this +// function again. A defined type with the same underlying struct has no methods, +// so encoding/json falls back to its usual struct encoding for the fields. +func (r CreateAgentRequest) MarshalJSON() ([]byte, error) { + type plain CreateAgentRequest + return marshalWithAdditional(plain(r), r.AdditionalParameters) } // CreateAgentResponse mirrors the backend's CreateAgentResponse model. @@ -695,6 +760,18 @@ type CreateToolRequest struct { // named field on CreateAgentRequest: a false bool is indistinguishable from // an absent one, so omitempty would drop an explicit --createHttpRoute=false. CreateHTTPRoute bool `json:"createHttpRoute"` + + // AdditionalParameters are extra top-level members overlaid onto the request + // body, as on CreateAgentRequest. See MarshalJSON below. + AdditionalParameters map[string]any `json:"-"` +} + +// MarshalJSON overlays AdditionalParameters onto the encoded request, exactly as +// CreateAgentRequest.MarshalJSON does; see that method for why the overlay +// happens after encoding and why the alias type is required. +func (r CreateToolRequest) MarshalJSON() ([]byte, error) { + type plain CreateToolRequest + return marshalWithAdditional(plain(r), r.AdditionalParameters) } // CreateToolResponse mirrors the backend's CreateToolResponse model. diff --git a/internal/apiclient/apiclient_test.go b/internal/apiclient/apiclient_test.go index b4a6c12..d9d8a25 100644 --- a/internal/apiclient/apiclient_test.go +++ b/internal/apiclient/apiclient_test.go @@ -664,3 +664,165 @@ func TestStatusErrorUsesStatusLineWhenBodyEmpty(t *testing.T) { t.Error("Body is empty; want the status line as a fallback") } } + +// TestCreateRequestAdditionalParametersOverlay verifies AdditionalParameters are +// merged into the encoded request as top-level members, winning over the struct's +// own fields. +// +// Both request types are covered in one table because their marshalers differ only +// in which struct is encoded. +func TestCreateRequestAdditionalParametersOverlay(t *testing.T) { + additional := map[string]any{ + // Replaces a field the struct populates. + "containerImage": "override:1", + // Sets a field tagged omitempty, which the struct left absent. + "gitBranch": "release", + // Replaces a bool that has no omitempty, so it is present either way. + "createHttpRoute": true, + // A member the struct has no field for at all. + "serviceAccount": "sa", + } + + for _, tc := range []struct { + name string + req any + }{ + {"agent", CreateAgentRequest{ + Name: "a", Namespace: "team1", DeploymentMethod: "image", WorkloadType: "deployment", + ContainerImage: "img", AdditionalParameters: additional, + }}, + {"tool", CreateToolRequest{ + Name: "a", Namespace: "team1", DeploymentMethod: "image", WorkloadType: "deployment", + ContainerImage: "img", AdditionalParameters: additional, + }}, + } { + t.Run(tc.name, func(t *testing.T) { + data, err := json.Marshal(tc.req) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var got map[string]any + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + if got["containerImage"] != "override:1" { + t.Errorf("containerImage = %v, want the overlay to win", got["containerImage"]) + } + if got["gitBranch"] != "release" { + t.Errorf("gitBranch = %v, want release (an omitted field can be set)", got["gitBranch"]) + } + if got["createHttpRoute"] != true { + t.Errorf("createHttpRoute = %v, want true", got["createHttpRoute"]) + } + if got["serviceAccount"] != "sa" { + t.Errorf("serviceAccount = %v, want sa", got["serviceAccount"]) + } + // Fields the overlay did not name are untouched. + if got["name"] != "a" || got["workloadType"] != "deployment" { + t.Errorf("unrelated fields changed: %+v", got) + } + // The map itself must never appear as a member: it is tagged json:"-", + // and a nested "AdditionalParameters" object would be a field no server + // knows. + if _, present := got["AdditionalParameters"]; present { + t.Errorf("the overlay map leaked in as a member: %+v", got) + } + }) + } +} + +// TestCreateRequestWithoutAdditionalParametersIsUnchanged verifies a request that +// does not use the feature encodes exactly as it did before it existed. +// +// Byte comparison against the same struct marshaled through the alias type is the +// assertion: it catches the overlay path being taken for an empty map, which would +// reorder keys and could add a member, and would break a server that rejects +// unknown fields for callers who never asked for any of this. +func TestCreateRequestWithoutAdditionalParametersIsUnchanged(t *testing.T) { + agent := CreateAgentRequest{ + Name: "a", Namespace: "team1", DeploymentMethod: "image", WorkloadType: "deployment", + ContainerImage: "img", EnvVars: []EnvVar{{Name: "FOO", Value: "bar"}}, + } + type plainAgent CreateAgentRequest + want, err := json.Marshal(plainAgent(agent)) + if err != nil { + t.Fatalf("marshal baseline: %v", err) + } + got, err := json.Marshal(agent) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(got) != string(want) { + t.Errorf("encoding changed with no additional parameters:\n got %s\nwant %s", got, want) + } + + tool := CreateToolRequest{ + Name: "a", Namespace: "team1", DeploymentMethod: "image", WorkloadType: "deployment", + ContainerImage: "img", ServicePorts: []CreateServicePort{{Name: "http", Port: 1, TargetPort: 1, Protocol: "TCP"}}, + } + type plainTool CreateToolRequest + wantTool, err := json.Marshal(plainTool(tool)) + if err != nil { + t.Fatalf("marshal baseline: %v", err) + } + gotTool, err := json.Marshal(tool) + if err != nil { + t.Fatalf("marshal: %v", err) + } + if string(gotTool) != string(wantTool) { + t.Errorf("encoding changed with no additional parameters:\n got %s\nwant %s", gotTool, wantTool) + } +} + +// TestCreateRequestAdditionalParametersThroughPointer verifies the overlay applies +// when the request is marshaled as a pointer, which is how both client methods +// send it. +// +// Not redundant with the value-receiver tests above: a value-receiver MarshalJSON +// is in a pointer's method set, but the reverse is not true, so a marshaler +// written on *CreateAgentRequest would silently do nothing for a value and one on +// the value works for both. This pins the direction the callers actually use. +func TestCreateRequestAdditionalParametersThroughPointer(t *testing.T) { + data, err := json.Marshal(&CreateAgentRequest{ + Name: "a", ContainerImage: "img", + AdditionalParameters: map[string]any{"containerImage": "override:1"}, + }) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var got map[string]any + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if got["containerImage"] != "override:1" { + t.Errorf("containerImage = %v, want the overlay applied through the pointer", got["containerImage"]) + } +} + +// TestCreateAgentSendsAdditionalParameters verifies the overlay survives the real +// POST path, not just a direct json.Marshal. +func TestCreateAgentSendsAdditionalParameters(t *testing.T) { + var gotBody map[string]any + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + _ = json.NewDecoder(r.Body).Decode(&gotBody) + _, _ = w.Write([]byte(`{"success": true}`)) + })) + defer srv.Close() + + c := &Client{BaseURL: srv.URL + "/api/v1/"} + if _, err := c.CreateAgent(context.Background(), &CreateAgentRequest{ + Name: "a", Namespace: "team1", DeploymentMethod: "image", WorkloadType: "deployment", + ContainerImage: "img", + AdditionalParameters: map[string]any{"replicas": 3, "containerImage": "override:1"}, + }); err != nil { + t.Fatalf("CreateAgent: %v", err) + } + + if gotBody["replicas"] != float64(3) { + t.Errorf("replicas = %#v, want 3", gotBody["replicas"]) + } + if gotBody["containerImage"] != "override:1" { + t.Errorf("containerImage = %v, want the overlay to win on the wire", gotBody["containerImage"]) + } +}