Conversation
Part of agentconnect-md#2732 (CP1.1). - add off-by-default sourceCache Helm values with member-only delivery - validate daemon configuration and add SigV4 GET/PUT presigning - cover chart rendering, config validation, signature contracts, and MinIO
![agentconnect-md-test[bot]](https://avatars.githubusercontent.com/in/4157507?s=60&v=4){kind=small}
There was a problem hiding this comment.
Architecture review: CP1.1 Source Cache config + presigner (01c58ad)
The scope matches docs/designs/source-cache.md P1 and the PR states clearly what it leaves out. Most of the rules settled in #2729 hold:
- Only the pool member gets credentials; sandbox pods get no bucket config (§6.4).
- Credentials come from
serviceAccountor a referenced Secret, and nothing is inlined into values. - The defaults are GET 5 min and PUT 15 min.
- The PUT signs
Content-Length,x-amz-checksum-sha256and theac-cache=pendingtag (§9.3). - The §10 limits are Helm values, and the feature is off by default as a no-op (§12).
- The chart refuses
extraEnvnames that collide with its own.
One thing blocks approval. It's cheap to fix now and expensive once later checkpoints build on it.
Blocking: Source Cache config is in the general daemon Config, not a pool-member-only document
The design says "Cluster daemons only" and "Only pool members receive them" (§12). The chart does deliver it only to members, but the daemon side doesn't enforce that:
sourceCacheis added to the sharedConfigSchemawith asecretcredentials arm. So any daemon'sconfig.json(self-hosted,srt, microsandbox) can enable it and store static S3 keys at rest.load-config.tsalso says "a file that states sourceCache wins", which makes the file a supported way to configure it.loadConfigreadsAC_SOURCE_CACHE_*from the environment for every caller, not justrun --k8s. That includeschat,agent list,auth,prepare-upgradeand the evaluation runner (which also copies the raw config file into evaluation roots).- Once it's in
Config, the bucket credentials reach every consumer of the daemon config object. That widens the secret surface past the one component that signs.
The codebase already has the pattern for member-only config: the data-plane document (store/postgres-config.ts). It has its own schema and is read only under --k8s from the pool's Secret mount. It's never part of Config and never read elsewhere. Please give the Source Cache the same shape: a separate member-only document and loader, called from the K8s/pool composition path. With no document the result is undefined, the disabled no-op. Static keys can arrive the same way, as a mounted Secret file, so they don't sit in the member's process environment and any child process it starts can't inherit them. Making that change now means CP1.2+ consumers inject a member-scoped signer and never read cfg.sourceCache.
Non-blocking (please settle before the signer gets its first caller)
- Callers shouldn't choose the presign TTL.
presignGetandpresignPutaccept attlSecondsup to the 7-day SigV4 ceiling. The §10 sweep's safety rule is defined by the GET lifetime: it retags a bundleunreferencedonly after the pointer has stopped naming it for longer than 5 min. A longer GET URL could therefore outlive a collectable object. Please make the design lifetimes fixed ceilings, ideally with no per-call override. serviceAccountonly works with AWS identities. The chain covers env keys, IRSA web-identity and the ECS/EKS Pod Identity container endpoint. GKE Workload Identity and R2 can't produce SigV4 credentials this way, yet the values comment says "IAM/Workload Identity". Also, the default pairregion: auto(an R2 value) pluscredentialSource: serviceAccount(AWS only) can't work together. Please say in the values and chart docs that the other stores needsecret, and pick defaults that are consistent with each other.- Hand-rolled SigV4 and STS. This is a reasonable way to avoid the AWS SDK dependency. But it means this module owns every S3 call. The later
HEAD, retag, conditionalIf-Matchpointer write and sweep deletes should extend it rather than bring in an SDK client alongside it. Otherwise there will be two signing and credential stacks to keep correct. - Lifecycle rule template. The chart change index (§15) includes the bucket lifecycle rule template. The guarantee "no object without a row or a
pendingtag" depends on it. It needs to ship no later than the checkpoint that issues the first PUT URL, and I'll look for it there.
sent by architect (Claude Agent · default) · open in session
| relays: z.array(RelayRosterEntry).default([]), | ||
| // Optional Source Cache. Absence is the disabled no-op; the environment document is | ||
| // parsed by loadConfig and never exposed to sandboxes or the control plane. | ||
| sourceCache: SourceCacheConfigSchema.optional(), |
There was a problem hiding this comment.
Blocking: this makes Source Cache, including the static-key secret arm, part of every daemon's config.json. That goes against §12, which says only pool members get it. Please follow the DataPlaneConfigSchema precedent instead: a separate member-only document read only under --k8s, never part of Config.
There was a problem hiding this comment.
Fixed in 45d9a61. sourceCache is no longer part of ConfigSchema; readSourceCacheConfig now owns a strict versioned member-only document at /var/run/ac-source-cache/config.json, and Daemon.startClusterPlanes composes the signer only under --k8s. Static credentials come from member-local Secret files, not config.json or the process environment. Regression coverage asserts the shared Config has no sourceCache arm.
| // Pool members have no writable config file. The chart supplies the Source Cache | ||
| // document by environment; a file that states sourceCache wins (same precedence as | ||
| // the other operator-owned policy overrides below). | ||
| if ((raw as { sourceCache?: unknown } | null)?.sourceCache === undefined) { |
There was a problem hiding this comment.
Blocking, same root cause: loadConfig serves chat, agent list, auth, prepare-upgrade and evaluation, and all of them would now read AC_SOURCE_CACHE_* and accept a file-stated sourceCache. Please load it in the pool-member composition path only.
There was a problem hiding this comment.
Fixed in 45d9a61. loadConfig no longer imports or reads Source Cache configuration; production no longer uses AC_SOURCE_CACHE_*. The regression sets a malformed AC_SOURCE_CACHE_CONFIG and a file-stated sourceCache arm and proves both are ignored/stripped, while the k8s-mode test proves the member loader is called only under --k8s.
| return { timestamp, day: timestamp.slice(0, 8) } | ||
| } | ||
|
|
||
| function ttlSeconds(value: number | undefined, fallback: number): number { |
There was a problem hiding this comment.
Non-blocking: the §10 sweep's retag rule assumes the GET lifetime is 5 min. If callers can pick up to 7 days, an issued URL could outlive that rule. Please make the design lifetimes fixed ceilings.
There was a problem hiding this comment.
Fixed in 45d9a61. Removed the per-call ttlSeconds override entirely. GET is fixed at 300s and PUT at 900s, with unit and real-MinIO coverage for the contract.
| forcePathStyle: false | ||
| # serviceAccount uses the pool member's IAM/Workload Identity. secret projects static keys | ||
| # from an existing Secret by name; it never embeds credential values in this values file. | ||
| credentialSource: serviceAccount |
There was a problem hiding this comment.
Non-blocking: serviceAccount only resolves AWS identities (env, IRSA, container endpoint). The comment above suggests Workload Identity works too. Also, region: auto (R2) together with serviceAccount can't work. Please document which stores need secret.
There was a problem hiding this comment.
Fixed in 45d9a61. Values and chart README now say serviceAccount resolves AWS identities only, requires a concrete region, and that R2/MinIO/GKE Workload Identity need secret. The default region is us-east-1; the chart now rejects region=auto with serviceAccount, and chart render coverage asserts the AWS-only behavior.
There was a problem hiding this comment.
Request changes: enabling Source Cache with a normal two-key static S3 Secret can prevent the daemon pool from starting. The chart defaults sourceCache.sessionTokenKey to AWS_SESSION_TOKEN and renders it as a required Secret key reference, although static credentials do not have a session token. Make that reference optional or leave it disabled by default. The rest of the CP1.1 config and presigner diff did not reveal another blocking issue.
sent by review-bot (Codex · gpt-6-sol) · open in session
| accessKeyIdKey: AWS_ACCESS_KEY_ID | ||
| secretAccessKeyKey: AWS_SECRET_ACCESS_KEY | ||
| # Optional; set to "" for stores or identities that do not issue a session token. | ||
| sessionTokenKey: AWS_SESSION_TOKEN |
There was a problem hiding this comment.
[P1] Defaulting this optional token key to AWS_SESSION_TOKEN makes the static-key Secret path require three keys. When an operator supplies the ordinary AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY pair and enables credentialSource: secret, the template renders AC_SOURCE_CACHE_SESSION_TOKEN without secretKeyRef.optional; a missing key prevents the daemon-pool pod from starting, so turning on this optional cache can take pool members down. Default this to '' or mark only this Secret key reference optional. Kubernetes documents the missing-key startup behavior: https://kubernetes.io/docs/concepts/configuration/secret/.
There was a problem hiding this comment.
Fixed in 45d9a61. sessionTokenKey now defaults to ``; when empty the credential Secret volume contains only the access-key and secret-key items, so ordinary two-key credentials cannot block the pod. The chart-render contract also covers the explicit session-token case.
|
@agentconnect-md-test please re-review 45d9a61. The two blocking findings and non-blocking 3-5 are addressed in-thread; local verification includes Node config/signer tests, the targeted k8s composition regression, real MinIO PUT/GET, chart render/helm lint, full typecheck, and lint with 0 errors. |
|
This agent responds to requests from this repository's maintainers and trusted contributors. A maintainer can mention it on this thread to have it take a look. |
There was a problem hiding this comment.
Architecture review: CP1.1 Source Cache config + presigner (45d9a61), approved
I checked this against docs/designs/source-cache.md. The blocking finding from the last round is fixed, and so are the non-blocking points.
- Only pool members get the config (§12).
sourceCacheis gone fromConfigSchema, andloadConfigno longer reads Source Cache settings. A strict, versioned document at/var/run/ac-source-cache/config.jsonreplaces it.Daemon.startClusterPlanesreads it and builds the signer only under--k8s. If the document isn't mounted, the feature stays off. If it's present but malformed, startup fails. This follows the data-plane config pattern (store/postgres-config.ts). - Credentials. The ConfigMap holds only file paths. Static keys come from a Secret mounted as files, so they never sit in the member's process environment or in
config.json. Sandbox pods still get nothing (§6.4). - Presign lifetimes. GET is fixed at 300 s and PUT at 900 s, and callers can no longer override them. That keeps the §10 sweep's retag rule, which assumes a 5-minute GET, sound.
- Credential sources. The chart now documents
serviceAccountas AWS-only, defaults the region tous-east-1, and rejectsregion=autotogether withserviceAccount. R2, MinIO and GKE needsecret. - Session token.
sessionTokenKeynow defaults to'', so an ordinary two-key Secret can no longer keep pool pods from starting. That was the other reviewer's finding.
For later checkpoints (not blocking)
- One S3 client. The later
HEAD, retag, conditionalIf-Matchpointer write and sweep deletes should extend this signer and credentials module rather than add an SDK client alongside it. - Lifecycle rule template. The chart's bucket lifecycle rule (§15) must ship no later than the checkpoint that issues the first PUT URL. The guarantee "no object without a row or a
pendingtag" depends on it. - Static key rotation. The member reads the static key files once, at startup. Rotating the Secret therefore takes a pool rollout. Either document that or re-read the files when the signer needs credentials.
- How later code gets the signer. CP1.2+ consumers should receive the member-scoped signer from
startClusterPlanes. They shouldn't import the reader or open the mount themselves, so--k8sremains the only place the signer is created.
sent by architect (Claude Agent · default) · open in session
There was a problem hiding this comment.
Approved for CP1.1 at 45d9a61. The two-key static Secret now mounts without requiring a session token; Source Cache config is a versioned, member-only document read only during --k8s startup, outside the shared daemon Config. The signer has the fixed 5-minute GET and 15-minute PUT lifetimes, and the chart keeps the document and credentials out of sandbox pods. One non-blocking follow-up before cache use: changing an already enabled Source Cache ConfigMap does not change the Deployment pod template, while the signer reads its values only at startup, so a Helm update to endpoint, bucket, prefix, region, or limits needs a member rollout.
sent by review-bot (Codex · gpt-6-sol) · open in session
| {{- include "agentconnect.labels" . | nindent 4 }} | ||
| {{- include "agentconnect.componentLabels" (dict "ctx" . "component" "daemon-pool") | nindent 4 }} | ||
| data: | ||
| config.json: {{ $sourceCacheDocument | toJson | quote }} |
There was a problem hiding this comment.
Non-blocking follow-up: this ConfigMap's data can change on helm upgrade without changing the daemon-pool pod template (its annotation covers only the runtime image). readSourceCacheConfig() runs once during member startup, so an already enabled pool keeps signing with the old endpoint, bucket, region, prefix, and limits until a manual restart. Add a document hash to the pod template before the first cache callers ship, or document the required rollout.
|
Non-blocking, from the P0 S3 matrix (#2731). The signer is right to keep all three headers in The MinIO test covers the tag tamper (403). Since the failure mode is "accepted when not enforced", two more negative cases would pin the other two guarantees against a future signer change:
|
2 participants
Part of #2732. This PR implements CP1.1 only: Source Cache configuration and presigning.
Included
sourceCache.*values for endpoint, region, bucket, prefix, path-style addressing, credential source, and the §10 limits.secretKeyRef; sandbox pods and the reconciler receive no Source Cache configuration.Content-Length,x-amz-checksum-sha256, andx-amz-tagging=ac-cache=pending.Not included
CP1.2+ tables, quota accounting, data-plane paths, shim/inventory changes, workspace read/write-back, sweep, metrics, or end-to-end cache rollout. Those remain in #2732 and its later checkpoints.
Verification
pnpm exec vitest run packages/daemon/test/source-cache-config.test.ts packages/daemon/test/source-cache-signer.test.ts packages/daemon/test/config.test.ts— 62 passed.x-amz-taggingvalue was rejected with 403.helm lint charts/agentconnectandruby scripts/test-chart-render.rb— passed.pnpm lint— 0 errors.pnpm typecheck— passed.Risk
The signer is intentionally not wired into a data-plane path yet; CP1.2+ remains a no-op with respect to object access. ServiceAccount credentials support the environment, IRSA/web-identity, and container-credential provider shapes; the opt-in MinIO test covers static Secret credentials.