diff --git a/internal/toolplane/allowlist.go b/internal/toolplane/allowlist.go index a4d3f5a..c95bbda 100644 --- a/internal/toolplane/allowlist.go +++ b/internal/toolplane/allowlist.go @@ -38,6 +38,8 @@ import ( "fmt" "strings" "time" + + "github.com/bigstack-oss/cube-advisor-agent/pkg/tunnelproto" ) // Impact is what a tool does to the cluster it runs against. It mirrors the @@ -208,8 +210,10 @@ func (b Backend) String() string { return fmt.Sprintf("backend(%d)", int(b)) } -// ControlOp identifies a built-in probe-plane operation. Like Impact it starts -// at 1, so a tool that sets nothing is not accidentally a control tool. +// ControlOp identifies a built-in operation — one this process answers from +// its own state rather than by running a command or calling an API. Like +// Impact it starts at 1, so a tool that sets nothing is not accidentally a +// control tool. type ControlOp int const ( @@ -217,6 +221,9 @@ const ( ControlProbeStart ControlOp = iota + 1 // ControlProbeStatus reports a run's state and, when finished, its metrics. ControlProbeStatus + // ControlInstanceProfile reports what a create would make, for the + // sentence a person approves. + ControlInstanceProfile ) func (c ControlOp) String() string { @@ -225,6 +232,8 @@ func (c ControlOp) String() string { return "probe_start" case ControlProbeStatus: return "probe_status" + case ControlInstanceProfile: + return "describe_instance_profile" } return fmt.Sprintf("control(%d)", int(c)) } @@ -237,6 +246,18 @@ type Tool struct { // Description is for the operator reading the allowlist, not the model. Description string + // Unlisted marks a tool the SaaS calls on its own account and never + // offers to the model. + // + // It is not a security boundary — the executor serves the name to whoever + // holds the tunnel, listed or not, and the level and impact checks are + // what decide whether a call runs. It is an honesty flag for the + // catalogue: without it, a SaaS that compares its model-facing tool list + // against this allowlist must either advertise a tool the model has no + // use for, or carry an exception typed out on that side — which is the + // hand-kept copy toolcatalog exists to abolish. + Unlisted bool + // Argv is the exact command to run. Elements equal to a parameter // placeholder (see Params) are replaced; everything else is literal. // The first element is the executable — resolved from PATH, never a shell. @@ -599,6 +620,23 @@ var Allowlist = []Tool{ // boots afterwards. This bounds the acceptance, not the boot. Timeout: 60 * time.Second, }, + // The create above takes its flavour, image and network from a file on + // this node, and until now the SaaS had no way to learn them. So the + // sentence a person approved said the values were "the cluster's own + // settings" — true, and not something anyone can consent to. This reports + // them, so the sentence can name what will exist. + // + // Read class: it discloses configuration this node's own operator wrote, + // creates nothing and changes nothing. Unlisted, because the model cannot + // choose these values and has no use for them; the SaaS calls it while + // composing an approval prompt. + { + Name: tunnelproto.DescribeInstanceProfile, + Description: "Report the flavour, image and network a create would use, for the approval prompt.", + Control: ControlInstanceProfile, + Impact: ImpactRead, + Unlisted: true, + }, } // ProbeControls is the probe plane's half of the allowlist, kept separate @@ -785,14 +823,17 @@ func (t Tool) validateControl() error { if len(t.Params) > 0 { return fmt.Errorf("tool %q is a control tool and must declare no parameters; its argument is checked by the probe runner, not substituted", t.Name) } - if t.Control != ControlProbeStart && t.Control != ControlProbeStatus { + switch t.Control { + case ControlProbeStart, ControlProbeStatus, ControlInstanceProfile: + default: return fmt.Errorf("tool %q declares an unknown control operation", t.Name) } - // The probe plane creates and destroys its own scratch resources and - // changes no configuration; a control tool claiming a configuring class is - // claiming to be something the probe runner cannot do. + // A control tool answers from this process's own state — a probe run it + // started, a setting its operator wrote. None of that reaches a cluster's + // configuration, so a control tool claiming a configuring class is + // claiming to be something no control op can do. if t.Impact != ImpactRead && t.Impact != ImpactScratch { - return fmt.Errorf("tool %q is a probe-plane control but declares impact %s; the probe plane changes no configuration", t.Name, t.Impact) + return fmt.Errorf("tool %q is a control tool but declares impact %s; a control operation changes no configuration", t.Name, t.Impact) } return nil } diff --git a/internal/toolplane/describe_test.go b/internal/toolplane/describe_test.go new file mode 100644 index 0000000..f0255a6 --- /dev/null +++ b/internal/toolplane/describe_test.go @@ -0,0 +1,129 @@ +package toolplane + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/bigstack-oss/cube-advisor-agent/pkg/tunnelproto" +) + +func describe(t *testing.T, r *Registry) tunnelproto.InstanceProfile { + t.Helper() + out, err := r.Call(context.Background(), tunnelproto.DescribeInstanceProfile, nil) + if err != nil { + t.Fatalf("describe: %v", err) + } + var got tunnelproto.InstanceProfile + if err := json.Unmarshal(out, &got); err != nil { + t.Fatalf("describe returned %s: %v", out, err) + } + return got +} + +// The values a person is asked to approve come from this call, so it has to +// report the ones a create would actually use — not a summary, not a subset. +func TestDescribeReportsTheProfileACreateWouldUse(t *testing.T) { + r, _, _ := newTestRegistry(t) + r.ConfigureInstanceProfile(InstanceProfile{ + Flavor: "m1.large", Image: "ubuntu-24.04", Network: "tenant-net", Project: "acme-prod", + }) + + got := describe(t, r) + want := tunnelproto.InstanceProfile{ + Configured: true, + Flavor: "m1.large", Image: "ubuntu-24.04", Network: "tenant-net", Project: "acme-prod", + } + if got != want { + t.Errorf("describe = %+v, want %+v", got, want) + } +} + +// An agent nobody has configured says so and invents nothing. The caller is +// composing a sentence for a person: empty strings rendered into it would +// promise a machine with no image. +func TestAnUnconfiguredAgentDescribesNoProfile(t *testing.T) { + r, _, _ := newTestRegistry(t) + + got := describe(t, r) + if got.Configured { + t.Errorf("an unconfigured agent reports a profile: %+v", got) + } + if got.Flavor != "" || got.Image != "" || got.Network != "" { + t.Errorf("an unconfigured agent named values: %+v", got) + } +} + +// A half-configured profile refuses every create, so reporting it as +// configured would put values in front of a person for a call that cannot run. +func TestAPartialProfileIsNotReportedAsConfigured(t *testing.T) { + for _, p := range []InstanceProfile{ + {Image: "ubuntu-24.04", Network: "tenant-net"}, + {Flavor: "m1.large", Network: "tenant-net"}, + {Flavor: "m1.large", Image: "ubuntu-24.04"}, + } { + r, _, _ := newTestRegistry(t) + r.ConfigureInstanceProfile(p) + if got := describe(t, r); got.Configured { + t.Errorf("profile %+v reported as configured", p) + } + } +} + +// It answers one question and takes nothing. A caller passing an argument has +// misunderstood what this reports, and ignoring it would let that +// misunderstanding reach an approval prompt. +func TestDescribeRefusesArguments(t *testing.T) { + r, rec, _ := newTestRegistry(t) + r.ConfigureInstanceProfile(InstanceProfile{Flavor: "f", Image: "i", Network: "n"}) + + _, err := r.Call(context.Background(), tunnelproto.DescribeInstanceProfile, + map[string]string{"cluster": "other"}) + if !errors.Is(err, ErrBadArgument) { + t.Fatalf("describe with an argument = %v, want ErrBadArgument", err) + } + if len(rec.calls) == 0 || rec.calls[len(rec.calls)-1].Allowed { + t.Error("the refusal was not audited") + } +} + +// The describing tool reaches no probe runner, and an agent built without one +// still has to say what a create would make. This is the guard on the +// restructure that let a non-probe control op past the runner check. +func TestDescribeWorksWithoutAProbeRunner(t *testing.T) { + r, err := New(Allowlist, &recorder{}) + if err != nil { + t.Fatalf("New: %v", err) + } + if r.probes != nil { + t.Fatal("a registry built without WithProbes has a runner") + } + r.ConfigureInstanceProfile(InstanceProfile{Flavor: "f", Image: "i", Network: "n"}) + + if got := describe(t, r); !got.Configured { + t.Errorf("describe without a probe runner = %+v, want the profile", got) + } +} + +// The model cannot choose a flavour or an image, so a tool reporting them has +// no business in its tool list. Unlisted is what lets the SaaS know that +// absence is deliberate rather than a list gone stale. +func TestDescribeIsNotOfferedToTheModel(t *testing.T) { + var found bool + for _, tool := range Allowlist { + if tool.Name != tunnelproto.DescribeInstanceProfile { + continue + } + found = true + if !tool.Unlisted { + t.Error("describe_instance_profile is listed to the model") + } + if tool.Impact != ImpactRead { + t.Errorf("describe_instance_profile impact = %s, want read", tool.Impact) + } + } + if !found { + t.Fatal("the allowlist has no describe_instance_profile") + } +} diff --git a/internal/toolplane/registry.go b/internal/toolplane/registry.go index c5d77a9..fa5967d 100644 --- a/internal/toolplane/registry.go +++ b/internal/toolplane/registry.go @@ -448,13 +448,20 @@ func (r *Registry) Call(ctx context.Context, name string, args map[string]string return out, runErr } -// callControl dispatches a probe-plane control call. +// callControl dispatches a control call — one this process answers from its +// own state rather than by running a command or calling an API. // -// Both operations are short by construction — starting a probe returns once -// the goroutine is launched, polling one reads a map — so neither needs the -// timeout ladder stretched to fit the measurement it controls. Results go back -// as JSON so the SaaS forwards numbers rather than prose. +// Every operation is short by construction: starting a probe returns once the +// goroutine is launched, polling one reads a map, describing the profile reads +// a struct. None needs the timeout ladder stretched to fit what it reports on. +// Results go back as JSON so the SaaS forwards values rather than prose. func (r *Registry) callControl(ctx context.Context, tool Tool, args map[string]string) ([]byte, error) { + // The profile description is answered before the probe runner is + // considered, because it needs none: an agent with no probe plane still + // has to be able to say what a create would make. + if tool.Control == ControlInstanceProfile { + return r.callDescribeProfile(tool, args) + } if r.probes == nil { // Unreachable: the scratch class is refused at registration without a // runner. Kept so a future control tool that is not scratch-class @@ -509,6 +516,45 @@ func (r *Registry) callControl(ctx context.Context, tool Tool, args map[string]s return nil, fmt.Errorf("%w: %q", ErrUnknownTool, tool.Name) } +// callDescribeProfile reports what a create would make. +// +// It takes no arguments at all, and says so rather than ignoring them: a caller +// passing one has misunderstood what this answers, and silence would let that +// misunderstanding reach an approval prompt. +// +// An unconfigured agent reports Configured false rather than failing. The +// caller is composing a sentence for a person, and "this cluster has not been +// told what to create" is something worth saying — a failure here would leave +// it with nothing and no reason. +func (r *Registry) callDescribeProfile(tool Tool, args map[string]string) ([]byte, error) { + for k := range args { + r.audit.RecordToolCall(ToolCall{ + Tool: tool.Name, Args: args, Allowed: false, + Reason: fmt.Sprintf("unexpected argument %q", k), At: time.Now().UTC(), + }) + return nil, fmt.Errorf("%w: unexpected argument %q", ErrBadArgument, k) + } + p := r.profile + out, err := json.Marshal(tunnelproto.InstanceProfile{ + // A profile is usable only with all three; a half-configured one + // refuses creates, and reporting it as configured would put values in + // an approval prompt for a call that cannot run. + Configured: p.Flavor != "" && p.Image != "" && p.Network != "", + Flavor: p.Flavor, + Image: p.Image, + Network: p.Network, + Project: p.Project, + }) + if err != nil { + return nil, err + } + r.audit.RecordToolCall(ToolCall{ + Tool: tool.Name, Args: args, Allowed: true, + At: time.Now().UTC(), Bytes: len(out), + }) + return out, nil +} + // callGet resolves a Get tool's path and fetches it from cube-cos-api. // // The path is the whole request: the method is GET (the getter offers nothing diff --git a/pkg/toolcatalog/toolcatalog.go b/pkg/toolcatalog/toolcatalog.go index f9176fc..8d10af6 100644 --- a/pkg/toolcatalog/toolcatalog.go +++ b/pkg/toolcatalog/toolcatalog.go @@ -38,6 +38,16 @@ type Entry struct { // comparison that ignored the distinction would demand the two lists // match when they legitimately do not. Probe bool + // Unlisted reports whether this entry is one the SaaS calls on its own + // account rather than offering to the model — describing the instance + // profile for an approval prompt, for instance. + // + // Published for the same reason Probe is. A SaaS comparing its tool list + // against this catalogue has to know which absences are deliberate, and + // the alternative is a list of exceptions typed out over there: a + // hand-kept copy of a fact this side already knows, which is the shape of + // failure this package exists to remove. + Unlisted bool // Reads is the sorted set of catalogue keys this entry accepts, empty for // a tool that is not a catalogue read. @@ -59,7 +69,10 @@ type Entry struct { func Entries() []Entry { out := make([]Entry, 0, len(toolplane.Allowlist)+len(toolplane.ProbeControls)) for _, t := range toolplane.Allowlist { - out = append(out, Entry{Name: t.Name, Impact: t.Impact.String(), Reads: readsOf(t)}) + out = append(out, Entry{ + Name: t.Name, Impact: t.Impact.String(), + Unlisted: t.Unlisted, Reads: readsOf(t), + }) } for _, t := range toolplane.ProbeControls { out = append(out, Entry{Name: t.Name, Impact: t.Impact.String(), Probe: true}) diff --git a/pkg/toolcatalog/toolcatalog_test.go b/pkg/toolcatalog/toolcatalog_test.go index 716c575..5536e00 100644 --- a/pkg/toolcatalog/toolcatalog_test.go +++ b/pkg/toolcatalog/toolcatalog_test.go @@ -29,6 +29,14 @@ func TestEveryAllowlistEntryIsPublished(t *testing.T) { if e.Probe { t.Errorf("%s is not a probe control but is published as one", tool.Name) } + // Whether a tool is offered to the model is the SaaS's only reason to + // tolerate an absence from its own list, so a mismatch here would + // leave that side unable to tell a deliberate omission from a stale + // copy — which is the whole failure this package removes. + if e.Unlisted != tool.Unlisted { + t.Errorf("%s: catalogue says unlisted=%v, allowlist says %v", + tool.Name, e.Unlisted, tool.Unlisted) + } } for _, tool := range toolplane.ProbeControls { if e := got[tool.Name]; !e.Probe { diff --git a/pkg/tunnelproto/toolcall.go b/pkg/tunnelproto/toolcall.go index 24923f5..4cceedb 100644 --- a/pkg/tunnelproto/toolcall.go +++ b/pkg/tunnelproto/toolcall.go @@ -133,3 +133,32 @@ func ReadToolResult(r io.Reader) (ToolResult, error) { // MaxToolOutputBytes bounds a result frame. Generous next to `cluster check` // output, small next to memory exhaustion from a hostile or broken peer. const MaxToolOutputBytes = 1 << 20 + +// DescribeInstanceProfile is the tool the SaaS calls to learn what a create +// would make, so the sentence a person approves can name it. +// +// It is not offered to the model. The model already cannot choose a flavour or +// an image — that is what the profile is for — so advertising a tool that +// reports them would widen what the model sees without widening what it can do. +const DescribeInstanceProfile = "describe_instance_profile" + +// InstanceProfile is what a cluster creates, as the SaaS needs to state it. +// +// It crosses the tool channel as that tool's result. Defined here because both +// repositories read it: the executor renders it from the file its operator +// wrote, the SaaS renders a sentence from it. A shape only one side could +// import gets restated on the other, which is how two definitions of one +// protocol come to disagree — cube-ai-advisor#121 and cube-advisor-agent#24 +// were both green while doing exactly that. +// +// Configured separates "this cluster has no profile" from "its fields are +// empty". Those are different things to tell a person: the first is an operator +// who has not opted in, the second would be a bug. A caller reading only the +// fields could not tell them apart. +type InstanceProfile struct { + Configured bool `json:"configured"` + Flavor string `json:"flavor,omitempty"` + Image string `json:"image,omitempty"` + Network string `json:"network,omitempty"` + Project string `json:"project,omitempty"` +}