feat(coldfront): control-plane support for ColdFront (single-node)#421
Draft
dpage wants to merge 25 commits into
Draft
feat(coldfront): control-plane support for ColdFront (single-node)#421dpage wants to merge 25 commits into
dpage wants to merge 25 commits into
Conversation
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 4 critical (2 false positives) |
| Complexity | 13 medium |
🟢 Metrics 232 complexity · 47 duplication
Metric Results Complexity 232 Duplication 47
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
dpage
marked this pull request as draft
July 2, 2026 14:46
Add the control-plane side of ColdFront transparent data tiering: deploy and bootstrap the Lakekeeper Iceberg catalog per database, load the extension config, and schedule the tiering jobs. Consumes the `lakekeeper` service the saas control plane sends. This is the single-node scope; it is one of several per-repo PRs for the feature. Included: - Register the `lakekeeper` service type (image, launch on port 8181, config resource, validator, Goa enum), following the MCP recipe. - External Lakekeeper catalog Postgres via a configurable connection URL (Cloud supplies a managed instance; the control plane does not provision it), with a `migrate`-before-`serve` dependency and fail-loud if the URL is absent. - Post-deploy bootstrap: idempotent Lakekeeper REST warehouse creation (bootstrap -> warehouse -> namespace) with the correct S3 storage-profile (`flavor`/`path-style-access`/`key-prefix`), and `coldfront.set_storage_secret` / `_azure` on the database with the object-store credential bound as query arguments (never interpolated or logged). - Schedule the archiver/partitioner/compactor via the existing gocron/etcd scheduler, running each single-pass in the primary node's Postgres container and capturing the exit code (recorded as `task.TypeTiering`); the archiver's "no tables configured" exit is treated as benign. - Reject enabling ColdFront on a multi-node database (fail-loud), pending the deferred mesh `snowflake.node` reconciliation. Deferred to follow-ups (see PR description): the per-node mesh GUCs for multi-node ColdFront (needs a CP + ColdFront-author decision on `snowflake.node` ownership); expansion of the saas lakekeeper contract (`catalog_db_url`, `pg_encryption_key`, `provider`/`bucket`/`region`/ `endpoint`); the ColdFront-enabled Postgres image; and confirmation of the pinned Lakekeeper image tag.
AntTheLimey
force-pushed
the
feat/coldfront
branch
from
July 16, 2026 17:51
a331459 to
f94b21d
Compare
|
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:
📝 WalkthroughWalkthroughChangesLakekeeper and ColdFront integration
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
server/internal/workflows/activities/coldfront_tiering.go (2)
22-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBinary-name strings duplicated across packages with no shared constant.
coldFrontArchiverBinary = "archiver"gates the benign no-tables-configured classification, but the caller in the scheduler package passes independent string literals for all three binaries; nothing ties them together at compile time.
server/internal/workflows/activities/coldfront_tiering.go#L22-L24: export this constant (and addColdFrontPartitionerBinary/ColdFrontCompactorBinarysiblings) so callers reference the same source of truth instead of re-typing the strings.server/internal/scheduler/scheduled_job_executor.go#L68-L74: replace the"archiver"/"partitioner"/"compactor"literals with the exported constants from the activities package.🤖 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 `@server/internal/workflows/activities/coldfront_tiering.go` around lines 22 - 24, The binary names are duplicated instead of sharing compile-time constants. In server/internal/workflows/activities/coldfront_tiering.go lines 22-24, export coldFrontArchiverBinary as ColdFrontArchiverBinary and add exported ColdFrontPartitionerBinary and ColdFrontCompactorBinary constants; in server/internal/scheduler/scheduled_job_executor.go lines 68-74, replace the three binary string literals with those activities-package constants.
41-74: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winValidate required credential fields per provider.
parseColdFrontStorageConfigvalidates theprovidervalue but doesn't check that the provider-specific credential fields (e.g.access_key_id/secret_access_keyfor aws,connection_stringfor azure) are non-empty. A misconfiguredcredentialblob will silently render an incomplete config and only fail later with an opaque exit code from inside the container.🤖 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 `@server/internal/workflows/activities/coldfront_tiering.go` around lines 41 - 74, Update parseColdFrontStorageConfig to validate required fields in the parsed Credential map after provider validation: require access_key_id and secret_access_key for aws, connection_string for azure, and the established required credential fields for gcs. Return a descriptive configuration error naming the provider and missing field instead of returning an incomplete config; preserve the existing behavior for providers without credentials only if no required fields are defined.server/internal/orchestrator/swarm/service_spec_test.go (1)
696-703: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only checks for a substring, so it can't catch a broken healthcheck invocation.
require.Contains(t, hc.Test, "healthcheck")passes whetherTestis["CMD", "healthcheck"]or the correct["CMD", "/home/nonroot/lakekeeper", "healthcheck"]. Asserting the full expectedTestslice (once the binary path is confirmed/fixed inservice_spec.go) would have caught the missing-path issue flagged there.✅ Tighten the assertion
hc := spec.TaskTemplate.ContainerSpec.Healthcheck require.NotNil(t, hc, "healthcheck must be set for lakekeeper") - require.Contains(t, hc.Test, "healthcheck") + assert.Equal(t, []string{"CMD", "/home/nonroot/lakekeeper", "healthcheck"}, hc.Test)🤖 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 `@server/internal/orchestrator/swarm/service_spec_test.go` around lines 696 - 703, Strengthen TestServiceContainerSpec_Lakekeeper_Healthcheck by asserting that hc.Test exactly matches the expected healthcheck command, including the Lakekeeper binary path and the "healthcheck" argument. Replace the substring assertion while preserving the existing non-nil healthcheck validation.
🤖 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 `@server/internal/api/apiv1/validate.go`:
- Around line 552-619: Extend validateLakekeeperServiceConfig to parse the
non-empty credential as provider-specific JSON and validate the required
sub-keys before returning. Require access_key_id and secret_access_key for aws,
hmac_access_id and hmac_secret for gcs, and connection_string for azure; report
malformed JSON, missing keys, or empty values through validation.NewError at
path.Append("credential"), while preserving the existing provider and
required-field checks.
In `@server/internal/orchestrator/swarm/lakekeeper_bootstrap_resource.go`:
- Around line 61-64: Separate Lakekeeper’s host and overlay endpoints across the
bootstrap flow: in
server/internal/orchestrator/swarm/lakekeeper_bootstrap_resource.go:61-64,
execute bootstrap inside a container attached to the overlay network or use a
resolvable host endpoint; at :116-123, use serviceName:8181 only for
overlay-network calls. In
server/internal/orchestrator/swarm/orchestrator.go:952-958, stop passing
ServiceSpec.Port as Lakekeeper’s internal listener, and at :986-991 build
scheduled workflow endpoints with container port 8181.
In `@server/internal/orchestrator/swarm/lakekeeper_bootstrap_test.go`:
- Around line 81-111: Replace the require.NoError calls inside the httptest
server handler with t.Errorf and an immediate return when JSON decoding fails,
covering both the warehouse request body and namespaceBody. Keep successful
decoding and response handling unchanged.
In `@server/internal/orchestrator/swarm/lakekeeper_bootstrap.go`:
- Around line 292-340: Update lakekeeperDo so tolerateConflict treats only HTTP
409 Conflict as an idempotent success, rather than all 4xx responses. Preserve
normal success decoding for 2xx responses and return an error containing the
response details for 400, 401, 403, and other non-409 failures.
- Around line 117-166: Update buildWarehouseRequestBody so provider "gcs" uses
storage.googleapis.com when cfg.Endpoint is empty, matching
parseLakekeeperStorageConfig and storage-secret generation. Ensure the default
flows through the existing endpoint-present branch, producing flavor "s3-compat"
and path-style access, while preserving explicit endpoint handling and native
AWS behavior.
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 1022-1030: Update the jobID construction in the tierings loop to
include the service instance identifier in addition to t.suffix,
spec.DatabaseID, and spec.NodeName. Use the existing service-instance symbol
from the surrounding orchestration context, ensuring scheduled-job identifiers
are unique across Lakekeeper instances while preserving the current naming
components.
In `@server/internal/orchestrator/swarm/service_spec.go`:
- Around line 243-271: The Lakekeeper healthcheck currently invokes
“healthcheck” as a standalone command instead of through the service binary.
Update the healthcheck configuration in the “lakekeeper” service case to execute
/home/nonroot/lakekeeper with the healthcheck argument, preserving the existing
timing and retry settings.
In `@server/internal/workflows/activities/coldfront_tiering_test.go`:
- Around line 22-74: Update the table-driven test around
buildColdFrontConfigYAML to assert that the rendered YAML content contains each
case’s wantKey value. Keep the existing structural YAML assertions unchanged so
the AWS and Azure provider-specific credential keys are both validated.
In `@server/internal/workflows/activities/coldfront_tiering.go`:
- Around line 283-296: Update the config execution flow around configPath and
runColdFrontBinary to remove /tmp/coldfront-config.yaml from the container after
the binary finishes, regardless of whether execution succeeds or fails. Ensure
cleanup runs before returning the binary’s error and preserves the original
execution result.
---
Nitpick comments:
In `@server/internal/orchestrator/swarm/service_spec_test.go`:
- Around line 696-703: Strengthen
TestServiceContainerSpec_Lakekeeper_Healthcheck by asserting that hc.Test
exactly matches the expected healthcheck command, including the Lakekeeper
binary path and the "healthcheck" argument. Replace the substring assertion
while preserving the existing non-nil healthcheck validation.
In `@server/internal/workflows/activities/coldfront_tiering.go`:
- Around line 22-24: The binary names are duplicated instead of sharing
compile-time constants. In
server/internal/workflows/activities/coldfront_tiering.go lines 22-24, export
coldFrontArchiverBinary as ColdFrontArchiverBinary and add exported
ColdFrontPartitionerBinary and ColdFrontCompactorBinary constants; in
server/internal/scheduler/scheduled_job_executor.go lines 68-74, replace the
three binary string literals with those activities-package constants.
- Around line 41-74: Update parseColdFrontStorageConfig to validate required
fields in the parsed Credential map after provider validation: require
access_key_id and secret_access_key for aws, connection_string for azure, and
the established required credential fields for gcs. Return a descriptive
configuration error naming the provider and missing field instead of returning
an incomplete config; preserve the existing behavior for providers without
credentials only if no required fields are defined.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d393020-1694-4a2b-98d3-4735ec70affd
⛔ Files ignored due to path filters (6)
api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (32)
.gitignoreapi/apiv1/design/database.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/orchestrator/swarm/coldfront_tiering_wiring_test.goserver/internal/orchestrator/swarm/lakekeeper_bootstrap.goserver/internal/orchestrator/swarm/lakekeeper_bootstrap_resource.goserver/internal/orchestrator/swarm/lakekeeper_bootstrap_test.goserver/internal/orchestrator/swarm/lakekeeper_config_resource.goserver/internal/orchestrator/swarm/lakekeeper_migrate_resource.goserver/internal/orchestrator/swarm/lakekeeper_storage_secret_resource.goserver/internal/orchestrator/swarm/lakekeeper_storage_secret_resource_test.goserver/internal/orchestrator/swarm/manifest_loader.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/orchestrator_test.goserver/internal/orchestrator/swarm/resources.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_spec_test.goserver/internal/orchestrator/swarm/version-manifest.jsonserver/internal/scheduler/coldfront_tiering_executor_test.goserver/internal/scheduler/scheduled_job_executor.goserver/internal/scheduler/types.goserver/internal/task/task.goserver/internal/workflows/activities/activities.goserver/internal/workflows/activities/coldfront_tiering.goserver/internal/workflows/activities/coldfront_tiering_test.goserver/internal/workflows/coldfront_tiering.goserver/internal/workflows/plan_update.goserver/internal/workflows/service.goserver/internal/workflows/workflows.go
Comment on lines
+552
to
+619
| // validateLakekeeperServiceConfig checks that the two required catalog | ||
| // configuration keys are present and non-empty. An absent or empty | ||
| // catalog_db_url would cause Lakekeeper to start with a blank connection | ||
| // string and crash-loop silently; we surface the error at spec-validation | ||
| // time instead. | ||
| func validateLakekeeperServiceConfig(config map[string]any, path validation.Path) []error { | ||
| var errs []error | ||
|
|
||
| catalogDBURL, _ := config["catalog_db_url"].(string) | ||
| if catalogDBURL == "" { | ||
| errs = append(errs, validation.NewError( | ||
| errors.New("catalog_db_url is required: provide the connection URL for the external Lakekeeper catalog Postgres"), | ||
| path.Append("catalog_db_url"), | ||
| )) | ||
| } | ||
|
|
||
| pgEncryptionKey, _ := config["pg_encryption_key"].(string) | ||
| if pgEncryptionKey == "" { | ||
| errs = append(errs, validation.NewError( | ||
| errors.New("pg_encryption_key is required: provide the encryption key for Lakekeeper's catalog Postgres"), | ||
| path.Append("pg_encryption_key"), | ||
| )) | ||
| } | ||
|
|
||
| // The warehouse bootstrap and coldfront.set_storage_secret both need the | ||
| // object-store coordinates and a provider-specific credential. Fail loud | ||
| // here so a database is never left with an unbootstrapped (broken) | ||
| // warehouse. Some of these keys are a saas follow-up. | ||
| provider, _ := config["provider"].(string) | ||
| switch provider { | ||
| case "aws", "azure", "gcs": | ||
| case "": | ||
| errs = append(errs, validation.NewError( | ||
| errors.New("provider is required: one of aws, azure, gcs"), | ||
| path.Append("provider"), | ||
| )) | ||
| default: | ||
| errs = append(errs, validation.NewError( | ||
| fmt.Errorf("unsupported provider %q: expected one of aws, azure, gcs", provider), | ||
| path.Append("provider"), | ||
| )) | ||
| } | ||
|
|
||
| if warehouse, _ := config["warehouse"].(string); warehouse == "" { | ||
| errs = append(errs, validation.NewError( | ||
| errors.New("warehouse is required: the warehouse name to create in Lakekeeper"), | ||
| path.Append("warehouse"), | ||
| )) | ||
| } | ||
|
|
||
| if credential, _ := config["credential"].(string); credential == "" { | ||
| errs = append(errs, validation.NewError( | ||
| errors.New("credential is required: a provider-specific credential JSON string"), | ||
| path.Append("credential"), | ||
| )) | ||
| } | ||
|
|
||
| if provider == "aws" || provider == "gcs" { | ||
| if bucket, _ := config["bucket"].(string); bucket == "" { | ||
| errs = append(errs, validation.NewError( | ||
| errors.New("bucket is required for aws and gcs providers"), | ||
| path.Append("bucket"), | ||
| )) | ||
| } | ||
| } | ||
|
|
||
| return errs | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
credential is validated only for non-emptiness, not for the shape each provider actually needs.
validateLakekeeperServiceConfig requires catalog_db_url, pg_encryption_key, provider, warehouse, credential, and bucket (aws/gcs) to be non-empty, but never parses credential to confirm it contains the provider-specific sub-keys that lakekeeper_storage_secret_resource.go later expects (access_key_id/secret_access_key for aws, hmac_access_id/hmac_secret for gcs, connection_string for azure). A malformed or wrong-shaped credential JSON passes spec validation and only fails — or worse, silently binds empty strings as SQL parameters — much later inside LakekeeperStorageSecretResource.Create. This undercuts the "fail loud" intent stated in this very function's comment ("a database is never left with an unbootstrapped (broken) warehouse").
🤖 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 `@server/internal/api/apiv1/validate.go` around lines 552 - 619, Extend
validateLakekeeperServiceConfig to parse the non-empty credential as
provider-specific JSON and validate the required sub-keys before returning.
Require access_key_id and secret_access_key for aws, hmac_access_id and
hmac_secret for gcs, and connection_string for azure; report malformed JSON,
missing keys, or empty values through validation.NewError at
path.Append("credential"), while preserving the existing provider and
required-field checks.
Comment on lines
+81
to
+111
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| calls = append(calls, r.Method+" "+r.URL.Path) | ||
| switch { | ||
| case r.URL.Path == "/management/v1/bootstrap": | ||
| w.WriteHeader(http.StatusOK) | ||
| case r.URL.Path == "/management/v1/warehouse" && r.Method == http.MethodPost: | ||
| // verify the storage profile is well-formed for aws | ||
| var body map[string]any | ||
| require.NoError(t, json.NewDecoder(r.Body).Decode(&body)) | ||
| assert.Equal(t, "wh1", body["warehouse-name"]) | ||
| profile := body["storage-profile"].(map[string]any) | ||
| assert.Equal(t, "s3", profile["type"]) | ||
| assert.Equal(t, false, profile["sts-enabled"]) | ||
| assert.Equal(t, false, profile["remote-signing-enabled"]) | ||
| // Cloud AWS (no endpoint): virtual-hosted addressing. | ||
| assert.Equal(t, "aws", profile["flavor"]) | ||
| assert.Equal(t, false, profile["path-style-access"]) | ||
| assert.NotContains(t, profile, "endpoint") | ||
| // key-prefix, not path-prefix. | ||
| assert.Equal(t, "iceberg", profile["key-prefix"]) | ||
| assert.NotContains(t, profile, "path-prefix") | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write([]byte(`{"warehouse-id":"wh-uuid-123"}`)) | ||
| case strings.HasPrefix(r.URL.Path, "/catalog/v1/") && strings.HasSuffix(r.URL.Path, "/namespaces"): | ||
| require.NoError(t, json.NewDecoder(r.Body).Decode(&namespaceBody)) | ||
| w.WriteHeader(http.StatusOK) | ||
| default: | ||
| t.Errorf("unexpected call: %s %s", r.Method, r.URL.Path) | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| } | ||
| })) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the test file around the cited lines.
sed -n '1,220p' server/internal/orchestrator/swarm/lakekeeper_bootstrap_test.go
printf '\n---\n'
# Find all require/assert uses in this test file.
rg -n 'require\.(NoError|Error|Equal|NotNil|True|False|Contains|NotContains)|assert\.' server/internal/orchestrator/swarm/lakekeeper_bootstrap_test.goRepository: pgEdge/control-plane
Length of output: 10804
Avoid require.NoError inside the HTTP handler. FailNow runs in the server goroutine, so a decode failure exits the handler before it can write a response; use t.Errorf plus return here and for namespaceBody.
🤖 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 `@server/internal/orchestrator/swarm/lakekeeper_bootstrap_test.go` around lines
81 - 111, Replace the require.NoError calls inside the httptest server handler
with t.Errorf and an immediate return when JSON decoding fails, covering both
the warehouse request body and namespaceBody. Keep successful decoding and
response handling unchanged.
Comment on lines
+117
to
+166
| func buildWarehouseRequestBody(cfg *lakekeeperStorageConfig) (map[string]any, error) { | ||
| switch cfg.Provider { | ||
| case "aws", "gcs": | ||
| profile := map[string]any{ | ||
| "type": "s3", | ||
| "bucket": cfg.Bucket, | ||
| "region": cfg.Region, | ||
| "sts-enabled": false, | ||
| "remote-signing-enabled": false, | ||
| } | ||
| // Endpoint presence is the discriminator between native cloud AWS and | ||
| // an S3-compatible store (GCS via storage.googleapis.com, SeaweedFS, | ||
| // etc.). Cloud AWS must use virtual-hosted addressing (flavor "aws", | ||
| // path-style-access false); path-style fails on any Region launched | ||
| // after 2019. An S3-compatible endpoint uses flavor "s3-compat" with | ||
| // path-style addressing. (ColdFront docs/object_store.md, installation.md.) | ||
| if cfg.Endpoint != "" { | ||
| profile["endpoint"] = cfg.Endpoint | ||
| profile["flavor"] = "s3-compat" | ||
| profile["path-style-access"] = true | ||
| } else { | ||
| profile["flavor"] = "aws" | ||
| profile["path-style-access"] = false | ||
| } | ||
| if cfg.PathPrefix != "" { | ||
| profile["key-prefix"] = cfg.PathPrefix | ||
| } | ||
|
|
||
| var credential map[string]any | ||
| if cfg.Provider == "aws" { | ||
| credential = map[string]any{ | ||
| "type": "s3", | ||
| "credential-type": "access-key", | ||
| "aws-access-key-id": cfg.Credential["access_key_id"], | ||
| "aws-secret-access-key": cfg.Credential["secret_access_key"], | ||
| } | ||
| } else { // gcs via S3-compatible HMAC | ||
| credential = map[string]any{ | ||
| "type": "s3", | ||
| "credential-type": "access-key", | ||
| "aws-access-key-id": cfg.Credential["hmac_access_id"], | ||
| "aws-secret-access-key": cfg.Credential["hmac_secret"], | ||
| } | ||
| } | ||
|
|
||
| return map[string]any{ | ||
| "warehouse-name": cfg.Warehouse, | ||
| "storage-profile": profile, | ||
| "storage-credential": credential, | ||
| }, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Default the endpoint for GCS instead of treating it as native AWS.
parseLakekeeperStorageConfig permits GCS without an endpoint, but this branch then emits flavor: "aws" and disables path-style access. Default GCS to its S3-compatible endpoint, consistent with storage-secret generation.
Proposed fix
func buildWarehouseRequestBody(cfg *lakekeeperStorageConfig) (map[string]any, error) {
switch cfg.Provider {
case "aws", "gcs":
+ endpoint := cfg.Endpoint
+ if cfg.Provider == "gcs" && endpoint == "" {
+ endpoint = "https://storage.googleapis.com"
+ }
profile := map[string]any{
"type": "s3",
"bucket": cfg.Bucket,
"region": cfg.Region,
"sts-enabled": false,
"remote-signing-enabled": false,
}
- if cfg.Endpoint != "" {
- profile["endpoint"] = cfg.Endpoint
+ if endpoint != "" {
+ profile["endpoint"] = endpoint📝 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
| func buildWarehouseRequestBody(cfg *lakekeeperStorageConfig) (map[string]any, error) { | |
| switch cfg.Provider { | |
| case "aws", "gcs": | |
| profile := map[string]any{ | |
| "type": "s3", | |
| "bucket": cfg.Bucket, | |
| "region": cfg.Region, | |
| "sts-enabled": false, | |
| "remote-signing-enabled": false, | |
| } | |
| // Endpoint presence is the discriminator between native cloud AWS and | |
| // an S3-compatible store (GCS via storage.googleapis.com, SeaweedFS, | |
| // etc.). Cloud AWS must use virtual-hosted addressing (flavor "aws", | |
| // path-style-access false); path-style fails on any Region launched | |
| // after 2019. An S3-compatible endpoint uses flavor "s3-compat" with | |
| // path-style addressing. (ColdFront docs/object_store.md, installation.md.) | |
| if cfg.Endpoint != "" { | |
| profile["endpoint"] = cfg.Endpoint | |
| profile["flavor"] = "s3-compat" | |
| profile["path-style-access"] = true | |
| } else { | |
| profile["flavor"] = "aws" | |
| profile["path-style-access"] = false | |
| } | |
| if cfg.PathPrefix != "" { | |
| profile["key-prefix"] = cfg.PathPrefix | |
| } | |
| var credential map[string]any | |
| if cfg.Provider == "aws" { | |
| credential = map[string]any{ | |
| "type": "s3", | |
| "credential-type": "access-key", | |
| "aws-access-key-id": cfg.Credential["access_key_id"], | |
| "aws-secret-access-key": cfg.Credential["secret_access_key"], | |
| } | |
| } else { // gcs via S3-compatible HMAC | |
| credential = map[string]any{ | |
| "type": "s3", | |
| "credential-type": "access-key", | |
| "aws-access-key-id": cfg.Credential["hmac_access_id"], | |
| "aws-secret-access-key": cfg.Credential["hmac_secret"], | |
| } | |
| } | |
| return map[string]any{ | |
| "warehouse-name": cfg.Warehouse, | |
| "storage-profile": profile, | |
| "storage-credential": credential, | |
| }, nil | |
| func buildWarehouseRequestBody(cfg *lakekeeperStorageConfig) (map[string]any, error) { | |
| switch cfg.Provider { | |
| case "aws", "gcs": | |
| endpoint := cfg.Endpoint | |
| if cfg.Provider == "gcs" && endpoint == "" { | |
| endpoint = "https://storage.googleapis.com" | |
| } | |
| profile := map[string]any{ | |
| "type": "s3", | |
| "bucket": cfg.Bucket, | |
| "region": cfg.Region, | |
| "sts-enabled": false, | |
| "remote-signing-enabled": false, | |
| } | |
| // Endpoint presence is the discriminator between native cloud AWS and | |
| // an S3-compatible store (GCS via storage.googleapis.com, SeaweedFS, | |
| // etc.). Cloud AWS must use virtual-hosted addressing (flavor "aws", | |
| // path-style-access false); path-style fails on any Region launched | |
| // after 2019. An S3-compatible endpoint uses flavor "s3-compat" with | |
| // path-style addressing. (ColdFront docs/object_store.md, installation.md.) | |
| if endpoint != "" { | |
| profile["endpoint"] = endpoint | |
| profile["flavor"] = "s3-compat" | |
| profile["path-style-access"] = true | |
| } else { | |
| profile["flavor"] = "aws" | |
| profile["path-style-access"] = false | |
| } | |
| if cfg.PathPrefix != "" { | |
| profile["key-prefix"] = cfg.PathPrefix | |
| } | |
| var credential map[string]any | |
| if cfg.Provider == "aws" { | |
| credential = map[string]any{ | |
| "type": "s3", | |
| "credential-type": "access-key", | |
| "aws-access-key-id": cfg.Credential["access_key_id"], | |
| "aws-secret-access-key": cfg.Credential["secret_access_key"], | |
| } | |
| } else { // gcs via S3-compatible HMAC | |
| credential = map[string]any{ | |
| "type": "s3", | |
| "credential-type": "access-key", | |
| "aws-access-key-id": cfg.Credential["hmac_access_id"], | |
| "aws-secret-access-key": cfg.Credential["hmac_secret"], | |
| } | |
| } | |
| return map[string]any{ | |
| "warehouse-name": cfg.Warehouse, | |
| "storage-profile": profile, | |
| "storage-credential": credential, | |
| }, nil |
🤖 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 `@server/internal/orchestrator/swarm/lakekeeper_bootstrap.go` around lines 117
- 166, Update buildWarehouseRequestBody so provider "gcs" uses
storage.googleapis.com when cfg.Endpoint is empty, matching
parseLakekeeperStorageConfig and storage-secret generation. Ensure the default
flows through the existing endpoint-present branch, producing flavor "s3-compat"
and path-style access, while preserving explicit endpoint handling and native
AWS behavior.
Comment on lines
+292
to
+340
| // lakekeeperPost issues a POST with a JSON body. When out is non-nil the | ||
| // response body is decoded into it. When tolerateConflict is true a 4xx | ||
| // response is treated as success (idempotent already-exists), otherwise any | ||
| // status >= 300 is an error. | ||
| func lakekeeperPost(ctx context.Context, client *http.Client, url string, body any, out any, tolerateConflict bool) error { | ||
| _, err := lakekeeperDo(ctx, client, url, body, out, tolerateConflict) | ||
| return err | ||
| } | ||
|
|
||
| // lakekeeperPostDecode issues a POST that always tolerates conflicts and | ||
| // decodes a success response into out. It returns whether the resource was | ||
| // newly created (2xx) as opposed to already existing (4xx conflict). | ||
| func lakekeeperPostDecode(ctx context.Context, client *http.Client, url string, body any, out any) (created bool, err error) { | ||
| return lakekeeperDo(ctx, client, url, body, out, true) | ||
| } | ||
|
|
||
| func lakekeeperDo(ctx context.Context, client *http.Client, url string, body any, out any, tolerateConflict bool) (created bool, err error) { | ||
| payload, err := json.Marshal(body) | ||
| if err != nil { | ||
| return false, fmt.Errorf("marshal request body: %w", err) | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload)) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Accept", "application/json") | ||
|
|
||
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| defer resp.Body.Close() | ||
| data, _ := io.ReadAll(resp.Body) | ||
|
|
||
| switch { | ||
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | ||
| if out != nil && len(data) > 0 { | ||
| if err := json.Unmarshal(data, out); err != nil { | ||
| return true, fmt.Errorf("decode response: %w", err) | ||
| } | ||
| } | ||
| return true, nil | ||
| case tolerateConflict && resp.StatusCode >= 400 && resp.StatusCode < 500: | ||
| // Already bootstrapped / already exists — idempotent success. | ||
| return false, nil | ||
| default: | ||
| return false, fmt.Errorf("POST %s returned status %d: %s", url, resp.StatusCode, string(data)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Only tolerate the specific “already exists” response.
The helper currently converts every 4xx—including 400, 401, 403, and invalid namespace requests—into success. A rejected namespace creation can therefore set BootstrapDone while leaving the warehouse unusable. Match only 409 Conflict, or inspect Lakekeeper’s structured error code for the known idempotent case.
- case tolerateConflict && resp.StatusCode >= 400 && resp.StatusCode < 500:
+ case tolerateConflict && resp.StatusCode == http.StatusConflict:📝 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
| // lakekeeperPost issues a POST with a JSON body. When out is non-nil the | |
| // response body is decoded into it. When tolerateConflict is true a 4xx | |
| // response is treated as success (idempotent already-exists), otherwise any | |
| // status >= 300 is an error. | |
| func lakekeeperPost(ctx context.Context, client *http.Client, url string, body any, out any, tolerateConflict bool) error { | |
| _, err := lakekeeperDo(ctx, client, url, body, out, tolerateConflict) | |
| return err | |
| } | |
| // lakekeeperPostDecode issues a POST that always tolerates conflicts and | |
| // decodes a success response into out. It returns whether the resource was | |
| // newly created (2xx) as opposed to already existing (4xx conflict). | |
| func lakekeeperPostDecode(ctx context.Context, client *http.Client, url string, body any, out any) (created bool, err error) { | |
| return lakekeeperDo(ctx, client, url, body, out, true) | |
| } | |
| func lakekeeperDo(ctx context.Context, client *http.Client, url string, body any, out any, tolerateConflict bool) (created bool, err error) { | |
| payload, err := json.Marshal(body) | |
| if err != nil { | |
| return false, fmt.Errorf("marshal request body: %w", err) | |
| } | |
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload)) | |
| if err != nil { | |
| return false, err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| req.Header.Set("Accept", "application/json") | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return false, err | |
| } | |
| defer resp.Body.Close() | |
| data, _ := io.ReadAll(resp.Body) | |
| switch { | |
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | |
| if out != nil && len(data) > 0 { | |
| if err := json.Unmarshal(data, out); err != nil { | |
| return true, fmt.Errorf("decode response: %w", err) | |
| } | |
| } | |
| return true, nil | |
| case tolerateConflict && resp.StatusCode >= 400 && resp.StatusCode < 500: | |
| // Already bootstrapped / already exists — idempotent success. | |
| return false, nil | |
| default: | |
| return false, fmt.Errorf("POST %s returned status %d: %s", url, resp.StatusCode, string(data)) | |
| } | |
| // lakekeeperPost issues a POST with a JSON body. When out is non-nil the | |
| // response body is decoded into it. When tolerateConflict is true a 4xx | |
| // response is treated as success (idempotent already-exists), otherwise any | |
| // status >= 300 is an error. | |
| func lakekeeperPost(ctx context.Context, client *http.Client, url string, body any, out any, tolerateConflict bool) error { | |
| _, err := lakekeeperDo(ctx, client, url, body, out, tolerateConflict) | |
| return err | |
| } | |
| // lakekeeperPostDecode issues a POST that always tolerates conflicts and | |
| // decodes a success response into out. It returns whether the resource was | |
| // newly created (2xx) as opposed to already existing (4xx conflict). | |
| func lakekeeperPostDecode(ctx context.Context, client *http.Client, url string, body any, out any) (created bool, err error) { | |
| return lakekeeperDo(ctx, client, url, body, out, true) | |
| } | |
| func lakekeeperDo(ctx context.Context, client *http.Client, url string, body any, out any, tolerateConflict bool) (created bool, err error) { | |
| payload, err := json.Marshal(body) | |
| if err != nil { | |
| return false, fmt.Errorf("marshal request body: %w", err) | |
| } | |
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload)) | |
| if err != nil { | |
| return false, err | |
| } | |
| req.Header.Set("Content-Type", "application/json") | |
| req.Header.Set("Accept", "application/json") | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return false, err | |
| } | |
| defer resp.Body.Close() | |
| data, _ := io.ReadAll(resp.Body) | |
| switch { | |
| case resp.StatusCode >= 200 && resp.StatusCode < 300: | |
| if out != nil && len(data) > 0 { | |
| if err := json.Unmarshal(data, out); err != nil { | |
| return true, fmt.Errorf("decode response: %w", err) | |
| } | |
| } | |
| return true, nil | |
| case tolerateConflict && resp.StatusCode == http.StatusConflict: | |
| // Already bootstrapped / already exists — idempotent success. | |
| return false, nil | |
| default: | |
| return false, fmt.Errorf("POST %s returned status %d: %s", url, resp.StatusCode, string(data)) | |
| } |
🤖 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 `@server/internal/orchestrator/swarm/lakekeeper_bootstrap.go` around lines 292
- 340, Update lakekeeperDo so tolerateConflict treats only HTTP 409 Conflict as
an idempotent success, rather than all 4xx responses. Preserve normal success
decoding for 2xx responses and return an error containing the response details
for 400, 401, 403, and other non-409 failures.
Comment on lines
+1022
to
+1030
| for _, t := range tierings { | ||
| jobID := fmt.Sprintf("coldfront-%s-%s-%s", t.suffix, spec.DatabaseID, spec.NodeName) | ||
| orchestratorResources = append(orchestratorResources, scheduler.NewScheduledJobResource( | ||
| jobID, | ||
| getCron(t.cronKey, t.cron), | ||
| t.workflow, | ||
| tieringArgs, | ||
| nil, | ||
| )) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include the service instance in scheduled-job identifiers.
Multiple Lakekeeper instances targeting the same database node generate identical job IDs, causing resource collisions or one instance’s endpoint/configuration to replace another’s.
Proposed fix
- jobID := fmt.Sprintf("coldfront-%s-%s-%s", t.suffix, spec.DatabaseID, spec.NodeName)
+ jobID := fmt.Sprintf(
+ "coldfront-%s-%s-%s-%s",
+ t.suffix,
+ spec.DatabaseID,
+ spec.NodeName,
+ spec.ServiceInstanceID,
+ )📝 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
| for _, t := range tierings { | |
| jobID := fmt.Sprintf("coldfront-%s-%s-%s", t.suffix, spec.DatabaseID, spec.NodeName) | |
| orchestratorResources = append(orchestratorResources, scheduler.NewScheduledJobResource( | |
| jobID, | |
| getCron(t.cronKey, t.cron), | |
| t.workflow, | |
| tieringArgs, | |
| nil, | |
| )) | |
| for _, t := range tierings { | |
| jobID := fmt.Sprintf( | |
| "coldfront-%s-%s-%s-%s", | |
| t.suffix, | |
| spec.DatabaseID, | |
| spec.NodeName, | |
| spec.ServiceInstanceID, | |
| ) | |
| orchestratorResources = append(orchestratorResources, scheduler.NewScheduledJobResource( | |
| jobID, | |
| getCron(t.cronKey, t.cron), | |
| t.workflow, | |
| tieringArgs, | |
| nil, | |
| )) |
🤖 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 `@server/internal/orchestrator/swarm/orchestrator.go` around lines 1022 - 1030,
Update the jobID construction in the tierings loop to include the service
instance identifier in addition to t.suffix, spec.DatabaseID, and spec.NodeName.
Use the existing service-instance symbol from the surrounding orchestration
context, ensuring scheduled-job identifiers are unique across Lakekeeper
instances while preserving the current naming components.
Comment on lines
+283
to
+296
| // Write the config file into the container using base64 to avoid any shell | ||
| // quoting or injection issues, then run the binary. | ||
| encoded := base64.StdEncoding.EncodeToString(configYAML) | ||
| configPath := "/tmp/coldfront-config.yaml" | ||
| binaryPath := "/usr/local/bin/" + input.Binary | ||
| cmd := []string{ | ||
| "sh", "-c", | ||
| fmt.Sprintf("printf '%%s' '%s' | base64 -d > %s && %s --config %s", | ||
| encoded, configPath, binaryPath, configPath), | ||
| } | ||
|
|
||
| if err := runColdFrontBinary(ctx, dockerTieringExecer{docker: dockerClient}, pgContainer.ID, input.Binary, cmd); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Clean up the rendered credentials file after use.
configYAML embeds the raw storage Credential (e.g. secret_access_key, connection_string) and is written to /tmp/coldfront-config.yaml inside the container but is never removed after the binary runs (success or failure). The file's own comment says the caller must ensure it's ephemeral, but nothing enforces that.
🔒 Proposed fix: remove the config file after the run
cmd := []string{
"sh", "-c",
- fmt.Sprintf("printf '%%s' '%s' | base64 -d > %s && %s --config %s",
- encoded, configPath, binaryPath, configPath),
+ fmt.Sprintf("printf '%%s' '%s' | base64 -d > %s && %s --config %s; rc=$?; rm -f %s; exit $rc",
+ encoded, configPath, binaryPath, configPath, configPath),
}📝 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
| // Write the config file into the container using base64 to avoid any shell | |
| // quoting or injection issues, then run the binary. | |
| encoded := base64.StdEncoding.EncodeToString(configYAML) | |
| configPath := "/tmp/coldfront-config.yaml" | |
| binaryPath := "/usr/local/bin/" + input.Binary | |
| cmd := []string{ | |
| "sh", "-c", | |
| fmt.Sprintf("printf '%%s' '%s' | base64 -d > %s && %s --config %s", | |
| encoded, configPath, binaryPath, configPath), | |
| } | |
| if err := runColdFrontBinary(ctx, dockerTieringExecer{docker: dockerClient}, pgContainer.ID, input.Binary, cmd); err != nil { | |
| return nil, err | |
| } | |
| // Write the config file into the container using base64 to avoid any shell | |
| // quoting or injection issues, then run the binary. | |
| encoded := base64.StdEncoding.EncodeToString(configYAML) | |
| configPath := "/tmp/coldfront-config.yaml" | |
| binaryPath := "/usr/local/bin/" + input.Binary | |
| cmd := []string{ | |
| "sh", "-c", | |
| fmt.Sprintf("printf '%%s' '%s' | base64 -d > %s && %s --config %s; rc=$?; rm -f %s; exit $rc", | |
| encoded, configPath, binaryPath, configPath, configPath), | |
| } | |
| if err := runColdFrontBinary(ctx, dockerTieringExecer{docker: dockerClient}, pgContainer.ID, input.Binary, cmd); err != nil { | |
| return nil, err | |
| } |
🤖 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 `@server/internal/workflows/activities/coldfront_tiering.go` around lines 283 -
296, Update the config execution flow around configPath and runColdFrontBinary
to remove /tmp/coldfront-config.yaml from the container after the binary
finishes, regardless of whether execution succeeds or fails. Ensure cleanup runs
before returning the binary’s error and preserves the original execution result.
catalog_db_url is required unless catalog_db_create is set, in which case the control plane provisions and connects to a managed catalog database itself. pg_encryption_key remains required in both modes.
- managed catalog URL: set sslmode=prefer explicitly, matching how MCP/RAG connect to the node's Postgres over the swarm overlay (was relying on the client driver default). - external-mode regression test: assert the caller-supplied catalog_db_url passes through unchanged to the migrate resource and serve-container config. - validator test: add a negative case for missing pg_encryption_key in managed mode (independent of the catalog_db_url gate).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/internal/orchestrator/swarm/orchestrator.go (1)
1040-1047: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not copy catalog credentials into scheduled-job arguments.
serviceConfigCopyincludescatalog_db_urlandpg_encryption_key, although tiering jobs only need storage configuration and the Lakekeeper endpoint. This unnecessarily persists and transports database credentials through scheduler/task state.Proposed fix
serviceConfigCopy := maps.Clone(serviceConfig) +delete(serviceConfigCopy, "catalog_db_url") +delete(serviceConfigCopy, "pg_encryption_key") serviceConfigCopy["lakekeeper_endpoint"] = lakekeeperEndpoint🤖 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 `@server/internal/orchestrator/swarm/orchestrator.go` around lines 1040 - 1047, Remove catalog credentials from the service configuration passed in tieringArgs: update the serviceConfigCopy construction in the tiering job setup to exclude catalog_db_url and pg_encryption_key while retaining storage configuration and lakekeeper_endpoint. Ensure the resulting service_config contains only the settings required by the scheduled tiering job.
🤖 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 `@server/internal/orchestrator/swarm/lakekeeper_catalog_db_resource.go`:
- Around line 111-116: Update LakekeeperCatalogDBResource.Refresh to verify the
catalog database exists in PostgreSQL rather than relying solely on the cached
Created flag. Query pg_database using the primary connection, return
resource.ErrNotFound when the database is absent, and preserve the existing
not-created error behavior before performing the query.
---
Outside diff comments:
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 1040-1047: Remove catalog credentials from the service
configuration passed in tieringArgs: update the serviceConfigCopy construction
in the tiering job setup to exclude catalog_db_url and pg_encryption_key while
retaining storage configuration and lakekeeper_endpoint. Ensure the resulting
service_config contains only the settings required by the scheduled tiering job.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b16f0c9-d629-491a-a9ae-1546719dcf5f
📒 Files selected for processing (9)
server/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/orchestrator/swarm/lakekeeper_bootstrap.goserver/internal/orchestrator/swarm/lakekeeper_catalog_db_resource.goserver/internal/orchestrator/swarm/lakekeeper_catalog_db_resource_test.goserver/internal/orchestrator/swarm/lakekeeper_managed_catalog_test.goserver/internal/orchestrator/swarm/lakekeeper_migrate_resource.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/resources.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/internal/orchestrator/swarm/resources.go
- server/internal/api/apiv1/validate.go
- server/internal/orchestrator/swarm/lakekeeper_bootstrap.go
Comment on lines
+111
to
+116
| func (r *LakekeeperCatalogDBResource) Refresh(ctx context.Context, rc *resource.Context) error { | ||
| if !r.Created { | ||
| return fmt.Errorf("%w: lakekeeper catalog database has not yet been created", resource.ErrNotFound) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Verify the catalog database exists during refresh.
Created is only cached state. If the database is dropped or lost during node replacement/restore, Refresh still succeeds, so reconciliation never reruns ensure and Lakekeeper remains broken. Query pg_database through the primary connection and return resource.ErrNotFound when absent.
🤖 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 `@server/internal/orchestrator/swarm/lakekeeper_catalog_db_resource.go` around
lines 111 - 116, Update LakekeeperCatalogDBResource.Refresh to verify the
catalog database exists in PostgreSQL rather than relying solely on the cached
Created flag. Query pg_database using the primary connection, return
resource.ErrNotFound when the database is absent, and preserve the existing
not-created error behavior before performing the query.
The lakekeeper serve provisioning never ran end-to-end; three defects found during a single-node trial: - migrate: the managed-catalog one-shot container had no network, so it could not resolve the catalog DSN host (postgres-<instanceID>, an overlay-only alias). The per-database overlay is now created attachable and the migrate container joins it in managed-catalog mode; external catalogs keep default networking. - serve: used ContainerSpec.Command=["serve"], which overrides the image entrypoint (the lakekeeper binary) and fails with exec "serve" not found. Pass "serve" as Args instead. - healthcheck: "healthcheck" is a subcommand of the distroless lakekeeper binary, not a standalone executable; invoke it by absolute path. Corrects the pre-existing lakekeeper tests, which had encoded the buggy Command/healthcheck behavior, and adds overlay-wiring coverage. Note: Attachable is applied at network CREATE only; reconciling an existing non-attachable overlay is tracked with the migrate connectivity decision.
buildS3SetStorageSecretSQL hardcoded use_ssl=true and passed the endpoint through verbatim for any endpoint-present store, which broke plain-HTTP S3 (MinIO / self-hosted) and stored a URL where DuckDB's set_storage_secret wants a bare host:port. Derive use_ssl from the endpoint scheme (https => true, http => false, schemeless => true) and strip the scheme, trimming surrounding whitespace and a trailing slash. The cloud-AWS (endpoint-absent) and Azure paths are unchanged. Finding #7 of the ColdFront single-node trial remediation.
CheckImageExists relied solely on a registry DistributionInspect, so a locally-built image never pushed to a registry (or a private registry without resolvable auth) failed spec validation. Add a local Docker daemon ImageInspectWithRaw fallback: registry first, then local cache, error only if both miss. The two lookups get independent timeout budgets so a slow-failing registry lookup cannot starve the local check of its context. Finding #10 of the ColdFront single-node trial remediation.
The archiver/partitioner/compactor DSN hardcoded user=coldfront. Thread the database's connect-as user through the tiering schedule args (as local_pg_dsn_user in the service config, mirroring lakekeeper_endpoint) so the binaries authenticate to local Postgres as the database owner. buildColdFrontConfigYAML falls back to "coldfront" when absent and logs a warning, so a misconfiguration is visible rather than silent. Finding #8 of the ColdFront single-node trial remediation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@server/internal/orchestrator/swarm/network.go`:
- Around line 40-53: Update the network provisioning flow around Create and the
Attachable field to detect an existing overlay whose live configuration is
non-attachable before scheduling Lakekeeper migration. Safely recreate the
network or return an actionable error identifying the required attachable state,
and ensure provisioning does not proceed until the migration can join the
network.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a47c041-7897-42fb-9f68-e891225291b9
📒 Files selected for processing (12)
server/internal/docker/docker.goserver/internal/docker/docker_test.goserver/internal/orchestrator/swarm/coldfront_tiering_wiring_test.goserver/internal/orchestrator/swarm/lakekeeper_migrate_resource.goserver/internal/orchestrator/swarm/lakekeeper_storage_secret_resource.goserver/internal/orchestrator/swarm/lakekeeper_storage_secret_resource_test.goserver/internal/orchestrator/swarm/network.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_spec_test.goserver/internal/workflows/activities/coldfront_tiering.goserver/internal/workflows/activities/coldfront_tiering_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- server/internal/orchestrator/swarm/lakekeeper_migrate_resource.go
- server/internal/orchestrator/swarm/service_spec.go
- server/internal/orchestrator/swarm/service_spec_test.go
- server/internal/workflows/activities/coldfront_tiering.go
- server/internal/orchestrator/swarm/orchestrator.go
Comment on lines
+40
to
+53
| // Attachable allows standalone (non-service) containers to join this | ||
| // overlay network. Required for one-shot containers (e.g. the lakekeeper | ||
| // migrate container) that must resolve the postgres-<instanceID> overlay | ||
| // alias. | ||
| // | ||
| // NOTE: this is applied at CREATE only. Docker cannot toggle Attachable on | ||
| // an existing network, and Refresh does not read the live value back, so a | ||
| // network that already exists as non-attachable will NOT be reconciled to | ||
| // attachable — the lakekeeper migrate step would still fail for a database | ||
| // whose overlay predates this field (e.g. adding a catalog_db_create | ||
| // lakekeeper service to a pre-existing database). Handling that case | ||
| // (recreate-on-mismatch or a loud error) is tracked with the #11 | ||
| // permanent-form decision (attachable overlay vs. migrate-as-swarm-service). | ||
| Attachable bool `json:"attachable"` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Handle legacy non-attachable overlays before scheduling Lakekeeper migration.
Create treats an existing network as ready, while Docker cannot retrofit Attachable. Thus adding Lakekeeper to a database with a pre-existing overlay deterministically leaves the migrate container unable to join its required network. Recreate safely or fail early with an actionable error before provisioning proceeds; the generated-resource test only covers new overlays.
🤖 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 `@server/internal/orchestrator/swarm/network.go` around lines 40 - 53, Update
the network provisioning flow around Create and the Attachable field to detect
an existing overlay whose live configuration is non-attachable before scheduling
Lakekeeper migration. Safely recreate the network or return an actionable error
identifying the required attachable state, and ensure provisioning does not
proceed until the migration can join the network.
RunColdFrontBinary invoked the archiver/partitioner/compactor from a
hardcoded /usr/local/bin. The pgedge-coldfront package installs them to
/usr/bin (RPM %{_bindir}), and CP's other externally-installed binaries
(pgbackrest, patroni) already default to /usr/bin. Extract a testable
buildTieringCommand helper + coldFrontBinDir const and target /usr/bin.
The interim hand-grafted data-node image must place the binaries in
/usr/bin to match.
Finding #15 of the ColdFront single-node trial remediation.
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)
server/internal/workflows/activities/coldfront_tiering_test.go (1)
171-180: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the production status mapping instead of duplicating it.
mapResultToTaskStatushard-codes the workflow’s expected mapping, so the test can remain green while the actual workflow regresses. Assert the status produced by the workflow/activity path, or reuse the production mapper directly.🤖 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 `@server/internal/workflows/activities/coldfront_tiering_test.go` around lines 171 - 180, Remove the test-only mapResultToTaskStatus helper and update the affected tests to obtain and assert the task status from the actual ColdFrontTiering workflow/activity path. Alternatively, call the existing production status-mapping function directly; do not duplicate the nil-error-to-status mapping in test code.
🤖 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 `@server/internal/workflows/activities/coldfront_tiering_test.go`:
- Around line 171-180: Remove the test-only mapResultToTaskStatus helper and
update the affected tests to obtain and assert the task status from the actual
ColdFrontTiering workflow/activity path. Alternatively, call the existing
production status-mapping function directly; do not duplicate the
nil-error-to-status mapping in test code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5446d3f8-0367-48ff-9b2e-17f96d922c53
📒 Files selected for processing (2)
server/internal/workflows/activities/coldfront_tiering.goserver/internal/workflows/activities/coldfront_tiering_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- server/internal/workflows/activities/coldfront_tiering.go
CP never created the coldfront extension in the application database, yet the storage-secret step called a coldfront function and only assumed the extension existed (nothing created it). Close that gap. Add LakekeeperColdfrontExtensionResource, which runs `CREATE EXTENSION IF NOT EXISTS coldfront CASCADE` against the node's primary in the app database. CASCADE pulls in the required pg_duckdb dependency. Generated unconditionally for a lakekeeper service in both managed and external catalog modes — a lakekeeper service is non-functional without the extension, so there is no opt-in flag. Repoint LakekeeperStorageSecretResource.Dependencies() at the new extension resource (which in turn depends on the database), so the graph orders database -> extension -> storage-secret. Modeled on LakekeeperCatalogDBResource: Created sentinel + DiffIgnore for idempotency, no-op Delete (dropping coldfront would CASCADE to pg_duckdb and make cold data unreadable). pg_duckdb must be present in shared_preload_libraries for CREATE EXTENSION to succeed; the node config seam that adds it is a separate change (finding #6b).
The coldfront extension's Iceberg attach path reads three per-database GUCs that nothing set: coldfront.warehouse (warehouse name), coldfront.lakekeeper_endpoint (catalog URL, which must carry the /catalog path segment), and coldfront.local_pg_dsn (libpq loopback DSN for the pglocal read path). Without them ensure_attached() no-ops and every tiered read/write fails. Fold GUC-setting into the storage-secret resource, which already holds the app-DB superuser connection and now depends on the coldfront extension. It runs ALTER DATABASE <db> SET for the three GUCs before storing the storage secret. All three are PGC_SUSET, read per statement via current_setting, so ALTER DATABASE (new-session scope) suffices with no restart. ALTER DATABASE SET is DDL and takes no bound parameters, so values are single-quoted with embedded quotes doubled (safe with standard_conforming_strings=on) and the db name is quoted as an identifier, matching the roles.go literal-quoting precedent. The orchestrator threads the /catalog endpoint (derived from the generated service name) and the connect-as user into the resource; the tiering block keeps its bare endpoint form and now shares the same port default. The local_pg_dsn keeps the sslmode=disable + local-trust assumption tracked as the finding #8 residual.
A ColdFront data node needs the coldfront / pg_duckdb stack loaded at server start, which nothing configured: pg_duckdb and coldfront must be in shared_preload_libraries, and pg_duckdb needs three duckdb.* GUCs (extension_directory pointing at the package path, allow_unsigned_extensions=true, and autoinstall_known_extensions=false — true would silently pull the unpatched upstream iceberg and 409 under concurrent cold writes). Detect the coldfront requirement once in Spec.NodeInstances() (any service of type lakekeeper), carry it as InstanceSpec.ColdFrontEnabled through Clone(), and consume it in the Patroni config generator. The append + duckdb GUCs are applied as the last step of parameters(), after the user postgresql_conf merge, so they win over both the defaults and any override while preserving the existing libraries — shared_preload is appended (with dedup, keeping reconciles stable), never replaced, so spock is retained. The generator is the single choke point feeding both the DCS and postgresql.conf parameters, and NodeInstances() is regenerated on every create/update/restore, so adding a lakekeeper service to an existing database recomputes the flag and the shared_preload change is applied by the standard update path (PatroniConfig.Update -> restartIfNeeded), which restarts the instance to load the new libraries. Resolves findings #6a and #6b (the latter subsumes #2 — the duckdb extension directory is a GUC, not a $PGDATA copy).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/internal/orchestrator/common/patroni_config_generator_coldfront_test.go (1)
79-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that all ColdFront DuckDB GUCs are absent.
This only checks
duckdb.extension_directory; a regression that leaks either remaining DuckDB setting into non-ColdFront databases would still pass.Proposed test update
- assert.NotContains(t, params, "duckdb.extension_directory") + for _, guc := range []string{ + "duckdb.extension_directory", + "duckdb.allow_unsigned_extensions", + "duckdb.autoinstall_known_extensions", + } { + assert.NotContains(t, params, guc) + }🤖 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 `@server/internal/orchestrator/common/patroni_config_generator_coldfront_test.go` around lines 79 - 82, Extend the non-ColdFront assertions in the relevant test to verify that every ColdFront-specific DuckDB GUC is absent from params, not only duckdb.extension_directory. Use the complete set of DuckDB setting keys configured by the ColdFront path, while preserving the existing shared_preload_libraries checks for pg_duckdb and coldfront.
🤖 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 `@server/internal/database/spec.go`:
- Around line 699-704: The ColdFront extension retention and preload
configuration become inconsistent when Lakekeeper is removed. In
server/internal/database/spec.go:699-704, make ColdFront enablement persistent
after provisioning or reject service removal until explicit cleanup; in
server/internal/orchestrator/swarm/lakekeeper_coldfront_extension_resource.go:105-108,
update the deletion behavior so retaining ColdFront data also retains its
preload configuration.
---
Nitpick comments:
In
`@server/internal/orchestrator/common/patroni_config_generator_coldfront_test.go`:
- Around line 79-82: Extend the non-ColdFront assertions in the relevant test to
verify that every ColdFront-specific DuckDB GUC is absent from params, not only
duckdb.extension_directory. Use the complete set of DuckDB setting keys
configured by the ColdFront path, while preserving the existing
shared_preload_libraries checks for pg_duckdb and coldfront.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58d9b023-c417-49fc-8e22-8d221aa2b184
📒 Files selected for processing (12)
server/internal/database/spec.goserver/internal/database/spec_test.goserver/internal/orchestrator/common/patroni_config_generator.goserver/internal/orchestrator/common/patroni_config_generator_coldfront_test.goserver/internal/orchestrator/swarm/lakekeeper_coldfront_extension_resource.goserver/internal/orchestrator/swarm/lakekeeper_coldfront_extension_resource_test.goserver/internal/orchestrator/swarm/lakekeeper_storage_secret_resource.goserver/internal/orchestrator/swarm/lakekeeper_storage_secret_resource_test.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/resources.goserver/internal/postgres/gucs.goserver/internal/postgres/gucs_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- server/internal/orchestrator/swarm/resources.go
- server/internal/orchestrator/swarm/lakekeeper_storage_secret_resource.go
- server/internal/orchestrator/swarm/orchestrator.go
The coldfront extension was created with `CREATE EXTENSION coldfront CASCADE`, relying on coldfront.control declaring `requires = pg_duckdb` so CASCADE pulls it in. That holds on the coldfront `main` branch but the native-packages build (PR #44 — the future packaged image) dropped the `requires` line, so CASCADE no longer installs pg_duckdb there and CREATE EXTENSION coldfront would fail. The single-node trial masked this because its image was built from `main`. Create `pg_duckdb` explicitly (IF NOT EXISTS) before coldfront. Robust regardless of the control file; idempotent on reconcile. CASCADE is kept on the coldfront statement as belt-and-braces for any future dependency. Build follow-up (flagged in the Phase-0 image plan): restoring `requires = pg_duckdb` in coldfront.control for native-packages is the cleaner semantic home; this fix stays regardless.
Reword the comments on the coldfront extension resource: creating pg_duckdb explicitly before coldfront is a deliberate independent-of-packaging choice, not a workaround for a dropped control-file dependency. Confirmed with Build that coldfront.control is unchanged by the native-packages PR and keeps `requires = pg_duckdb` after merge; the explicit create stays as belt-and-braces. Comment-only, no behavior change.
The post-deploy REST bootstrap POSTed to the swarm service name (http://<service-name>:8181), which resolves only via overlay DNS from inside the overlay — the host-networked CP process can't resolve it, so bootstrap failed (the trial worked around it with a single-node loopback hack, reverted). Reach the serve container at its IP on the Docker default bridge network instead — the same scheme GetInstanceConnectionInfo already uses to reach Patroni and Postgres. bootstrap() now finds the serve container by its service instance ID, inspects it, and builds the URL from the bridge IP. Extract a shared bridgeIPAddress() helper (with nil/empty-IP guards) and use it in both the new bootstrap path and GetInstanceConnectionInfo, removing the duplicated inline extraction. This retires the loopback hack and needs no host-port publishing or overlay membership. The pattern is host-local (CP reaches containers on its own host), matching Patroni — so it carries to multi-host without a new limitation.
The manifest pinned lakekeeper quay.io/lakekeeper/catalog:v0.9.0 — set in the foundational coldfront commit (f94b21d, Dave) and never refreshed. By that date v0.13.1 was already the latest, and there's no sign coldfront ever validated 0.9.0 (its compose floats on :latest, the native package defaults to 0.13.1). So the pin was stale, not a considered choice. Move to v0.13.1 — the current latest, the native-package default, and what coldfront's :latest resolves to today. Update the manifest block and the test fixtures/assertion that encode the version (service_images_test, the makeLakekeeperSpec fixtures, a service_spec_test image tag) plus the version-specific comment on catalogDBExtensions. Changelog review (v0.9.0 -> v0.13.1): no breaking changes on the surface CP uses — the /management/v1/{bootstrap,warehouse} endpoints, the /catalog/v1/{warehouse}/namespaces path, the migrate/serve/healthcheck subcommands, and the LAKEKEEPER__PG_* env vars are unchanged. Two watch-items for the end-to-end re-validation, neither client-breaking: v0.13.0 changed the default storage layout for new namespaces (// -> /), and switched migration locking to the built-in pg_advisory_xact_lock (so the catalog extension set is expected unchanged — re-confirm on the run). Enables the #11 follow-on: set LAKEKEEPER__DEBUG__MIGRATE_BEFORE_SERVE on serve, delete the migrate container, and drop the attachable overlay.
Delete the standalone lakekeeper migrate one-shot container and let the serve container migrate the Iceberg catalog schema in-process on startup via LAKEKEEPER__DEBUG__MIGRATE_BEFORE_SERVE=true (finding #11 permanent form). This removes the only thing that needed the per-database overlay to be attachable, so that flag goes too. What changes: - service_spec.go: set LAKEKEEPER__DEBUG__MIGRATE_BEFORE_SERVE=true on the serve container. Give serve a longer health-check StartPeriod (2m vs the shared 30s) so the first-start in-process migration can't be marked unhealthy and restarted mid-migration; serve still goes healthy the instant its first check passes, and WaitForService's 5-minute budget still bounds a genuinely stuck serve. - Delete LakekeeperMigrateResource (file), its registration in resources.go, and its instantiation/wiring in orchestrator.go. - service_instance_spec.go: repoint the serve dependency. It previously depended on the migrate resource; now, in managed-catalog mode (catalog_db_create), it depends on LakekeeperCatalogDBResource so serve only starts once control-plane has created the catalog database. In external-catalog mode there is no such resource — the URL is validated at spec time — so no dependency is added (a dependency on a resource absent from the graph would be unsatisfiable). This mirrors the migrate resource's old conditional dependency. - network.go / orchestrator.go: drop the Attachable field and its NetworkCreate plumbing, and remove Attachable from the four <db>-database overlay literals. The field was this branch's own #11 stopgap and is not on main; ensureNetworks still validates user-supplied extra networks via the Docker inspect Attachable field, which is unrelated and untouched. Why serve-not-a-separate-migrate is safe: WaitForService returns on task state Running, and with a health check configured swarm gates Running on the check passing (the earlier #13 fix proved this — a broken health check made WaitForService time out). The check exercises serve's HTTP endpoint, so WaitForService returns only once serve is listening, i.e. after in-process migration. The bootstrap resource depends on the serve ServiceInstanceResource, so it still runs against a listening serve. Tests: assert MIGRATE_BEFORE_SERVE env and the longer StartPeriod on the serve spec; assert the serve->catalog-DB dependency in managed mode and its absence in external mode; drop the now-obsolete MigrateAttachesOverlay test. Ruled-out alternatives (bridge-IP migrate, apply-SQL-ourselves, temp hba hole, migrate-as-swarm-job) are recorded in the remediation report. make test green, gofmt + go vet clean. Multi-master caveat: with N serve replicas each would attempt the migration; safe single-node (idempotent/transactional), revisit a dedicated migrate job for multi-master (tracked in the readiness ledger).
Rename the customer-facing service type for ColdFront tiered storage
from "lakekeeper" to "coldfront". In CP's model a "service" is a sidecar
container, and the only separate container ColdFront needs is the
Lakekeeper Iceberg catalog engine — so the service was named after the
engine. But CP always auto-installs the ColdFront extension when this
service is present (there is no bare-Lakekeeper mode), so from a
customer's view the capability is ColdFront; "lakekeeper" leaked an
implementation detail. The feature is unreleased, so the enum can change
now with no released contract to break.
Only the customer-facing surface changes to "coldfront":
- the Goa service_type enum (+ regenerated gen/http OpenAPI/validation)
- every dispatch/validation site: validate.go (allow-list, config
dispatch, single-node guard), spec.go NodeInstances, service_spec.go
(port + container build), service_instance_spec.go dependencies,
orchestrator.go resource-generation dispatch, plan_update.go
- the manifest service-type -> image map key (now {"coldfront" ->
the Lakekeeper image block})
Internal names stay "Lakekeeper" — they accurately name the engine and
are invisible to customers: the Go resource types
(LakekeeperConfigResource, LakekeeperBootstrapResource,
LakekeeperCatalogDBResource, LakekeeperStorageSecretResource,
LakekeeperColdfrontExtensionResource), the swarm.lakekeeper_* resource
types, generateLakekeeperInstanceResources, and the manifest's
json:"lakekeeper" image block / mf.Images.Lakekeeper field.
Consumers that emit this service type (saas, product-ui — both draft,
unreleased) must switch to "coldfront" in lockstep; tracked separately.
make test green (1398/1 skip), gofmt + go vet + golangci-lint clean.
The two single-node guard comments justified the guard by the now-resolved snowflake.node=hashtext(spock_node_name)&1023 mesh coupling (dropped upstream, ColdFront #41), which invites lifting the guard the moment that lands. Restate the live reasons: the ColdFront resource set is keyed to a single node, so multi-node would ship divergent per-node catalogs and duplicate archiving / Iceberg write races. Comment-only; guards and the user-facing message string are unchanged.
The user-facing multi-node rejection message blamed the resolved snowflake.node mesh coupling (mesh alignment pending). Restate the live reason: shared catalog and cross-node tiering coordination pending. Both verbatim-duplicated strings (validate.go const + orchestrator.go guard) updated in lockstep; the validate_test assertion follows.
The archiver and partitioner log.Fatalf with exit 1 and the message "no tables in coldfront.partition_config" when the registry is empty — a normal state for a freshly-deployed ColdFront database. The benign-empty classifier matched the stale substring "no tables configured", which the binaries never emit, so every scheduled archiver run (hourly) on a database without tiering tables was marked FAILED. Match the real upstream substring, and extend the benign classification to the partitioner (same message, same benign meaning: a tiered-only database has no partition-only tables, and vice-versa). The compactor stays non-benign — it takes an explicit --table and never emits this message. Rename the helper to isBenignEmptyPartitionConfig to reflect the widened scope.
buildColdFrontConfigYAML emitted the s3 block with access_key_id / secret_access_key / bucket, but the ColdFront tiering binaries (archiver, partitioner, compactor) parse the s3 block as access_key / secret_key / region / endpoint (coldfront internal/config.S3Config and cmd/compactor/config). yaml.v3 silently drops the unknown keys, so cfg.S3.AccessKey/SecretKey came back empty and the archiver treated a static-credential deployment as having no credentials, fatally aborting at attachIceberg before any S3 write. Emit access_key / secret_key / region (+ conditional endpoint) and drop the never-parsed bucket and path_prefix. The credential input field names are unchanged (AWS-standard access_key_id / secret_access_key in the store credential JSON); only the rendered YAML keys change.
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
2 participants
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.
Overview
The control-plane side of ColdFront (transparent Postgres→Iceberg data tiering), consuming the
lakekeeperservice the saas control plane sends. This is the single-node scope and one of several per-repo PRs for the feature; it is not end-to-end usable alone (see Dependencies & deferred).What's included
lakekeeperservice type — image (quay.io/lakekeeper/catalog), launch recipe (serve, port 8181), config resource, validator, Goa enum; follows the MCP recipe indocs/development/supported-services.md.migrate→serveis enforced as a resource dependency; missing catalog config fails loudly.bootstrap→warehouse→namespace) with the correct S3 storage-profile (flavor/path-style-access/key-prefix, verified against the ColdFront docs), andcoldfront.set_storage_secret/_azureon the database. The object-store credential is bound as query arguments (never interpolated into SQL, never logged); signatures matchcoldfront--1.0.sql.task.TypeTiering, following the pgBackRest schedule precedent). The tables to tier are resolved by the binaries from the DB registry (coldfront.partition_config, customer-driven), so no table list is passed. The archiver's "no tables configured" exit is treated as benign.Ordering & safety
migrate → serve (health-gated) → REST bootstrap (blocking, after healthy serve) →
set_storage_secret(after thecoldfrontextension exists) → scheduled jobs. All enforced by real resource dependencies. Credentials live in etcd resource state / job args exactly as existing services (RAG keys,ServiceSpec.Config) do — no new plaintext-at-rest or plaintext-in-logs exposure. Everything is runtime-gated by the (unpublished) ColdFront Postgres image, so no unsafe partial state is reachable today.Dependencies & deferred (follow-ups — not in this PR)
warehouse/path_prefix/credentialin the lakekeeperServiceSpec.Config. For this to function it must also supplycatalog_db_url,pg_encryption_key, and the store coordinatesprovider/bucket/region/endpoint(all resolvable from thecoldfront_storerecord). The control plane fails loudly where these are absent.snowflake.node) reconciliation: ColdFront's bakery requiressnowflake.node = hashtext(spock_node_name)&1023, which conflicts with the control plane's ordinal-basedsnowflake.node(Spock/lolor). Solvable in principle (CP'ssnowflake.nodevalue is consumed only by the snowflake/lolor extensions), but the clean fix needs a CP + ColdFront-author decision (likely a small ColdFront upstream change) plus a node-name hash-collision check. Hence single-node-first here.v0.9.0(currently a plausible placeholder).coldfront/localhost DSN used by the tiering binaries is an implicit contract with the image; and a ColdFront upstream benign-exit-code would let us stop keying the archiver's empty-run detection on log text.Testing & review
Built task-by-task with TDD; unit-tested throughout (exit-code capture, REST bootstrap ordering/idempotency, per-provider
set_storage_secret, fail-loud config, multi-node rejection). Contract details (SQL signatures, S3 warehouse profile) were verified against the pgEdge/coldfront source.go build ./...clean; the Goa regen is minimal (canonicalised via the pinned goa v3.23.4 + yamlfmt v0.21.0 under go1.25.8). Real end-to-end awaits the ColdFront image.Managed catalog DB (update 2026-07-17)
Since the initial single-node scope above, the control plane can now provision the Lakekeeper catalog database itself. This is opt-in and updates the "control plane does not provision it" note above — external-catalog behavior is unchanged when the new flag is absent.
catalog_db_create: truein the lakekeeperServiceSpec.Config. When set, the control plane derives the catalog DB name (<database_name>_lakekeeper, capped to Postgres's 63-byte identifier limit on a whole-rune boundary), idempotently creates it on the node's primary, transfers ownership to the service's connect-as user, and pre-creates the four extensions Lakekeeper's migrations need (uuid-ossp,pgcrypto,pg_trgm,btree_gin).postgres-{instanceID}:5432) plus the connect-as credentials,sslmode=prefer(matching how MCP/RAG connect over the overlay). No consumer can construct a reachable catalog URL at spec-build time, so the URL is never accepted from the caller in managed mode. It is threaded into every downstream consumer (migrate, serve-container env, bootstrap, storage-secret) via a cloned config map — the caller's spec is never mutated in place.CREATE DATABASE+ALTER OWNER+CREATE EXTENSIONonly). It is its own resource type (swarm.lakekeeper_catalog_db), never enrolled as a Spock/cluster node and never registered in the managed-database model — verified across review.Deleteis a deliberate no-op: the catalog maps every Iceberg table's cold data, so dropping it would make that cold data unreadable. The database rides the node's lifecycle and is covered automatically by the node's pgBackRest physical (cluster-level) backups.catalog_db_createabsent/false the caller suppliescatalog_db_urlexactly as before; the validator now requirescatalog_db_urlunlesscatalog_db_createis set, matching the orchestrator's plan-time gate.This lands as 6 commits (
175dff2..8aface0): name/URL helpers, theLakekeeperCatalogDBResource, orchestrator wiring, conditional validation, review follow-ups, and consumer-neutral comment cleanup. Every task got spec-compliance + code-quality review (the resource and orchestrator wiring on the more thorough tier), plus a final whole-branch review.go build ./...,go vet, gofmt clean; fullmake testgreen (1361 pass / 1 pre-existing integration skip).Ordering note: this reduces the saas dependency above — saas no longer needs to send
catalog_db_url(it sendscatalog_db_create: trueinstead). The saas contract expansion for the store coordinates (provider/bucket/region/endpoint) andpg_encryption_keyis still required and lands in the companion saas PR; that PR should merge first or together.Manifest follow-ups (from the main-rebase, not this feature)
version-manifest.jsoncarries the lakekeeper block, but the remoteDefaultManifestURLmanifest should get the same block or prod resolves lakekeeper only via the embedded fallback.latestalias is auto-registered per service; lakekeeper (a pinned third-party image) inherits one — drop it if a floating alias is undesired.