docs(deploy): rewrite the Helm runbook against chart 0.4.43 and split out connector config - #1924
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated the Helm deployment runbook and added connector provisioning documentation covering umbrella installation, Secrets, signing keys, migrations, reconciliation, troubleshooting, and supported connector examples. Constructor Studio plan files are now ignored. ChangesDeployment documentation
Local working-state ignore rule
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deploy/HELM_DEPLOY.md (2)
317-321: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPin chart version
0.4.31in the primary installation command.This PR documents chart
0.4.31, but the command installs whatever the registry currently considers latest. Add--version 0.4.31; mention omitting it only as an intentional, non-reproducible alternative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/HELM_DEPLOY.md` around lines 317 - 321, Update the primary helm upgrade command in deploy/HELM_DEPLOY.md to include --version 0.4.31, matching the documented chart release. Revise the surrounding note to clarify that omitting --version is an intentional non-reproducible alternative for installing the latest chart.
258-264: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAvoid printing datastore passwords during Secret migration.
These commands decode credentials to stdout, which can leak them through terminal scrollback or CI logs. Prefer a direct Secret-to-Secret transformation that preserves the base64 data, or explicitly warn that this workflow must only run in a non-logged terminal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/HELM_DEPLOY.md` around lines 258 - 264, Update the credential migration commands in the documented Secret migration workflow to avoid decoding datastore passwords to stdout. Use a direct Secret-to-Secret transformation that preserves the existing base64-encoded data; if that is not feasible, explicitly warn that the commands must only run in a non-logged terminal.
🤖 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 `@deploy/CONNECTORS.md`:
- Around line 73-75: Update the “Example Secret for every connector” guidance in
CONNECTORS.md to explicitly warn users never to commit populated Secret
manifests or plaintext credentials to version control. Recommend using an
external secret manager or creating the Secret locally through a protected
workflow, while retaining the existing example instructions for placeholder
values.
- Around line 61-69: Update the connector application instructions to prevent
applying the entire connectors directory when manifests may contain CHANGE_ME
placeholders. Replace the bulk kubectl command with a safe workflow that applies
only configured connector files, or add a reliable preflight excluding unfilled
manifests, while preserving the one-at-a-time example.
In `@deploy/HELM_DEPLOY.md`:
- Around line 171-175: Update the Helm OIDC configuration documentation around
authenticator.oidc.clientSecret to explicitly warn users to protect the values
file with restrictive permissions and never commit or log it. If the chart
already supports a Kubernetes Secret reference, document that supported
mechanism instead of exposing the secret directly.
- Around line 270-277: Update the setup flow around the signing-key commands and
the namespace-creation step so the insight namespace exists before creating the
Secret. Make the Secret workflow consistent: either generate the referenced
secrets/insight-authenticator-signing-keys.yaml manifest and apply it with the
other secrets, or explicitly apply the imperative signing-key Secret during Step
3; ensure the documented steps no longer imply both approaches.
---
Outside diff comments:
In `@deploy/HELM_DEPLOY.md`:
- Around line 317-321: Update the primary helm upgrade command in
deploy/HELM_DEPLOY.md to include --version 0.4.31, matching the documented chart
release. Revise the surrounding note to clarify that omitting --version is an
intentional non-reproducible alternative for installing the latest chart.
- Around line 258-264: Update the credential migration commands in the
documented Secret migration workflow to avoid decoding datastore passwords to
stdout. Use a direct Secret-to-Secret transformation that preserves the existing
base64-encoded data; if that is not feasible, explicitly warn that the commands
must only run in a non-logged terminal.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 17e596c1-7156-4644-a694-2c68626c1bcc
📥 Commits
Reviewing files that changed from the base of the PR and between a3e865f and 699a213527a36abe1af3e5bc91f951c20ae9de1b.
📒 Files selected for processing (3)
.gitignoredeploy/CONNECTORS.mddeploy/HELM_DEPLOY.md
Comment on lines
+61
to
+69
| Apply all of them at once, or one at a time: | ||
|
|
||
| ```sh | ||
| kubectl -n insight apply -f connectors/ # all 25 connectors at once | ||
| # or one at a time: | ||
| kubectl -n insight apply -f connectors/jira.yaml | ||
| ``` | ||
|
|
||
| You only need to create Secrets for the tools you actually use — an unused connector file can be left unfilled and simply not applied. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not apply the entire directory when manifests may still contain placeholders.
Line 64 applies every Secret under connectors/, including files that still contain CHANGE_ME. This contradicts Line 69 and causes the reconcile loop to create invalid Airbyte resources with placeholder credentials. Restrict the command to configured files, or add a reliable preflight that excludes unfilled manifests.
Proposed fix
- kubectl -n insight apply -f connectors/ # all 25 connectors at once
+# Apply only manifests whose placeholders have been replaced:
+kubectl -n insight apply -f connectors/jira.yaml connectors/slack.yaml📝 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
| Apply all of them at once, or one at a time: | |
| ```sh | |
| kubectl -n insight apply -f connectors/ # all 25 connectors at once | |
| # or one at a time: | |
| kubectl -n insight apply -f connectors/jira.yaml | |
| ``` | |
| You only need to create Secrets for the tools you actually use — an unused connector file can be left unfilled and simply not applied. | |
| Apply all of them at once, or one at a time: | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/CONNECTORS.md` around lines 61 - 69, Update the connector application
instructions to prevent applying the entire connectors directory when manifests
may contain CHANGE_ME placeholders. Replace the bulk kubectl command with a safe
workflow that applies only configured connector files, or add a reliable
preflight excluding unfilled manifests, while preserving the one-at-a-time
example.
Comment on lines
+73
to
+75
| ## Example Secret for every connector | ||
|
|
||
| Each block below is a complete, copy-paste-ready Secret for one connector. Fill in the `CHANGE_ME` (and any other placeholder) values, save it under `connectors/<name>.yaml`, and apply it as shown above. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Warn users not to persist populated Secret manifests in version control.
Line 75 instructs users to save real credentials under connectors/<name>.yaml, while every example uses plaintext stringData. Add explicit guidance not to commit populated files and recommend an external secret manager or protected local Secret creation workflow.
Proposed documentation addition
Each block below is a complete, copy-paste-ready Secret for one connector. Fill in the `CHANGE_ME` (and any other placeholder) values, save it under `connectors/<name>.yaml`, and apply it as shown above.
+
+Do not commit populated connector manifests: `stringData` contains plaintext credentials.
+Prefer an external secret manager or a protected local workflow, and keep populated files
+out of version control.📝 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
| ## Example Secret for every connector | |
| Each block below is a complete, copy-paste-ready Secret for one connector. Fill in the `CHANGE_ME` (and any other placeholder) values, save it under `connectors/<name>.yaml`, and apply it as shown above. | |
| ## Example Secret for every connector | |
| Each block below is a complete, copy-paste-ready Secret for one connector. Fill in the `CHANGE_ME` (and any other placeholder) values, save it under `connectors/<name>.yaml`, and apply it as shown above. | |
| Do not commit populated connector manifests: `stringData` contains plaintext credentials. | |
| Prefer an external secret manager or a protected local workflow, and keep populated files | |
| out of version control. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/CONNECTORS.md` around lines 73 - 75, Update the “Example Secret for
every connector” guidance in CONNECTORS.md to explicitly warn users never to
commit populated Secret manifests or plaintext credentials to version control.
Recommend using an external secret manager or creating the Secret locally
through a protected workflow, while retaining the existing example instructions
for placeholder values.
Comment on lines
+171
to
+175
| oidc: | ||
| issuerUrl: "<OIDC_ISSUER>" # MUST be set — your IdP's issuer URL | ||
| clientId: "<OIDC_CLIENT_ID>" | ||
| clientSecret: "<OIDC_CLIENT_SECRET>" | ||
| redirectUri: "https://<HOST>/auth/callback" # MUST be set — browser-facing callback through the gateway |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Document the sensitive handling required for clientSecret.
authenticator.oidc.clientSecret is entered directly into the values file, but the runbook does not warn users not to commit or log that file. Add explicit file-permission/VCS guidance, or document a supported Secret-reference mechanism if the chart provides one.
Also applies to: 227-227
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/HELM_DEPLOY.md` around lines 171 - 175, Update the Helm OIDC
configuration documentation around authenticator.oidc.clientSecret to explicitly
warn users to protect the values file with restrictive permissions and never
commit or log it. If the chart already supports a Kubernetes Secret reference,
document that supported mechanism instead of exposing the secret directly.
cyberantonz
approved these changes
Jul 27, 2026
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 (2)
deploy/HELM_DEPLOY.md (2)
3-3: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the deployment-model wording.
The introduction must distinguish “no GitOps/CI manifest synchronization” from the in-cluster
insight-reconcile-loop, which automatically processes connector Secrets.
deploy/HELM_DEPLOY.md#L3-L3: stop implying every change requireshelm upgrade; distinguish Helm/value changes from Secret changes.deploy/HELM_DEPLOY.md#L51-L51: replace “no ... auto-reconciliation” with wording that accurately describes the in-cluster reconcile loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/HELM_DEPLOY.md` at line 3, Update the introduction at deploy/HELM_DEPLOY.md:3-3 to distinguish absent GitOps/CI manifest synchronization from the in-cluster insight-reconcile-loop, stating that Helm/value changes use helm upgrade while connector Secret changes are processed automatically. Update deploy/HELM_DEPLOY.md:51-51 to remove the claim of no auto-reconciliation and accurately describe the reconcile loop’s Secret-processing behavior.
314-320: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse one secure workflow for all generated Secret material.
The runbook currently exposes datastore passwords on stdout and leaves the authenticator’s private key in unprotected local artifacts.
deploy/HELM_DEPLOY.md#L314-L320: generate the Secret without printing decoded passwords.deploy/HELM_DEPLOY.md#L330-L335: use restrictive permissions, clean upcurrent.pem, and explicitly prevent committing the generated manifest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/HELM_DEPLOY.md` around lines 314 - 320, Update deploy/HELM_DEPLOY.md lines 314-320 to retrieve generated Secret values without decoding or printing passwords to stdout, using a secure Secret-generation workflow. Update lines 330-335 to apply restrictive permissions to generated private-key artifacts, remove current.pem after use, and explicitly prevent committing the generated manifest.
🤖 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 `@deploy/HELM_DEPLOY.md`:
- Line 3: Update the introduction at deploy/HELM_DEPLOY.md:3-3 to distinguish
absent GitOps/CI manifest synchronization from the in-cluster
insight-reconcile-loop, stating that Helm/value changes use helm upgrade while
connector Secret changes are processed automatically. Update
deploy/HELM_DEPLOY.md:51-51 to remove the claim of no auto-reconciliation and
accurately describe the reconcile loop’s Secret-processing behavior.
- Around line 314-320: Update deploy/HELM_DEPLOY.md lines 314-320 to retrieve
generated Secret values without decoding or printing passwords to stdout, using
a secure Secret-generation workflow. Update lines 330-335 to apply restrictive
permissions to generated private-key artifacts, remove current.pem after use,
and explicitly prevent committing the generated manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67641ec8-7356-40f5-a442-2cbdcd336e90
📥 Commits
Reviewing files that changed from the base of the PR and between 345688d6ffc480dec4733ca7480f5c5a83726ba4 and e98208d7f7e573589cd2952d31cf6cc2dc161483.
📒 Files selected for processing (1)
deploy/HELM_DEPLOY.md
ktursunov
changed the title
added 14 commits
July 27, 2026 19:33
The runbook described a pre-split architecture that no longer exists. Conform it to chart 0.4.31: - Overview: 5 first-party services (gateway, authenticator, analytics, identity, frontend) + request/JWT diagram; authenticator is a separate pod. - Prerequisites: add cert-manager + ClusterIssuer + ingress controller. - Step 1 values: real gateway:/authenticator:/identity:/frontend: blocks; drop phantom apiGateway/authDisabled/oidc.existingSecret and inert *.deploy infra flags; image.tag documented as optional (appVersion fallback). - Step 2: drop fictional insight-oidc; add insight-authenticator-signing-keys (ES256 current.pem). - Steps 4-5: document the clickhouse-migrate hook Job; correct pod/CronWorkflow names. - Remove the fictional non-idempotent-migration note (the gold-view migration is idempotent) and correct the gateway auth description (cached Lua cosocket to /internal/authz, not nginx auth_request). - Connectors: 25 (drop unimplemented bitbucket-server); mark hubspot as a baked-url_base CDK connector. - Note fakeidp/keycloak dev images are not on public GHCR. Validated live: all 5 services install and reach Running following the runbook; helm template renders with no schema errors; the migrate hook is idempotent across two consecutive helm upgrades. Also gitignore .cf-studio/.plans/ (local Constructor Studio plan working state). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…CTORS.md Deploying the app and configuring data connectors are distinct operations, so separate the docs: - New deploy/CONNECTORS.md: connector Secret anatomy, the 25-connector catalog (a copy-paste example Secret per connector, CDK/url_base markers intact), reconcile-loop behavior, and a connector-syncing Troubleshooting section. - HELM_DEPLOY.md: Step 6 reduced to a short cross-linked pointer; Overview/Prerequisites updated to send connector examples to CONNECTORS.md; connector-syncing troubleshooting relocated there. - Fix a pre-existing wrong reconcile-log command: reconcile runs as the insight-reconcile-loop Argo CronWorkflow, not the analytics Deployment — point at its Workflow pods. - Mark github-copilot github_start_date as optional (spec.json requires only github_token + github_org). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…steps The runbook had operators inventing a tenant ID inline in Step 1, described it as a "UUID/slug", and carried an architecture diagram plus step prose that read as explanation rather than instruction. - New Step 0: generate the tenant UUID in one command before anything else. It must be a lowercase UUID — the identity tables type insight_tenant_id as ClickHouse UUID (20260408000000_init-identity.sql), and the silver models pass the connector's string through verbatim with no toUUID cast, so a slug fails to insert and mixed case stops comparing equal. - Document why global.tenantDefaultId and ingestion.reconcile.tenantId must match: the first resolves the tenant for the app, the second is exported as INSIGHT_TENANT_ID and injected into every Airbyte source config as insight_tenant_id, so it is what lands in bronze. Warn that changing it after the first sync orphans ingested data, and point local/dev at the existing 00000000-df51-5b42-9538-d2b56b7ee953 sentinel. - Add uuidgen (or python3) to the CLI prerequisites; Step 1 skeleton and both placeholder tables now reference Step 0 and say lowercase UUID. - Rewrite Steps 1-6 as instructions: gotcha paragraphs become imperative one-liners, the Step 4 migrate-hook prose becomes three bullets, Step 5's four checks merge into one block with expectations as inline comments, and redundant code comments and duplicated <IMAGE_TAG> notes are dropped. - Remove the mermaid request-flow diagram. No commands, placeholders, or constraints were lost — the reference tables, Troubleshooting, and Appendix are unchanged. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
- Rewrite the two bullets in the runbook's imperative style and add a verify block (ingressclass / cert-manager CRD / clusterissuer) so the prerequisite is checkable instead of assumed, matching the check blocks in Steps 3 and 5. - Drop the claim that the ingress blocks are "hardcoded" to className: nginx — it is a values default (values.yaml:318 gateway, :529 frontend), which the same sentence then contradicted by telling the reader to override it. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Step 0 only generated the tenant ID, leaving operators to guess where the external service addresses come from — and <REDPANDA_BROKERS> was documented as "host:9093 form", which hides both its shape and how to find it. - Retitle Step 0 "Collect the values Step 1 needs" and split it into three subsections: generate the tenant ID, look up the external service addresses, compose the Redpanda brokers string. Every datastore is external, so the addresses are looked up, not created. - Address lookup: kubectl get svc in the infra namespace, with the <svc>.<ns>.svc.cluster.local:<port> form, the ports already fixed in the skeleton, and where AIRBYTE_API_URL comes from. - Redpanda gets its own subsection because it is the exception: one comma-separated host:port bootstrap string aimed at the internal Kafka API listener, not the host/port pair the other datastores take. Commands find the Service and print its ports; notes cover reading the port instead of assuming 9093 (the redpanda/redpanda chart's internal listener; the compose stack uses 9092), single-broker bootstrap, and the unroutable-placeholder escape hatch for clusters with no Redpanda (functional-ci's redpanda-disabled:9093), since redpanda.brokers is a required field. - Update both <REDPANDA_BROKERS> table rows to the real shape, add the new subsections to the ToC, and point the external-infrastructure prerequisite at Step 0. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…Version) The values skeleton presented <TOOLBOX_IMAGE> as a required fill-in, but the umbrella chart already ships a real default for ingestion.toolboxImage pinned to the chart appVersion, and the ClickHouse gold-view migrate Job / ingestion WorkflowTemplates inherit it when unset. Comment the field out in the skeleton and note it as optional in both placeholder tables. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…m infra <ARGO_INSTANCE_ID> was presented with only a guessed example and no way to obtain the real value. Add a Step 0 subsection that reads instanceID off the workflow-controller config map (covering both the nested config: key and the legacy top-level key), explains that an unpinned controller — the common case — takes an empty argoInstanceId, and cross-checks via the controller args. Wire it into the TOC, the values-skeleton comment, and both placeholder tables. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…defaults Step 0 had grown bullet lists under each item, and three values were documented from the local sandbox's point of view rather than a real cluster's. - Every Step 0 item is now one sentence plus one example command: tenant ID, external service addresses, Airbyte API URL, Redpanda brokers, Argo instance ID. The dropped rationale already lives in the placeholder tables and Troubleshooting. - Airbyte gets its own subsection, and apiUrl is no longer presented as unconditionally required: insight.airbyte.url falls back to http://<releaseName>-airbyte-server-svc.<.Release.Namespace>:8001 (_helpers.tpl:113-118), which interpolates the umbrella's own namespace, so the fallback only resolves when Airbyte is a release in `insight` and must be set explicitly otherwise. - Stop telling operators to provision a ClusterIssuer named local-ca. That issuer is the self-signed root deploy/gitops/bootstrap/local/selfsigned-issuer.yaml creates for the local k3s sandbox via `make bootstrap-cert-manager ENV=local`; no other cluster has it. issuerRef.name is now a <CLUSTER_ISSUER> placeholder with rows in both tables, and the prerequisite notes the cert is internal-only — trusted via the mounted CA, unrelated to the public ingress cert — so any issuer, self-signed included, is fine. - Drop the five <IMAGE_TAG> blocks and both placeholder rows: subcharts render `image.tag | default .Chart.AppVersion` and the release pipeline pins appVersions in lockstep, so a chart release already carries a coherent image set. The Appendix records why they are absent and how to override one service. - Stop assuming a single infra namespace anywhere: services are located with `kubectl get svc -A`, and the Step 2 password copy, the Step 3 Airbyte mirror, the Redpanda lookup, and the Argo lookup each scope to the namespace that dependency actually runs in. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ge placeholder The runbook treated the bundled dev IdP subcharts as the fallback for a stand with no identity provider, and still asked operators to fill in an image placeholder the chart defaults correctly. - Add a real OIDC identity provider to the cluster-level prerequisites, alongside the ingress controller and cert-manager: OIDC is mandatory in every environment, so the authenticator needs something to log in against. Where a stand has none, the suggested route is installing Keycloak as its own release like any other external dependency — real OIDC, public image, admin console for the realm and confidential client — not the bundled subcharts, which are dev-mode servers on an embedded database with known passwords and are now described that way in the Overview, the Step 1 checklist, the Appendix, and Troubleshooting. - New Step 0 subsection collects the OIDC client details from that IdP (confidential client, redirect https://<HOST>/auth/callback, issuer/clientId/clientSecret; on Keycloak the issuer is <base-url>/realms/<realm>) and verifies discovery from inside the cluster with an oidc-probe pod, since that is where the authenticator resolves it. - Generalise the issuer troubleshooting row off the bundled chart's values onto any IdP, naming the split-horizon DNS trap (public hostname outside, Service DNS inside) that breaks `iss` validation. - Remove <TOOLBOX_IMAGE> everywhere — skeleton line and both placeholder rows — for the same reason <IMAGE_TAG> went: ingestion.toolboxImage defaults to the chart's appVersion-pinned toolbox. The Step 4 migrate-hook bullet now refers to that pinned image rather than the removed placeholder. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ndix Both covered situations the runbook does not need to carry right now. - Remove the previous.pem rotation window: the Step 2 rotation command, the appendix key row, and the Step 3 verify comment's "1-2 keys" expectation, which now reads "1 key (current.pem)". The signing-keys appendix is a single sentence rather than a one-row table. - Remove the values/umbrella.orbstack.yaml appendix section and its ToC entry. It described a local OrbStack/k3s variant built on host-less ingress, disabled TLS, and the bundled dev IdP — none of which this runbook points operators at any more. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Drop the section, its seven symptom rows, the pointer to CONNECTORS.md's own troubleshooting, and the ToC entry. The constraints behind the fail-fast rows still live where they are configured — the managed-by label warning in Step 2, the issuer guidance in Step 0, the ClusterIssuer caveat in Prerequisites — so what goes away is the symptom-to-cause mapping, not the requirements themselves. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Validated every claim in HELM_DEPLOY.md against the published umbrella chart 0.4.43 (pulled from ghcr, not the working tree, which is behind main) and against the services' own code, then applied the confirmed fixes. The doc's own values skeleton, extracted mechanically and rendered against 0.4.43, produces 34 objects with exit 0 after every edit here. Wrong instructions, in order of what they cost an operator: - The host placeholder rows told operators to fill `host:8123` form, but the chart appends the separate `port:` key itself, so every composed DSN got a doubled port (`http://host:8123:8123`) — render exits 0 and it only breaks inside pods. Hosts are now documented as host-only. - Nothing said the MariaDB `insight` database and login must pre-exist; the chart's hook creates only Identity's `identity`, and Analytics' blocking migrate initContainer needs the other one, so `--wait` burned its whole timeout and failed. Added to Prerequisites with the SQL. Filed as #1945 — the chart should create it, like it does for identity. - Step 2 created a Secret in the `insight` namespace one step before Step 3 created the namespace, and named a `secrets/` file it never wrote. The signing key is now written to that file with `--dry-run=client -o yaml`, so `apply -f secrets/` creates it, the heading is true, and a re-run is idempotent. - Step 5 said the ClickHouse migrate Job "must be Complete", but Helm deletes that hook Job on success, so the check failed on every healthy install. Replaced with `helm history` and the real pass signal. - `frontend.ingress` set a `host:` and `tls:` the frontend subchart's template does not read, publishing a hostless TLS-less catch-all next to the gateway Ingress. Dropped; the gateway owns the only Ingress (render goes from 2 Ingresses to 1). - `frontend.oidc` documented the pre-BFF login model ADR-0001 retired; the authenticator is the only OIDC client. Dropped, with both Appendix rows. - `<IMAGE_TAG>` and `<TOOLBOX_IMAGE>` removed: subcharts render `image.tag | default .Chart.AppVersion`. The Appendix no longer claims those appVersions move in lockstep — 0.4.43 bundles three different build stamps. - `local-ca` was presented as an issuer to provision; it only exists in this repo's local k3s sandbox. Now a `<CLUSTER_ISSUER>` placeholder, with `issuerRef.kind` documented. - Corrected consumer attributions: Identity fetches JWKS over plain HTTP with no CA (only Analytics verifies against it), the Airbyte auth Secret is read by the reconcile loop and airbyte-sync rather than Analytics, `clickhouse-password` belongs to `clickhouse.username` rather than an admin, `mariadb-root-password` is the account literally named `root`, and the managed-by label aborts the install rather than overwriting passwords. Undocumented prerequisites and constraints now stated: a real OIDC IdP, the `argo-workflow` ServiceAccount and executor RBAC, the Argo CRDs at >= 3.5, the ClickHouse CREATE DATABASE grant, the URL-reserved-character rule on all four passwords, `clickhouse.nativePort`, the `authn-tls` Certificate that blocks both pods, `csrfOrigins`, and that `global.tenantDefaultId` is a claim-absent fallback. `offline_access` is now requested by default. Then a simplification pass over the prose the corrections had bloated, verified by a two-lens fact diff against the previous commit: prerequisites became a three-item "yours to create" list, the CLI tools collapsed from seven bullets to three, the six infra-table cells and Step 0's four subsections were re-cut, and the duplicated "your infrastructure already runs" paragraph came out. That check caught one bug of its own — a truncated `tlsDiscovery.enabled` that named no real values path — now restored to `authenticator.tlsDiscovery.enabled`. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ll routing The gateway.ingress block gave no hint where traffic goes after the edge, so the skeleton read as if a route table were missing. It is not: insight-gateway's own values template every upstream through `tpl` off the release name — frontUrl to `<release>-frontend`, /api/analytics to `<release>-analytics:8081`, /api/identity to `<release>-identity:8082` — verified by rendering under two different release names. Nothing for the operator to set, so the comment just says so. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Rebasing onto main brought in #1935, which retires the manual Airbyte auth Secret mirror and reads the Secret from Airbyte's own namespace instead. The rebase resolved HELM_DEPLOY.md in favour of this branch's rewrite, so that change had to be re-applied on top rather than inherited. - Step 3 drops the kubectl|jq mirror and says plainly not to copy `airbyte-auth-secrets`: the reconcile loop and airbyte-sync read it from Airbyte's namespace at run time, and a copy freezes credentials Airbyte regenerates on reinstall. Step 3's heading and the ToC follow. - The skeleton sets `airbyte.namespace` and leaves `apiUrl` empty: `insight.airbyte.url` now composes the URL from `releaseName` + that namespace (_helpers.tpl:113-123), so an explicit apiUrl is only for a non-standard URL. - Step 0's Airbyte subsection asks for the namespace rather than the URL, and both placeholder tables swap `<AIRBYTE_API_URL>` for `<AIRBYTE_NAMESPACE>`. - Prerequisites now require permission to create Roles/RoleBindings, including in Airbyte's namespace, which must exist before the install — the chart renders `insight-airbyte-auth-reader` there. - `jq` leaves the CLI tool list; nothing else in the runbook uses it. Verified against published 0.4.47: the doc's own skeleton renders 36 objects, exit 0, one Ingress, no doubled ports, the computed URL resolves to airbyte-airbyte-server-svc in the namespace given, and the Role lands in that namespace scoped to `get` on `airbyte-auth-secrets` alone. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ktursunov
force-pushed
the
docs/helm-deploy-connectors
branch
from
July 27, 2026 11:35
88e741c to
1132b62
Compare
ktursunov
enabled auto-merge
July 27, 2026 11:40
mitasovr
approved these changes
Jul 27, 2026
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
3 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.
What
deploy/HELM_DEPLOY.mddescribed a pre-split architecture that no longer exists, documented several values from the local sandbox's point of view rather than a real cluster's, and read as explanation rather than as a runbook. Following it literally could not produce a working install. Documentation only — no chart or code changes. Four themes across thirteen commits:0.4.31(5 DevOps findings from a0.4.18install), then re-validated end to end against the published0.4.43chart.deploy/CONNECTORS.md. Deploying the app and wiring connectors are distinct operations; Step 6 is a short cross-linked pointer.Final shape: Overview → Prerequisites → Steps 0–6 → Appendix, ~3.9k words.
Wrong instructions that are now fixed
Ranked by what each cost an operator. Every one was verified against published
0.4.43and the service code, not inferred.<CLICKHOUSE_HOST>etc. "inhost:8123form"port:key itself → every DSN got a doubled port (http://host:8123:8123).helm templateexits 0; it only breaks inside pods. Hosts are now documented host-onlyinsightdatabaseidentity, and Analytics' blockingmigrateinitContainer needs the other one →--waitburns its whole timeout and fails. Now in Prerequisites with the SQL, and filed as #1945 — the chart should create itkubectl -n insight create secret …secrets/file it never wrote. The key is now written to that file with--dry-run=client -o yaml, soapply -f secrets/creates it and a re-run is idempotentComplete"hook-delete-policy: hook-succeeded), so the check failed on every healthy install. Replaced withhelm historyand the real pass signalfrontend.ingresswithhost:andtls:frontend.oidcblockClusterIssuernamedlocal-ca"local-caexists only in this repo's local k3s sandbox (bootstrap/local/selfsigned-issuer.yaml). Now a<CLUSTER_ISSUER>placeholder, withissuerRef.kinddocumented<IMAGE_TAG>/<TOOLBOX_IMAGE>placeholdersimage.tag | default .Chart.AppVersion. Both removed; the Appendix no longer claims those appVersions move "in lockstep" —0.4.43bundles three different build stampsairbyte-syncclickhouse-password= "ClickHouse admin password"clickhouse.username's password, and that account must pre-exist with CREATE DATABASEmariadb-root-password= "MariaDB root password"-uroot, so it must be the account literally namedroot; a wrong one surfaces as a reachability timeout, not an auth errorinvalid ownership metadata; generation is only reachable when the Secret is absentclassName: nginx"hardcoded"kubectl get svc -A), including the Step 2 password copy and the Step 3 Airbyte mirrorPrerequisites and constraints that were missing entirely
A real OIDC IdP (with Keycloak-as-its-own-release as the suggested route where a stand has none) · the
argo-workflowServiceAccount and executor RBAC · the Argo CRDs at>= 3.5for the pluralschedules:field · the ClickHouse CREATE DATABASE grant · the URL-reserved-character rule (@ : / ? # %) on all four datastore passwords ·clickhouse.nativePort: 9000, which Identity dials instead of the HTTP port · theinsight-authenticator-authn-tlsCertificate both pods block on, which--waitdoes not wait for ·csrfOriginsbeing fail-closed ·global.tenantDefaultIdbeing a claim-absent fallback rather than the tenant itself · a sixth bundled first-party subchart (identityResolution) ·offline_access, now requested by default.Step 0 — collect the values Step 1 needs
global.tenantDefaultIdandingestion.reconcile.tenantId, never changed after the first synckubectl get svc -A— any namespace, or off-clusterhost:portbootstrap at the internal Kafka listenerhttps://<HOST>/auth/callback, verified in-cluster with anoidc-probepodinstanceIDwhen pinned; empty otherwiseWhy the tenant ID must be a lowercase UUID: the identity tables type the column
UUIDand the Silver models pass the connector's string through verbatim with notoUUIDcast, so a slug fails to insert and mixed case stops comparing equal. The old text called it a "UUID/slug".Deliberately removed
The Mermaid request-flow diagram · the
previous.pemkey-rotation window · thevalues/umbrella.orbstack.yamlappendix · the Troubleshooting section. The requirements behind the Troubleshooting rows still live where they are configured — the managed-by warning in Step 2, issuer guidance in Step 0, theClusterIssuercaveat in Prerequisites — so what went is the symptom-to-cause mapping, not the constraints.Validation
Running, and the ClickHouse gold-view migration is idempotent across two consecutivehelm upgrades — which is how the removed "non-idempotent migration" note was shown to be a misdiagnosis.0.4.43: the chart was pulled from ghcr (not the working tree, which is behind main) and every claim in the doc checked against it and against the services' source. The doc's own values skeleton, extracted mechanically and rendered, produces 34 objects, exit 0 — re-run after each fix, including the drop from 2 Ingresses to 1 and the disappearance of the doubled ports.tlsDiscovery.enablednaming no real values path, nowauthenticator.tlsDiscovery.enabled.Known and deliberate: the chart still ships
local-caas itsissuerRef.namedefault (charts/insight/values.yaml:347) and still omits the Analytics database from its provisioning hook (#1945). This PR documents both rather than changing the chart.Summary by CodeRabbit
.gitignoreto ignore local Constructor Studio plan working files.