Summary
Client.CollectSnapshot requires Image, JobName, and ServiceAccountName on AgentConfig, but validates none of them. An SDK caller who omits them gets an opaque Kubernetes rejection deep inside Deploy rather than an ErrCodeInvalidRequest naming the field.
Found in review of #2253 by @njhensley and @yuanchen8911.
Mechanism
DeployAndCollect (pkg/snapshotter/agent.go) validates only ClusterConfigPath and Namespace. The other three are copied verbatim into the Job and RBAC objects (pkg/k8s/agent/job.go, rbac.go), and nothing in pkg/k8s/agent defaults them. The CLI works only because pkg/cli/snapshot.go supplies flag defaults the facade does not share.
Because Deploy creates the ServiceAccount before the Job, the empty ServiceAccount name is rejected first; the empty image and Job name sit behind it.
A comment at pkg/snapshotter/agent.go already calls the Namespace check "the SDK-caller path", which makes the omission of the other three more conspicuous rather than less.
Options
- Default them in the facade to mirror the CLI, so the SDK path is not a foot-gun. Needs a decision on where the agent image default lives, since the CLI derives it from the binary version (
defaultAgentImage() in pkg/cli/root.go) — which the facade has via WithVersion.
- Validate and fail closed with
ErrCodeInvalidRequest naming the missing field. Cheaper, and better than the status quo, but still leaves every caller writing the same three lines.
Option 1 is preferable; option 2 is the fallback if the image default proves contentious.
Interim
#2253 documents the requirement in ExampleClient_CollectSnapshot and in the integration guide, so callers are no longer silently misled. This issue tracks removing the foot-gun rather than documenting it.
Acceptance criteria
- Omitting
Image, JobName, or ServiceAccountName either works (defaulted) or fails with a coded error naming the field, not a raw API-server rejection.
- A test covers the SDK path with a minimal
AgentConfig.
- The example and guide drop the "all three are required" caveat if defaulting lands.
Parent: #2016
Summary
Client.CollectSnapshotrequiresImage,JobName, andServiceAccountNameonAgentConfig, but validates none of them. An SDK caller who omits them gets an opaque Kubernetes rejection deep insideDeployrather than anErrCodeInvalidRequestnaming the field.Found in review of #2253 by @njhensley and @yuanchen8911.
Mechanism
DeployAndCollect(pkg/snapshotter/agent.go) validates onlyClusterConfigPathandNamespace. The other three are copied verbatim into the Job and RBAC objects (pkg/k8s/agent/job.go,rbac.go), and nothing inpkg/k8s/agentdefaults them. The CLI works only becausepkg/cli/snapshot.gosupplies flag defaults the facade does not share.Because
Deploycreates the ServiceAccount before the Job, the empty ServiceAccount name is rejected first; the empty image and Job name sit behind it.A comment at
pkg/snapshotter/agent.goalready calls theNamespacecheck "the SDK-caller path", which makes the omission of the other three more conspicuous rather than less.Options
defaultAgentImage()inpkg/cli/root.go) — which the facade has viaWithVersion.ErrCodeInvalidRequestnaming the missing field. Cheaper, and better than the status quo, but still leaves every caller writing the same three lines.Option 1 is preferable; option 2 is the fallback if the image default proves contentious.
Interim
#2253 documents the requirement in
ExampleClient_CollectSnapshotand in the integration guide, so callers are no longer silently misled. This issue tracks removing the foot-gun rather than documenting it.Acceptance criteria
Image,JobName, orServiceAccountNameeither works (defaulted) or fails with a coded error naming the field, not a raw API-server rejection.AgentConfig.Parent: #2016