fix(api): remove Loki startup dependency - #3600
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Looks good — a focused, well-tested fix that makes Loki optional and replaces the old boolean routing with an explicit three-way persistentLogBackend enum, returning a proper 503 instead of silently falling back or bypassing the flag.
What was reviewed: the LokiURL env tag change and newLokiQueryProvider nil-provider helper in store.go/model.go; the persistentLogBackend routing logic and 503 error path in resources_local.go for both GetSandboxLogs and GetBuildLogs; the interface change from concrete *loki.LokiQueryProvider to lokiLogsReader (correctly guards against the nil-pointer-wrapped-in-non-nil-interface pitfall by nil-checking before assignment); and the accompanying docs/ARCHITECTURE.md update. Test coverage across model_test.go, resources_local_test.go, and store_test.go exercises the flag-enabled/disabled, Loki-present/absent, and no-backend-available combinations.
Extended reasoning...
Overview
This PR makes LOKI_URL optional in the API config, adds a newLokiQueryProvider helper that returns (nil, nil) when unconfigured, and reworks LocalClusterResourceProvider's log-backend selection from a boolean readFromClickhouse into an explicit persistentLogBackend enum (Unavailable/Loki/Clickhouse). When neither backend is usable it now returns a stable 503 APIError instead of panicking on a nil Loki provider or silently bypassing the logs-read-config flag. docs/ARCHITECTURE.md is updated to reflect the new behavior, satisfying the CLAUDE.md requirement to keep architecture docs in sync with behavioral changes.
Security risks
No auth, crypto, or permission-related code is touched. The change only affects availability/routing of log reads (a 503 on misconfiguration is a safer failure mode than the previous crash-on-nil-Loki risk). No injection or data-exposure concerns were introduced.
Level of scrutiny
This is a moderate but self-contained change to a single feature area (log-read backend selection) in the API service, not a security-sensitive path. The routing logic is small, the enum-based switch is exhaustive and easy to follow, and the interface change correctly handles the classic Go "non-nil interface wrapping nil pointer" trap by explicitly nil-checking the concrete *loki.LokiQueryProvider before assigning to the lokiLogsReader interface in newLocalClusterResourceProvider.
Other factors
Test coverage is thorough: new tests cover flag-enabled-without-Loki (ClickHouse used), flag-disabled-without-Loki (503, ClickHouse not touched), no-backend-at-all (503), and Loki-fallback preservation in both the flag-disabled and flag-enabled-without-ClickHouse-reader cases, plus existing ClickHouse-error-metric tests continue to pass. The PR description accurately matches the code, and no bugs were reported by the bug-hunting pass. The repo's blanket CODEOWNERS (* @ ValentaTomas @ jakubno @ dobrac) applies to every PR by construction and isn't a narrower/specialized ownership signal that should raise scrutiny here.
Summary
LOKI_URLoptional and only construct the Loki query provider when it is configuredlogs-read-configas the routing authority: enabled reads use the existing ClickHouse reader, while an unavailable ClickHouse reader falls back to Loki only when Loki existssandbox_logsschema/migration unchangedValidation
make testinpackages/api(full API unit suite with-race)make lintinpackages/api(golangci-lint run --fix ./..., 0 issues)go test -race ./pkg/sandboxlogsinpackages/clickhousegopls checkon all changed Go filesgit diff --checkRollout
No DDL, LaunchDarkly, image, Argo, or infrastructure changes are included. Activating this behavior requires building a new API image and updating Argo separately. Before removing
LOKI_URLfrom a deployment, keeplogs-read-configenabled and the ClickHouse log reader configured; if the flag is disabled/unavailable and Loki is absent, local persistent-log reads intentionally return 503.