fix(chart): read Airbyte auth Secret cross-namespace instead of assuming app namespace - #1935
Merged
cyberantonz merged 5 commits intoJul 27, 2026
Conversation
Contributor
|
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between b4026bf38c0e5e6c9772e1000c5f7f0451479b3d and ab83e1b. 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughInsight now supports Airbyte deployments in a separate namespace by resolving the Airbyte namespace, granting scoped Secret access, retrieving OAuth credentials through the Kubernetes API, and updating deployment and architecture documentation. ChangesAirbyte namespace access
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReconcileWorkflow
participant KubernetesAPI
participant AirbyteAuthSecret
participant AirbyteAPI
ReconcileWorkflow->>KubernetesAPI: Read auth Secret from configured namespace
KubernetesAPI->>AirbyteAuthSecret: Return encoded OAuth credentials
ReconcileWorkflow->>AirbyteAPI: Request token with decoded credentials
AirbyteAPI-->>ReconcileWorkflow: Return access token
Possibly related issues
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 (1)
charts/insight/templates/ingestion/airbyte-sync.yaml (1)
272-295: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail fast for permanent credential-read errors.
The broad catch around
oauth_token()treats RBAC denials, missing Secrets/data keys, and decode failures as transient. The poller then loops for up to 48 hours instead of failing the Workflow and allowing its retry policy to surface the configuration error.🤖 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 `@charts/insight/templates/ingestion/airbyte-sync.yaml` around lines 272 - 295, Update the error handling around oauth_token() so permanent credential-read failures from airbyte_creds(), including Kubernetes RBAC denials, missing Secrets or data keys, and base64 decode errors, are propagated immediately rather than treated as transient poller errors. Preserve retries only for genuinely transient token-fetch failures, allowing the Workflow to fail and its retry policy to handle the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/insight/templates/ingestion/airbyte-auth-rbac.yaml`:
- Around line 23-35: Update the $secretNames declaration in the
airbyte-auth-reader Role to include only .Values.airbyte.authSecret.name,
removing the jwtSecret reference and any unnecessary deduplication. Preserve the
existing resourceNames rendering and Role permissions for the OAuth auth Secret.
In `@charts/insight/templates/ingestion/reconcile-cron.yaml`:
- Around line 74-77: Honor configurable OAuth Secret data keys across all
reconciliation paths: in charts/insight/templates/ingestion/reconcile-cron.yaml
lines 74-77 and charts/insight/templates/ingestion/airbyte-sync.yaml lines
91-94, render AIRBYTE_CLIENT_ID_KEY and AIRBYTE_CLIENT_SECRET_KEY from
airbyte.authSecret; in src/ingestion/reconcile-connectors/lib/airbyte.sh lines
71-75, have ab_get_token use these variables when indexing .data while
defaulting to the existing Airbyte key names.
In `@deploy/HELM_DEPLOY.md`:
- Line 255: The documentation must qualify the cross-namespace RBAC behavior: at
deploy/HELM_DEPLOY.md lines 255-255, state that the Role/RoleBinding is
installed only when airbyte.namespace differs from the application namespace,
while same-namespace installs use normal reconcile RBAC; at lines 755-755, make
the *-airbyte-auth-reader troubleshooting check conditional on split-namespace
deployments.
In `@docs/components/deployment/specs/PRD.md`:
- Line 516: The deployment documentation must consistently describe Airbyte
namespace configurability: in docs/components/deployment/specs/PRD.md lines
516-516, present insight-infra as the default/example rather than a universal
requirement; in docs/domain/ingestion/specs/DESIGN.md lines 692-692, update the
deployment model to explicitly support same-namespace deployment and split
namespaces through the airbyte.namespace override, while preserving the existing
API URL and cross-namespace Secret access details.
---
Outside diff comments:
In `@charts/insight/templates/ingestion/airbyte-sync.yaml`:
- Around line 272-295: Update the error handling around oauth_token() so
permanent credential-read failures from airbyte_creds(), including Kubernetes
RBAC denials, missing Secrets or data keys, and base64 decode errors, are
propagated immediately rather than treated as transient poller errors. Preserve
retries only for genuinely transient token-fetch failures, allowing the Workflow
to fail and its retry policy to handle the error.
🪄 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: 53bd4d43-1299-4dc3-a9e6-f73e889d3bb6
📥 Commits
Reviewing files that changed from the base of the PR and between 8e5bb89 and e7d9c37d97d340e51bc4599d64980f7f74eef048.
📒 Files selected for processing (11)
charts/insight/templates/_helpers.tplcharts/insight/templates/ingestion/airbyte-auth-rbac.yamlcharts/insight/templates/ingestion/airbyte-sync.yamlcharts/insight/templates/ingestion/reconcile-cron.yamlcharts/insight/values.yamldeploy/HELM_DEPLOY.mddocs/components/airbyte-toolkit/specs/ADR/0013-oauth-everywhere.mddocs/components/deployment/specs/PRD.mddocs/domain/ingestion/specs/DESIGN.mdsrc/ingestion/README.mdsrc/ingestion/reconcile-connectors/lib/airbyte.sh
ktursunov
approved these changes
Jul 27, 2026
ktursunov
approved these changes
Jul 27, 2026
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 `@charts/insight/templates/ingestion/airbyte-auth-rbac.yaml`:
- Around line 18-21: Update the RoleBinding in airbyte-auth-rbac.yaml so the
Airbyte auth Secret is accessible only to the reconcile service account, or to a
dedicated workflow service account with explicit trusted-submission controls;
remove the shared argo-workflow service account binding while preserving
get-only access to airbyte.authSecret.
🪄 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: 15b97e53-ee56-4135-9851-b2f431d95adb
📥 Commits
Reviewing files that changed from the base of the PR and between e7d9c37d97d340e51bc4599d64980f7f74eef048 and b4026bf38c0e5e6c9772e1000c5f7f0451479b3d.
📒 Files selected for processing (8)
charts/insight/templates/ingestion/airbyte-auth-rbac.yamlcharts/insight/templates/ingestion/airbyte-sync.yamlcharts/insight/templates/ingestion/reconcile-cron.yamlcharts/insight/values.yamldeploy/HELM_DEPLOY.mddocs/components/deployment/specs/PRD.mddocs/domain/ingestion/specs/DESIGN.mdsrc/ingestion/reconcile-connectors/lib/airbyte.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- charts/insight/templates/ingestion/reconcile-cron.yaml
- charts/insight/values.yaml
- charts/insight/templates/ingestion/airbyte-sync.yaml
- src/ingestion/reconcile-connectors/lib/airbyte.sh
- deploy/HELM_DEPLOY.md
- docs/components/deployment/specs/PRD.md
- docs/domain/ingestion/specs/DESIGN.md
Comment on lines
+18
to
+21
| Grant is `get` on the auth Secret only. Binding the shared argo-workflow | ||
| SA (the WorkflowTemplates' fallback for manual submissions) means any | ||
| workflow under it can fetch this one Secret — accepted trade-off, | ||
| still tighter than the mount-without-RBAC exposure it replaces. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Locate target file"
fd -a 'airbyte-auth-rbac\.yaml$' . || true
echo
echo "## Git diff stat/name status"
git diff --stat || true
git diff --name-only || true
echo
echo "## Target file content"
file="$(fd 'airbyte-auth-rbac\.yaml$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
cat -n "$file"
fi
echo
echo "## Search ServiceAccount names and relevant references"
rg -n "argo-workflow|WorkflowTemplate|airbyte\.authSecret|authSecret|serviceAccount|serviceaccount|serviceAccountName|serviceAccount" charts/insight/templates charts -g '*.yaml' || trueRepository: constructorfabric/insight
Length of output: 13597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## System argo workflows manifests"
fd -a 'argo.*\.ya?ml$|system-argo.*' . || true
rg -n "argo-workflow|ServiceAccount|RoleBinding|Role|workflow" deploy/gitops/system -g '*.yaml' || true
echo
echo "## Ingestion pipeline comments around submitters"
file="$(fd 'ingestion-pipeline\.yaml$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
wc -l "$file"
cat -n "$file"
fi
echo
echo "## WorkflowTemplate serviceAccounts references with context"
python3 - <<'PY'
from pathlib import Path
for p in sorted(Path('charts/insight/templates/ingestion').glob('*.yaml')):
text = p.read_text(errors='replace')
if 'serviceAccountName: argo-workflow' in text:
print(f'\n--- {p} ---')
for i,line in enumerate(text.splitlines(),1):
if 'serviceAccountName: argo-workflow' in line or 'kind: WorkflowTemplate' in line or 'serviceAccount' in line.lower() or 'argoInstanceId' in line:
start=max(1,i-3); end=min(len(text.splitlines()),i+3)
for j in range(start,end+1):
print(f'{j}: {text.splitlines()[j-1]}')
PYRepository: constructorfabric/insight
Length of output: 12494
Do not expose Airbyte credentials through the shared argo-workflow service account.
airbyte-auth-rbac.yaml grants get of airbyte.authSecret to the shared argo-workflow SA, and the Airbyte sync WorkflowTemplate passes that Secret directly into AIRBYTE_INSTANCE_CLIENT_ID / AIRBYTE_INSTANCE_CLIENT_SECRET. Any workflow running under that SA, including submitted/overridden jobs, can read and exfiltrate those credentials. Bind only the reconcile SA, or use a dedicated workflow service account with explicit trusted-submission controls.
🤖 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 `@charts/insight/templates/ingestion/airbyte-auth-rbac.yaml` around lines 18 -
21, Update the RoleBinding in airbyte-auth-rbac.yaml so the Airbyte auth Secret
is accessible only to the reconcile service account, or to a dedicated workflow
service account with explicit trusted-submission controls; remove the shared
argo-workflow service account binding while preserving get-only access to
airbyte.authSecret.
…ing app namespace
On a split-namespace install (app in insight, Airbyte in insight-infra)
the reconcile and airbyte-sync jobs read airbyte-auth-secrets in the app
namespace, where it never exists: the Airbyte chart creates it in its own
namespace and nothing mirrors it. Mirroring is also the wrong fix — a
copy freezes randomly-generated credentials and silently drifts when
Airbyte regenerates them on reinstall.
Read the Secret at its source instead:
- values: new airbyte.namespace ('' = app namespace); the computed
airbyte.apiUrl now follows it via the insight.airbyte.namespace helper.
- reconcile: lib/airbyte.sh honours AIRBYTE_AUTH_SECRET_NAMESPACE (falls
back to INSIGHT_NAMESPACE); reconcile-cron passes it.
- airbyte-sync: trigger-sync/poll-job drop the same-namespace-only
secretKeyRef and read the Secret via the K8s API with the pod SA,
per poll iteration — so rotation is picked up mid-sync.
- new airbyte-auth-rbac.yaml renders a Role/RoleBinding into Airbyte's
namespace (get on the named Secret only) for split installs.
- HELM_DEPLOY.md: retire the manual kubectl|jq mirror step; document
airbyte.namespace. Spec/README references updated (cfs validate:
no new errors; the 278 existing ones pre-date this change).
Closes constructorfabric#1885
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Independent review findings on the cross-namespace read: - Render the airbyte-auth-reader Role/RoleBinding in BOTH modes, not only split-namespace: the K8s API read needs an RBAC grant even in the app namespace (unlike the old secretKeyRef, which any pod could use with no RBAC), so same-namespace manual workflow submissions under the argo-workflow SA keep working. - Scope the Role to the auth Secret only; the JWT secret has no API-read consumer. - trigger-sync: retry transient token-mint failures in-script (retryPolicy OnError never re-runs script exits). - Both scripts: classify K8s API 401/403/404 as FatalError and fail fast with a pointed message — previously poll-job would log 'transient error' every 30s for up to 48h on a permanent RBAC or wrong-namespace misconfiguration. - Document the legacy pitfall (apiUrl pointed off-namespace with namespace unset keeps reading a stale mirrored copy) and the installer-permission/namespace-ordering prerequisites. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
…deRabbit) airbyte.authSecret.clientIdKey/clientSecretKey were rendered for the python sync steps but lib/airbyte.sh hardcoded the .data keys, so an override silently applied to trigger/poll and not to reconcile or resolve-connection-by-name. Pass AIRBYTE_CLIENT_ID_KEY / AIRBYTE_CLIENT_SECRET_KEY through reconcile-cron and the resolve step and consume them in ab_get_token (defaults unchanged). Also PRD/DESIGN: describe insight-infra as the gitops layout example rather than a universal requirement, and note the split-namespace Airbyte exception in the ingestion DESIGN namespace paragraph. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
force-pushed
the
fix/airbyte-auth-cross-ns
branch
from
July 27, 2026 07:40
b4026bf to
d49a322
Compare
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
cyberantonz
force-pushed
the
fix/airbyte-auth-cross-ns
branch
from
July 27, 2026 07:56
dcff3cf to
a715953
Compare
ktursunov
approved these changes
Jul 27, 2026
mitasovr
approved these changes
Jul 27, 2026
cyberantonz
enabled auto-merge (squash)
July 27, 2026 08:05
Contributor
|
@cyberantonz seems that we're fixing the same issue. Check my previous PR - #1913 |
Contributor
Author
@cyberdima but why are you fixing bugs assigned to me ?)) |
cyberdima
added a commit
that referenced
this pull request
Jul 27, 2026
My earlier conflict resolution left unresolved merge markers (broken bash) and duplicated logic that #1935 already merged to main under the same issue (#1885/#1906). Restoring these 5 files to main's content leaves no diff — PR #1913 is superseded, closing it. Signed-off-by: Dmitry Saukh <38005371+cyberdima@users.noreply.github.com>
ktursunov
pushed a commit
that referenced
this pull request
Jul 27, 2026
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>
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
4 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.
Closes #1885.
Problem
On a split-namespace install (app in
insight, Airbyte ininsight-infra) the reconcile and airbyte-sync jobs readairbyte-auth-secretsin the app namespace, where it never exists: the Airbyte chart creates it in its own namespace and nothing mirrors it. Connectors never sync, bronze stays empty.Mirroring the Secret (the workaround qaclust uses today via a hand-sealed snapshot) is the wrong fix: a copy freezes randomly-generated credentials and silently drifts the moment Airbyte regenerates them on reinstall — auth then fails with the Secret present, which is worse to debug than
NotFound.Fix — read the Secret at its source, no second copy
airbyte.namespace(""= app namespace, so same-namespace installs are untouched). The computedairbyte.apiUrlnow follows it via the newinsight.airbyte.namespacehelper.lib/airbyte.shhonoursAIRBYTE_AUTH_SECRET_NAMESPACE(falls back toINSIGHT_NAMESPACE);reconcile-cron.yamlpasses it. The read was already akubectl get secret— only the namespace was pinned.trigger-sync/poll-jobdrop the same-namespace-onlysecretKeyRefand read the Secret via the K8s API with the pod ServiceAccount token (stdlib, ~10 lines). The poll loop reads per iteration, so a rotated credential is picked up mid-sync. Transient K8s-API blips are retried in-script; permanent failures (401/403/404 — RBAC missing, wrong namespace, Secret deleted) fail fast with a pointed message instead of spinning untilactiveDeadlineSeconds.airbyte-auth-rbac.yamlrenders a Role/RoleBinding into Airbyte's namespace —geton the auth Secret only, bound to the<release>-reconcileandargo-workflowSAs. Rendered in both modes: the API read needs a grant even same-namespace, and it is still strictly narrower exposure thansecretKeyRef, which any pod in the namespace could use with no RBAC at all.HELM_DEPLOY.mdStep 3's manualkubectl | jqmirror is retired in favour ofairbyte.namespace; installer-permission and namespace-ordering prerequisites documented; spec/README references updated.Verification
helm templatein both modes: default keeps identical URLs/behavior;airbyte.namespace=insight-infrarenders the Role/RoleBinding ininsight-infra, URL + secret-read envs pointing there.py_compiled in both modes;bash -nonlib/airbyte.sh;helm lintclean; pre-commit passed.cfs validate --local-only: identical error count with and without this change (existing errors pre-date it).Deploy-side follow-up (insight-gitops, after this releases)
airbyte.namespace: insight-infra, drop theairbyte.apiUrloverride, delete theairbyte-auth-secretssealed-secret snapshot (and the stale Secret in the cluster).airbyte.namespace: insight-infra.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation