Repository navigation
feat(cluster): ankra cluster logs-ship status|enable|disable (ankra-t5jf5.34.11.7) - #431
Conversation
…5jf5.34.11.7)
Hosted log shipping is opt-in per cluster and off by default: customer log
content leaves a cluster only after a member with clusters.write turns the
switch on. These verbs are the CLI surface over the platform's bearer routes
GET/PUT /api/v1/org/clusters/{cluster_id}/hosted-logs.
- status shows the switch, whether hosted logging is available on the
platform yet, whether the agent is new enough to follow it (with the
upgrade command when not) and when it last changed.
- enable says what is sent (every running container's log lines, kept 7
days, readable by the organisation's members) and asks first; without a
terminal it requires --yes (exit 2), a declined prompt exits 4. When the
platform's store is not live yet it says the switch is stored and nothing
ships until Ankra turns hosted logging on.
- disable turns it off without asking.
- A 403 on a write exits 7 naming clusters.write, the platform's 404 is a
cluster outside the organisation (exit 3), the router's bare 404 is a
platform without the switch yet (exit 1), anything else keeps the detail.
-o json|yaml prints the platform's answer.
Bead: ankra-t5jf5.34.11.7
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Ankra AI reviewVerdict: comments. A well-structured, thoroughly tested addition: the client pins method/path/body on the wire, the enable flow explains and confirms before shipping data out, and the error mapping carefully distinguishes unknown 404s from cluster-not-found and route-absent. The main residual concern is the agent-capability field being presented as a definitive fact when a false value can also mean the agent simply has not reported yet. Findings
Reviewed commit 6f36602. This review is read-only and advisory. |
![ankra-platform[bot]](https://avatars.githubusercontent.com/in/1164800?s=60&v=4){kind=small}
| return withExitCode(exitForbidden, fmt.Errorf("%s: %w", operation, apiError)) | ||
| } | ||
| if hasStatus && unexpected.StatusCode == http.StatusNotFound { | ||
| if unexpected.Detail == "" || unexpected.Detail == routeAbsentDetail { |
There was a problem hiding this comment.
[low] 404 with empty detail is assumed to be the router, not the platform
A 404 whose Detail is empty is classified as 'platform predates the switch' (exit 1), while any other detail means 'cluster not in this organisation' (exit 3). That heuristic is only sound if the platform's real cluster-not-found response always carries a non-empty detail and the router's 404 always carries routeAbsentDetail or nothing; the diff does not show how UnexpectedResponseError.Detail is populated (e.g. for a non-JSON or empty 404 body from the platform itself). If a genuine missing-cluster 404 can arrive with empty detail, the user is told the platform lacks the feature instead of being pointed at 'ankra org current'. Worth confirming the client's Detail extraction guarantees this split.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
The split holds because of how the platform answers.
- The platform's cluster-not-found answer is always the JSON body
{"detail": "Cluster not found"}. That covers both the cookie route and the bearer twin (writeUsecaseGateOr500in clusterinternal/cliapi), and it is pinned by cluster#3974'stestForeignClusterNotFound. - A 404 with no
detailcan only come from a router that has no such route, which is a platform that predates the switch. - A non-JSON 404 body from the platform itself has no
detaileither, and that is the same case: something other than the hosted-logs handler answered.
| if getError != nil { | ||
| return hostedLogsError(getError, cluster, false) | ||
| } | ||
| if state == nil { |
There was a problem hiding this comment.
[low] Nil-state checks are unreachable given the client's contract
GetClusterHostedLogs and SetClusterHostedLogShipping only return (nil, err) or (&state, nil) — they never return (nil, nil) — so the state == nil branches here and at line 198 can never fire. Harmless as defensive code, but if the intent is to guard against an empty 200 body, that case would already fail inside sendJSONContext's decode, so the message here would never be the one shown. Consider dropping them or documenting what they guard.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Agreed: the client answers either an error or a decoded state, so these branches could not fire. Removed in 153cae3.
The hosted-logs client methods answer either an error or a decoded state, never neither, so the two branches could not fire. Bead: ankra-t5jf5.34.11.7 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| return withExitCode(exitForbidden, fmt.Errorf("%s: %w", operation, apiError)) | ||
| } | ||
| if hasStatus && unexpected.StatusCode == http.StatusNotFound { | ||
| if unexpected.Detail == "" || unexpected.Detail == routeAbsentDetail { |
There was a problem hiding this comment.
[low] Empty-detail 404 is assumed to be the router, not the platform
Treating unexpected.Detail == "" as 'route absent' means a platform that legitimately answers 404 with an empty body (or a proxy that strips bodies) is reported as 'this Ankra platform does not offer hosted log shipping yet' with exit 1, when the true state is unknown. An absent detail is not evidence of which 404 fired; consider keying the router-404 branch only on the exact routeAbsentDetail sentinel and letting empty detail fall through to the generic not-found handling, or at least wording the message as a possibility rather than a conclusion.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Agreed, and fixed in this push. A 404 is now classified as the router's only when it carries the router's own "Not Found" detail (routeAbsentDetail, which a missing route on the live platform answers: {"detail":"Not Found"}). A 404 with no detail is reported as unknown, naming both possibilities, and is pinned by the new table case "a 404 with no detail is unknown, not a guess".
| switch { | ||
| case state.ShippingEnabled != enabled: |
There was a problem hiding this comment.
[low] Stored state disagreeing with the request still exits 0
When the platform stores a state opposite to what was requested, the command prints a note but returns nil, so a script running enable --yes sees success while shipping is actually off. Since the requested change demonstrably did not take effect, consider returning a non-zero exit (or at least a distinct diagnostic) so automation does not read this as the switch having been flipped.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Agreed, fixed. A stored state that disagrees with the request now prints the stored state (text or -o json) and exits 1 with an error naming both states. This is pinned by TestLogsShipWriteThatDidNotTakeEffectExitsNonZero for both output modes.
| return withExitCode(exitForbidden, fmt.Errorf("%s: %w", operation, apiError)) | ||
| } | ||
| if hasStatus && unexpected.StatusCode == http.StatusNotFound { | ||
| if unexpected.Detail == "" || unexpected.Detail == routeAbsentDetail { |
There was a problem hiding this comment.
[low] Depends on helpers and constants not visible in the diff
This file references routeAbsentDetail, resolveActiveCluster, structuredFormatFromFlags, registerStructuredOutputFlags, encodeStructured, outputDefault, promptIsInteractive, confirmPrompt, errCancelled, formatTimeAgo, withExitCode, exitUsage/exitForbidden/exitNotFound/exitError, and the apiClient package-level variable, none of which are declared in the diff. They presumably exist as shared helpers used by the sibling cluster agent commands, but I could not verify their signatures (e.g. that routeAbsentDetail is the exact detail string the router emits, or that resolveActiveCluster honours --cluster). Please confirm these match the existing definitions.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
These are the existing shared helpers in cmd/ that the sibling cluster agent and targets commands use. routeAbsentDetail is cmd/targets.go:30 ("Not Found"), and resolveActiveCluster honours --cluster (exercised by the --cluster <id> tests here). The package builds, and go test ./... and golangci-lint pass on this commit. The commands were also run against the live platform after cluster#3974 deployed: status, enable --yes, -o json and disable against a playground, an unknown cluster id (404 Cluster not found), and a string body (422 bool_type).
… take effect exits non-zero - A 404 is the router's only when it carries the router's own "Not Found" detail; one with no detail says neither which route nor which cluster, so it is reported as unknown instead of guessed to be a platform without the switch. - When the stored state disagrees with the request, the command prints the stored state (text or JSON) and exits 1, so automation running `enable --yes` cannot read a change that did not happen as success. Bead: ankra-t5jf5.34.11.7 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| if supportsSwitch { | ||
| return "follows the switch" | ||
| } | ||
| return "too old to follow the switch (upgrade the agent)" |
There was a problem hiding this comment.
[low] AgentSupportsSwitch=false presented as 'too old' when it may be unknown
AgentSupportsSwitch is what the agent advertised at its last identify, so a false value conflates 'agent is too old' with 'agent has never identified / has not checked in since the platform learned the capability' — an absent observation, not a negative one. In those cases status will tell the user to upgrade an agent that may already be new enough. Consider having the platform expose a tri-state (or an identified-at timestamp) and rendering the unobserved case as 'not yet known' rather than 'too old'.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Agreed, fixed in f74c3de. agent_supports_switch: false covers two cases: an agent too old to obey the switch, and an agent opted out locally with logs_ship.enabled: false. The status label, the note and the embedded skill doc now name both and say how to fix each.
| if hasStatus && unexpected.StatusCode == http.StatusNotFound { | ||
| if unexpected.Detail == "" { | ||
| return withExitCode(exitError, fmt.Errorf( | ||
| "%s: the platform answered 404 without saying why, so it is not known whether the cluster is "+ | ||
| "outside this organisation or the platform does not offer hosted log shipping yet. "+ | ||
| "Check the selected organisation with 'ankra org current' and try again", operation)) | ||
| } | ||
| if unexpected.Detail == routeAbsentDetail { | ||
| return withExitCode(exitError, fmt.Errorf( | ||
| "%s: this Ankra platform does not offer hosted log shipping yet (the route answered 404). "+ | ||
| "Check the selected organisation with 'ankra org current' and try again once the platform has it", | ||
| operation)) | ||
| } | ||
| return withExitCode(exitNotFound, fmt.Errorf( |
There was a problem hiding this comment.
[low] Any 404 detail other than the router's is assumed to mean cluster-not-found
The 404 mapping treats the router's 'Not Found' as route-absent and every other detail as 'cluster not found in this organisation', but a platform could legitimately return 404 with a different detail (e.g. organisation not found, or a future route-level refusal), which would then be misreported to the user as a cluster problem with exit 3. Matching on a known cluster-not-found detail (or a machine-readable code) rather than 'anything that is not the router default' would make this classification safer.
Ankra AI review. Read-only and advisory.
There was a problem hiding this comment.
Agreed, fixed in f74c3de.
- A 404 is read as "cluster not found" only when it carries the platform's own
Cluster not founddetail. - The router's
Not Foundis still read as "route absent", and an empty detail is still reported as unknown. - Any other detail is surfaced as-is with exit 1. This is pinned by the new table case "a 404 with some other detail keeps that detail".
…ch; read only the platform's own 404 as not-found - agent_supports_switch false is an agent too old to obey the switch OR one opted out locally (logs_ship.enabled: false); the label, note and skill doc now say both instead of only "too old". - A 404 is "cluster not found" only when it carries the platform's "Cluster not found" detail; any other detail is surfaced as-is. Bead: ankra-t5jf5.34.11.7 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 participant
What & why
Hosted log shipping is opt-in per cluster and off by default (decision 2026-10-05): customer log content leaves a cluster only after a member with
clusters.writeturns the switch on. This adds the CLI surface over the platform's bearer routesGET/PUT /api/v1/org/clusters/{cluster_id}/hosted-logs(body{"shipping_enabled": bool}).ankra cluster logs-ship statusshows whether shipping is on, whether hosted logging is available on the platform yet, whether the cluster's agent is new enough to follow the switch (it prints theankra cluster agent upgradehint when not), and when the switch last changed.-o jsonprints the response body (cluster_id,shipping_enabled,available,agent_supports_switch,changed_at).ankra cluster logs-ship enableexplains in one paragraph what it does: the log lines of every running container are sent to Ankra's hosted log store, kept 7 days, readable by the organisation's members, and it can be turned off any time withdisable. Then it asks[y/N]. Without a terminal it refuses unless--yes/-yis passed (exit 2), and declining the prompt exits 4. Whenavailableis false it says the switch is stored but nothing ships until Ankra turns hosted logging on. The explanation and prompt go to stderr, so-o jsonstays parseable.ankra cluster logs-ship disableturns shipping off without asking.permission_deniedshape or a plain 403): "you need permission to change cluster settings (clusters.write)…", exit 7.Cluster not found): cluster not found in this organisation, exit 3.Not Found: the platform does not offer the switch yet, exit 1. This follows thecmd/targets.goprecedent.cluster agent …verbs (--cluster <name|id>or the selected cluster), and--orgapplies as usual.internal/client/hosted_logs.go. I also added the methods to theAPIClientinterface andbaseMock. CHANGELOG (Unreleased), the README command table and the embeddedankra-cliskill are updated.Depends on the platform half (ankraio/cluster, not merged yet). Until the cluster PR that registers
/api/v1/org/clusters/{cluster_id}/hosted-logslands on clustermainandroutes.jsonis regenerated,TestClusterRoutesAreRegisteredfails in CI on/api/v1/org/clusters/%s/hosted-logs. That failure is expected. I left the path out of the allowlist on purpose: that list is a ratchet, so the entry would break CLImasteras soon as the route ships. Re-run CI after the cluster PR merges.Test plan
go build ./...go test ./...(all green locally; the route census test is skipped locally unlessANKRA_CLUSTER_ROUTES_JSONis set)golangci-lint run ./...(go1.26.6 toolchain): 0 issuescmd/cluster_logs_ship_test.go: the real client runs against an httptest platform:status: text output, the caveats (not available, agent too old, never changed),-o jsonas the exact response body includingchanged_at: null, and--cluster <id>overriding the selectionenable --yes: the explanation, the PUT body{"shipping_enabled": true}, and the resultenablewithout--yesand without a terminal is refused (exit 2) with no writeenableon a terminal:ywrites the switch, anything else cancels (exit 4) with no writeenablewhenavailable=falseprints the "stored, nothing ships" noteenable/disablewith-o jsonstays parseabledisablewrites{"shipping_enabled": false}without askingclusters.write(exit 7), 403 on a read keeps the platform's permission, platform 404 → cluster not found (exit 3), router 404 → platform lacks the switch (exit 1), 422 detail surfacedinternal/client/hosted_logs_test.go: method, path and body for each lane,falseis sent rather than dropped, and the cluster id is path-escapedenable --yes,status -o jsonanddisableall rendered as intendedmainDocs checklist
tools/gendocs); ensureShort/Long/Exampleon new commands are written for docs readersBead: ankra-t5jf5.34.11.7
🤖 Generated with Claude Code