analytics-api: run as an idiomatic gears-rust gear#1546
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe analytics-api service is converted into a ToolKit gear. It adds bootstrap-based startup and config loading, stateless tenant override middleware, route registration through a shared OpenAPI registry, and updates handlers and domain logic to use ChangesAnalytics-api ToolKit gear migration
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/backend/services/analytics-api/config/insight.yaml`:
- Line 78: The runnable analytics config currently commits a real MySQL
credential in the database_url field, which should not be present in the
checked-in insight.yaml. Update the config entry to use an empty or
placeholder-only value and rely on deployment-time secret injection instead,
keeping the change localized to the database_url setting in insight.yaml.
- Around line 6-10: The no-auth listener in analytics-api is still reachable
from arbitrary in-cluster workloads, so tighten the trust boundary around the
listener setup and tenant override path. Update the server binding/auth
configuration in insight.yaml and the related crate::auth handling so only
gateway traffic can reach the REST host, either by adding a
NetworkPolicy/mTLS/trusted-proxy validation or by binding the listener to a
gateway-only interface. Make sure the X-Insight-Tenant-Id override is accepted
only after verifying the request came through the expected gateway path.
In `@src/backend/services/analytics-api/src/auth.rs`:
- Around line 44-55: tenant_middleware currently falls back to
SecurityContext::anonymous when the host-injected context is missing, which
should be treated as a deny/fail-closed case instead. Update tenant_middleware
to require a SecurityContext in the request extensions and return an
unauthorized/forbidden Response when it is absent, rather than continuing with
anonymous. Keep read_session_tenant and rebuild_with_tenant for valid contexts
only, and ensure any tenant override logic cannot run without the
gateway-provided SecurityContext.
In `@src/backend/services/analytics-api/src/gear.rs`:
- Around line 83-92: The Redis connection logs in the connection success/error
path leak sensitive credentials by printing cfg.redis_url. Update the logging in
the Redis backend setup in gear.rs to use a redacted representation or only safe
metadata (such as host, port, or db) in both the tracing::info! and
tracing::warn! calls, keeping the error context but never emitting the full
redis:// URL.
In `@src/backend/services/analytics-api/src/main.rs`:
- Around line 95-96: The `--print-config` path in `main` currently prints the
full `config.to_yaml()` output, which can expose secrets. Update the
`cli.print_config` branch to serialize a redacted config instead, masking
sensitive fields like `database_url`, `clickhouse_password`, and `redis_url`
before calling `println!`. Keep the change localized to the
configuration-printing logic so the normal runtime config handling remains
unchanged.
- Around line 100-103: The `Commands::Check` branch in `main` only succeeds
after loading `AppConfig`, so it never verifies the analytics gear config is
present or valid. Update the `check` path to call `extract_gear_config` and then
run a `GearConfig::validate()` helper (or a `gear::validate_config` wrapper) so
`gears.analytics-api.config` and required URLs / partial ClickHouse credentials
are actually validated. Keep the change centered around the `Commands::Check`
match arm and the gear config extraction/validation symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28a136b5-45a3-4533-8209-ca8eaa09d6fd
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
src/backend/services/analytics-api/Cargo.tomlsrc/backend/services/analytics-api/config/insight.yamlsrc/backend/services/analytics-api/src/api/admin/handlers.rssrc/backend/services/analytics-api/src/api/catalog.rssrc/backend/services/analytics-api/src/api/error.rssrc/backend/services/analytics-api/src/api/handlers.rssrc/backend/services/analytics-api/src/api/mod.rssrc/backend/services/analytics-api/src/api/tenant_resolution_tests.rssrc/backend/services/analytics-api/src/auth.rssrc/backend/services/analytics-api/src/config.rssrc/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rssrc/backend/services/analytics-api/src/domain/admin_threshold/live_tests.rssrc/backend/services/analytics-api/src/domain/admin_threshold/service.rssrc/backend/services/analytics-api/src/domain/auth.rssrc/backend/services/analytics-api/src/gear.rssrc/backend/services/analytics-api/src/main.rs
| # The `api-gateway` system gear is the REST host with auth DISABLED — the | ||
| # platform api-gateway is the sole authenticator and proxies to us, so this | ||
| # host injects a single-tenant SecurityContext (DEFAULT_TENANT_ID). The | ||
| # `analytics-api` gear then optionally overrides the tenant from the | ||
| # `X-Insight-Tenant-Id` header (see crate::auth). NO oidc plugin is registered. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for deployment-level isolation around analytics-api:8081.
fd -i 'networkpolicy|service|deployment|ingress|values|kustomization|helm' . \
| xargs -r rg -n -C3 'analytics-api|8081|NetworkPolicy|auth_disabled|X-Insight-Tenant-Id'Repository: constructorfabric/insight
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== analytics-api helm files =="
for f in \
src/backend/services/analytics-api/helm/values.yaml \
src/backend/services/analytics-api/helm/templates/service.yaml \
src/backend/services/analytics-api/helm/templates/deployment.yaml \
src/backend/services/analytics-api/helm/Chart.yaml \
charts/insight/values.yaml
do
echo
echo "---- $f ----"
nl -ba "$f" | sed -n '1,220p'
done
echo
echo "== network policy / exposure search =="
rg -n -C2 'kind: NetworkPolicy|api-gateway|analytics-api|8081|auth_disabled|X-Insight-Tenant-Id|ingress:' \
src/backend/services charts/insight deploy/gitops environments docs \
--glob '!**/target/**' --glob '!**/node_modules/**' || trueRepository: constructorfabric/insight
Length of output: 298
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== analytics-api helm values (selected) =="
sed -n '1,120p' src/backend/services/analytics-api/helm/values.yaml | cat -n
echo
echo "== analytics-api service template =="
sed -n '1,120p' src/backend/services/analytics-api/helm/templates/service.yaml | cat -n
echo
echo "== analytics-api deployment template =="
sed -n '1,140p' src/backend/services/analytics-api/helm/templates/deployment.yaml | cat -n
echo
echo "== NetworkPolicy search =="
rg -n -C3 'kind: NetworkPolicy|apiVersion: networking.k8s.io/v1|podSelector:|namespaceSelector:|policyTypes:|analytics-api|8081' \
src/backend/services charts/insight deploy/gitops docs \
--glob '!**/target/**' --glob '!**/node_modules/**' || trueRepository: constructorfabric/insight
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact NetworkPolicy hits =="
rg -n 'kind: NetworkPolicy|apiVersion: networking.k8s.io/v1' \
src/backend/services charts/insight deploy/gitops environments docs \
--glob '!**/target/**' --glob '!**/node_modules/**' || true
echo
echo "== api-gateway route and auth context =="
sed -n '340,380p' charts/insight/values.yaml | cat -n
echo
sed -n '1,120p' src/backend/services/api-gateway/src/proxy.rs | cat -nRepository: constructorfabric/insight
Length of output: 6763
Restrict the no-auth listener to gateway traffic. The service is ClusterIP, but it still binds 0.0.0.0:8081 and there’s no NetworkPolicy here, so any in-cluster workload can hit analytics-api directly, bypass the gateway’s JWT checks, and set X-Insight-Tenant-Id. Add a policy/mTLS/trusted-proxy check, or bind the host to a gateway-only interface.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/analytics-api/config/insight.yaml` around lines 6 - 10,
The no-auth listener in analytics-api is still reachable from arbitrary
in-cluster workloads, so tighten the trust boundary around the listener setup
and tenant override path. Update the server binding/auth configuration in
insight.yaml and the related crate::auth handling so only gateway traffic can
reach the REST host, either by adding a NetworkPolicy/mTLS/trusted-proxy
validation or by binding the listener to a gateway-only interface. Make sure the
X-Insight-Tenant-Id override is accepted only after verifying the request came
through the expected gateway path.
| pub async fn tenant_middleware(mut req: Request, next: Next) -> Response { | ||
| // Host-injected context (fallback to anonymous if, for some reason, the | ||
| // host didn't inject one — e.g. a direct pod hit on a non-public route). | ||
| let base = req | ||
| .extensions() | ||
| .get::<SecurityContext>() | ||
| .cloned() | ||
| .unwrap_or_else(SecurityContext::anonymous); | ||
|
|
||
| let Some(tenant_id) = tenant_auth.resolve_tenant(session_tenant) else { | ||
| return tenant_unresolved_response(); | ||
| }; | ||
|
|
||
| let ctx = SecurityContext { | ||
| // TODO: populate from JWT `sub` claim when JWT validation lands. | ||
| subject_id: Uuid::nil(), | ||
| insight_tenant_id: tenant_id, | ||
| let ctx = match read_session_tenant(&req) { | ||
| Some(tenant_id) => rebuild_with_tenant(&base, tenant_id), | ||
| None => base, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed when the host SecurityContext is missing.
Line 51 allows tenant-scoped routes to continue with SecurityContext::anonymous, and Line 54 can still honor a tenant override header. Since downstream writes and filters use ctx.subject_tenant_id(), a direct/internal hit without the gateway-injected context can bind requests to an anonymous or header-selected tenant.
🛡️ Proposed fail-closed guard
use axum::extract::Request;
+use axum::http::StatusCode;
use axum::middleware::Next;
-use axum::response::Response;
+use axum::response::{IntoResponse, Response};
@@
- let base = req
- .extensions()
- .get::<SecurityContext>()
- .cloned()
- .unwrap_or_else(SecurityContext::anonymous);
+ let Some(base) = req.extensions().get::<SecurityContext>().cloned() else {
+ tracing::warn!("missing host-injected SecurityContext on tenant-scoped route");
+ return StatusCode::UNAUTHORIZED.into_response();
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub async fn tenant_middleware(mut req: Request, next: Next) -> Response { | |
| // Host-injected context (fallback to anonymous if, for some reason, the | |
| // host didn't inject one — e.g. a direct pod hit on a non-public route). | |
| let base = req | |
| .extensions() | |
| .get::<SecurityContext>() | |
| .cloned() | |
| .unwrap_or_else(SecurityContext::anonymous); | |
| let Some(tenant_id) = tenant_auth.resolve_tenant(session_tenant) else { | |
| return tenant_unresolved_response(); | |
| }; | |
| let ctx = SecurityContext { | |
| // TODO: populate from JWT `sub` claim when JWT validation lands. | |
| subject_id: Uuid::nil(), | |
| insight_tenant_id: tenant_id, | |
| let ctx = match read_session_tenant(&req) { | |
| Some(tenant_id) => rebuild_with_tenant(&base, tenant_id), | |
| None => base, | |
| pub async fn tenant_middleware(mut req: Request, next: Next) -> Response { | |
| // Host-injected context (fallback to anonymous if, for some reason, the | |
| // host didn't inject one — e.g. a direct pod hit on a non-public route). | |
| let Some(base) = req.extensions().get::<SecurityContext>().cloned() else { | |
| tracing::warn!("missing host-injected SecurityContext on tenant-scoped route"); | |
| return StatusCode::UNAUTHORIZED.into_response(); | |
| }; | |
| let ctx = match read_session_tenant(&req) { | |
| Some(tenant_id) => rebuild_with_tenant(&base, tenant_id), | |
| None => base, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/analytics-api/src/auth.rs` around lines 44 - 55,
tenant_middleware currently falls back to SecurityContext::anonymous when the
host-injected context is missing, which should be treated as a deny/fail-closed
case instead. Update tenant_middleware to require a SecurityContext in the
request extensions and return an unauthorized/forbidden Response when it is
absent, rather than continuing with anonymous. Keep read_session_tenant and
rebuild_with_tenant for valid contexts only, and ensure any tenant override
logic cannot run without the gateway-provided SecurityContext.
| if cli.print_config { | ||
| println!("Effective configuration:\n{}", config.to_yaml()?); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact secrets in --print-config.
config.to_yaml() can include database_url, clickhouse_password, and redis_url. Printing the full effective config risks leaking credentials into terminals, CI logs, or support bundles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/analytics-api/src/main.rs` around lines 95 - 96, The
`--print-config` path in `main` currently prints the full `config.to_yaml()`
output, which can expose secrets. Update the `cli.print_config` branch to
serialize a redacted config instead, masking sensitive fields like
`database_url`, `clickhouse_password`, and `redis_url` before calling
`println!`. Keep the change localized to the configuration-printing logic so the
normal runtime config handling remains unchanged.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/insight/templates/secrets.yaml`:
- Around line 156-162: The analytics-api MariaDB DSN in the secrets template is
building `APP__gears__analytics-api__config__database_url` with raw credentials,
unlike the existing `IDENTITY__mariadb__url` handling. Update the DSN
construction in the `charts/insight/templates/secrets.yaml` template to
percent-encode the username and password before formatting the MySQL URL, so
credentials containing special characters are parsed correctly by analytics-api.
In `@deploy/gitops/scripts/compose-app-secrets.sh`:
- Around line 132-138: The DSN composition in compose-app-secrets.sh is writing
raw secret values into APP__gears__analytics-api__config__database_url and
APP__gears__analytics-api__config__redis_url, which can break when credentials
contain reserved characters. Update the secret-generation logic to URL-encode
the user/password parts before building these URLs, and make the behavior match
the existing Helm template handling for the same DSNs. Use the analytics-api
config keys in this script as the place to apply the fix.
In `@src/backend/services/analytics-api/helm/templates/deployment.yaml`:
- Around line 17-18: The deployment rollout trigger only covers the ConfigMap,
so `insight-analytics-api` pods can keep stale values from `envFrom` secrets
such as `tenant_default_id` or rotated credentials. Update the `deployment.yaml`
template to add a checksum annotation for the relevant secret/config source used
by `envFrom`, or wire in a reloader, alongside the existing `checksum/config`
annotation so pods restart when secret-backed config changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98589891-2583-414d-9855-b9105921ea50
📒 Files selected for processing (9)
charts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/gitops/environments/local/values.yaml.templatedeploy/gitops/scripts/compose-app-secrets.shdocker-compose.ymlsrc/backend/services/analytics-api/Dockerfilesrc/backend/services/analytics-api/helm/templates/configmap.yamlsrc/backend/services/analytics-api/helm/templates/deployment.yamlsrc/backend/services/analytics-api/helm/values.yaml
✅ Files skipped from review due to trivial changes (3)
- src/backend/services/analytics-api/helm/values.yaml
- deploy/gitops/environments/local/values.yaml.template
- charts/insight/values.yaml
| APP__gears__analytics-api__config__database_url: "mysql://${MDB_USER}:${MDB_PW}@${MDB_HOST}:${MDB_PORT}/${MDB_DB}" | ||
| APP__gears__analytics-api__config__clickhouse_url: "http://${CH_HOST}:${CH_PORT}" | ||
| APP__gears__analytics-api__config__clickhouse_database: "${CH_DB}" | ||
| APP__gears__analytics-api__config__clickhouse_user: "${CH_USER}" | ||
| APP__gears__analytics-api__config__clickhouse_password: "${CH_PW}" | ||
| APP__gears__analytics-api__config__identity_url: "http://${RELEASE}-identity:8082" | ||
| APP__gears__analytics-api__config__redis_url: "${REDIS_URL}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
URL-encode credentials before writing DSNs.
These values come from Secrets, so BYO passwords containing @, :, /, %, or quotes can produce an invalid MySQL/Redis URL in the generated Secret. Encode credentials before composing the URL strings, matching the Helm template behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/scripts/compose-app-secrets.sh` around lines 132 - 138, The DSN
composition in compose-app-secrets.sh is writing raw secret values into
APP__gears__analytics-api__config__database_url and
APP__gears__analytics-api__config__redis_url, which can break when credentials
contain reserved characters. Update the secret-generation logic to URL-encode
the user/password parts before building these URLs, and make the behavior match
the existing Helm template handling for the same DSNs. Use the analytics-api
config keys in this script as the place to apply the fix.
| # Roll pods when the gears config changes. | ||
| checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether analytics-api has a Secret checksum/reloader contract elsewhere.
rg -n 'checksum/(secret|config)|reloader|insight-analytics-api-config|existingSecret' \
src/backend/services/analytics-api/helm charts/insight deploy/gitopsRepository: constructorfabric/insight
Length of output: 5202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the deployment and related config/secret templates.
for f in \
src/backend/services/analytics-api/helm/templates/deployment.yaml \
src/backend/services/analytics-api/helm/templates/configmap.yaml \
src/backend/services/analytics-api/helm/values.yaml \
charts/insight/templates/secrets.yaml \
deploy/gitops/scripts/compose-app-secrets.sh
do
echo "===== $f ====="
sed -n '1,220p' "$f"
echo
doneRepository: constructorfabric/insight
Length of output: 27004
Add a rollout trigger for insight-analytics-api-config envFrom secrets are only read at pod start, and this deployment only hashes the ConfigMap. Secret rotations or tenant_default_id changes can leave running pods on stale credentials/config unless a secret checksum or reloader is wired in.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/backend/services/analytics-api/helm/templates/deployment.yaml` around
lines 17 - 18, The deployment rollout trigger only covers the ConfigMap, so
`insight-analytics-api` pods can keep stale values from `envFrom` secrets such
as `tenant_default_id` or rotated credentials. Update the `deployment.yaml`
template to add a checksum annotation for the relevant secret/config source used
by `envFrom`, or wire in a reloader, alongside the existing `checksum/config`
annotation so pods restart when secret-backed config changes.
- config/insight.yaml: drop the committed `mysql://insight:insight@…` default; database_url/clickhouse_url are empty and injected per-deploy via APP__* env overrides (no credential in the checked-in runnable config). - gear.rs: redact userinfo from redis_url before logging (redis:// URLs can embed credentials); add `check_config` that validates the gear config section + required URLs without touching any backend. - main.rs: `check` subcommand now runs `gear::check_config` instead of a no-op success. - charts secrets.yaml: percent-encode (urlquery) the MariaDB user/password in the analytics DSN, matching the identity DSN, so operator credentials with :/@/%/ chars produce a valid connection string. Deferred (replied on PR): NetworkPolicy/trust-boundary + auth fail-closed (the deliberate header-trust posture — infra follow-up), --print-config redaction (operator debug flag, parity with api-gateway), bash DSN url-encoding (pre-existing, also affects identity — needs shared helper), Secret-change reload (Secret is externally managed in gitops). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Thanks @coderabbitai — addressed in c08de15. Per-finding: Fixed
Already fixed earlier in this PR
Deferred, with reason
Also flagged as follow-up (not CodeRabbit): the gear's ConfigMap renders as |
|
Tip For best results, initiate chat on the files or code changes.
On the deferred items, your reasoning holds up:
Sounds like a solid state to land this in. Let me know when the NetworkPolicy follow-up or the ConfigMap/Secret naming collision fix are up and I'll take a look. ᕦ(ò_óˇ)ᕤ ✏️ Learnings added
|
- config/insight.yaml: drop the committed `mysql://insight:insight@…` default; database_url/clickhouse_url are empty and injected per-deploy via APP__* env overrides (no credential in the checked-in runnable config). - gear.rs: redact userinfo from redis_url before logging (redis:// URLs can embed credentials); add `check_config` that validates the gear config section + required URLs without touching any backend. - main.rs: `check` subcommand now runs `gear::check_config` instead of a no-op success. - charts secrets.yaml: percent-encode (urlquery) the MariaDB user/password in the analytics DSN, matching the identity DSN, so operator credentials with :/@/%/ chars produce a valid connection string. Deferred (replied on PR): NetworkPolicy/trust-boundary + auth fail-closed (the deliberate header-trust posture — infra follow-up), --print-config redaction (operator debug flag, parity with api-gateway), bash DSN url-encoding (pre-existing, also affects identity — needs shared helper), Secret-change reload (Secret is externally managed in gitops). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
1176808 to
93a80d5
Compare
Migrate analytics-api from a self-hosted axum::serve service to a gears-rust host on toolkit::bootstrap::run_server: - main.rs: toolkit bootstrap (AppConfig::load_or_default → run_server / gear::run_migrate); links the api-gateway REST host + auth/authz/tenant system gears via inventory. - gear.rs (new): AnalyticsApiGear #[toolkit::gear(capabilities=[rest])]. Gear::init reconstructs the full former run_server startup (self-managed MariaDB pool + 45 migrations + CHECK/product-default probes, redis catalog cache, ClickHouse + Identity clients, schema validator, admin-CRUD service), stores AppState in a OnceLock, and tokio::spawns the one-shot validate_all() scan. RestApiCapability::register_rest wires the OperationBuilder routes into the host router + Extension(AppState). - Auth: host runs auth_disabled (the platform api-gateway is the sole authenticator); it injects a SecurityContext with DEFAULT_TENANT_ID (== the single-tenant default the seeded data uses). The former hand-rolled tenant_middleware becomes a thin, stateless layer that reads toolkit_security::SecurityContext and overrides the tenant from X-Insight-Tenant-Id when present. The local SecurityContext struct is removed; handlers + domain services now use toolkit_security only. - DB stays self-managed (no toolkit `db` capability; ClickHouse is not a toolkit-db backend anyway). - config.rs: figment loader removed; GearConfig is deserialized from gears.analytics-api.config via ctx.config(). New config/insight.yaml in the gears layout (auth_disabled: true), modeled on api-gateway's no-auth.yaml. Build, clippy (pedantic), and tests (325 pass) all green. Helm/compose manifests still reference the old ANALYTICS__* env model and are updated in a follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The gear migration switched analytics-api from figment (`ANALYTICS__*` env, no config file) to toolkit AppConfig (a gears-layout YAML at `-c` plus `APP__*` env overrides). Update both deploy paths to match: - Dockerfile: bake config/ into the image; launch `-c /app/config/insight.yaml run`. - docker-compose.yml: bind-mount the config dir, add `-c … run`, and rename every `ANALYTICS__<x>` env var to `APP__gears__analytics-api__config__<x>`. - Helm (services/analytics-api): new configmap.yaml (the gears structure with the auth-disabled api-gateway host); deployment mounts it at /app/config/analytics.yaml, launches `-c … run`, fixes the migrate initContainer likewise, and rolls on config change via checksum/config. - Umbrella secret (charts/insight) + compose-app-secrets.sh: rename the generated `ANALYTICS__*` keys to `APP__gears__analytics-api__config__*` (hyphenated names delivered via envFrom, same as api-gateway's `APP__gears__oidc-authn-plugin__*`). Structural config now lives in the mounted ConfigMap; the Secret carries only the leaf overrides (DB/clickhouse/redis/identity URLs + tenant_default_id). - Drop the now-host-owned `bind_addr` env var; refresh stale comments (the /health-400 CrashLoop rationale no longer applies — the auth-disabled host injects DEFAULT_TENANT_ID and /health is off the auth path). Validated: `docker compose config -q`, `helm template` of the chart, and the gears config YAML all parse cleanly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Apply rustfmt to the files touched by the gears-rust migration — the migration left several formatting violations (fmt is not covered by clippy, so build/clippy/test were green while `cargo fmt --check` was not). No behavioral change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Runtime fix: with the gears-rust host, config overrides arrive as env vars under `gears.<name>.config.*`. The gear name is `analytics-api` (kebab, macro-enforced), so the natural env key was `APP__gears__analytics-api__config__*` — but hyphenated env-var names are silently dropped by the compose `sh`/dash entrypoint (dash discards env names that aren't valid identifiers) and skipped by Kubernetes `envFrom`. The result: every override (DB URL, tenant, clickhouse, redis, identity) was stranded at its YAML default, so the gear tried to reach `localhost:3306` and failed init with "pool timed out". (The old service worked because `ANALYTICS__*` had no hyphen.) Fix, keeping the kebab gear name: - Rename every override key to the identifier-safe alias `APP__gears__analytics_api__config__*` (compose env, umbrella Secret, gitops compose-app-secrets.sh) — survives dash and k8s envFrom. - The toolkit does no kebab/snake normalization, so bridge it in main.rs: `fold_gear_env_alias` deep-merges the `analytics_api` gears alias into `analytics-api` right after config load (both `run` and `migrate` paths). Verified via `--print-config`: the underscore env keys now land in `gears.analytics-api.config`. Build, clippy (-D warnings), fmt, and tests (325) all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
With the toolkit host, the api-gateway system gear's `rest_prepare` already registers `/health` and `/healthz`. Our gear also registered `/health`, so route composition panicked at boot with "Overlapping method route. Handler for `GET /health` already exists". Drop the gear's `/health` route (and the now-unused `handlers::health`). Also build the gear's endpoints on a fresh sub-router so the tenant-override middleware + `AppState` extension scope to analytics-api's own routes and don't wrap the host's `/health`/`/healthz`/`/openapi.json`. DB now connects (the earlier env-key fix landed); this was the next boot panic. Build, clippy (-D warnings), fmt, tests (325) green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The analytics-api/identity config Secrets are emitted via an unquoted
`cat <<EOF` heredoc (unquoted so ${MDB_USER} etc. expand). Backticks in
the heredoc *body* comments were therefore run as commands, printing
`keep: command not found`, `APP__gears__analytics_api__config__*: command
not found`, `ANALYTICS__*: command not found` during `make deploy`. The
`keep` one was pre-existing; the APP__/ANALYTICS__ ones came in with the
env-key rename.
Remove the backticks from the heredoc comments (shell comments elsewhere
in the file are unaffected — bash doesn't substitute in `#` lines). Only
comment text was corrupted, not the Secret data, but the noise is gone
and the script is correct.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Migration 20260623_ai_personal_gold_views (from constructorfabric#1514 ai-personal-gold) selects and GROUP BYs c.source / c.source_id from silver.class_ai_dev_usage, but the seed's placeholder DDL was never updated to add those columns. On a fresh ClickHouse the view creation fails with UNKNOWN_IDENTIFIER ("Identifier 'c.source' cannot be resolved"), breaking `seed silver`. Add source/source_id (String) to the class_ai_dev_usage placeholder, matching the real silver model — same placeholder-mirror gap the file already documents for the post-20260601 columns. Not gear-related; surfaced while validating the analytics-api gear on a clean k8s cluster. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
- config/insight.yaml: drop the committed `mysql://insight:insight@…` default; database_url/clickhouse_url are empty and injected per-deploy via APP__* env overrides (no credential in the checked-in runnable config). - gear.rs: redact userinfo from redis_url before logging (redis:// URLs can embed credentials); add `check_config` that validates the gear config section + required URLs without touching any backend. - main.rs: `check` subcommand now runs `gear::check_config` instead of a no-op success. - charts secrets.yaml: percent-encode (urlquery) the MariaDB user/password in the analytics DSN, matching the identity DSN, so operator credentials with :/@/%/ chars produce a valid connection string. Deferred (replied on PR): NetworkPolicy/trust-boundary + auth fail-closed (the deliberate header-trust posture — infra follow-up), --print-config redaction (operator debug flag, parity with api-gateway), bash DSN url-encoding (pre-existing, also affects identity — needs shared helper), Secret-change reload (Secret is externally managed in gitops). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
The gear migration changed how analytics-api boots — it's now a toolkit::bootstrap host that needs `-c <gears-config> run` and reads `APP__gears__analytics_api__config__*` (not the old `ANALYTICS__*`). The e2e harness still launched the bare binary with `ANALYTICS__*` env, so the new binary came up with empty config and the suite failed. Update lib/analytics_api.py to spawn the way the deployment does: - pass `-c <repo>/services/analytics-api/config/insight.yaml run` - inject leaf overrides via APP__* (direct Popen execve preserves the hyphenated gear-name segments, unlike the compose sh entrypoint) - override the REST-host bind on the api-gateway gear to loopback:port - give grpc-hub a per-spawn UDS so parallel workers/re-runs don't collide - widen the health-wait to 60s (host inits all system gears + migrations) - /health is now host-served (public); refresh the stale tenant comments Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Adds tests for the pure/new logic introduced by the gear migration and refactors the route table to be testable without a DB: - api/mod.rs: split the OperationBuilder chains into a stateless `build_operations(router, openapi)`; `register_routes` now just layers state/middleware over it and merges into the host router. A test drives `build_operations` so the full 20-endpoint route table + OpenAPI registration is exercised with no AppState/DB (0% → ~95%). - main.rs: tests for `fold_gear_env_alias` + `deep_merge` (alias→canonical fold, object merge, leaf overwrite). - gear.rs: tests for `redact_url` (strips userinfo) and `check_config` (missing section / empty required URLs). - config.rs reaches 100% via the above. Patch coverage for the crate rises ~40% → 60%. The remainder is MariaDB/ClickHouse-bound boot + handler code (gear init, infra::db connect, AdminThresholdService, REST handler bodies) that needs a live DB — exercised by the e2e suite + the #[ignore]d live_tests, neither of which runs in the unit-coverage job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…torfabric#1564) Wire the existing `rust:` CI job to exercise analytics-api's `#[ignore]`d live_tests against a real MariaDB — no new job, no ClickHouse (CH-gated tests skip themselves). CI (`.github/workflows/ci.yml`, `scripts/ci/*`): - Add a `live_db` component flag (components.py) threaded into the matrix entry (changed.py); default false, true only for analytics-api. - For live_db entries the rust job starts a MariaDB 11.4 container, applies migrations once up front via `analytics-api migrate` (a failed migration fails the job), then runs the live_tests in parallel with INTEGRATION_TESTS_MARIADB_URL set (`--include-ignored`). Both the migrate and the tests run under llvm-cov, so the DB paths now count toward coverage (patch coverage 60% -> 73%). Parallel-safe live_tests (no DB reset): - Migrations run once before the suite; tests never migrate or reset the DB. - Isolation is per-tenant / per-metric-key, so the suite is parallel-safe: admin_threshold + catalog + schema_validator each own unique keys/tenants. - Drop the per-test `reset_catalog` + `Migrator::up` blocks and the helper. - Fix an admin_threshold key collision: use the full UUID (`metric_id.simple()`) instead of the v7 timestamp prefix `[..8]`, which collided in same-ms runs. - Delete migration/live_tests.rs: obsolete now that the CI migrate step (plus the startup CHECK probe) covers "migrations apply on an empty DB". Identity resolution already uses Testcontainers MariaDB (MariaDbFixture) in the dotnet job — no CI change needed there. Verified: 8/8 fresh-DB parallel runs green (362 tests, ~3s); no-DB run skips all 24 live_tests cleanly; fmt + clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
294f333 to
99d1a2e
Compare
…age gap (constructorfabric#1564) The service-layer live_tests exercise the domain logic directly but never drive the axum handlers, leaving the extract -> delegate -> Result->Response glue at 0% (new-code patch coverage stuck at 73%, under the 80% gate). Add `api/http_live_tests.rs`: builds a full AppState against the live DB and mounts the real route table via `register_routes`, then drives it with `tower::oneshot`. ClickHouse/Identity clients point at an unreachable address on purpose — DB-backed handlers return real 2xx; the CH/Identity ones exercise their entry + error-mapping path. Covers the admin-threshold CRUD round-trip (201/200/204 status mapping), the metric/column/catalog reads, and the unknown-field rejection envelope. Same `#[ignore]` + `INTEGRATION_TESTS_MARIADB_URL` gating as the domain live_tests; a fresh tenant per write keeps it parallel-safe. Result: `admin/handlers.rs` and `catalog.rs` 100%, `api/mod.rs` (register_routes) 100%, patch coverage 73% -> 84% (clears the gate). Also fix a latent bug the reset-removal exposed: `response_link_map_omits_metrics_dropped_by_walk_all` DELETEs the GLOBAL `product-default` seed threshold for `ic_kpis.tasks_closed` and never restored it — `reset_catalog` used to rebuild the seed each run and hid this. Left as-is it corrupts the shared seed (every subsequent run/reader sees the metric permanently dropped) and fails on re-run. Fix: capture the row (as CHAR via CAST — sqlx-mysql can't decode DECIMAL->f64), delete, resolve, then RESTORE it before any assertion can unwind. The `fetch_links` surfaced-ids filter already keeps metrics + links consistent during the delete window, so concurrent readers never observe a phantom. Verified: full suite 3x on one DB, no reset, seed intact (good/warn preserved), 372 passed each; no-DB run skips all 34 live_tests; fmt + clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/services/analytics-api/src/domain/catalog/live_tests.rs (1)
388-460: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore-before-return ordering bug: resolve() error skips seed restoration.
resolver.resolve(...).await?at line 428 propagates immediately on error, bypassing the restore loop below it (lines 430-459). Since these tests intentionally never reset/migrate the DB anymore, any transient failure here (not just an assertion failure) permanently deletes theproduct-defaultthreshold row fortarget_metric_keyfrom the shared seed — corrupting state for this test's own pre-conditions (lines 376-386) and any other test/resolver call depending on that row, for the rest of the CI run.Capture the resolve outcome without short-circuiting, so restoration always runs before any error is propagated:
🛡️ Proposed fix
- let after = resolver.resolve(Uuid::now_v7(), "", "").await?; + let after_result = resolver.resolve(Uuid::now_v7(), "", "").await; - // Restore the deleted seed row(s) before any assertion can unwind. + // Restore the deleted seed row(s) unconditionally — even if `resolve()` + // itself failed — so a transient error can never permanently corrupt + // the shared, non-reset seed data. for row in &saved { let id_hex: String = row.try_get("", "id_hex")?; let role_slug: String = row.try_get("", "role_slug")?; let team_id: String = row.try_get("", "team_id")?; let good_s: String = row.try_get("", "good_s")?; let warn_s: String = row.try_get("", "warn_s")?; let is_locked_s: String = row.try_get("", "is_locked_s")?; let lock_reason: Option<String> = row.try_get("", "lock_reason")?; db.execute(Statement::from_sql_and_values( db.get_database_backend(), "INSERT INTO metric_threshold \ (id, tenant_id, metric_key, scope, role_slug, team_id, good, warn, is_locked, lock_reason) \ VALUES (UNHEX(?), NULL, ?, 'product-default', ?, ?, ?, ?, ?, ?)", [ Value::from(id_hex), Value::from(target_metric_key), Value::from(role_slug), Value::from(team_id), Value::from(good_s), Value::from(warn_s), Value::from(is_locked_s), match lock_reason { Some(r) => Value::from(r), None => Value::String(None), }, ], )) .await?; } + + let after = after_result?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/analytics-api/src/domain/catalog/live_tests.rs` around lines 388 - 460, The resolve call in the live test can short-circuit before the deleted seed row is restored, leaving shared `metric_threshold` state corrupted. Update the `resolver.resolve(...)` flow so restoration in the `saved` loop always runs even when resolve fails, then propagate the resolve error afterward; use the surrounding test block and the `saved`/restore logic as the anchor points.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/backend/services/analytics-api/src/domain/catalog/live_tests.rs`:
- Around line 388-460: The resolve call in the live test can short-circuit
before the deleted seed row is restored, leaving shared `metric_threshold` state
corrupted. Update the `resolver.resolve(...)` flow so restoration in the `saved`
loop always runs even when resolve fails, then propagate the resolve error
afterward; use the surrounding test block and the `saved`/restore logic as the
anchor points.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3aec5df1-88a3-4c41-a94c-56c4ba636c9d
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (35)
.github/workflows/ci.ymlcharts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/gitops/environments/local/values.yaml.templatedeploy/gitops/scripts/compose-app-secrets.shdeploy/seed/sql/placeholders.sqldocker-compose.ymlscripts/ci/changed.pyscripts/ci/components.pysrc/backend/services/analytics-api/Cargo.tomlsrc/backend/services/analytics-api/Dockerfilesrc/backend/services/analytics-api/config/insight.yamlsrc/backend/services/analytics-api/helm/templates/configmap.yamlsrc/backend/services/analytics-api/helm/templates/deployment.yamlsrc/backend/services/analytics-api/helm/values.yamlsrc/backend/services/analytics-api/src/api/admin/handlers.rssrc/backend/services/analytics-api/src/api/catalog.rssrc/backend/services/analytics-api/src/api/error.rssrc/backend/services/analytics-api/src/api/handlers.rssrc/backend/services/analytics-api/src/api/http_live_tests.rssrc/backend/services/analytics-api/src/api/mod.rssrc/backend/services/analytics-api/src/api/tenant_resolution_tests.rssrc/backend/services/analytics-api/src/auth.rssrc/backend/services/analytics-api/src/config.rssrc/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rssrc/backend/services/analytics-api/src/domain/admin_threshold/live_tests.rssrc/backend/services/analytics-api/src/domain/admin_threshold/service.rssrc/backend/services/analytics-api/src/domain/auth.rssrc/backend/services/analytics-api/src/domain/catalog/live_tests.rssrc/backend/services/analytics-api/src/domain/schema_validator/live_tests.rssrc/backend/services/analytics-api/src/gear.rssrc/backend/services/analytics-api/src/main.rssrc/backend/services/analytics-api/src/migration/live_tests.rssrc/backend/services/analytics-api/src/migration/mod.rssrc/ingestion/tests/e2e/lib/analytics_api.py
💤 Files with no reviewable changes (2)
- src/backend/services/analytics-api/src/migration/live_tests.rs
- src/backend/services/analytics-api/src/migration/mod.rs
✅ Files skipped from review due to trivial changes (4)
- src/backend/services/analytics-api/src/api/error.rs
- scripts/ci/components.py
- src/backend/services/analytics-api/helm/values.yaml
- charts/insight/values.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
- deploy/gitops/environments/local/values.yaml.template
- src/backend/services/analytics-api/src/domain/admin_threshold/audit_emitter.rs
- docker-compose.yml
- src/backend/services/analytics-api/helm/templates/deployment.yaml
- src/backend/services/analytics-api/Cargo.toml
- deploy/seed/sql/placeholders.sql
- src/backend/services/analytics-api/src/api/admin/handlers.rs
- src/backend/services/analytics-api/Dockerfile
- src/backend/services/analytics-api/src/api/tenant_resolution_tests.rs
- src/backend/services/analytics-api/src/api/catalog.rs
- src/backend/services/analytics-api/config/insight.yaml
- src/backend/services/analytics-api/src/main.rs
- src/backend/services/analytics-api/src/domain/admin_threshold/service.rs
- src/backend/services/analytics-api/src/gear.rs
- deploy/gitops/scripts/compose-app-secrets.sh
- src/backend/services/analytics-api/src/api/mod.rs
- src/backend/services/analytics-api/src/auth.rs
- src/backend/services/analytics-api/src/api/handlers.rs
- src/backend/services/analytics-api/src/config.rs
…ot shared mutation Replace the capture->delete->restore approach (which mutated the GLOBAL `product-default` seed row for `ic_kpis.tasks_closed`) with a fully isolated, purely-additive fixture that proves the same ADR-003 `surfaced_ids` invariant through tenant scoping alone — the model's own rule: a DEFAULT metric (`tenant_id IS NULL`) is visible to everyone, and per-tenant threshold data never intersects across tenants. The test now inserts a GLOBAL catalog row with a UNIQUE key (catalog is global-only in v1), links it into `metric_query_catalog`, and gives it a threshold for ONE tenant. Resolving as that tenant surfaces it + its link; resolving as any other (never-seen) tenant has no candidate, so `walk_all` drops it and the surfaced-ids filter drops its link. No shared row is read or mutated, nothing is deleted or restored. Verified: shared seed untouched (tasks_closed product-default stays intact), full suite 3x on one DB with no reset (372 passed each), no-DB run skips all 34 live_tests, fmt + clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…cy spillover) `cargo llvm-cov --package X` reports every instrumented file, including path-dependency crates compiled into X's test binary. So the analytics-api report also carried `libs/insight-clickhouse` at ~15% (only `Client::new` is hit from analytics-api) — and the gate buckets files into components via `component_for`, so those lines land on the insight-clickhouse component. Today this is masked: the gate merges all reports with max-hits, and this branch also changes insight-clickhouse (its own job reports ~93%), so the union wins. But on a PR that touches analytics-api and NOT insight-clickhouse, the latter's job never runs — the only insight-clickhouse data is analytics-api's 15% spillover, which would fail the component's OVERALL_MIN (80%) gate. No amount of insight-clickhouse tests fixes that; the spillover must not be counted here in the first place. Add a per-component `cover_ignore_regex` (threaded through changed.py) wired to `cargo llvm-cov report --ignore-filename-regex`. Set `src/backend/libs/` on analytics-api (the only rust service that links a shared lib). Each library crate keeps self-reporting in its own job. Verified: the analytics-api report no longer lists any insight-clickhouse file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
… (no ClickHouse) Raise the crate's own coverage without a live ClickHouse by testing the pure, connection-free surface that was untested: `Client::new` auth branches, `query()` with/without a timeout, the `inner()`/`config()` accessors, and `tenant_query` validation (lib.rs 46% -> 100% line); the password-redacting `Config` Debug impl (config.rs -> 100%); and `Error` Display (error.rs -> 100%). Crate line coverage 88% -> 94%. The remaining gap is `QueryBuilder::fetch_all` — query EXECUTION, which needs a live ClickHouse and is out of scope here (cf/insight#1564 skips CH). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Migrate analytics-api from a self-hosted axum::serve service to a gears-rust host on toolkit::bootstrap::run_server:
dbcapability; ClickHouse is not a toolkit-db backend anyway).Build, clippy (pedantic), and tests (325 pass) all green. Helm/compose manifests still reference the old ANALYTICS__* env model and are updated in a follow-up.
Summary by CodeRabbit
migrateandcheckcommands, with improved startup/config loading.X-Insight-Tenant-Idoverride behavior; updated routing so health works with host auth-disabled behavior.APP__gears__analytics_api__config__*env-var contract.sourceandsource_idplaceholder columns for seeded schema.