feat(gitops): prod environment — InsightCfabric stand under gitops management - #2459
feat(gitops): prod environment — InsightCfabric stand under gitops management#2459ktursunov wants to merge 26 commits into
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughThe pull request adds a production GitOps environment with inventory-driven deployment, Kubernetes infrastructure, service configurations, Keycloak broker authentication, sealed credentials, application values, and Semgrep SARIF preprocessing. ChangesProduction GitOps deployment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ProdMakefile
participant Kubernetes
participant Operators
participant Helm
participant Insight
ProdMakefile->>Kubernetes: Validate context and bootstrap cluster resources
ProdMakefile->>Operators: Install operators and wait for CRDs
ProdMakefile->>Kubernetes: Apply sealed secrets and service manifests
ProdMakefile->>Helm: Install platform charts and Insight release
Helm->>Insight: Deploy configured production services
ProdMakefile->>Kubernetes: Wait for rollouts and report status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Trivy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| encryptedData: | ||
| GITHUB_CLIENT_ID: AgA3DT5dyPz1LBaLQiuPN67kCM459Gc+fxklxLi83t3BnI45UksehMit90OKYVEM9WlAPo7jSkCJNMGA5DoO6CO0uQqDsI4gwGFvzartfsMAWoVg6elYX0/+SCzbwaCouKBg+mGrTtxfF0Y+Cf0UTdMmH1mUgq6HVp+HISQTADzKT5ceoMIVKt/FEfDSYxt/kYKrbmfcWsfKJZgWWw70hzFk6GxUu/lgvDMn8H3sI8HbekcKv+nX3/I0rftk4Xv4M9dIUijdJ3wjRGpuzbr5p4RNDTOZv0+eOyW4e56KTuBFa+aPv16lqklb0sp7qkvyWIPqR598rVfpj7eCf5P6Q+4XXbkW3fUlKIktUYwLmGelWf6rML5VA706g2tlcfineNT6vzdihta2MpFa59o+SG57kSDlM37m3D2KiRo+Lc6AwrtM2dIkWKFnBEG3DpiusEZq1a4pDve3E6dJU2CTeBLCT7jEvsEmDTH1JwqM/OLH2OoRVQg8My0kJIvKhiMPrYLJK2QaBY5rAbGF6qLLwmgYdBSmYJyJtV4iZ9WUvwJIw0tK+99GwQO1j90o21fEjW20+zC4aoXaF7rs9BV391BS+4x1HF219GR3xsXt+y+3QOBzbTgq7Go5/tFTtj5BcZrVMX/PI9y0XTN4IN6Itk2FaNkia/awxD7dDe2wDOxWjG3JdOLobCIF1BkoSgChKIujeyHcVboc8d+LFTBWfwUY99XSLw== | ||
| GITHUB_CLIENT_SECRET: AgBEHQbuNAeNfD1qKZsoEWufz7zJLq9HkyEaM0zlxUGoQNc6bVpAKSuZp63R/IGYVIyT1nELf8skxfTDpr6ZFPbFRgWXIa5eKWYOI4VbeXChTftMKPVZgsOttrkeogqSmIrwNokS1wQGoEZBeto5dZrJFDK2p30CPbSrgu9jcUNHWIoOCnoKZjwyd0Kwr3oQX1gViWJnFB8txiwYtpMWl6rwCrGv73O1KEZmCBF0oQm78nokAgVLqfyp1meWv2IJEPbCTEXZMSeNz/OaQF1sUDMRqFdh+kzWBiZqntsZMBn1rXJBd0Z2ImtWH1paUQc6T4gsCcy8s3/KhOQuEePg/Lnvj5Es4sh3GILJqS+VNdrv7u9B+2O41fCrAw48PlXrbZh5Lajv/v5dUQOO/lIVtnn3SgiwHtY4ibcjcHXAi+wGDLynrPDigS2dpYqubhGdPqNwkHGwpAsvbBCML2+2l+uJCLBq1bpVuLV8ujrSZolpF0X0lJL1btqJpj9K74MMKTNrR8gtIIkCs3IPhUblpqgOtG+CGx0pPThMuQ2EHWNHOhbT9jUkGg0LcfCwb9QpOo83RgZNufQ2tp+/rHD05qAq8CldhEaLPyvjEtKauqqbUbpcHPI6ovHRRlOPCw9hLsojlPDUt93R4yD0GxeUF8aXHxknXjeYAP7fJq7PhvTkSYR1XledShLSU7A2ir9JKx1eOFFp3C5KM7S1Sc6Dx/4a1nC+yCLyCMqyRkEJL7RnUI19dgqec0cj | ||
| INSIGHT_AUTHENTICATOR_CLIENT_SECRET: AgBlowZt8m5eQt0d9uXNMRDgHpFJdf2V+LuNARPjPbulklHXBm7utBFDEVE2VyFzkwYXWRThjeKRg3Na6oVZIptxbz1052dpTkqQTtExrN7uBl6NGJnwQDTyju5aGnTJKus0tkpgEyfItB/9cN0PqWvXzGzcMg+//pK4H1jUhCukIMS0/nkbfP+t0yRiaB0tsSLs7pAbAMuLjjThJqx8VlvvijNH03qVdKFztx0midSLCYzdgUaDxG4pWmrOHAeSs6PKkUQi0rEzNTI+kCjmTF58e5e/EVuXOKHKpDt2uGmv6C5NlXtrsD1lF8YqfOORrqZfbapRWNkWOJo0upzZc1BG7Fr+wK9KNx9zCs7+D7+Rw3OFoXlHd/4IvYJ2CwxpV++U5VMr8LbX64UowPQyqH0eWzuLFxtujq7zmi66DVIC92vkcNMG/OUQUxsTwYECaSU13bU9GxHfmSgLvFiha74hy6e4NdtLgVYLKCh0nqVGe4MFVUuWm3vqm7rvHqItNRZcmVCuOq8nQE5O3JhhikeE+eiVZfNGlB+7ZzdWCOt3o7BGRcUyAoVzmD5bdN3viHFC4cEPZsq31K6FKGB/m0C1ROlFQ5nNRK5ZLnHjLPIMgXw1OnthjqhTOVxi5s7FeLYYzkvUz/SqNmcrw51OjIZyX1egLMCzUYDDprijGj1zkC27+ajZ3lrUOAw2UrJi6uaPi+o96w47AzwvDGWFklRVAgee6PMqdB4B51qe65posaymzlpu4Ao7mppxbnpJUWpoZqNGxypSEwzSW2ts1V3t |
2608a58 to
a90c1b4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
.github/workflows/semgrep.yml-112-129 (1)
112-129: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove SARIF stripping before the job summary.
Lines 92-109 count suppressed results before Line 124 removes them. The job summary can report findings that GitHub Code Scanning does not receive. Move this step before
Summarize findings in the job summary, or label the summary as including suppressed results.🤖 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 @.github/workflows/semgrep.yml around lines 112 - 129, Move the “Strip suppressed (nosemgrep) results before upload” step so it runs before “Summarize findings in the job summary.” Keep the existing SARIF filtering behavior unchanged, ensuring summary counts are calculated from the stripped results that will be uploaded.deploy/gitops/environments/prod/airbyte-values.yaml-110-114 (1)
110-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the authentication comment.
Line 110 says authentication is disabled. Lines 41-42 enable Airbyte authentication. State that Keycloak integration is disabled instead.
Proposed fix
-# Authentication is disabled for this localhost-only evaluation deployment. +# Keycloak integration is disabled for this localhost-only evaluation deployment. keycloak:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/environments/prod/airbyte-values.yaml` around lines 110 - 114, Update the comment above the keycloak configuration to state that Keycloak integration is disabled, without claiming Airbyte authentication is disabled. Keep the existing keycloak and keycloak-setup enabled settings unchanged.deploy/gitops/environments/prod/inventory.yaml-98-99 (1)
98-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
make sealdoes not exist in this environment.The comment instructs the operator to "flip +
make sealonce created".deploy/gitops/environments/prod/Makefiledeclares nosealtarget; its.PHONYlist (lines 129-132) and its rules contain onlysecrets. The Makefile header (lines 31-33) describes the real procedure: runkubeseal --controller-name <release> --controller-namespace kube-systemand commit the manifest.Either correct this comment to name the
kubesealprocedure, or add asealtarget. Do you want me to open an issue to track adding asealtarget?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/environments/prod/inventory.yaml` around lines 98 - 99, Update the comment for insight-github-v2-main to replace the nonexistent “make seal” instruction with the documented kubeseal procedure, including the required controller name and namespace, while preserving the instruction to enable the connector and commit the sealed manifest.deploy/gitops/bootstrap/prod/envoyproxy.yaml-23-28 (1)
23-28: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBoth bootstrap manifests carry stale template headers from the insight-deployment flow. Each header declares the file a template with values substituted by
render_manifestfromstands/*.env, and forbids a barekubectl apply -f. Neither statement holds: the values are concrete and matchinventory.yaml, anddeploy/gitops/environments/prod/Makefiletargetbootstrap-envoy-gatewayapplies both files withkubectl apply -f(lines 177-178).deploy/gitops/environments/prod/README.mdalso describes these as concrete committed manifests.
deploy/gitops/bootstrap/prod/envoyproxy.yaml#L23-L28: drop therender_manifest/ENVOY_GATEWAY_LB_IPtemplate text, state that the IP mirrorsinventory.yamledge.loadBalancerIP, and keep theautonote as "delete theloadBalancerIPline for a fresh floating IP".deploy/gitops/bootstrap/prod/gateway.yaml#L9-L12: drop theINSIGHT_HOSTtemplate text and thedeploy-envoy-gateway.shinstruction, and point atmake bootstrap-envoy-gateway.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/bootstrap/prod/envoyproxy.yaml` around lines 23 - 28, The bootstrap manifest headers contain stale template and deployment instructions. In deploy/gitops/bootstrap/prod/envoyproxy.yaml lines 23-28, replace the render_manifest/ENVOY_GATEWAY_LB_IP text with a note that the IP mirrors inventory.yaml edge.loadBalancerIP, while retaining the auto guidance to delete loadBalancerIP for a fresh floating IP; in deploy/gitops/bootstrap/prod/gateway.yaml lines 9-12, remove the INSIGHT_HOST and deploy-envoy-gateway.sh text and direct users to make bootstrap-envoy-gateway.deploy/gitops/bootstrap/prod/cluster-issuers.yaml-1-22 (1)
1-22: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the header comments for the gitops flow, and note the namespace coupling.
Two problems in this header:
- The comments name
scripts/deploy-cert-manager.shand (line 59)validate-cert-manager.sh. In this environment,deploy/gitops/environments/prod/Makefiletargetbootstrap-cert-managerapplies this file directly, and no such guard script runs. The comment also names chart0.5.16and an "evaluation stand", whileinventory.yamlpinschartVersion: "0.5.127"for a production-like stand. Readers will look for a guard that does not exist here.- The stated guard is now absent, and the coupling it protected still exists. Line 36 hardcodes
namespace: cert-managerfor the CACertificate, but the Makefile exposesCERT_MANAGER_NAMESPACE ?= cert-manager. If an operator overrides that variable, Helm installs cert-manager elsewhere while theinsight-ca-key-pairSecret is still written tocert-manager. Theinsight-caClusterIssuer reads its key pair from the release namespace, so issuance fails with a missing-secret error.Add a guard in
bootstrap-cert-managerthat rejects aCERT_MANAGER_NAMESPACEother than the namespace in this file, and refresh the script/version references.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/bootstrap/prod/cluster-issuers.yaml` around lines 1 - 22, Add a namespace validation guard to the deploy/gitops/environments/prod/Makefile target bootstrap-cert-manager, rejecting any CERT_MANAGER_NAMESPACE value other than cert-manager before applying this manifest. Update this header’s deployment references to describe the GitOps Makefile flow, remove the nonexistent guard-script references and outdated chart/evaluation-stand details, and retain the explanation of the fixed namespace coupling for the Certificate and ClusterIssuer.
🧹 Nitpick comments (3)
deploy/gitops/environments/prod/Makefile (2)
323-331: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe
|| truehides a failed Airbyte setup call.The pod command ends with
|| true, so the container always exits 0 and the pod always reachesSucceeded. Thekubectl waiton line 330 therefore passes even whencurl -sfgets a non-2xx response or cannot resolve the Service. A half-initialised Airbyte instance then blocks the ingestion path, and the failure surfaces much later.Print the curl status before swallowing it, or drop
|| trueand accept the pod failure as the signal.♻️ Proposed change
curl -sf -X POST "$$B/api/v1/instance_configuration/setup" \ -H "Content-Type: application/json" \ -d "{\"email\":\"$(AIRBYTE_SETUP_EMAIL)\",\"organizationName\":\"$(AIRBYTE_SETUP_ORG)\",\"initialSetupComplete\":true,\"anonymousDataCollection\":false}" \ - || true' + || echo "WARNING: airbyte setup call failed (already initialised?)"'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/environments/prod/Makefile` around lines 323 - 331, Remove the trailing `|| true` from the `curl` command in the `airbyte-setup-oneshot` Makefile target so non-2xx responses and connection failures propagate as a failed pod, allowing the existing `kubectl wait` to detect setup failure.
396-399: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an unmatched-glob guard if empty manifest directories are possible.
basename $$f -sealedsecret.yamlcorrectly returns names such asredis-auth; the parameter expansion produces the same result. The remaining guard prevents one iteration over the literal glob when no manifest matches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/environments/prod/Makefile` around lines 396 - 399, Add an unmatched-glob guard around the sealed-secret iteration so the loop skips when sealed-secrets/*/*-sealedsecret.yaml matches no files, instead of processing the literal pattern. Preserve the existing namespace extraction, name derivation, wait, and secret retrieval behavior for valid manifests.deploy/gitops/bootstrap/prod/envoyproxy.yaml (1)
49-54: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider two Envoy replicas and a higher memory limit for the production edge.
replicas: 1makes the sole cluster edge a single point of failure. Any node drain, eviction, or proxy restart drops all ingress until the new pod is Ready.externalTrafficPolicy: Clusteralready removes the node-affinity constraint, so a second replica needs no other change. A 256Mi limit is also tight for an edge proxy that terminates TLS for every request; an OOMKill takes the whole edge down.The cluster has 3 workers at 16 vCPU / 32 GB per
inventory.yaml, so the extra replica fits.♻️ Proposed change
envoyDeployment: - replicas: 1 + replicas: 2 container: resources: requests: {cpu: 50m, memory: 64Mi} - limits: {cpu: 500m, memory: 256Mi} + limits: {cpu: 1, memory: 512Mi}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/gitops/bootstrap/prod/envoyproxy.yaml` around lines 49 - 54, Update the production envoyDeployment configuration to run two replicas instead of one and increase the container memory limit above 256Mi to provide additional headroom for TLS-terminating edge traffic; leave the existing CPU resources and other settings unchanged.
🤖 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/compose/keycloak/tests/tenant_contract_guard.py`:
- Around line 152-156: Update the mapper validation in the surrounding contract
guard so each configuration key is checked only for its corresponding supported
mapper type: use user.attribute for OIDC broker mappers and userAttribute for
github-user-attribute-mapper/related social-provider mappers. Do not accept a
mapper solely because either key contains idp_sub; require the mapper type and
matching key together.
In `@deploy/gitops/bootstrap/prod/cinder-storageclass.yaml`:
- Line 8: Change the production default Cinder StorageClass reclaimPolicy from
Delete to Retain so deleting PVCs does not immediately destroy backing volumes.
If fast reclamation is required for scratch workloads, preserve it through a
separate cinder-delete class rather than the cluster-default class.
In `@deploy/gitops/environments/prod/argo-workflows-values.yaml`:
- Around line 65-72: Update the production Argo Workflows server configuration
around authModes and secure: replace server authentication with client or
configured SSO authentication, enable secure: true, and configure the required
TLS certificate settings while preserving the existing service exposure.
In `@deploy/gitops/environments/prod/keycloak/realms/insight-broker.yaml`:
- Around line 165-171: The github-idp-sub mapper must use GitHub’s immutable
numeric identifier rather than the mutable, case-sensitive login value. Change
its jsonField from login to id, and migrate the connector’s existing idp_sub
bindings to the corresponding GitHub numeric IDs while preserving the idp_sub
attribute mapping.
In `@deploy/gitops/environments/prod/Makefile`:
- Line 367: Update the deployment recipe around the Helm authenticator
clientSecret setting to capture and validate the decoded Secret value before
deploying, failing explicitly when retrieval, decoding, or the resulting value
is empty. Write the validated secret to a temporary Helm values file and pass
that file to helm instead of including the secret in argv, ensuring the
temporary file is cleaned up after use.
- Around line 155-161: Update the confirm target to fail closed by treating only
an explicit yq result of false as unprotected; missing, unreadable, malformed,
or empty results must require CONFIRM=yes-deploy-prod. In the deploy-insight
prerequisite flow, chain keycloak-broker-realms and argo-rbac after confirm
rather than listing them as parallel prerequisites, so the confirmation gate
runs before either mutation.
- Around line 283-287: Update the Redis master-election loop around the masters
count check so a non-matching count does not trigger set -e termination. Replace
the standalone failing AND-list pattern in the polling logic with conditional
control flow that continues sleeping and retrying until one master is elected or
the existing 300-second timeout exits with an error.
- Around line 236-242: Update the readiness loop in the ClickHouse startup
recipe so a failed readiness test is handled as normal polling while `set -e` is
enabled, rather than terminating the recipe on the first iteration. Preserve the
existing success path, 900-second timeout, and sleep behavior; only restructure
the condition around `ready` and the timeout as needed to allow continued
waiting until the replicas are Ready.
- Around line 202-205: Update the retry loop around the cluster-issuers kubectl
apply so a failed command is handled as an expected conditional failure under
set -e, allowing execution to continue to the attempt limit, retry message, and
sleep. Preserve the successful break and final error behavior after 30 failed
attempts.
- Around line 183-191: Replace each standalone “[ -n "$$addr" ] && { ... }” poll
condition with an if-based conditional in the affected Makefile recipes,
including the occurrences near the gateway polling blocks. Preserve the existing
address validation, warning, timeout, diagnostic, and sleep behavior so an empty
address continues polling until the deadline.
In `@deploy/gitops/environments/prod/manifests/airbyte/rbac.yaml`:
- Around line 10-55: Narrow the RBAC resources and bindings for
airbyte-admin-role: introduce and bind a dedicated launcher/job ServiceAccount
for workload-launcher and connector pods, remove pods/exec access, and restrict
Secret permissions to only the required resources and verbs. Keep the separate
Insight auth binding independent without granting it namespace-wide Secret
access, while preserving only the permissions needed for Airbyte job execution.
In `@deploy/gitops/environments/prod/manifests/airbyte/storage-init.yaml`:
- Around line 28-43: Set readOnlyRootFilesystem: true in the database
initialization container’s securityContext at
deploy/gitops/environments/prod/manifests/airbyte/storage-init.yaml:28-43 and in
the MinIO initialization container’s securityContext at
deploy/gitops/environments/prod/manifests/airbyte/storage-init.yaml:79-94; keep
the writable /data PVC mounts unchanged.
In
`@deploy/gitops/environments/prod/manifests/argo-workflows/validation-workflow.yaml`:
- Around line 3-6: Add metadata.namespace set to insight in the Workflow
manifest’s metadata alongside generateName, ensuring it uses the insight
namespace where the argo-workflow ServiceAccount exists.
In `@deploy/gitops/environments/prod/manifests/clickhouse/clickhouse.yaml`:
- Around line 37-59: The production ClickHouse topology must be fault-tolerant
rather than single-member. In
deploy/gitops/environments/prod/manifests/clickhouse/clickhouse.yaml lines
37-59, make the Insight schema and DDL path replica-safe, then configure
multiple ClickHouse replicas with the matching CLICKHOUSE_REPLICAS setting. In
deploy/gitops/environments/prod/manifests/clickhouse/keeper.yaml lines 8-23,
replace the single Keeper member with a quorum-capable topology whose replica
configuration matches ClickHouse.
In `@deploy/gitops/environments/prod/manifests/mariadb/insight.yaml`:
- Around line 47-52: Update the MariaDB manifest to create a dedicated Keycloak
database and user, then add a scoped Grant permitting that user access to the
keycloak database. Keep the existing insight database grant unchanged and ensure
the credentials match the Keycloak connection settings in values.yaml.
- Around line 27-36: Adjust the MariaDB connection-pool configuration associated
with the shared insight user so the aggregate maximums for analytics,
identity-resolution, migrate/init Jobs, and bundled Keycloak remain below
maxUserConnections (100). Update the relevant pool-size settings in the
referenced values/configuration while preserving the existing user and limit
unless separate users are explicitly configured.
In `@deploy/gitops/environments/prod/manifests/mariadb/mariadb.yaml`:
- Around line 125-126: Update the MariaDB TLS configuration by enabling tls and
supplying the required server certificates, then update every MariaDB client and
Galera replication connection to require TLS before release. Preserve the
existing production cluster configuration while ensuring both client traffic and
replication traffic use encrypted transport.
In `@deploy/gitops/environments/prod/redpanda-values.yaml`:
- Around line 3-9: Update the production Redpanda values under tls.enabled and
auth.sasl.enabled to enable encrypted, authenticated connections, and configure
the required application credentials and ACLs before workloads connect. Keep
external exposure settings unchanged unless needed for the secured production
setup.
In `@deploy/gitops/Makefile`:
- Around line 62-67: Update the chart-version resolution logic around
ENV_CHART_PIN and INSIGHT_VERSION to distinguish an absent chartVersion from an
inventory read or yq parse failure. Make inventory resolution failures fail
closed rather than falling back to .insight-version, and apply the same behavior
in both Makefiles while retaining the fallback only when chartVersion is
genuinely absent.
---
Minor comments:
In @.github/workflows/semgrep.yml:
- Around line 112-129: Move the “Strip suppressed (nosemgrep) results before
upload” step so it runs before “Summarize findings in the job summary.” Keep the
existing SARIF filtering behavior unchanged, ensuring summary counts are
calculated from the stripped results that will be uploaded.
In `@deploy/gitops/bootstrap/prod/cluster-issuers.yaml`:
- Around line 1-22: Add a namespace validation guard to the
deploy/gitops/environments/prod/Makefile target bootstrap-cert-manager,
rejecting any CERT_MANAGER_NAMESPACE value other than cert-manager before
applying this manifest. Update this header’s deployment references to describe
the GitOps Makefile flow, remove the nonexistent guard-script references and
outdated chart/evaluation-stand details, and retain the explanation of the fixed
namespace coupling for the Certificate and ClusterIssuer.
In `@deploy/gitops/bootstrap/prod/envoyproxy.yaml`:
- Around line 23-28: The bootstrap manifest headers contain stale template and
deployment instructions. In deploy/gitops/bootstrap/prod/envoyproxy.yaml lines
23-28, replace the render_manifest/ENVOY_GATEWAY_LB_IP text with a note that the
IP mirrors inventory.yaml edge.loadBalancerIP, while retaining the auto guidance
to delete loadBalancerIP for a fresh floating IP; in
deploy/gitops/bootstrap/prod/gateway.yaml lines 9-12, remove the INSIGHT_HOST
and deploy-envoy-gateway.sh text and direct users to make
bootstrap-envoy-gateway.
In `@deploy/gitops/environments/prod/airbyte-values.yaml`:
- Around line 110-114: Update the comment above the keycloak configuration to
state that Keycloak integration is disabled, without claiming Airbyte
authentication is disabled. Keep the existing keycloak and keycloak-setup
enabled settings unchanged.
In `@deploy/gitops/environments/prod/inventory.yaml`:
- Around line 98-99: Update the comment for insight-github-v2-main to replace
the nonexistent “make seal” instruction with the documented kubeseal procedure,
including the required controller name and namespace, while preserving the
instruction to enable the connector and commit the sealed manifest.
---
Nitpick comments:
In `@deploy/gitops/bootstrap/prod/envoyproxy.yaml`:
- Around line 49-54: Update the production envoyDeployment configuration to run
two replicas instead of one and increase the container memory limit above 256Mi
to provide additional headroom for TLS-terminating edge traffic; leave the
existing CPU resources and other settings unchanged.
In `@deploy/gitops/environments/prod/Makefile`:
- Around line 323-331: Remove the trailing `|| true` from the `curl` command in
the `airbyte-setup-oneshot` Makefile target so non-2xx responses and connection
failures propagate as a failed pod, allowing the existing `kubectl wait` to
detect setup failure.
- Around line 396-399: Add an unmatched-glob guard around the sealed-secret
iteration so the loop skips when sealed-secrets/*/*-sealedsecret.yaml matches no
files, instead of processing the literal pattern. Preserve the existing
namespace extraction, name derivation, wait, and secret retrieval behavior for
valid manifests.
🪄 Autofix
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: b84877ac-ead5-4f35-9289-6cc6ef178263
📒 Files selected for processing (38)
.github/workflows/semgrep.ymldeploy/compose/keycloak/tests/tenant_contract_guard.pydeploy/gitops/Makefiledeploy/gitops/bootstrap/prod/cinder-storageclass.yamldeploy/gitops/bootstrap/prod/cluster-issuers.yamldeploy/gitops/bootstrap/prod/envoyproxy.yamldeploy/gitops/bootstrap/prod/gateway.yamldeploy/gitops/environments/prod/Makefiledeploy/gitops/environments/prod/README.mddeploy/gitops/environments/prod/airbyte-values.yamldeploy/gitops/environments/prod/argo-workflows-values.yamldeploy/gitops/environments/prod/inventory.yamldeploy/gitops/environments/prod/keycloak/realms/insight-broker.yamldeploy/gitops/environments/prod/manifests/airbyte/pvcs.yamldeploy/gitops/environments/prod/manifests/airbyte/rbac.yamldeploy/gitops/environments/prod/manifests/airbyte/storage-init.yamldeploy/gitops/environments/prod/manifests/argo-rbac.yamldeploy/gitops/environments/prod/manifests/argo-workflows/validation-workflow.yamldeploy/gitops/environments/prod/manifests/clickhouse/clickhouse.yamldeploy/gitops/environments/prod/manifests/clickhouse/keeper.yamldeploy/gitops/environments/prod/manifests/mariadb/insight.yamldeploy/gitops/environments/prod/manifests/mariadb/mariadb.yamldeploy/gitops/environments/prod/redis-operator-values.yamldeploy/gitops/environments/prod/redis-values.yamldeploy/gitops/environments/prod/redpanda-values.yamldeploy/gitops/environments/prod/sealed-secrets/clickhouse/clickhouse-default-credentials-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/clickhouse/clickhouse-insight-credentials-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/github-oauth-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/insight-authenticator-signing-keys-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/insight-db-creds-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/insight-github-directory-main-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/insight-keycloak-admin-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/insight-keycloak-config-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/insight/insight-oidc-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/mariadb/mariadb-insight-credentials-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/mariadb/mariadb-root-sealedsecret.yamldeploy/gitops/environments/prod/sealed-secrets/redis/redis-auth-sealedsecret.yamldeploy/gitops/environments/prod/values.yaml
| # OIDC broker mappers key the target as `user.attribute`; social-provider | ||
| # mappers (github-user-attribute-mapper and kin) use `userAttribute`. | ||
| if not any( | ||
| (m.get("config") or {}).get(key) == "idp_sub" for m in mine for key in ("user.attribute", "userAttribute") | ||
| ): |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C4 \
--glob '*.yaml' --glob '*.yml' \
'identityProviderMapper:|user\.attribute: idp_sub|userAttribute: idp_sub' \
deploy/gitops/environmentsRepository: constructorfabric/insight
Length of output: 4560
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- guard implementation ---'
cat -n deploy/compose/keycloak/tests/tenant_contract_guard.py | sed -n '110,180p'
printf '%s\n' '--- all IdP mapper definitions and idp_sub assignments ---'
rg -n -C3 \
--glob '*.yaml' --glob '*.yml' --glob '*.py' --glob '*.md' \
'identityProviderMapper:|user\.attribute:|userAttribute:|idp_sub' \
deploy/compose deploy/gitops
printf '%s\n' '--- mapper names in repository ---'
rg -n \
--glob '*.yaml' --glob '*.yml' --glob '*.py' --glob '*.md' \
'oidc-user-attribute-idp-mapper|github-user-attribute-mapper|hardcoded-attribute-idp-mapper' \
.Repository: constructorfabric/insight
Length of output: 18035
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import ast
source = Path("deploy/compose/keycloak/tests/tenant_contract_guard.py").read_text()
tree = ast.parse(source)
fn = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "registration_violations"
)
# Reproduce the predicate under review without importing or executing repository code.
def stamps_idp_sub(mappers, alias):
mine = [m for m in mappers if m.get("identityProviderAlias") == alias]
return any(
(m.get("config") or {}).get(key) == "idp_sub"
for m in mine
for key in ("user.attribute", "userAttribute")
)
cases = [
("supported OIDC pair", [{
"identityProviderAlias": "x",
"identityProviderMapper": "oidc-user-attribute-idp-mapper",
"config": {"user.attribute": "idp_sub"},
}], True),
("supported GitHub pair", [{
"identityProviderAlias": "x",
"identityProviderMapper": "github-user-attribute-mapper",
"config": {"userAttribute": "idp_sub"},
}], True),
("unrelated mapper with GitHub key", [{
"identityProviderAlias": "x",
"identityProviderMapper": "hardcoded-attribute-idp-mapper",
"config": {"userAttribute": "idp_sub"},
}], True),
("unrelated mapper with OIDC key", [{
"identityProviderAlias": "x",
"identityProviderMapper": "hardcoded-attribute-idp-mapper",
"config": {"user.attribute": "idp_sub"},
}], True),
]
print(f"found registration_violations: {ast.unparse(fn).splitlines()[0]}")
for name, mappers, result in cases:
print(f"{name}: predicate={stamps_idp_sub(mappers, 'x')}, expected current predicate={result}")
# Report actual committed registration mapper/key pairs.
try:
import yaml
except ImportError:
print("PyYAML unavailable; skipped YAML shape check")
else:
for path in sorted(Path("deploy/gitops/environments").glob("*/keycloak/realms/*.yaml")):
doc = yaml.safe_load(path.read_text()) or {}
ids = {i.get("alias") for i in doc.get("identityProviders") or []}
for mapper in doc.get("identityProviderMappers") or []:
if mapper.get("identityProviderAlias") in ids:
cfg = mapper.get("config") or {}
keys = [k for k in ("user.attribute", "userAttribute") if cfg.get(k) == "idp_sub"]
if keys:
print(path, mapper.get("identityProviderMapper"), keys)
PYRepository: constructorfabric/insight
Length of output: 600
🌐 Web query:
Keycloak identity provider mapper github-user-attribute-mapper userAttributeandoidc-user-attribute-idp-mapper user.attribute configuration semantics
💡 Result:
In Keycloak, identity provider (IdP) mappers and protocol mappers serve distinct functions in the authentication and token issuance process. The configuration semantics you referenced reflect these roles. Identity Provider Mappers (e.g., oidc-user-attribute-idp-mapper, github-user-attribute-mapper) are used during the federation flow to import data from an external IdP into the Keycloak user's profile [1][2]. 1. oidc-user-attribute-idp-mapper: This is a general-purpose IdP mapper used to map claims provided by an OIDC identity provider into a user attribute in the local Keycloak user database [1][3]. The configuration property "User Attribute" (or userAttribute) defines the name of the attribute in the Keycloak user model where the incoming claim value will be stored [4][1]. Once stored in the user profile, this attribute can be used by other parts of Keycloak, such as in tokens or authentication flows [5][2]. 2. github-user-attribute-mapper: This is a specialized IdP mapper designed specifically for the GitHub social provider [6]. It functions similarly to the generic OIDC attribute mapper but is tailored to handle the specific JSON structures provided by the GitHub API. It also uses the "User Attribute" configuration to specify the target field in the local Keycloak user profile [4][6]. Distinctly, Protocol Mappers (often found under the "Client Scopes" or "Client Mappers" tab) are used to map attributes from the user's profile or session into issued tokens (e.g., ID tokens, access tokens) [7][8]. While they may also use a "User Attribute" property to specify the source of the data in the user profile, their semantic purpose is token construction rather than identity importation [7][8]. Key Takeaways: - Identity Provider Mappers ("IdP mappers") import information from an external provider into the Keycloak user profile [1][2]. - Protocol Mappers map information from the Keycloak user profile (or session) into outgoing tokens issued by Keycloak to applications [7][8]. - The userAttribute configuration in an IdP mapper determines the target field in the local user profile [4][3]. - The user.attribute syntax sometimes seen in documentation or configuration interfaces typically refers to the OIDC protocol mapper, which specifies the user profile attribute to be included as a claim in the token [7]. Always verify the provider type in the Keycloak admin console; IdP mappers are managed under the Identity Provider's "Mappers" tab, while Protocol mappers are managed under Client or Client Scope "Protocol Mappers" tabs [2][8].
Citations:
- 1: https://www.keycloak.org/docs-api/latest/javadocs/org/keycloak/broker/oidc/mappers/UserAttributeMapper.html
- 2: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/identity-broker/mappers.adoc
- 3: https://www.keycloak.org/docs-api/26.2.4/javadocs/org/keycloak/broker/oidc/mappers/UserAttributeMapper.html
- 4: https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/broker/oidc/mappers/UserAttributeMapper.java
- 5: Mapping a claim from a brokered IdP keycloak/keycloak#24945
- 6: https://www.keycloak.org/docs-api/26.4.6/javadocs/org/keycloak/social/github/GitHubUserAttributeMapper.html
- 7: https://www.keycloak.org/docs-api/23.0.7/javadocs/org/keycloak/protocol/oidc/mappers/UserAttributeMapper.html
- 8: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/clients/con-protocol-mappers.adoc
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
import json
import urllib.request
urls = {
"oidc": "https://raw.githubusercontent.com/keycloak/keycloak/main/services/src/main/java/org/keycloak/broker/oidc/mappers/UserAttributeMapper.java",
"github": "https://raw.githubusercontent.com/keycloak/keycloak/main/services/src/main/java/org/keycloak/social/github/GitHubUserAttributeMapper.java",
"hardcoded": "https://raw.githubusercontent.com/keycloak/keycloak/main/services/src/main/java/org/keycloak/broker/provider/HardcodedAttributeMapper.java",
}
for name, url in urls.items():
print(f"--- {name}: {url} ---")
try:
with urllib.request.urlopen(url, timeout=10) as response:
text = response.read().decode()
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in ("USER_ATTRIBUTE", "user.attribute", "userAttribute", "getConfig", "setAttribute")):
print(f"{i}:{line}")
except Exception as exc:
print(f"fetch failed: {exc}")
PY
printf '%s\n' '--- repository documentation contract ---'
cat -n deploy/gitops/README.md | sed -n '250,290p'Repository: constructorfabric/insight
Length of output: 3139
Validate the mapper type with its configuration key.
The check accepts an unrelated mapper when its configuration contains userAttribute: idp_sub or user.attribute: idp_sub. Match each key to its supported mapper type before accepting the idp_sub contract.
🤖 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/compose/keycloak/tests/tenant_contract_guard.py` around lines 152 -
156, Update the mapper validation in the surrounding contract guard so each
configuration key is checked only for its corresponding supported mapper type:
use user.attribute for OIDC broker mappers and userAttribute for
github-user-attribute-mapper/related social-provider mappers. Do not accept a
mapper solely because either key contains idp_sub; require the mapper type and
matching key together.
Source: Learnings
| annotations: | ||
| storageclass.kubernetes.io/is-default-class: "true" | ||
| provisioner: cinder.csi.openstack.org | ||
| reclaimPolicy: Delete |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Consider reclaimPolicy: Retain for this production stand.
This StorageClass is the cluster default and backs MariaDB (100Gi), ClickHouse, Redis, and Airbyte volumes. With reclaimPolicy: Delete, deletion of a PVC destroys the Cinder volume and its data immediately. deploy/gitops/environments/prod/manifests/mariadb/mariadb.yaml already sets pvcRetentionPolicy: Retain at the operator level, which shows the intent to keep data, but that protection ends at the PVC layer.
If you want fast reclaim for scratch volumes, keep this class as is and add a second cinder-retain class for the datastores.
🛡️ Proposed change
provisioner: cinder.csi.openstack.org
-reclaimPolicy: Delete
+reclaimPolicy: Retain
allowVolumeExpansion: true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/bootstrap/prod/cinder-storageclass.yaml` at line 8, Change the
production default Cinder StorageClass reclaimPolicy from Delete to Retain so
deleting PVCs does not immediately destroy backing volumes. If fast reclamation
is required for scratch workloads, preserve it through a separate cinder-delete
class rather than the cluster-default class.
| server: | ||
| enabled: true | ||
| clusterWorkflowTemplates: | ||
| enabled: false | ||
| authModes: | ||
| - server | ||
| secure: false | ||
| serviceType: ClusterIP |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect Argo Server exposure and TLS references without executing repository code.
rg -n -C 4 'secure:\s*false|serviceType:\s*ClusterIP' \
deploy/gitops/environments/prod/argo-workflows-values.yaml
rg -n -i -C 4 \
'argo[- ]?server|argo-workflows-server|tls:|certificateRefs:' \
deploy/gitops || trueRepository: constructorfabric/insight
Length of output: 4161
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Argo-related files and references ---'
git ls-files | rg -i 'argo|workflow' | head -200
printf '%s\n' '--- Argo chart and server configuration references ---'
rg -n -i -C 5 \
'argo-workflows|server:\s*$|authModes|serviceType|secure:|workflows-server|argo-server' \
deploy charts .github 2>/dev/null | head -400
printf '%s\n' '--- Production gateway routes and backend references ---'
cat -n deploy/gitops/bootstrap/prod/gateway.yaml | sed -n '1,130p'
printf '%s\n' '--- Production Argo values ---'
cat -n deploy/gitops/environments/prod/argo-workflows-values.yaml | sed -n '1,130p'Repository: constructorfabric/insight
Length of output: 39582
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Repository metadata and chart dependency versions ---'
find . -maxdepth 5 -type f \( \
-name 'Chart.yaml' -o -name 'Chart.lock' -o -name 'values.yaml' -o \
-name 'kustomization.yaml' -o -name '*.yaml' -o -name '*.yml' \
\) -print0 |
xargs -0 rg -n -i -C 3 \
'argo-workflows|argoproj/argo-workflows|workflow-controller|argo-server|secure:' 2>/dev/null |
head -500
printf '%s\n' '--- References to the Argo service or routes ---'
rg -n -i -C 6 \
'argo-workflows-server|workflows-server|argo-server|workflow.*(httproute|ingress|service)|httproute|parentRefs|backendRefs' \
deploy/gitops 2>/dev/null | head -500Repository: constructorfabric/insight
Length of output: 15520
🌐 Web query:
Argo Workflows 1.0.13 Helm chart server secure authModes server TLS behavior
💡 Result:
In the Argo Workflows Helm chart (including version 1.0.13), the configuration for the Argo server authentication and TLS behavior is primarily managed through values under the server block [1][2][3]. 1. Authentication Modes (server.authModes): The server.authModes parameter is a list that defines which authentication mechanisms the Argo server supports [2][4]. Accepted values include server, client, or sso [1][3]. Setting this parameter adds the corresponding --auth-mode flags to the Argo server deployment [5]. If sso is included, the server.sso configuration must also be provided [1][2]. The legacy server.authMode string parameter is deprecated in favor of this list format [1][4]. 2. TLS Behavior (server.secure): The server.secure boolean setting controls whether the Argo server operates in "secure" mode [1][6]. - When server.secure is set to true, the Argo server container is passed the --secure flag, and its internal liveness and readiness probes are configured to use the HTTPS scheme [5]. - When server.secure is false (the default), the server runs in insecure mode using HTTP [5][6]. - In secure mode, if you are using an Ingress or Gateway API to expose the server, you must ensure your ingress controller or gateway is configured to communicate with the backend using HTTPS (e.g., via annotations like nginx.ingress.kubernetes.io/backend-protocol: "HTTPS" or by configuring a BackendTLSPolicy for Gateway API) [2][3][7]. In summary, server.authModes dictates how the server authenticates users, while server.secure acts as a master switch to toggle the server's internal protocol between HTTPS (secure) and HTTP [2][5][6].
Citations:
- 1: https://github.com/argoproj/argo-helm/blob/main/charts/argo-workflows/values.yaml
- 2: https://github.com/argoproj/argo-helm/tree/main/charts/argo-workflows
- 3: https://artifacthub.io/packages/helm/wener/argo-workflows/1.0.13
- 4: https://github.com/argoproj/argo-helm/blob/d1a211b8/charts/argo-workflows/README.md
- 5: https://github.com/argoproj/argo-helm/blob/master/charts/argo-workflows/templates/server/server-deployment.yaml
- 6: https://github.com/argoproj/argo-helm/blob/f40606a2/charts/argo-workflows/values.yaml
- 7: https://argo-workflows.readthedocs.io/en/stable/argo-server/
🌐 Web query:
Argo Workflows documentation auth-mode server secure TLS Argo Server
💡 Result:
To configure the Argo Server for secure communication and authentication, you must address both Transport Layer Security (TLS) and authentication modes. Transport Layer Security (TLS) By default, the Argo Server runs with TLS enabled (secure mode) [1][2][3]. - To enable or confirm secure communication, ensure the --secure flag is set (this is the default) [2][4]. - You can provide custom certificates by using the --tls-certificate-secret-name flag, which points to a Kubernetes secret containing your certificates [2]. - If you are running the server behind an Ingress controller, you must configure the Ingress to recognize that the backend uses HTTPS (e.g., using annotations like nginx.ingress.kubernetes.io/backend-protocol) [1][3]. - The minimum TLS version can be configured using the TLS_MIN_VERSION environment variable (defaulting to v1.2) [4]. Authentication Modes Authentication for the Argo Server is managed via the --auth-mode flag, which can accept one or more modes [5][2][6]. The available modes are: - client: Uses the Kubernetes bearer token of the client (default for v3.0+) [5][6]. - server: Uses the Argo Server's Service Account (default prior to v3.0) [5][6]. - sso: Enables Single Sign-On for authentication [5][6]. For a secure production setup, it is recommended to enable TLS and configure an authentication mode (such as SSO or client-based authentication) to restrict access to authorized users [7][4]. The Argo Server does not perform authentication directly, delegating it instead to the Kubernetes API server or an external OAuth provider [1][3]. Therefore, it is best practice to implement rate limiting on the delegated authentication provider [1][3].
Citations:
- 1: https://argo-workflows.readthedocs.io/en/stable/argo-server/
- 2: https://argo-workflows.readthedocs.io/en/stable/cli/argo_server/
- 3: https://github.com/argoproj/argo-workflows/blob/main/docs/argo-server.md
- 4: https://argo-workflows.readthedocs.io/en/latest/tls/
- 5: https://argo-workflows.readthedocs.io/en/stable/argo-server-auth-mode/
- 6: https://argo-workflows.readthedocs.io/en/latest/argo-server-auth-mode/
- 7: https://github.com/argoproj/argo-workflows/blob/main/docs/security.md
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- All production HTTPRoute backends ---'
rg -n -i -C 8 'kind:\s*HTTPRoute|backendRefs:|name:\s*argo|name:\s*insight|namespace:\s*argo' \
deploy/gitops charts 2>/dev/null | head -500
printf '%s\n' '--- Argo system values and production deployment commands ---'
cat -n deploy/gitops/system/argo-workflows/values.yaml | sed -n '1,100p'
cat -n deploy/gitops/environments/prod/Makefile | sed -n '120,155p;325,345p'
printf '%s\n' '--- Upstream chart metadata and templates for version 1.0.13 ---'
tmpdir="$(mktemp -d)"
curl -fsSL https://raw.githubusercontent.com/argoproj/argo-helm/argo-workflows-1.0.13/charts/argo-workflows/values.yaml |
rg -n -C 5 '^(server:| secure:| authModes:| serviceType:| enabled:)' | head -120
curl -fsSL https://raw.githubusercontent.com/argoproj/argo-helm/argo-workflows-1.0.13/charts/argo-workflows/templates/server/deployment-serverช.yaml 2>/dev/null || true
rm -rf "$tmpdir"Repository: constructorfabric/insight
Length of output: 50381
🏁 Script executed:
#!/bin/bash
set -eu
base='https://raw.githubusercontent.com/argoproj/argo-helm/argo-workflows-1.0.13/charts/argo-workflows'
printf '%s\n' '--- Chart values relevant to server TLS and authentication ---'
curl -fsSL "$base/values.yaml" |
rg -n -C 8 'server:|secure:|authModes:|serviceType:|extraArgs:' | head -180
printf '%s\n' '--- Server template paths ---'
curl -fsSL 'https://api.github.com/repos/argoproj/argo-helm/git/trees/argo-workflows-1.0.13?recursive=1' |
jq -r '.tree[].path' |
rg 'charts/argo-workflows/templates/.*server|charts/argo-workflows/templates/.*service'
printf '%s\n' '--- Server templates containing secure/auth arguments ---'
paths="$(
curl -fsSL 'https://api.github.com/repos/argoproj/argo-helm/git/trees/argo-workflows-1.0.13?recursive=1' |
jq -r '.tree[].path' |
rg '^charts/argo-workflows/templates/.*(server|service)' || true
)"
while IFS= read -r path; do
[ -n "$path" ] || continue
printf '%s\n' "--- $path ---"
curl -fsSL "https://raw.githubusercontent.com/argoproj/argo-helm/argo-workflows-1.0.13/$path" |
rg -n -C 5 'secure|auth|https|tls|service' || true
done <<< "$paths"Repository: constructorfabric/insight
Length of output: 7089
Do not use server authentication over HTTP in production.
authModes: [server] uses the Argo Server ServiceAccount instead of authenticating callers. With secure: false, workloads that can reach the ClusterIP can send unauthenticated requests over plaintext. Use client or configured sso authentication, enable secure: true, and configure the TLS certificate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/prod/argo-workflows-values.yaml` around lines 65 -
72, Update the production Argo Workflows server configuration around authModes
and secure: replace server authentication with client or configured SSO
authentication, enable secure: true, and configure the required TLS certificate
settings while preserving the existing service exposure.
| - name: github-idp-sub | ||
| identityProviderAlias: github | ||
| identityProviderMapper: github-user-attribute-mapper | ||
| config: | ||
| syncMode: FORCE | ||
| jsonField: login | ||
| userAttribute: idp_sub |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C5 \
--glob '*.yaml' --glob '*.yml' --glob '*.rs' --glob '*.sql' \
'github-directory|sourceType: github|source_type.?github|externalIdClaim: idp_sub|jsonField: (login|id)|idp_sub' \
deploy src || trueRepository: constructorfabric/insight
Length of output: 45541
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- prod realm relevant sections ---'
sed -n '130,172p' deploy/gitops/environments/prod/keycloak/realms/insight-broker.yaml
printf '%s\n' '--- identity input model ---'
sed -n '1,220p' src/ingestion/connectors/git/github-directory/dbt/github_directory__identity_inputs.sql
printf '%s\n' '--- GitHub directory connector identity fields ---'
rg -n -C8 \
'entity_id|login|github_id|user_id|identity_inputs|value_type|lower' \
src/ingestion/connectors/git/github-directory \
src/ingestion/connectors/macros 2>/dev/null || true
printf '%s\n' '--- mapper and login-bootstrap references ---'
rg -n -C6 \
'github-user-attribute-mapper|identityProviderMapper|login-bootstrap|source_type|external_id|externalIdClaim' \
deploy src/backend src/ingestion \
--glob '*.yaml' --glob '*.yml' --glob '*.rs' --glob '*.sql' || trueRepository: constructorfabric/insight
Length of output: 50382
🌐 Web query:
Keycloak GitHub identity provider github-user-attribute-mapper jsonField login GitHub provider mapper documentation
💡 Result:
In Keycloak, the github-user-attribute-mapper is a specific identity provider mapper used to extract fields from the JSON user profile returned by GitHub and store them as attributes on the Keycloak user [1][2][3]. To configure this mapper, you typically navigate to your GitHub Identity Provider in the Keycloak admin console, go to the Mappers tab, and create a new mapper using the "Attribute Importer" type (specifically associated with the github-user-attribute-mapper provider ID) [4][5]. Key Configuration Fields: - Mapper Type: Attribute Importer (or specifically configured as github-user-attribute-mapper in raw JSON configurations) [5]. - JSON Field (jsonField): This field specifies the name of the property in the GitHub user profile JSON response (e.g., login, name, email, or organizations_url) [6][5]. - User Attribute: The name of the attribute in the Keycloak user profile where you want to store the mapped value [6][5]. The jsonField parameter is a core configuration setting inherited from the AbstractJsonUserAttributeMapper class [6]. When Keycloak processes the GitHub login, it fetches the user profile, and this mapper uses the jsonField string to locate the corresponding value in the JSON payload and map it to the target user attribute [6][7]. For nested JSON structures, Keycloak supports dot notation (e.g., contact.address.country) [4]. Example JSON configuration for this mapper: { "name": "github-username", "identityProviderAlias": "github", "identityProviderMapper": "github-user-attribute-mapper", "config": { "syncMode": "INHERIT", "jsonField": "login", "userAttribute": "github_username" } } [5] You can debug the structure of the JSON data returned by GitHub by enabling the DEBUG level logger org.keycloak.social.user_profile_dump in your Keycloak configuration [4][6]. This will print the raw user profile data to the server logs, making it easier to identify the correct jsonField values to map [4][6].
Citations:
- 1: https://www.keycloak.org/docs-api/26.4.6/javadocs/org/keycloak/social/github/GitHubUserAttributeMapper.html
- 2: https://www.keycloak.org/docs-api/21.1.2/javadocs/org/keycloak/social/github/GitHubUserAttributeMapper.html
- 3: https://www.javatips.net/api/keycloak-master/services/src/main/java/org/keycloak/social/github/GitHubUserAttributeMapper.java
- 4: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/identity-broker/mappers.adoc
- 5: https://oneuptime.com/blog/post/2026-01-27-keycloak-social-login/view
- 6: https://www.keycloak.org/docs-api/26.2.5/javadocs/org/keycloak/broker/oidc/mappers/AbstractJsonUserAttributeMapper.html
- 7: https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- realm mapper ---'
sed -n '142,171p' deploy/gitops/environments/prod/keycloak/realms/insight-broker.yaml
printf '%s\n' '--- identity inputs SQL ---'
cat -n src/ingestion/connectors/git/github-directory/dbt/github_directory__identity_inputs.sql
printf '%s\n' '--- exact GitHub identity transformations ---'
rg -n -C12 \
'entity_id|lowercase|lowercased|lower\(|login|value_type|value_id' \
src/ingestion/connectors/git/github-directory/dbt \
src/ingestion/connectors/git/github-directory \
--glob '*.sql' --glob '*.yaml' --glob '*.yml'
printf '%s\n' '--- relevant identity lookup implementation ---'
rg -n -C10 \
'by-external-id|external_id|source_type|value_id|COLLATE|LOWER' \
src/backend/services/identity-resolution/src \
src/backend/services/authenticator/src \
--glob '*.rs' --glob '*.sql' \
| head -n 500Repository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path("src/ingestion/connectors/git/github-directory/dbt/github_directory__identity_inputs.sql"),
Path("src/ingestion/connectors/git/github-directory/dbt/github_directory__org_members_snapshot.sql"),
Path("src/ingestion/connectors/git/github-directory/dbt/github_directory__org_members_fields_history.sql"),
]
terms = ("entity_id", "login", "lower", "value_type", "value_id", "identity")
for path in paths:
print(f"--- {path} ---")
lines = path.read_text().splitlines()
hits = [i for i, line in enumerate(lines) if any(t in line.lower() for t in terms)]
shown = set()
for i in hits:
for j in range(max(0, i - 8), min(len(lines), i + 9)):
if j not in shown:
print(f"{j+1}: {lines[j]}")
shown.add(j)
print()
PYRepository: constructorfabric/insight
Length of output: 4318
🌐 Web query:
site:github.com/keycloak/keycloak GitHubUserAttributeMapper jsonField login userAttribute source code
💡 Result:
In Keycloak, the term "GitHubUserAttributeMapper" is not a standard class or built-in mapper name; however, user attribute mapping logic is handled by standard Keycloak components such as AbstractJsonUserAttributeMapper [1] and UserAttributeMapper [2][3]. These mappers allow you to map attributes from an external Identity Provider (IDP)—such as GitHub—into the local user's profile [4]. To map JSON fields (including those from GitHub's user API) to a user attribute in Keycloak: 1. JSON Field Configuration: When using an OIDC-based IDP, you use a mapper that supports JSON claim paths [4]. You can specify the JSON field using dot notation for nested objects and square brackets for arrays (e.g., contact.address.country) [4]. 2. User Attribute: This is the name of the attribute in the local Keycloak user profile where the mapped value will be stored [2][3]. 3. Implementation Context: - The AbstractJsonUserAttributeMapper is frequently used as a base class for mappers that need to extract values from a JSON claim [1]. - If you are mapping directly from an IDP, the UserAttributeMapper [2] handles the logic of extracting the claim value and calling context.setUserAttribute to save it to the local user [2]. If you are developing a custom provider, you can extend these classes to define how specific JSON fields map to user attributes. The configuration for these mappers is defined through the ProviderConfigProperty API, which registers settings like the User Attribute Name and the JSON field path in the Keycloak admin console [2][3].
Citations:
- 1: keycloak/keycloak@bfec073
- 2: https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/broker/oidc/mappers/UserAttributeMapper.java
- 3: https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/broker/saml/mappers/UserAttributeMapper.java
- 4: https://github.com/keycloak/keycloak/blob/main/docs/documentation/server_admin/topics/identity-broker/mappers.adoc
🌐 Web query:
site:github.com/keycloak/keycloak GitHubIdentityProvider normalize username lowercase login
💡 Result:
In Keycloak, the behavior regarding username normalization is centralized at the system level rather than being specific to the GitHubIdentityProvider. By design, Keycloak enforces lowercase usernames for local users in its database [1][2][3]. Keycloak's GitHubIdentityProvider implementation extracts the 'login' field from the GitHub profile and sets it as the username [4]. While this initial extraction happens, the broader Keycloak infrastructure often applies normalization, which includes forcing usernames to lowercase [1][5]. Recent developments and community discussions highlight the following: 1. System-wide Design: Keycloak is architected to treat usernames as case-insensitive, which historically leads to them being stored in lowercase within the local database [1][2][6]. 2. Federated Identity: There have been issues where the username from an Identity Provider (IdP) was incorrectly lowercased during the creation of federated identities [6]. Fixes, such as in pull request #29148, have been introduced to ensure that the federated username respects the original case-sensitive value provided by the IdP during the initial import process [6][7]. 3. Customization: If you need to preserve original casing, the "Username Template Importer" mapper can sometimes be used to control the formatting of substituted values [2]. However, because Keycloak's core user storage layer is designed around lowercase usernames, changing this behavior system-wide is generally not supported and can be difficult to implement [1][2]. If you are experiencing unexpected lowercase conversion, ensure you are using a recent version of Keycloak that incorporates fixes for preserving identity provider attributes [6][7].
Citations:
- 1: Username login has been converted to lowercase keycloak/keycloak#41243
- 2: Preserve the username/sub case-sensitive from the identity provider when Keycloak is acting as the identity broker keycloak/keycloak#21757
- 3: SSO Identity Provider user creation is not case insensitive on Username. keycloak/keycloak#9405
- 4: https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java
- 5: Make sure username is lowercase when normalizing attributes keycloak/keycloak#25173
- 6: IdP Linking: Usernames sometimes lowercase and sometimes uppercase keycloak/keycloak#28495
- 7: Do not lower-case the username from the IdP when creating the federated identity keycloak/keycloak#29148
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- repository contract checks for IdP registrations ---'
rg -n -C12 \
'registration_violations|github-idp-sub|jsonField|hardcoded-attribute-idp-mapper|oidc-user-attribute-idp-mapper|idp_sub' \
deploy/compose/keycloak deploy/gitops \
--glob '*.py' --glob '*.yaml' --glob '*.yml' \
| head -n 800
printf '%s\n' '--- all GitHub mapper configurations ---'
rg -n -C8 \
'identityProviderMapper: github-user-attribute-mapper|jsonField: (login|id)' \
deploy \
--glob '*.yaml' --glob '*.yml'
printf '%s\n' '--- normalization and lookup tests ---'
rg -n -C10 \
'login_normalized|lowercase.*login|Keycloak lowercases|case-sensitive|by-external-id|value_type.*id' \
src deploy docs \
--glob '*.rs' --glob '*.sql' --glob '*.yaml' --glob '*.yml' --glob '*.md' \
| head -n 800Repository: constructorfabric/insight
Length of output: 50384
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
for ref in 26.4.0 26.4.1 26.4.2 main; do
url="https://raw.githubusercontent.com/keycloak/keycloak/${ref}/services/src/main/java/org/keycloak/social/github/GitHubIdentityProvider.java"
body="$(curl -fsSL "$url" 2>/dev/null || true)"
if [ -n "$body" ]; then
printf '%s\n' "--- Keycloak ${ref}: GitHubIdentityProvider ---"
printf '%s\n' "$body" | rg -n -C6 'login|setUsername|username|GitHubUserAttributeMapper'
break
fi
done
for ref in 26.4.0 26.4.1 26.4.2 main; do
url="https://raw.githubusercontent.com/keycloak/keycloak/${ref}/services/src/main/java/org/keycloak/social/github/GitHubUserAttributeMapper.java"
body="$(curl -fsSL "$url" 2>/dev/null || true)"
if [ -n "$body" ]; then
printf '%s\n' "--- Keycloak ${ref}: GitHubUserAttributeMapper ---"
printf '%s\n' "$body" | rg -n -C8 'jsonField|userAttribute|setUserAttribute|login'
break
fi
doneRepository: constructorfabric/insight
Length of output: 1686
Use an immutable GitHub identifier for idp_sub.
The connector stores lowercased login_normalized, while Keycloak copies GitHub’s raw login value. Mixed-case logins therefore fail the byte-exact lookup, and renamed logins invalidate existing bindings. Use jsonField: id and migrate the connector bindings to GitHub numeric IDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/prod/keycloak/realms/insight-broker.yaml` around
lines 165 - 171, The github-idp-sub mapper must use GitHub’s immutable numeric
identifier rather than the mutable, case-sensitive login value. Change its
jsonField from login to id, and migrate the connector’s existing idp_sub
bindings to the corresponding GitHub numeric IDs while preserving the idp_sub
attribute mapping.
Source: Learnings
| # The operator's default is 10, and this one account now backs FOUR pools: | ||
| # analytics, identity-resolution, the chart's migrate/init Jobs, and — since | ||
| # the 2026-08-06 IdP cutover — the bundled Keycloak, which reuses this user | ||
| # (keycloak.database.username in insight/values.yaml) and whose own pool | ||
| # defaults to 100. Ten is not enough for that, and running out does not look | ||
| # like a connection limit: identity-resolution simply crash-loops at startup | ||
| # with `1226 (42000): User 'insight' has exceeded the max_user_connections | ||
| # resource`, ten restarts deep, while every other service stays Ready. | ||
| # 100 of the server's 151, leaving headroom for root and the operator's agent. | ||
| maxUserConnections: 100 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound aggregate database connection pools below the user limit.
Line 36 caps the shared insight user at 100 connections. The comment states that Keycloak can use a pool of 100 by itself. Keycloak can therefore consume the complete allowance and cause startup failures or request failures in the other three pools.
Set per-service pool limits whose total remains below maxUserConnections, or use separate database users with independent limits.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/prod/manifests/mariadb/insight.yaml` around lines
27 - 36, Adjust the MariaDB connection-pool configuration associated with the
shared insight user so the aggregate maximums for analytics,
identity-resolution, migrate/init Jobs, and bundled Keycloak remain below
maxUserConnections (100). Update the relevant pool-size settings in the
referenced values/configuration while preserving the existing user and limit
unless separate users are explicitly configured.
| privileges: | ||
| - ALL PRIVILEGES | ||
| database: insight | ||
| table: "*" | ||
| username: insight | ||
| host: "%" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Create and grant access to the Keycloak database.
deploy/gitops/environments/prod/values.yaml configures Keycloak to connect as insight to database keycloak. This Grant only permits access to insight, and this file only creates the insight database. Keycloak startup will fail unless another unshown resource creates keycloak and grants this user access.
Create a Database and a scoped Grant for keycloak. Prefer a dedicated Keycloak database user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/prod/manifests/mariadb/insight.yaml` around lines
47 - 52, Update the MariaDB manifest to create a dedicated Keycloak database and
user, then add a scoped Grant permitting that user access to the keycloak
database. Keep the existing insight database grant unchanged and ensure the
credentials match the Keycloak connection settings in values.yaml.
| tls: | ||
| enabled: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enable TLS for MariaDB client and replication traffic.
This production MariaDB cluster explicitly disables TLS. Clients then send database credentials and application data without MariaDB transport encryption. Galera replication traffic also lacks this protection.
Configure server certificates and update all MariaDB clients to require TLS before release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/prod/manifests/mariadb/mariadb.yaml` around lines
125 - 126, Update the MariaDB TLS configuration by enabling tls and supplying
the required server certificates, then update every MariaDB client and Galera
replication connection to require TLS before release. Preserve the existing
production cluster configuration while ensuring both client traffic and
replication traffic use encrypted transport.
| tls: | ||
| enabled: false | ||
| auth: | ||
| sasl: | ||
| enabled: false | ||
| external: | ||
| enabled: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enable Redpanda TLS and SASL authentication.
This production broker accepts unauthenticated, unencrypted client connections. Any pod with network access can read, produce, or administer Kafka resources as an arbitrary client.
Enable TLS and SASL. Configure application credentials and ACLs before exposing the broker to production workloads.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/environments/prod/redpanda-values.yaml` around lines 3 - 9,
Update the production Redpanda values under tls.enabled and auth.sasl.enabled to
enable encrypted, authenticated connections, and configure the required
application credentials and ACLs before workloads connect. Keep external
exposure settings unchanged unless needed for the secured production setup.
| ENV_CHART_PIN := $(shell yq -r '.chartVersion // ""' $(INVENTORY) 2>/dev/null) | ||
| ifneq ($(strip $(ENV_CHART_PIN)),) | ||
| INSIGHT_VERSION ?= $(ENV_CHART_PIN) | ||
| else | ||
| INSIGHT_VERSION ?= $(shell cat .insight-version 2>/dev/null) | ||
| endif |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show INVENTORY assignment and chart-version resolution in both Makefiles.
for f in deploy/gitops/Makefile deploy/gitops/environments/prod/Makefile; do
echo "── $f"
rg -n '^(INVENTORY|ENV_CHART_PIN|INSIGHT_VERSION)\b|^ifneq|^ifeq|^else|^endif' "$f"
doneRepository: constructorfabric/insight
Length of output: 915
🏁 Script executed:
#!/bin/bash
set -u
for f in deploy/gitops/Makefile deploy/gitops/environments/prod/Makefile; do
echo "=== $f (relevant definitions and resolution logic)"
sed -n '30,75p' "$f"
done
echo "=== inventory files"
find deploy/gitops -name inventory.yaml -type f -print -exec sed -n '1,80p' {} \;
echo "=== yq references"
rg -n -C 2 '\byq\b|chartVersion|protected' --glob '!*lock*' .Repository: constructorfabric/insight
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set +e
echo "=== inventory guards and deploy prerequisites"
rg -n -C 8 'inventory-present|kube-ctx-required|\.PHONY: deploy|^deploy:|INSIGHT_VERSION' \
deploy/gitops/Makefile deploy/gitops/environments/prod/Makefile
echo "=== yq availability"
command -v yq
yq --version
echo "=== current expression on the committed prod inventory"
out=$(yq -r '.chartVersion // ""' deploy/gitops/environments/prod/inventory.yaml 2>/dev/null)
printf 'status=%s output=<%s>\n' "$?" "$out"
echo "=== proposed expression on the committed prod inventory"
out=$(test -f deploy/gitops/environments/prod/inventory.yaml &&
yq -r '.chartVersion // ""' deploy/gitops/environments/prod/inventory.yaml ||
echo "")
printf 'status=%s output=<%s>\n' "$?" "$out"
echo "=== proposed expression with a missing yq command"
out=$(PATH=/nonexistent sh -c \
'test -f deploy/gitops/environments/prod/inventory.yaml &&
yq -r ".chartVersion // \"\"" deploy/gitops/environments/prod/inventory.yaml ||
echo ""')
printf 'status=%s output=<%s>\n' "$?" "$out"
echo "=== command-availability check from the proposed diff with a missing yq command"
out=$(PATH=/nonexistent sh -c \
'command -v yq >/dev/null 2>&1 && echo ok')
printf 'status=%s output=<%s>\n' "$?" "$out"Repository: constructorfabric/insight
Length of output: 24955
🏁 Script executed:
#!/bin/bash
set +e
echo "=== committed fallback file"
if [ -f .insight-version ]; then
printf '.insight-version='
cat .insight-version
else
echo ".insight-version is missing"
fi
echo "=== chart extraction failure with KUBE_CTX supplied"
PATH=/usr/bin:/bin bash -eu -o pipefail -c '
KUBE_CTX=insight-prod
ENV_CHART_PIN=$(yq -r ".chartVersion // \"\"" deploy/gitops/environments/prod/inventory.yaml 2>/dev/null)
if [ -n "$ENV_CHART_PIN" ]; then
INSIGHT_VERSION="$ENV_CHART_PIN"
else
INSIGHT_VERSION=$(cat .insight-version 2>/dev/null)
fi
printf "KUBE_CTX=%s ENV_CHART_PIN=<%s> INSIGHT_VERSION=<%s>\n" \
"$KUBE_CTX" "$ENV_CHART_PIN" "$INSIGHT_VERSION"
'
echo "=== protected check when yq fails"
PATH=/usr/bin:/bin bash -eu -o pipefail -c '
if [ "$(yq -r ".protected // false" deploy/gitops/environments/prod/inventory.yaml)" = "true" ]; then
echo "confirmation required"
else
echo "confirmation not required"
fi
'
printf 'protected-check-status=%s\n' "$?"Repository: constructorfabric/insight
Length of output: 392
🏁 Script executed:
#!/bin/bash
set -u
echo "=== version-file references"
rg -n -C 3 --hidden --glob '!.git/**' '\.insight-version|INSIGHT_VERSION' \
.github deploy docs README.md CONTRIBUTING.md 2>/dev/null
echo "=== version-file status and ignore rules"
git ls-files --stage -- .insight-version
git status --short --untracked-files=all -- .insight-version
git check-ignore -v .insight-version || true
echo "=== candidate version files"
find . -maxdepth 4 -type f \( -name '*insight*version*' -o -name '*version*' \) -print 2>/dev/null | head -100Repository: constructorfabric/insight
Length of output: 16522
🏁 Script executed:
#!/bin/bash
set +e
echo "=== fallback values"
printf 'top-level fallback: <'
cat deploy/gitops/.insight-version 2>/dev/null
printf '>\n'
printf 'prod fallback: <'
cat deploy/gitops/.insight-version 2>/dev/null
printf '>\n'
echo "=== exact fallback paths from each Makefile directory"
(
cd deploy/gitops
printf 'top-level cwd=%s fallback=<%s>\n' "$PWD" "$(cat .insight-version 2>/dev/null)"
)
(
cd deploy/gitops/environments/prod
printf 'prod cwd=%s fallback=<%s>\n' "$PWD" "$(cat ../../.insight-version 2>/dev/null)"
)
echo "=== failed yq resolution with an explicit KUBE_CTX override"
PATH=/usr/bin:/bin bash -eu -o pipefail -c '
cd deploy/gitops
KUBE_CTX=insight-prod
ENV_CHART_PIN=$(yq -r ".chartVersion // \"\"" environments/prod/inventory.yaml 2>/dev/null)
if [ -n "$ENV_CHART_PIN" ]; then
INSIGHT_VERSION="$ENV_CHART_PIN"
else
INSIGHT_VERSION=$(cat .insight-version 2>/dev/null)
fi
printf "ENV_CHART_PIN=<%s> INSIGHT_VERSION=<%s>\n" \
"$ENV_CHART_PIN" "$INSIGHT_VERSION"
'Repository: constructorfabric/insight
Length of output: 503
🏁 Script executed:
#!/bin/bash
set +e
cd deploy/gitops
KUBE_CTX=insight-prod
ENV_CHART_PIN=$(PATH=/usr/bin:/bin yq -r '.chartVersion // ""' environments/prod/inventory.yaml 2>/dev/null)
yq_status=$?
if [ -n "$ENV_CHART_PIN" ]; then
INSIGHT_VERSION="$ENV_CHART_PIN"
else
INSIGHT_VERSION=$(cat .insight-version 2>/dev/null)
fi
printf 'yq_status=%s ENV_CHART_PIN=<%s> INSIGHT_VERSION=<%s>\n' \
"$yq_status" "$ENV_CHART_PIN" "$INSIGHT_VERSION"Repository: constructorfabric/insight
Length of output: 220
Fail closed when inventory pin resolution fails.
deploy/gitops/environments/prod/inventory.yaml defines chartVersion, but a yq failure selects .insight-version instead. If the values differ, deployment can use the wrong chart version without a warning. Distinguish an absent chartVersion from an inventory read or parse failure in both Makefiles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/gitops/Makefile` around lines 62 - 67, Update the chart-version
resolution logic around ENV_CHART_PIN and INSIGHT_VERSION to distinguish an
absent chartVersion from an inventory read or yq parse failure. Make inventory
resolution failures fail closed rather than falling back to .insight-version,
and apply the same behavior in both Makefiles while retaining the fallback only
when chartVersion is genuinely absent.
|
Note for reviewers on the one red check: Trivy fails on (a) dependency CVEs in lockfiles this PR does not touch — they surface as "new" only because the merge-ref scan is fresher than main's nightly baseline, and will fold into it on the next scheduled run — and (b) a set of waived-by-design misconfiguration findings on the storage-init chown Job (root is its purpose; rationale in the manifest) and the Airbyte launcher RBAC, pending dismissal. Every finding this PR could genuinely fix is fixed: |
ktursunov
left a comment
ktursunov
left a comment
There was a problem hiding this comment.
Review outcome: blocking changes are required.
Sealed Secrets is a reasonable Git transport mechanism when strict object scope, a supported controller, an external source of truth, and tested key recovery are all in place. The manifests here use strict name/namespace scope and do not contain plaintext Secret fields, but the surrounding deployment flow currently weakens those guarantees. The main blockers are the repository confidentiality boundary, a dev-only impersonation switch enabled in a protected environment, a controller version below a published security fix, plaintext credentials flowing through Helm release values, insecure credential transport, and failure paths that can silently report success or rotate a credential.
I added implementation-focused inline comments with the required fixes. This review is submitted as a comment because GitHub does not allow an author to formally request changes on their own PR.
| @@ -0,0 +1,107 @@ | |||
| ## | |||
There was a problem hiding this comment.
[P0] Move concrete deployment state out of this repository
The root repository policy forbids GitHub-visible production-derived infrastructure and deployment details. This newly added concrete environment tree is therefore outside the allowed confidentiality boundary. Move the environment-specific overlays, manifests, ciphertext, and RBAC into the authorized private GitOps repository, and keep only clearly synthetic reusable templates in this repository.
|
|
||
| authenticator: | ||
| replicaCount: 1 | ||
| overrideEnabled: true |
There was a problem hiding this comment.
[P0] Disable unrestricted view-as in protected environments
The chart values document this switch as dev/demo-only and require it to remain false anywhere real users can authenticate. Enabling it lets a caller request an override identity without an authorization boundary in this configuration. Keep this false here; if administrator impersonation is needed, implement explicit role checks, allowlisting, and audit logging as a separate capability.
| # Everything under sealed-secrets/ is sealed against THIS cluster's controller | ||
| # key and decrypts only here. Re-seal everything if the key rotates or the | ||
| # cluster is rebuilt. | ||
| SEALED_SECRETS_VERSION ?= 2.17.4 |
There was a problem hiding this comment.
[P1] Upgrade the controller past the rotate scope-widening fix
Chart 2.17.4 deploys controller 0.31.0, which is below the 0.36.0 patched version for GHSA-465p-v42x-3fmj. An actor who can invoke the rotate endpoint can use attacker-controlled template annotations to widen sealing scope. Pin a supported chart whose controller appVersion is at least 0.36.0 and verify upgrade compatibility. Advisory: GHSA-465p-v42x-3fmj
| --version $(INSIGHT_VERSION) \ | ||
| --namespace $(NS_APP) --create-namespace \ | ||
| --values values.yaml \ | ||
| --set-string authenticator.oidc.clientSecret="$$(kubectl -n $(NS_APP) get secret insight-oidc -o jsonpath='{.data.client-secret}' | base64 --decode)" \ |
There was a problem hiding this comment.
[P1] Do not inject plaintext credentials through Helm values
This command substitution decrypts the credential into the Helm process arguments, and the set value is retained in Helm release configuration. That creates plaintext copies outside the Kubernetes Secret and defeats the intended Sealed Secrets boundary. Extend the chart to consume an existing Secret/key via secretKeyRef, then pass only the Secret name and key.
| keycloakConfig: | ||
| enabled: true | ||
| url: http://insight-keycloak:8085/kc | ||
| allowInsecureUrl: true |
There was a problem hiding this comment.
[P1] Do not send administrator credentials over plain HTTP
The chart explicitly limits allowInsecureUrl to dev/CI because the config Job authenticates to this URL with administrator credentials. Use HTTPS with a trusted CA or another authenticated encrypted in-cluster endpoint, and leave allowInsecureUrl false.
| @# Postgres password: reuse the chart-generated one when the release | ||
| @# already exists; only a fresh install mints a new one. Passing nothing | ||
| @# would silently fall back to the chart default (the literal 'airbyte'). | ||
| @existing=$$(kubectl -n $(AIRBYTE_NAMESPACE) get secret airbyte-airbyte-secrets \ |
There was a problem hiding this comment.
[P1] Do not rotate the database password on lookup failures
Redirecting errors and ending the lookup with true makes Forbidden, timeout, transport, and decode failures indistinguishable from Secret NotFound. Any such failure produces a new random password and can break an existing database during upgrade. Generate only after an explicit NotFound result and fail on every other error. Prefer an existing-Secret reference so the password is never stored as a Helm value.
| curl -sf -X POST "$$B/api/v1/instance_configuration/setup" \ | ||
| -H "Content-Type: application/json" \ | ||
| -d "{\"email\":\"$(AIRBYTE_SETUP_EMAIL)\",\"organizationName\":\"$(AIRBYTE_SETUP_ORG)\",\"initialSetupComplete\":true,\"anonymousDataCollection\":false}" \ | ||
| || true' |
There was a problem hiding this comment.
[P1] Do not make setup failures look successful
The unconditional true means DNS, authentication, transport, and server errors all make this Pod succeed, so the deployment continues without completing setup. If an already-configured response is the intended idempotent case, inspect the HTTP status/body and accept only that exact response; fail every other result.
| # inventory is the real wrong-cluster protection. Every mutating target | ||
| # depends on this — including the per-component ones. | ||
| kube-ctx: | ||
| @CUR=$$(kubectl config current-context 2>/dev/null || true); \ |
There was a problem hiding this comment.
[P1] Pin every mutation to the validated context
This prerequisite validates the current context once, but all later kubectl and Helm commands reread mutable global kubeconfig state. A context change between the check and a mutation bypasses the guard. Pass --context to every kubectl invocation and --kube-context to every Helm invocation, or run the target with an isolated kubeconfig.
| - **Secrets are sealed per service namespace** — the schema's | ||
| `secrets.services` list is joined by a `secrets.datastores` list (each entry | ||
| naming its own namespace), matching the one-namespace-per-service topology. | ||
| Ciphertext is bound to this cluster's controller key: a cluster rebuild or |
There was a problem hiding this comment.
[P1] Define a recoverable secret lifecycle
Re-sealing after a controller-key loss or cluster rebuild requires either recovery of the controller private key or authoritative plaintext credentials from an external source. The committed ciphertext alone is not a recovery source. Document and automate a password-manager-backed source plus encrypted off-cluster key backup/restore, or explicitly define full credential regeneration; test the restore path before treating this as the whole secret story.
0a88967 to
8818b71
Compare
|
Status after the fix rounds (branch rebased onto current main, all checks green). This supersedes my earlier note about Trivy — the waivers landed in Addressed from the review:
Open, tracked as follow-ups: chart-level secretKeyRef for the oidc secret; keycloak-config over in-cluster TLS; documented key-recovery/backup path; dependency-CVE bumps in the lockfiles (pre-existing, not this PR). A second review pass has surfaced ten further findings (top-level Makefile parity with the env hardening, two bugs in the fix round itself, a leftover sample DB account in the MariaDB CR); fixes are queued. |
|
@coderabbitai review |
3bbb307 to
a88b515
Compare
3ac2a16 to
cb54fef
Compare
…tenant guard The static registration check only recognised the OIDC broker mappers' user.attribute config key, so a GitHub (social) registration stamping idp_sub through github-user-attribute-mapper — whose key is userAttribute — failed the contract despite satisfying it. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…y findings semgrep.yml: the secret-rule exclusion id was missing its rule-name segment (…detected-generic-secret.detected-generic-secret), so it never matched; and a strip step now drops nosemgrep-suppressed results before the SARIF upload (Code Scanning ignores SARIF suppressions and would open alerts for waived findings, with the inline directive shifting fingerprints into duplicates). The strip writes a sibling and mv's it — the scan container owns semgrep.sarif as root. .trivyignore.yaml: scoped, statement-carrying waivers for the storage-init chown Jobs (root is the Job's purpose — the Cinder fsGroup gap) and the airbyte workload-launcher RBAC (mirrors the upstream chart's namespaced Role). Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Adds environments/prod (inventory, umbrella values, flat per-service value overlays, concrete operator-CR manifests, the broker realm as code, an env-scoped Makefile) and bootstrap/prod (L0 manifests) shaped after the reference insight-gitops flow: kube-ctx guard → bootstrap → secrets → system → deploy CONFIRM=yes-deploy-prod. The env Makefile exists because the stand runs operator-managed datastores one-namespace-per-service, which the top-level chart-based system-* targets do not fit. Design points baked in from review + live verification: - chart pin via inventory.chartVersion (top-level Makefile learns the same resolution order: CLI > per-env pin > .insight-version) - secrets are committed SealedSecrets under sealed-secrets/<ns>/, applied by `make secrets`; no cleartext or sealing tooling in the tree - confirm gate fails closed; every kubectl/helm call pins the named context; airbyte password/setup paths fail loud instead of masking; oidc client secret verified non-empty before upgrade - sealed-secrets controller pinned ≥2.19.1 (covers GHSA-465p-v42x-3fmj) - authenticator.overrideEnabled stays false (view-as is dev/demo-only) - storage-init Jobs hardened (read-only rootfs, RuntimeDefault seccomp, resources) with DAC_OVERRIDE for the re-runnable chown Raw cluster IPs (API server, Envoy floating IP) are NOT committed: they live in a gitignored secrets.local.env (shape in the committed .template), sourced by the Makefile, substituted into the EnvoyProxy at apply time, and redacted from all log output. No tracked file carries the addresses. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The rule-id fix and the suppression-strip step are repo-wide CI changes, not prod-stand configuration; they belong in their own change. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The github connector's descriptor requires git_proxy_url/git_proxy_token, so the proxy has to exist before that connector can be configured; reconcile injects both from the chart-owned Secret. Its cache PVC needs a filesystem type: the Cinder driver advertises fsGroupPolicy ReadWriteOnceWithFSType, the default storage class declares no parameters, and the service runs non-root on a read-only rootfs — so a volume from the default class is owned root:root and the service cannot boot. A second storage class carries the fstype rather than adding one to the default, whose parameters are immutable. Also turn on authenticator provisionOnLogin so a roster member whose directory publishes no e-mail address can complete a first login. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Reconcile injects git_proxy_url/git_proxy_token only for a descriptor that sets platform_config.git_proxy. No descriptor sets it, so the connector's own Secret has to carry both and the CronJob's env vars go unread. The previous wording would lead the next operator to omit the two fields, which the pre-flight validator answers with a WARN and a skipped connector. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Matches the name every other artifact in the tree uses for this connector, and records the source-id the identity seed requires. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The github connector's descriptor requires five fields, and reconcile injects none of them: platform_config.git_proxy is unset on every descriptor, so the proxy address and token travel in the connector's own Secret. Until a Secret annotated for this connector exists, every reconcile tick treats the connector as removed and cascades a delete over the sources whose name starts with its slug — which includes the sibling directory connector. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
0.5.171 flips gitCliProxy.networkPolicy.enabled to false by default, and this environment was inheriting the previous true — the upgrade would have dropped the policy and left the clone cache reachable from anywhere in the cluster. Say it in the values instead of inheriting it. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…ndpoints The platform resolves both but writes neither: connector pods take their config through emptyDir and secretKeyRef, which the kubelet resolves without a grant. Splitting them into a read-only rule clears one HIGH and one MEDIUM scanner finding; the remaining rule keeps every verb the workload launcher uses on pods, jobs and secrets. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The annotation carried github-directory-main, the sibling connector's value. reconcile reads it as source_id_label and composes the Airbyte source and connection names from it, and it reaches bronze as insight_source_id — so two connectors would have stamped the same source_id on their rows. The repo's own template (src/ingestion/secrets/connectors/github.yaml.example) names it github-main. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
First chart whose toolbox image carries the reconcile manifest-on-stdin fix, so the github connector can be registered and the loop stops aborting before it triggers syncs. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Every reference the stand publishes: the Gateway listener hostname, the gateway and keycloak route hosts, the OIDC issuer and redirect URI, the CSRF origin, and the broker realm's redirect URI. DNS for the new name already resolves to the same Cloudflare-proxied edge. The GitHub OAuth App's callback still points at the old host and is managed outside this repo, so sign-in stays broken until that is updated. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…dress The value was the bare Service name, which only resolves inside the namespace that holds it. Airbyte runs each connector job as a pod in its own namespace, so the connector could not reach the proxy at all. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
`kubeseal --merge-into` rewrites the document through a YAML parser, which drops comments. Re-adding it after a reseal is the price of that command. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
Seven values belong in more than one Kubernetes Secret and nothing at deploy time checks that the copies agree: each datastore password is carried both by its own namespace and by insight-db-creds, and the Keycloak admin, the brokered GitHub app and the authenticator client secret are carried both by the Secret that owns them and by the config Job that writes them into the realm. Sealing each one by hand is what lets them drift. The script seals only the variables that are set, so rotating one credential leaves the rest of a manifest untouched. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…s at constructorfabric New client id and secret for the identity broker, sealed into both the Secret that owns them and the config Job that writes them into the realm. New API token for each connector, and the organization list switched on both so the roster and the commit history describe the same org. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…s need The previous pair carried no scopes at all, so the roster sync failed on the member-email query before reading a row. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The seal script sources the credential file with the shell, which strips one level of quoting. An unquoted ["org"] reached the connector as [org], which is not a JSON array: the sync then completed with nothing read instead of failing, so the mistake looked like an empty organization rather than a malformed one. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
The default StorageClass reclaimed its volumes on delete, so removing a PVC — by hand, by an operator recreating a StatefulSet, or along with a namespace — destroyed the backing volume and its data at once. The mariadb CR's pvcRetentionPolicy guards the claim, not the volume beneath it. Retention has to live on the class the datastores already name: a PVC's storageClassName is immutable once bound, so they cannot be moved onto a second retaining class without recreating the volume the policy exists to protect. cinder-ext4 keeps Delete — it backs only the git-cli-proxy clone cache, which the remote can rebuild. reclaimPolicy is immutable on a StorageClass too, so bootstrap-storage recreates the class when the live policy differs, refusing while any PVC is still unbound. A volume keeps the policy it was provisioned under, so the target also reconciles existing ones, where the field is mutable. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
ee80cc7 to
314bb2f
Compare
|
Closing: this stand's gitops configuration now lives in the internal insight-gitops repository as environment Nothing here consumed it — no workflow, script or Makefile target referenced Three changes on this branch were environment-independent and are not carried over; if they are wanted they should come back as their own PR:
|
2 participants
Adds the
prodgitops environment for the InsightCfabric stand, shaped after the reference insight-gitops flow, plus the small pieces the rest of the tree needs to support it.What's in here
environments/prod/— inventory (cluster identity, edge,chartVersion:pin), umbrellavalues.yaml, flat per-service values overlays, concrete operator-CR manifests undermanifests/, the broker realm as code, and an env-scoped Makefile (kube-ctxguard →bootstrap→secrets→system→deploy CONFIRM=yes-deploy-prod). The env Makefile exists because this stand runs operator-managed datastores one-namespace-per-service, which the top-level chart-basedsystem-*targets don't fit; it keeps the same target vocabulary.bootstrap/prod/— concrete L0 manifests (storage class, EnvoyProxy + shared Gateway, cert-manager CA chain). No placeholder rendering anywhere; committed YAML is the applied state.sealed-secrets/<ns>/, applied bymake secrets(waits for Synced). No cleartext, templates, or sealing tooling in the tree; rotation is a plainkubesealagainst the cluster controller.inventory.chartVersionpin before falling back to.insight-version(same order as the reference repo).userAttribute) when verifying theidp_substamp; the OIDC key alone rejected valid GitHub-broker registrations.Verification
kubectl diffover every committed manifest against the live stand: zero drift.helm upgrade --dry-run=serverat the pinned chart rendered byte-identical to the deployed release at porting time; the stand runs the pinned chart deployed throughmake deploy, and the hardening in this PR (ClickHouse/Keeper securityContext) has been applied and re-verified drift-free.Notes for review
bootstrap:/system:inventory toggles are deliberately false — they drive the top-level targets; the env Makefile owns the operator path (documented in the inventory header and env README).