Merged
Conversation
Collaborator
chronark
commented
Mar 8, 2026
chronark
commented
- fix: improve rollbacks
- feat: scaling and region tables
- fix: replace AVAILABLE_REGIONS env var with clusterRegions table query
- fix: update instances mutation to use appScalingSettings table
- fix: update CPU mutation to use appScalingSettings table
- fix: update memory mutation to use appScalingSettings table
- fix: update regions mutation to use appScalingSettings and clusterRegions tables
- fix: revert cpu/memory mutations back to appRuntimeSettings
- feat: rename app_scaling_settings to app_regional_settings, add horizontal_autoscaling_policies table
- feat: replace regionConfig with appRegionalSettings across dashboard
- fix: remove AVAILABLE_REGIONS from .env.example
- feat: add migration script for region_config to app_regional_settings
- feat: add Heartbeat RPC to ClusterService proto
- feat: add heartbeat loop to krane agent
- feat: implement Heartbeat RPC handler and rename cluster_regions to regions
- feat: deploy workflow reads from appRegionalSettings, add region_id to deployment_topology
- fix: display region names in UI, add local region support
- feat: add X-Krane-Platform header, app_id to ApplySentinel, and appId labels
- fix: step conflict
- fix: rabbit
This addresses a few things. The most obvious one is lowering the freshness of some frontline caches to make rollbacks faster. I also moved the entire deployment workflow to a virtual object, keyed by workspace id. The reason is that we had some potential races and this is the pragmatic fix for right now. I could've done it per app too, but I actually think limiting concurrency in the beginning is not a bad thing. afterall, we're paying for all of these resources.
…ontal_autoscaling_policies table
Krane now sends periodic heartbeats to the control plane every 30s, reporting its region and platform. Also adds Platform config field, fixes duplicate r.Recover()/r.DeferCtx calls, and updates mock client.
…egions - Add Heartbeat handler on ctrl that upserts regions and clusters tables - Rename cluster_regions table to regions for clarity - Remove AvailableRegions config from ctrl api/worker - Deploy workflow now fails with terminal error if no regions configured - Certificate bootstrap queries regions from DB instead of config - Add SQL queries: UpsertRegion, FindRegionByNameAndPlatform, ListRegions, UpsertCluster
…o deployment_topology - Deploy handler now reads regions from app_regional_settings table instead of deprecated regionConfig on app_runtime_settings - Fails with terminal error if no regions are configured - Added region_id column to deployment_topology table - Added FindAppRegionalSettingsByAppAndEnv SQL query with regions join - Updated integration harness to create regions before topologies
- Join regions table when fetching regional settings to get region name - Use region name (not ID) for display in runtime settings - Add "local" flag code to sentinel node types and REGION_INFO
… labels - Add X-Krane-Platform header alongside X-Krane-Region in the krane interceptor so ctrl can identify regions by (name, platform) pair - All ctrl RPC handlers now validate both region and platform from headers - Add app_id field to ApplySentinel proto message - Add AppID label to all sentinel sub-resources (Service, PDB, gossip Service, CiliumNetworkPolicy) for consistency with deployment labels - Remove hardcoded region list from sentinel config validation (regions are now dynamic from DB) - Add platform field to sentinel config
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces static available_regions with a DB-backed, platform-aware regional model: adds regions/clusters/app_regional_settings tables and queries, Heartbeat RPC and handler, platform header propagation and client heartbeat, migration script, generated DB methods, schema/type updates, and frontend changes to use regions array (id,name,replicas). Changes
Sequence Diagram(s)sequenceDiagram
participant Krane as Krane (client)
participant Ctrl as Control Plane ClusterService
participant DB as Database
rect rgba(0,128,255,0.5)
Krane->>Ctrl: Heartbeat(region, platform)
Ctrl->>DB: UpsertRegion(name=region, platform)
DB-->>Ctrl: OK
Ctrl->>DB: FindRegionByNameAndPlatform(name=region, platform)
DB-->>Ctrl: region_id
Ctrl->>DB: UpsertCluster(id=clusterID, region_id, last_heartbeat_at)
DB-->>Ctrl: OK
Ctrl-->>Krane: HeartbeatResponse
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
chronark
requested review from
Flo4604,
MichaelUnkey,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
web/internal/db/src/schema/app_runtime_settings.ts (1)
45-49:⚠️ Potential issue | 🟡 MinorUse proper JSDoc syntax for deprecation annotation.
The deprecation comment uses single-line syntax
//instead of the TypeScript JSDoc standard/**@deprecated... */, which prevents IDEs and type checkers from recognizing and warning about the deprecated field usage.Suggested fix
- // `@deprecated`, use appRegionalSettings instead + /** `@deprecated` use appRegionalSettings instead */ regionConfig: json("region_config")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/internal/db/src/schema/app_runtime_settings.ts` around lines 45 - 49, Replace the single-line deprecation comment with a proper JSDoc deprecation annotation for the regionConfig schema field: change the leading // `@deprecated` comment to a JSDoc block /** `@deprecated` use appRegionalSettings instead */ immediately above the regionConfig declaration in app_runtime_settings.ts so IDEs/type checkers recognize the deprecation for the regionConfig json("region_config") field.web/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/onboarding-environment-provider.tsx (1)
74-82:⚠️ Potential issue | 🟠 MajorDon't seal initialization on an empty region list.
If
availableRegionsfirst resolves to[], Line 81 still flipshasInitializedRefand Lines 105-106 leavedraft.regionsempty. Because the ref never resets, later refetches with real regions are ignored, leaving new projects with no regions configured. That now blocks the new deploy flow.Also applies to: 105-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/new/steps/onboarding-environment-provider.tsx around lines 74 - 82, The effect is marking initialization done even when availableRegions is an empty array so later real regions are ignored; change the guard in the useEffect to treat an empty regions list as not-ready (e.g., require availableRegions.length > 0 before proceeding) and only set hasInitializedRef.current = true after you have a non-empty availableRegions; likewise, when assigning draft.regions (the code that writes to draft.regions at the later block around lines 105-106), only set draft.regions if availableRegions is non-empty to avoid persisting an empty regions list.web/internal/db/src/schema/deployment_topology.ts (1)
22-45:⚠️ Potential issue | 🟠 MajorMake
regionIdthe canonical topology key.
regionIdis introduced here, but this schema still allows missing IDs viadefault("")and still keys uniqueness on the legacyregionlabel. Since the newregionsmodel is only unique per(name, platform), two platforms sharing the same region name can still collide onunique_version_per_region, and""will hide missing wiring at insert time.Proposed fix
- regionId: varchar("region_id", { length: 64 }).notNull().default(""), + regionId: varchar("region_id", { length: 64 }).notNull(), @@ - uniqueIndex("unique_region_per_deployment").on(table.deploymentId, table.region), - uniqueIndex("unique_version_per_region").on(table.region, table.version), + uniqueIndex("unique_region_per_deployment").on(table.deploymentId, table.regionId), + uniqueIndex("unique_version_per_region").on(table.regionId, table.version),As per coding guidelines, "Make illegal states unrepresentable by modeling domain with discriminated unions and parsing inputs at boundaries into typed structures".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/internal/db/src/schema/deployment_topology.ts` around lines 22 - 45, The schema allows empty regionId and still keys uniqueness on the legacy region label; change regionId (varchar("region_id")) to not allow default "" (remove .default("") and make it .notNull()), switch unique indexes that currently use table.region to use table.regionId (replace unique_region_per_deployment and unique_version_per_region to key on table.regionId instead of table.region), and ensure any insertion/ORM layer validates/normalizes inputs so regionId is always provided (parse at API/DTO boundary into a typed structure) so the canonical topology key is regionId and legacy region remains non-unique label only.
🧹 Nitpick comments (5)
svc/ctrl/services/cluster/rpc_watch_deployments.go (1)
40-47:platformvalidated but unused.The
platformheader is extracted and validated but never used in this function. If it's required for future functionality or consistency across RPCs, consider adding a comment. Otherwise, this validation adds overhead without effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@svc/ctrl/services/cluster/rpc_watch_deployments.go` around lines 40 - 47, The code extracts and validates req.Header values region and platform (via region := req.Header().Get("X-Krane-Region"), platform := req.Header().Get("X-Krane-Platform") and assert.All) but platform is never used; either remove the platform extraction/validation or make its intent explicit. Fix by either deleting the platform variable and its NotEmpty assertion from the assert.All call (leaving region validation only) or, if platform is required for consistency/future use, keep the extraction/validation and add a brief comment above (referencing platform, req.Header(), and assert.All) explaining why platform must be validated now even though it is not yet used. Ensure the change updates the assert.All invocation accordingly so there are no unused variables or dead validation.web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-regions.ts (1)
54-66: Consider bulk insert to reduce N+1 queries.The current implementation executes individual
INSERTstatements for each new region. When multiple regions are added, this results in N+1 database round-trips.♻️ Proposed bulk insert optimization
// Insert rows for newly added regions - for (const regionId of input.regionIds) { - const existing = existingSettings.find((s) => s.regionId === regionId); - if (!existing) { - await db.insert(appRegionalSettings).values({ - workspaceId: ctx.workspace.id, - appId: env.appId, - environmentId: envId, - regionId, - replicas: defaults.replicas, - }); - } - } + const existingRegionIds = new Set(existingSettings.map((s) => s.regionId)); + const newRegions = input.regionIds.filter((regionId) => !existingRegionIds.has(regionId)); + + if (newRegions.length > 0) { + await db.insert(appRegionalSettings).values( + newRegions.map((regionId) => ({ + workspaceId: ctx.workspace.id, + appId: env.appId, + environmentId: envId, + regionId, + replicas: defaults.replicas, + })), + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-regions.ts` around lines 54 - 66, The loop that inserts new region rows performs one INSERT per region (see the for loop over input.regionIds, existingSettings lookup, and db.insert(appRegionalSettings).values({...})), causing N+1 queries; instead, collect the missing region rows into an array (using ctx.workspace.id, env.appId, envId, regionId and defaults.replicas) and perform a single bulk insert call (db.insert(appRegionalSettings).values(rows)) for all new regions in one operation to eliminate multiple round-trips.svc/sentinel/config.go (1)
77-80: Consider removing empty Validate method.The
Validate()method body is now empty. If no cross-field validation is needed, consider removing the method entirely or adding a comment explaining it's reserved for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@svc/sentinel/config.go` around lines 77 - 80, The empty Validate method on the Config type (func (c *Config) Validate()) should be removed if there's no cross-field validation required; either delete the method entirely to avoid dead code or, if you intend to keep it for future validation, add a short comment above Config.Validate explaining it's intentionally no-op/reserved for future use so linters/reviewers understand it's deliberate.svc/krane/internal/sentinel/apply.go (1)
170-177: AppID missing from PodTemplateSpec labels.AppID is added to the Deployment's ObjectMeta labels (line 173) but not to PodTemplateSpec labels (lines 198-202). If AppID is needed for pod-level queries or policies, consider adding it to the pod template labels as well.
Proposed fix
Template: corev1.PodTemplateSpec{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ "reloader.stakater.com/auto": "true", }, Labels: labels.New(). WorkspaceID(sentinel.GetWorkspaceId()). + AppID(sentinel.GetAppId()). EnvironmentID(sentinel.GetEnvironmentId()). SentinelID(sentinel.GetSentinelId()). ComponentSentinel(), },Also applies to: 193-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@svc/krane/internal/sentinel/apply.go` around lines 170 - 177, The PodTemplateSpec labels currently omit the AppID which is present on the Deployment ObjectMeta; update the labels builder used for the pod template (the labels.New() chain that currently calls WorkspaceID, ProjectID, EnvironmentID, SentinelID, ComponentSentinel, ManagedByKrane) to include AppID by calling AppID(sentinel.GetAppId()), and apply the same addition to the other PodTemplateSpec labels block referenced in the review so pod-level queries/policies can use the AppID label.web/internal/db/src/schema/app_regional_settings.ts (1)
27-30: Consider adding relation forhorizontalAutoscalingPolicyId.The schema defines
horizontalAutoscalingPolicyIdas an optional reference, but there's no corresponding relation defined inappRegionalSettingsRelations. If ORM-level joins to the autoscaling policy are needed, consider adding:horizontalAutoscalingPolicy: one(horizontalAutoscalingPolicies, { fields: [appRegionalSettings.horizontalAutoscalingPolicyId], references: [horizontalAutoscalingPolicies.id], }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/internal/db/src/schema/app_regional_settings.ts` around lines 27 - 30, The schema declares horizontalAutoscalingPolicyId but appRegionalSettingsRelations lacks a relation, so add a one-to-one relation entry named horizontalAutoscalingPolicy to appRegionalSettingsRelations that links appRegionalSettings.horizontalAutoscalingPolicyId to horizontalAutoscalingPolicies.id (use the one(...) relation helper with fields: [appRegionalSettings.horizontalAutoscalingPolicyId] and references: [horizontalAutoscalingPolicies.id]) so ORM joins can resolve the referenced autoscaling policy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/db/queries/cluster_upsert.sql`:
- Around line 13-14: The upsert only updates last_heartbeat_at and ignores
conflicts on the other unique key (region_id), leaving stale mappings; update
the ON DUPLICATE KEY UPDATE clause in the cluster_upsert.sql so it also
refreshes the id and region_id from the incoming values (e.g., set id =
VALUES(id) or id = sqlc.arg(id), region_id = VALUES(region_id) or region_id =
sqlc.arg(region_id), and last_heartbeat_at = VALUES(last_heartbeat_at) /
sqlc.arg(last_heartbeat_at)) so that conflicts on either unique key reconcile to
the new mapping for the clusters table (referenced symbols: clusters, id,
region_id, last_heartbeat_at).
In `@svc/ctrl/proto/ctrl/v1/cluster.proto`:
- Around line 235-247: The proto was renumbered which breaks wire compatibility;
do not change existing tags for environment_id, sentinel_id, image, replicas,
cpu_millicores, memory_mib—keep their original tag numbers and instead add the
new field app_id at a new unused tag (e.g., tag 10 or another free tag) so
existing serialized messages still decode correctly; update the message
definition to preserve the original tags for environment_id, sentinel_id, image,
replicas, cpu_millicores, memory_mib and place app_id at a new unique tag while
leaving field names and types unchanged.
In `@svc/ctrl/services/cluster/rpc_get_desired_cilium_network_policy_state.go`:
- Around line 27-32: You validate platform but then call
FindCiliumNetworkPolicyByIDAndRegion with only Region (variable region) which
ignores platform and can return the wrong row because regions are unique by
(name, platform); either thread platform into the lookup by adding/using a
repository method that queries WHERE region = ? AND platform = ? (e.g.,
implement FindCiliumNetworkPolicyByIDAndRegionAndPlatform or extend
FindCiliumNetworkPolicyByIDAndRegion to accept platform), or resolve the region
to its region_id first (lookup region by name+platform then call
FindCiliumNetworkPolicyByIDAndRegion with region_id); update the call site in
rpc_get_desired_cilium_network_policy_state.go to pass platform (or resolved
region_id) instead of only region.
In `@svc/ctrl/services/cluster/rpc_heartbeat.go`:
- Around line 57-60: The UpsertCluster call currently uses
uid.New(uid.ClusterPrefix) (in db.Query.UpsertCluster with UpsertClusterParams)
which produces a random id per heartbeat; instead derive a stable cluster ID
from the region/request context (e.g., a deterministic function
deriveClusterID(regionID) or use a namespaced UID function) at the boundary and
pass that as ID so the cluster id is stable and represents region ownership;
update the upsert semantics to key on RegionID (respecting the
clusters_region_id_unique constraint) so subsequent heartbeats update
last_heartbeat_at for that region while preserving the deterministic ID.
In `@svc/ctrl/services/cluster/rpc_report_deployment_status.go`:
- Around line 36-45: The code currently reads X-Krane-Platform into platform but
doesn't use it to scope DB operations; update the transaction inside db.TxRetry
to include platform in all deployment resolution and instance deletion/upsert
filters so DB lookups use the composite (platform, region) key (e.g., when
resolving the deployment by K8s name and when deleting/upserting instances).
Ensure functions/methods that build the mutation path or queries (the logic
called inside the db.TxRetry callback, and any helpers that resolve deployments
or filter instances) accept and apply the platform value and/or validate the
region+platform pair before executing mutations.
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/components/runtime-settings/regions.tsx:
- Around line 28-29: The effect is being retriggered because defaultRegionNames
is a fresh array every render; instead compute a stable representation and use
that in your effects: memoize the region names inside this component (e.g. const
defaultRegionNamesMemo = useMemo(() => regions.map(r => r.name), [regions.map(r
=> r.name).join('|')]) or otherwise derive a stable key like namesKey =
regions.map(r=>r.name).sort().join(',')) and then use defaultRegionNamesMemo (or
namesKey) as the dependency for the effects that call setSelectedRegionNames and
update hasChanges (references: defaultRegionNames, regions,
setSelectedRegionNames, hasChanges, getAvailableRegions); this prevents
unrelated rerenders (like getAvailableRegions refetch) from resetting
in-progress selections.
- Around line 80-87: The form currently keys selected regions by region.name and
falls back to writing name into draft.regions.id (see draft.regions mapping,
availableRegions, defaultReplicas), which can create invalid ids; change the
combobox/value wiring so the selected value is the region.id (use id for value
and name only for label/search), update the mapping in draft.regions to lookup
availableRegions by id (not name) and set id from the selected id (never set id
= name), and apply the same id-based change for the other occurrence mentioned
(around the 142-145 region selection code) so onSubmit and rehydration no longer
reconstruct ids from names. Ensure all related props/state that currently expect
names are adjusted to expect ids.
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/[projectId]/components/region-flag.tsx:
- Around line 61-63: The BoringAvatar render in region-flag.tsx currently omits
accessible text; update the BoringAvatar invocation (the branch that renders
when flagCode === "local") to pass an explicit name and enable title so the
avatar exposes the accessible label — e.g., set name to the flag identifier (use
name={flagCode} or "local" for that branch) and set title={true} on the
BoringAvatar component.
- Around line 61-63: The local branch rendering of BoringAvatar doesn't preserve
the container's rounded-square shape; update the JSX where flagCode === "local"
renders BoringAvatar to pass square={shape === "rounded"} (using the existing
shape variable) so the avatar matches the container (rounded square when shape
=== "rounded", circular when shape === "circle"); keep other props (size,
variant) unchanged and ensure this change is applied in the component that
contains the flagCode check and BoringAvatar usage.
In `@web/apps/dashboard/lib/collections/deploy/environment-settings.ts`:
- Around line 215-237: The code only compares replicas using
original.regions.at(0) / modified.regions.at(0) and skips sending instance
updates when regionsChanged, so edits to replicas in non-first regions or
simultaneous region+replica changes are dropped; update the logic in the block
that computes origReplicas/modReplicas/instancesChanged to compute per-region
replica diffs by matching regions by id (use original.regions and
modified.regions maps keyed by id), build a payload of changed
regionIds->replicas (or a replicasPerRegion map) and call
trpcClient.deploy.environmentSettings.runtime.updateInstances.mutate with that
full per-region update (and do not suppress this mutation just because
regionsChanged), referencing origRegionIds, modRegionIds, original.regions,
modified.regions, instancesChanged, and updateInstances.mutate to locate where
to implement the change.
In
`@web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-instances.ts`:
- Around line 17-27: The query on appRegionalSettings updates replicas for all
apps in the given environment because it lacks an appId filter; fetch the
environment record to get its appId (like update-regions.ts does) and add
eq(appRegionalSettings.appId, environment.appId) to the WHERE clause alongside
eq(appRegionalSettings.workspaceId, ctx.workspace.id) and
inArray(appRegionalSettings.environmentId, envIds) so only the current app's
regional settings are updated when setting input.replicasPerRegion.
In
`@web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-regions.ts`:
- Around line 30-66: The bug is that existingSettings is queried only for
input.environmentId but then reused for every envId in the loop; change to fetch
or index existing settings per environment before inserting to avoid wrong
lookups and duplicate-key inserts. Either (1) move the
appRegionalSettings.findMany query into the for (const envId of envIds) loop and
use that per-env result when computing existing via existingSettings.find((s) =>
s.regionId === regionId), or (2) query all appRegionalSettings for the workspace
and envIds once and build a map keyed by environmentId (and regionId) and use
that map when checking for existence before db.insert; update references to
existingSettings, envIds, input.environmentId, env.appId, appRegionalSettings
and ensure replicas use defaults.replicas.
In `@web/internal/db/src/schema/clusters.ts`:
- Around line 21-26: The exported relation name and field are misleading: update
the export and relation field to reflect that this relation links clusters ->
regions (e.g., rename regionsRelations to clustersRelations or
clusterRegionsRelation) and rename the relation field currently named workspace
to something like region to match the referenced table; modify the
relations(...) call on clusters (the relations function, clusters, and inner
mapping) so it references clusters.regionId -> regions.id but exports and the
key use a clear name (reference symbols: regionsRelations, clusters, relations,
workspace, regions, regionId, id).
In `@web/internal/db/src/schema/horizontal_autoscaling_policies.ts`:
- Around line 20-26: The schema for horizontal_autoscaling_policies currently
allows invalid states (negative replicas, replicasMin > replicasMax, and
thresholds outside 0–100); fix this by adding DB-level constraints in the
table/migration for the columns replicasMin (replicas_min), replicasMax
(replicas_max), and memoryThreshold/cpuThreshold/rpsThreshold (memory_threshold,
cpu_threshold, rps_threshold): enforce replicas_min >= 0, replicas_max >=
replicas_min, and for each threshold allow NULL or require value BETWEEN 0 AND
100 (or convert to unsigned tinyint with CHECK), and include a migration step to
either sanitize or reject existing rows that violate these constraints before
applying them. Ensure the constraints are added in the same migration that
defines/updates horizontal_autoscaling_policies so the DB, not just the UI,
prevents illegal states.
In `@web/tools/migrate/region-config-to-regional-settings.ts`:
- Around line 36-37: The migration currently treats regionConfig and regions as
simple key/value pairs and writes legacy region names directly into
app_regional_settings.region_id; update the boundary parsing so you
parse/validate the incoming regionConfig into a typed structure, resolve each
legacy region key to the canonical region row (e.g., via an existing regions
lookup or a helper like findRegionByName), and insert the resolved region's id
into app_regional_settings.region_id instead of the legacy name; ensure you do
this where regionConfig and regions are created/iterated, avoid any use of any,
as-casts or non-null assertions, and return/throw on unresolved keys so illegal
states are unrepresentable.
---
Outside diff comments:
In
`@web/apps/dashboard/app/`(app)/[workspaceSlug]/projects/new/steps/onboarding-environment-provider.tsx:
- Around line 74-82: The effect is marking initialization done even when
availableRegions is an empty array so later real regions are ignored; change the
guard in the useEffect to treat an empty regions list as not-ready (e.g.,
require availableRegions.length > 0 before proceeding) and only set
hasInitializedRef.current = true after you have a non-empty availableRegions;
likewise, when assigning draft.regions (the code that writes to draft.regions at
the later block around lines 105-106), only set draft.regions if
availableRegions is non-empty to avoid persisting an empty regions list.
In `@web/internal/db/src/schema/app_runtime_settings.ts`:
- Around line 45-49: Replace the single-line deprecation comment with a proper
JSDoc deprecation annotation for the regionConfig schema field: change the
leading // `@deprecated` comment to a JSDoc block /** `@deprecated` use
appRegionalSettings instead */ immediately above the regionConfig declaration in
app_runtime_settings.ts so IDEs/type checkers recognize the deprecation for the
regionConfig json("region_config") field.
In `@web/internal/db/src/schema/deployment_topology.ts`:
- Around line 22-45: The schema allows empty regionId and still keys uniqueness
on the legacy region label; change regionId (varchar("region_id")) to not allow
default "" (remove .default("") and make it .notNull()), switch unique indexes
that currently use table.region to use table.regionId (replace
unique_region_per_deployment and unique_version_per_region to key on
table.regionId instead of table.region), and ensure any insertion/ORM layer
validates/normalizes inputs so regionId is always provided (parse at API/DTO
boundary into a typed structure) so the canonical topology key is regionId and
legacy region remains non-unique label only.
---
Nitpick comments:
In `@svc/ctrl/services/cluster/rpc_watch_deployments.go`:
- Around line 40-47: The code extracts and validates req.Header values region
and platform (via region := req.Header().Get("X-Krane-Region"), platform :=
req.Header().Get("X-Krane-Platform") and assert.All) but platform is never used;
either remove the platform extraction/validation or make its intent explicit.
Fix by either deleting the platform variable and its NotEmpty assertion from the
assert.All call (leaving region validation only) or, if platform is required for
consistency/future use, keep the extraction/validation and add a brief comment
above (referencing platform, req.Header(), and assert.All) explaining why
platform must be validated now even though it is not yet used. Ensure the change
updates the assert.All invocation accordingly so there are no unused variables
or dead validation.
In `@svc/krane/internal/sentinel/apply.go`:
- Around line 170-177: The PodTemplateSpec labels currently omit the AppID which
is present on the Deployment ObjectMeta; update the labels builder used for the
pod template (the labels.New() chain that currently calls WorkspaceID,
ProjectID, EnvironmentID, SentinelID, ComponentSentinel, ManagedByKrane) to
include AppID by calling AppID(sentinel.GetAppId()), and apply the same addition
to the other PodTemplateSpec labels block referenced in the review so pod-level
queries/policies can use the AppID label.
In `@svc/sentinel/config.go`:
- Around line 77-80: The empty Validate method on the Config type (func (c
*Config) Validate()) should be removed if there's no cross-field validation
required; either delete the method entirely to avoid dead code or, if you intend
to keep it for future validation, add a short comment above Config.Validate
explaining it's intentionally no-op/reserved for future use so linters/reviewers
understand it's deliberate.
In
`@web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-regions.ts`:
- Around line 54-66: The loop that inserts new region rows performs one INSERT
per region (see the for loop over input.regionIds, existingSettings lookup, and
db.insert(appRegionalSettings).values({...})), causing N+1 queries; instead,
collect the missing region rows into an array (using ctx.workspace.id,
env.appId, envId, regionId and defaults.replicas) and perform a single bulk
insert call (db.insert(appRegionalSettings).values(rows)) for all new regions in
one operation to eliminate multiple round-trips.
In `@web/internal/db/src/schema/app_regional_settings.ts`:
- Around line 27-30: The schema declares horizontalAutoscalingPolicyId but
appRegionalSettingsRelations lacks a relation, so add a one-to-one relation
entry named horizontalAutoscalingPolicy to appRegionalSettingsRelations that
links appRegionalSettings.horizontalAutoscalingPolicyId to
horizontalAutoscalingPolicies.id (use the one(...) relation helper with fields:
[appRegionalSettings.horizontalAutoscalingPolicyId] and references:
[horizontalAutoscalingPolicies.id]) so ORM joins can resolve the referenced
autoscaling policy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f755f3a1-064d-49f2-b5cf-8a6608ca3017
⛔ Files ignored due to path filters (4)
gen/proto/ctrl/v1/cluster.pb.gois excluded by!**/*.pb.go,!**/gen/**gen/proto/ctrl/v1/ctrlv1connect/cluster.connect.gois excluded by!**/gen/**gen/rpc/ctrl/cluster_generated.gois excluded by!**/gen/**web/apps/dashboard/gen/proto/ctrl/v1/cluster_pb.tsis excluded by!**/gen/**
📒 Files selected for processing (86)
dev/config/ctrl-api.tomldev/config/krane.tomldev/k8s/manifests/ctrl-api.yamldev/k8s/manifests/ctrl-worker.yamldev/k8s/manifests/krane.yamldocs/engineering/architecture/services/control-plane/api/configuration.mdxdocs/engineering/architecture/services/control-plane/worker/configuration.mdxpkg/db/BUILD.bazelpkg/db/app_regional_settings_find_by_app_and_env.sql_generated.gopkg/db/bulk_cluster_region_upsert.sql_generated.gopkg/db/bulk_cluster_upsert.sql_generated.gopkg/db/bulk_deployment_step_insert.sql_generated.gopkg/db/bulk_deployment_topology_insert.sql_generated.gopkg/db/cluster_region_find_by_name.sql_generated.gopkg/db/cluster_region_list.sql_generated.gopkg/db/cluster_region_upsert.sql_generated.gopkg/db/cluster_upsert.sql_generated.gopkg/db/deployment_step_insert.sql_generated.gopkg/db/deployment_topology_insert.sql_generated.gopkg/db/deployment_topology_list_by_versions.sql_generated.gopkg/db/deployment_topology_list_desired.sql_generated.gopkg/db/models_generated.gopkg/db/querier_bulk_generated.gopkg/db/querier_generated.gopkg/db/queries/app_regional_settings_find_by_app_and_env.sqlpkg/db/queries/cluster_region_find_by_name.sqlpkg/db/queries/cluster_region_list.sqlpkg/db/queries/cluster_region_upsert.sqlpkg/db/queries/cluster_upsert.sqlpkg/db/queries/deployment_step_insert.sqlpkg/db/queries/deployment_topology_insert.sqlpkg/db/schema.sqlpkg/uid/prefix.gosvc/ctrl/api/certificate.gosvc/ctrl/api/config.gosvc/ctrl/api/harness_test.gosvc/ctrl/api/run.gosvc/ctrl/integration/harness.gosvc/ctrl/integration/harness/harness.gosvc/ctrl/proto/ctrl/v1/cluster.protosvc/ctrl/services/cluster/BUILD.bazelsvc/ctrl/services/cluster/rpc_get_desired_cilium_network_policy_state.gosvc/ctrl/services/cluster/rpc_get_desired_deployment_state.gosvc/ctrl/services/cluster/rpc_get_desired_sentinel_state.gosvc/ctrl/services/cluster/rpc_heartbeat.gosvc/ctrl/services/cluster/rpc_report_deployment_status.gosvc/ctrl/services/cluster/rpc_report_sentinel_status.gosvc/ctrl/services/cluster/rpc_watch_cilium_network_policies.gosvc/ctrl/services/cluster/rpc_watch_deployments.gosvc/ctrl/services/cluster/rpc_watch_sentinels.gosvc/ctrl/services/deployment/create_deployment.gosvc/ctrl/services/deployment/doc.gosvc/ctrl/worker/config.gosvc/ctrl/worker/deploy/deploy_handler.gosvc/ctrl/worker/deploy/service.gosvc/ctrl/worker/run.gosvc/krane/BUILD.bazelsvc/krane/config.gosvc/krane/internal/sentinel/apply.gosvc/krane/internal/sentinel/controller.gosvc/krane/internal/testutil/mock_cluster_client.gosvc/krane/pkg/controlplane/client.gosvc/krane/pkg/controlplane/interceptor.gosvc/krane/run.gosvc/sentinel/config.gosvc/sentinel/run.goweb/apps/dashboard/.env.exampleweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/deployments/[deploymentId]/network/unkey-flow/components/nodes/types.tsweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/components/runtime-settings/instances.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/(overview)/settings/components/runtime-settings/regions.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/[projectId]/components/region-flag.tsxweb/apps/dashboard/app/(app)/[workspaceSlug]/projects/new/steps/onboarding-environment-provider.tsxweb/apps/dashboard/lib/collections/deploy/environment-settings.tsweb/apps/dashboard/lib/trpc/routers/deploy/environment-settings/get-available-regions.tsweb/apps/dashboard/lib/trpc/routers/deploy/environment-settings/get.tsweb/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-instances.tsweb/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-regions.tsweb/apps/dashboard/lib/trpc/routers/deploy/network/utils.tsweb/internal/db/src/schema/app_regional_settings.tsweb/internal/db/src/schema/app_runtime_settings.tsweb/internal/db/src/schema/clusters.tsweb/internal/db/src/schema/deployment_topology.tsweb/internal/db/src/schema/horizontal_autoscaling_policies.tsweb/internal/db/src/schema/index.tsweb/internal/db/src/schema/regions.tsweb/tools/migrate/region-config-to-regional-settings.ts
💤 Files with no reviewable changes (9)
- svc/ctrl/api/run.go
- svc/ctrl/api/config.go
- svc/ctrl/worker/config.go
- dev/k8s/manifests/ctrl-worker.yaml
- docs/engineering/architecture/services/control-plane/worker/configuration.mdx
- svc/ctrl/services/deployment/create_deployment.go
- dev/config/ctrl-api.toml
- web/apps/dashboard/.env.example
- dev/k8s/manifests/ctrl-api.yaml
Comment on lines
+13
to
+14
| ON DUPLICATE KEY UPDATE | ||
| last_heartbeat_at = sqlc.arg(last_heartbeat_at); |
Contributor
There was a problem hiding this comment.
Handle duplicate hits on both unique keys.
clusters is unique on both id and region_id, but this upsert only refreshes last_heartbeat_at. If a cluster is re-registered under a new region, or a region gets a new cluster ID, the stale mapping survives and the control plane keeps the old association.
Proposed fix
ON DUPLICATE KEY UPDATE
+ id = sqlc.arg(id),
+ region_id = sqlc.arg(region_id),
last_heartbeat_at = sqlc.arg(last_heartbeat_at);📝 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.
Suggested change
| ON DUPLICATE KEY UPDATE | |
| last_heartbeat_at = sqlc.arg(last_heartbeat_at); | |
| ON DUPLICATE KEY UPDATE | |
| id = sqlc.arg(id), | |
| region_id = sqlc.arg(region_id), | |
| last_heartbeat_at = sqlc.arg(last_heartbeat_at); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/db/queries/cluster_upsert.sql` around lines 13 - 14, The upsert only
updates last_heartbeat_at and ignores conflicts on the other unique key
(region_id), leaving stale mappings; update the ON DUPLICATE KEY UPDATE clause
in the cluster_upsert.sql so it also refreshes the id and region_id from the
incoming values (e.g., set id = VALUES(id) or id = sqlc.arg(id), region_id =
VALUES(region_id) or region_id = sqlc.arg(region_id), and last_heartbeat_at =
VALUES(last_heartbeat_at) / sqlc.arg(last_heartbeat_at)) so that conflicts on
either unique key reconcile to the new mapping for the clusters table
(referenced symbols: clusters, id, region_id, last_heartbeat_at).
svc/ctrl/services/cluster/rpc_get_desired_cilium_network_policy_state.go
Show resolved
Hide resolved
web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-instances.ts
Show resolved
Hide resolved
web/apps/dashboard/lib/trpc/routers/deploy/environment-settings/runtime/update-regions.ts
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
svc/frontline/services/router/service.go (1)
129-145:⚠️ Potential issue | 🔴 CriticalKey shape inconsistent.
Line 139 looks up
region.platform, but Line 113 storesgw.Regionand Line 124 still checkss.region. One path will miss: ifgw.Regionis region-only, proximity falls back to an arbitrary region and the proxy can build a host without platform; if it already includes platform, local routing never matches. Normalize this key once, then use it for map population, local lookup, and proximity lookup.As per coding guidelines, "Make illegal states unrepresentable by modeling domain with ADTs/discriminated unions and parsing inputs at boundaries into typed structures".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@svc/frontline/services/router/service.go` around lines 129 - 145, The code builds and looks up region keys inconsistently (sometimes using gw.Region, sometimes s.region plus s.platform), causing missed matches; update the logic to normalize region keys into a single canonical form (e.g., "region.platform") at the boundary where healthyByRegion is populated and then use that same canonical key for lookups in findNearestRegionPlatform (the self variable and proximity map lookup), and when setting decision.NearestNLBRegionPlatform ensure you store the canonical key; specifically, change the code paths that populate healthyByRegion (where gw.Region is used), the creation of self in findNearestRegionPlatform, and the regionProximity keys to all use the same normalized representation so lookups always match.
🧹 Nitpick comments (1)
svc/frontline/services/router/interface.go (1)
14-16: Don't smuggleregion+platformin one string.
NearestNLBRegionPlatformmakes malformed targets representable and forces every caller to know the"<region>.<platform>"encoding. Prefer a small typed value here and render the DNS label only at the proxy boundary.As per coding guidelines, "Make illegal states unrepresentable by modeling domain with ADTs/discriminated unions and parsing inputs at boundaries into typed structures".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@svc/frontline/services/router/interface.go` around lines 14 - 16, NearestNLBRegionPlatform currently smuggles "region.platform" into a single string which allows malformed values and leaks encoding to callers; replace this with a small typed ADT (e.g., a struct/type named RegionPlatform with explicit Region and Platform fields and a constructor ParseRegionPlatform) and update the interface to expose that type instead of NearestNLBRegionPlatform string; add a method on the new type (e.g., ToDNSLabel or String) that renders the DNS-safe "<region>.<platform>" only where the proxy/NLB boundary needs it, and update callers to construct/parse at input boundaries via ParseRegionPlatform and to call ToDNSLabel only at the proxy layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@svc/frontline/config.go`:
- Around line 33-35: The Platform field in the config struct is only marked
required but not validated, so invalid (non-DNS-label-safe) values can flow into
routing code; update the Validate() method to check the Platform string on load
and return an error if it is not a valid DNS label (RFC1123 style: lowercase
letters, digits and hyphens, starts/ends with alphanumeric, max 63 chars).
Locate the config struct's Platform field and add this check in the Validate()
function (return a descriptive error mentioning Platform), ensuring all callers
that load config will fail fast on invalid platform values.
In `@svc/frontline/services/proxy/director.go`:
- Around line 40-42: The sentinel director (makeSentinelDirector) is currently
forwarding X-Unkey-Frontline-Id, X-Unkey-Region, and X-Unkey-Request-Id but
omits the platform header, so same-region/local sentinel hops lose platform
context; update makeSentinelDirector to also set the HeaderPlatform
(X-Unkey-Platform) on the outgoing request alongside HeaderFrontlineID and
HeaderRegion (and ensure any use of req.Header.Set(HeaderPlatform, ...) mirrors
the remote-hop logic used elsewhere) so local sentinel paths include the
platform header consistently.
---
Outside diff comments:
In `@svc/frontline/services/router/service.go`:
- Around line 129-145: The code builds and looks up region keys inconsistently
(sometimes using gw.Region, sometimes s.region plus s.platform), causing missed
matches; update the logic to normalize region keys into a single canonical form
(e.g., "region.platform") at the boundary where healthyByRegion is populated and
then use that same canonical key for lookups in findNearestRegionPlatform (the
self variable and proximity map lookup), and when setting
decision.NearestNLBRegionPlatform ensure you store the canonical key;
specifically, change the code paths that populate healthyByRegion (where
gw.Region is used), the creation of self in findNearestRegionPlatform, and the
regionProximity keys to all use the same normalized representation so lookups
always match.
---
Nitpick comments:
In `@svc/frontline/services/router/interface.go`:
- Around line 14-16: NearestNLBRegionPlatform currently smuggles
"region.platform" into a single string which allows malformed values and leaks
encoding to callers; replace this with a small typed ADT (e.g., a struct/type
named RegionPlatform with explicit Region and Platform fields and a constructor
ParseRegionPlatform) and update the interface to expose that type instead of
NearestNLBRegionPlatform string; add a method on the new type (e.g., ToDNSLabel
or String) that renders the DNS-safe "<region>.<platform>" only where the
proxy/NLB boundary needs it, and update callers to construct/parse at input
boundaries via ParseRegionPlatform and to call ToDNSLabel only at the proxy
layer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 532e7bcf-c4ce-42d6-9b55-0aa8d7cb4d4c
📒 Files selected for processing (11)
dev/k8s/manifests/frontline.yamlsvc/frontline/config.gosvc/frontline/routes/proxy/handler.gosvc/frontline/run.gosvc/frontline/services/proxy/director.gosvc/frontline/services/proxy/forward.gosvc/frontline/services/proxy/headers.gosvc/frontline/services/proxy/interface.gosvc/frontline/services/proxy/service.gosvc/frontline/services/router/interface.gosvc/frontline/services/router/service.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@svc/ctrl/services/cluster/rpc_watch_deployments.go`:
- Around line 40-45: The request currently requires the X-Krane-Platform header
(variable platform) via assert.All(assert.NotEmpty(...)) even though the RPC
only uses region to scope results; remove platform from the mandatory checks so
older callers without that header aren't rejected—i.e., update the validation in
rpc_watch_deployments.go to only assert.NotEmpty(region, "region is required")
(leave platform as an optional header and validate it later only when it is
actually used).
In `@svc/ctrl/worker/deploy/deploy_handler.go`:
- Around line 391-408: The query that loads regionalSettings uses the
read-replica accessor w.db.RO(), which can return stale results and incorrectly
trigger the terminal "No regions configured" error; change the DB accessor
inside the restate.Run closure to use w.db.RW() (i.e., call
db.Query.FindAppRegionalSettingsByAppAndEnv with w.db.RW() instead of w.db.RO())
so the just-saved region is visible; keep the restate.Run,
FindAppRegionalSettingsByAppAndEnv call, and error handling unchanged.
- Around line 413-430: Reject or filter out regions with non-positive replica
counts before creating deployment topologies: validate regionalSettings entries
(the loop over regionalSettings and the rs.Replicas field) and return a
validation error or skip entries where rs.Replicas <= 0 so you never append a
topology with DesiredReplicas <= 0 (see where you build
db.InsertDeploymentTopologyParams and set DesiredReplicas). Ensure this check is
performed at the boundary (prior to appending to topologies and prior to
creating sentinels) so waitForDeployments cannot treat zero/non-positive
DesiredReplicas as healthy; update the handler that builds topologies (the loop
using regionalSettings and rs) to either filter invalid regions or return a
clear error indicating the invalid rs.Replicas value.
In `@web/internal/db/src/schema/deployment_topology.ts`:
- Around line 22-23: The schema uses the display column region as a
uniqueness/version key which causes collisions across platforms; change the
uniqueness and version index definitions in deployment_topology to use regionId
(the stable key) instead of region. Locate the deployment_topology table
definition where region and regionId are declared and update any unique
constraints/indexes and version stream indexes that reference region to
reference regionId (ensure columns named regionId are used in place of region
for uniqueness/versioning).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6eece26e-2534-48fb-90ca-33448d6a8299
📒 Files selected for processing (7)
pkg/db/models_generated.gopkg/db/schema.sqlsvc/ctrl/integration/harness.gosvc/ctrl/services/cluster/rpc_watch_deployments.gosvc/ctrl/worker/deploy/deploy_handler.gosvc/ctrl/worker/deployment/deployment_state.goweb/internal/db/src/schema/deployment_topology.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.