Nothing of substance has changed since my last review, and I still recommend approval. The branch is now a single commit, f381862, with the same content I reviewed at bd6087f. In servers/sandbox/pools.go:
filterInventoryApp still runs against each pool's real owner before the app label is read.poolVersion reads concurrency through coreutil.ResolveConfig. That's safe for MODE because ResolveAddonConfig only touches Variables and service Disks.TestPoolInventoryAddonPoolApp still pins the deny case for app-bound callers, and TestSandboxPoolListFilters still runs an addon pool through the real CLI flags.There are no new comments or threads, and evanphx has approved this head.
Who sees addon pools under --app shop. Only cluster admins see shop's database pool there. A caller who owns shop but isn't a cluster admin still won't see it, because ownerless pools need admin rights. That matches what this code did before, so it isn't a regression. I'm mentioning it in case someone reads the description as "app owners now see their addon pools."
Older servers. On servers without the RPC, the legacy fallback filters --app client-side against pool.App(). That path never reads the label, so addon pools there still show - and drop out of a filtered listing. This only affects servers that predate the RPC, so I don't think it needs to block.
Why I'd skip another human pass: the only security-relevant change is moving authorization from a single upfront check to a per-pool check, and that check still runs before any label is read. A test covers the case where an app-bound caller must be denied. Everything else is a contained CLI and inventory change, and each new behavior has its own test.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1426,
"verdict": "ready",
"approve": true,
"event": "approve",
"summary": "Nothing of substance has changed since my last review, and I still recommend approval. The branch is now a single commit, `f381862`, with the same content I reviewed at `bd6087f`. In `servers/sandbox/pools.go`:\n\n- `filterInventoryApp` still runs against each pool's real owner before the `app` label is read.\n- The label is used only to display ownerless pools and to match them against the app filter.\n- `poolVersion` reads concurrency through `coreutil.ResolveConfig`. That's safe for MODE because `ResolveAddonConfig` only touches `Variables` and service `Disks`.\n- `TestPoolInventoryAddonPoolApp` still pins the deny case for app-bound callers, and `TestSandboxPoolListFilters` still runs an addon pool through the real CLI flags.\n\nThere are no new comments or threads, and evanphx has approved this head.\n\n**Who sees addon pools under `--app shop`.** Only cluster admins see shop's database pool there. A caller who owns `shop` but isn't a cluster admin still won't see it, because ownerless pools need admin rights. That matches what this code did before, so it isn't a regression. I'm mentioning it in case someone reads the description as \"app owners now see their addon pools.\"\n\n**Older servers.** On servers without the RPC, the legacy fallback filters `--app` client-side against `pool.App()`. That path never reads the label, so addon pools there still show `-` and drop out of a filtered listing. This only affects servers that predate the RPC, so I don't think it needs to block.\n\nWhy I'd skip another human pass: the only security-relevant change is moving authorization from a single upfront check to a per-pool check, and that check still runs before any label is read. A test covers the case where an app-bound caller must be denied. Everything else is a contained CLI and inventory change, and each new behavior has its own test.",
"posted_to_pr": true,
"head_sha": "f381862a30545d4028d5cce77fe01dc75d40e27f"
}The blocker from my last review is fixed. Addon pools now report their app in the APP column and in JSON, and --app shop includes shop's database pool. I don't have anything left to block on.
What changed. In servers/sandbox/pools.go:70-80, when a pool has no resolved owner, PoolServer.List falls back to the metadata app label, and the app filter now compares against that reported name. The ordering here matters, because a label must never feed into an access decision, and it doesn't. filterInventoryApp(ctx, owner) still runs first, against the real owner, for every pool. For an ownerless pool that check requires cluster admin (servers/sandbox/server.go:192-200). The label is read only for pools that already passed, and a pool with a real owner never uses its label. TestPoolInventoryAddonPoolApp locks this in: an app-bound shop caller sees no ownerless pool, with or without the shop filter. Pinning that negative case is what I'd want from a test here. TestSandboxPoolListFilters runs an addon-style pool through the real CLI flags, which covers the gap I flagged before.
The trade-off. The early exit that used the SandboxPoolAppId index is gone, so a filtered listing now walks every pool, even when the filter names an app that doesn't exist. The commit message says so. Version lookups only happen for pools that match the filter, so the per-pool store calls you were cutting don't come back. That cost seems right to me for a debug inventory command. It also answers the question I left open last time about --app hiding addon pools.
The append(args[:4], …) aliasing trap is gone too; the table run now goes through slices.Clone(table).
Why I think this can merge without another human pass. The one security-relevant change is the move from an upfront authorization check to a per-pool one. It still checks each pool against its real owner before reading any label, and the deny case for app-bound callers has a test. The rest is a contained change to the CLI and inventory code, with a direct test for each new behavior.
Verdict: ready
The speed-up and the filters look correct to me. The new APP column doesn't deliver what the description promises, though. It says the column makes "addon pools and others without a version" identifiable, but addon pools will show - in APP, just as they already do in VERSION and SERVICE. MIR-2191 names exactly these rows as the hard ones to identify, and its done-when asks for each pool's app, so I'm calling this a blocker. The fix is small.
Why addon pools come out blank. ProviderFramework.CreateSandboxPool (pkg/addon/framework.go:162) never sets pool.App. Providers record the owning app only as an app label on the pool's metadata (for example pkg/addon/memcache/dedicated.go: types.LabelSet("addon", …, "app", in.AppName, …)). In PoolServer.List, apps[pool.App] therefore returns the zero ResourceRequest, item.SetApp(owner.Instance) sets "", and the CLI renders orDash(""). The sandbox inventory already handles this case. servers/sandbox/server.go:148-152 falls back to the metadata app label after authorization, for display only. The pool listing needs the same fallback, plus a test with a pool that has no App and an app label. TestSandboxPoolListFilters currently creates every pool with App set, so nothing exercises this path. One thing to decide while you're there: --app shop will still leave out shop's addon pools, because the server narrows by the SandboxPoolAppId index. That may be fine, but users could be surprised that --app hides the database pool they're looking for.
What I'm comfortable with. I checked whether resolving each version once could leak version details across apps. It can't: ver.app == pool.App still runs for every pool before the short ID or mode is set. Switching to ResolveConfig is safe for MODE, because ResolveAddonConfig only changes Variables and service Disks, never Concurrency. App names line up with the flag on the RPC path (owner.Instance), and moving the tests onto RunCommand means the new flags go through the real parser.
A small trap, not a blocker: the final append(args[:4], "--app", "missing") in TestSandboxPoolListFilters overwrites args[4:6] in place. It's harmless only because it's the last use of args.
pool.App set (pkg/addon/framework.go:162; providers put the owner in a metadata app label instead), so owner.Instance is empty here and the APP column shows - for exactly the pools the description says it makes identifiable. servers/sandbox/server.go:148-152 already handles this for sandboxes. After the filterInventoryApp check, decode core_v1alpha.Metadata from ent and, when owner.Instance == "", set the app from md.Labels.Get("app"), for display only. Please also add a test case with an addon-style pool (no App, an app label) that asserts the name appears in the table/JSON.Verdict: not_ready