Skip to content

Conversation

@Sheikah45
Copy link
Member

@Sheikah45 Sheikah45 commented Dec 13, 2025

Summary by CodeRabbit

  • Chores

    • Updated and consolidated secret contents (replaced placeholders, switched some keys to multi-line formats, removed an exposed license entry)
    • Added service accounts and role bindings; removed legacy init scripts
  • New Features

    • Helm-driven Jobs to automate DB and RabbitMQ user/database provisioning
    • Added RabbitMQ configuration entry and updated deployment values for Traefik and RabbitMQ
  • Bug Fixes

    • Added startup health checks for RabbitMQ, MariaDB, MongoDB, and PostgreSQL to improve startup reliability

✏️ Tip: You can customize this high-level summary in your review settings.

@Sheikah45 Sheikah45 requested a review from Brutus5000 December 13, 2025 16:21
@coderabbitai
Copy link

coderabbitai bot commented Dec 13, 2025

Walkthrough

Secret values/formats updated across apps; startupProbes added to multiple statefulsets; Traefik local values adjusted; new init Jobs and RBAC ServiceAccounts/RoleBindings added to orchestrate DB and RabbitMQ provisioning; legacy init scripts removed; RabbitMQ config and values extended.

Changes

Cohort / File(s) Summary
Secret templates
apps/faf-api/templates/local-secret.yaml, apps/faf-icebreaker/templates/local-secret.yaml, apps/faf-lobby-server/templates/local-secret.yaml, apps/ory-hydra/templates/local-secret.yaml
Replaced placeholder secret values with actual key material or multiline PEM blocks; removed GEO_IP_LICENSE_KEY; changed SECRETS_SYSTEM value content/format. No template control-flow changes.
RabbitMQ config & StatefulSet
apps/rabbitmq/templates/config.yaml, apps/rabbitmq/templates/statefulset.yaml, apps/rabbitmq/values.yaml
Added rabbitmq_conf ConfigMap entry; added startupProbe, envFrom (secret/configMap) and additional volumeMount to StatefulSet; introduced image and users values.
RabbitMQ init Jobs & scripts
apps/rabbitmq/templates/init-rabbitmq-with-user.yaml, scripts/init-rabbitmq.sh
Added Helm Job template (post-sync) to sync vhost/users from ConfigMaps/Secrets; removed legacy scripts/init-rabbitmq.sh.
Database startupProbes & Postgres templating
infra/mariadb/templates/statefulset.yaml, infra/mongodb/templates/statefulset.yaml, infra/postgres/templates/statefulset.yaml
Added startupProbes to MariaDB, MongoDB, and Postgres containers; Postgres container/initContainer images and POSTGRES_PASSWORD wiring templated from values.
Postgres init Jobs & values
infra/postgres/templates/init-database-with-user.yaml, infra/postgres/values.yaml
Added new Helm Job template (PostSync) to idempotently create DBs/users per values.databasesAndUsers; added image/upgrade-image and databasesAndUsers entries to values.yaml.
MariaDB init job changes
infra/mariadb/templates/init-database-with-user.yaml
Removed ServiceAccount and RoleBinding documents from the template; added set -e to init script and removed some 2>&1 redirections in job commands.
RBAC / ServiceAccount additions
infra/clusterroles/templates/service-account.yaml, infra/clusterroles/templates/role-binding.yaml
Added init-apps ServiceAccount resources (namespaces faf-infra and faf-apps) and RoleBinding resources granting read-cm-secrets so init Jobs can read ConfigMaps/Secrets.
Traefik / local config
config/local.yaml, cluster/traefik/values-local.yaml
Removed tlsStoreSecret from local config and replaced traefik mapping; added/updated Traefik Helm values (hostNetwork, port mappings including metrics, HTTPS redirect, timeouts, updateStrategy, logging and extra args).
Removed Postgres init script
scripts/init-postgres.sh
Removed legacy script that provisioned Postgres DBs/users from within the postgres pod.
Misc templating adjustments
infra/postgres/templates/statefulset.yaml (referenced above)
Minor helm-templating and secretKeyRef additions for Postgres initContainer; startupProbe added to main container.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Helm as Helm renderer
  participant KubeAPI as Kubernetes API
  participant Config as ConfigMap/Secret (faf-apps)
  participant InitCnt as Job initContainer (load-config)
  participant MainJob as Job main container (sync)
  participant DB as Database Pod

  Helm->>KubeAPI: Apply Job manifest per entry (PostSync hook)
  KubeAPI->>InitCnt: Start initContainer
  InitCnt->>Config: kubectl get ConfigMap/Secret (faf-apps)
  Config-->>InitCnt: return env keys -> write /config/env
  InitCnt-->>KubeAPI: initContainer completes
  KubeAPI->>MainJob: Start main job container
  MainJob->>MainJob: source /config/env, export credentials
  MainJob->>DB: connect -> check/create user
  MainJob->>DB: connect -> check/create database
  MainJob->>DB: grant privileges
  MainJob-->>KubeAPI: Exit success -> Job completes (HookSucceeded)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Areas needing extra attention:
    • Secret templates — verify no sensitive keys were unintentionally committed and PEM/SSH formatting is correct.
    • New Job templates (infra/postgres/templates/init-database-with-user.yaml, apps/rabbitmq/templates/init-rabbitmq-with-user.yaml) — verify idempotency, hook wave ordering, RBAC, serviceAccount usage, and failure/backoff settings.
    • RBAC changes — ensure RoleBindings and ServiceAccounts align with removed/added resources.
    • StartupProbe commands and Postgres image templating — validate probe commands, timings, and value substitutions.
    • Removed scripts (scripts/init-rabbitmq.sh, scripts/init-postgres.sh) — confirm operational replacement by Jobs.

Poem

🐰 I hid the keys beneath my paw,
I nudged the probes to check the thaw,
I hopped through Jobs that make DBs right,
I bound a SA in morning light,
Small rabbit, infra shining bright 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main objective of the PR, which is to update configurations and manifests to support tilt.dev local development workflows.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch tilt

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 94b23fe and c7a5923.

📒 Files selected for processing (1)
  • config/local.yaml (1 hunks)
🔇 Additional comments (1)
config/local.yaml (1)

6-6: LGTM! Empty object resolves the previous concern.

The empty object traefik: {} is a good compromise that addresses both the issue where tlsStoreSecret: null was breaking traefik locally and the concern about helm template evaluation. With this change, .Values.traefik.tlsStoreSecret will evaluate to false in the helm conditional while keeping the traefik object defined.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5be09d4 and c91ae27.

📒 Files selected for processing (11)
  • apps/faf-api/templates/local-secret.yaml (1 hunks)
  • apps/faf-icebreaker/templates/local-secret.yaml (1 hunks)
  • apps/faf-lobby-server/templates/local-secret.yaml (0 hunks)
  • apps/ory-hydra/templates/local-secret.yaml (1 hunks)
  • apps/rabbitmq/templates/statefulset.yaml (1 hunks)
  • config/local.yaml (0 hunks)
  • infra/mariadb/templates/statefulset.yaml (1 hunks)
  • infra/mongodb/templates/statefulset.yaml (1 hunks)
  • infra/postgres/templates/statefulset.yaml (2 hunks)
  • scripts/init-mariadb.sh (1 hunks)
  • scripts/init-rabbitmq.sh (1 hunks)
💤 Files with no reviewable changes (2)
  • config/local.yaml
  • apps/faf-lobby-server/templates/local-secret.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/init-mariadb.sh (1)
scripts/k8s-helpers.sh (3)
  • check_resource_exists_or_fail (40-50)
  • get_secret_value (61-63)
  • get_config_value (57-59)
🪛 Gitleaks (8.30.0)
apps/faf-api/templates/local-secret.yaml

[high] 21-47: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.

(private-key)

apps/faf-icebreaker/templates/local-secret.yaml

[high] 17-44: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.

(private-key)

🔇 Additional comments (8)
scripts/init-rabbitmq.sh (1)

55-55: Clarify the vhost name change from /faf-core to //faf-core.

The vhost identifier was changed to use a leading double slash, which is unusual. Standard RabbitMQ vhost naming uses a single leading slash (e.g., /faf-core). Verify whether this change is intentional or a typo, as it may cause connection failures for services expecting the original vhost path.

Is this vhost change intentional? If not, revert to the standard single-slash format:

-VHOST_FAF_CORE="//faf-core"
+VHOST_FAF_CORE="/faf-core"
apps/rabbitmq/templates/statefulset.yaml (1)

33-38: Startup probe configuration looks good.

The rabbitmq-diagnostics ping command is compatible with the image version (3.13.7), and the probe timing is appropriate. This will ensure the pod is ready before receiving traffic.

infra/postgres/templates/statefulset.yaml (1)

47-50: Verify the PostgreSQL user in the startup probe.

The pg_isready command uses -Uroot, but the default PostgreSQL user is typically postgres. This mismatch may cause the startup probe to fail. Verify whether root is the correct user or if it should be postgres.

If postgres is the correct user, apply this diff:

          startupProbe:
            exec:
-             command: ["pg_isready", "-Uroot", "-h127.0.0.1"]
+             command: ["pg_isready", "-Upostgres", "-h127.0.0.1"]
            periodSeconds: 5
apps/faf-icebreaker/templates/local-secret.yaml (1)

16-54: Verify that embedded cryptographic keys are local-development only.

The JWT private and public keys are committed directly in the template. This is acceptable for local development (per the PR's tilt.dev context), but they must never be used in production. Confirm that:

  1. These keys are exclusively for local development environments
  2. Production deployments use infisical-secret (as suggested by the conditional on line 1)
  3. If these keys are exposed, they should be considered compromised and rotated immediately

Additionally, consider adding a comment in the file documenting that these are local-dev placeholders:

 {{- if not (index .Values "infisical-secret" "enabled") }}
+# Local development only - do not use in production
 apiVersion: v1
apps/ory-hydra/templates/local-secret.yaml (1)

12-12: Local development secret value looks appropriate.

The placeholder value has been updated to increase entropy. This is acceptable for local development environments.

infra/mariadb/templates/statefulset.yaml (1)

29-34: Startup probe configuration is sound.

The mariadb-admin ping command is the standard approach for MariaDB health checks, and the timing parameters are appropriate for a database startup scenario.

apps/faf-api/templates/local-secret.yaml (1)

19-47: Verify that embedded RSA cryptographic keys are local-development only.

The RSA private and public keys are committed directly in the template. This is acceptable for local development (per the PR's tilt.dev context), but they must never be used in production. Confirm that:

  1. These keys are exclusively for local development environments
  2. Production deployments use infisical-secret (as suggested by the conditional on line 1)
  3. If these keys are exposed, they should be considered compromised and rotated immediately

Additionally, consider adding a comment in the file documenting that these are local-dev placeholders:

 {{- if not (index .Values "infisical-secret" "enabled") }}
+# Local development only - do not use in production
 apiVersion: v1
scripts/init-mariadb.sh (1)

44-44: Verify the database name for faf-icebreaker.

Line 44 uses DB_USERNAME as both the username and database name parameter. This may be a copy-paste error. Confirm whether the database name should be distinct (e.g., a different config key like DB_DATABASE or DB_NAME).

Comment on lines +31 to +36
startupProbe:
exec:
command: ["mongo", "--eval", "db.adminCommand('ping')"]
initialDelaySeconds: 10
timeoutSeconds: 15
periodSeconds: 5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Incompatible MongoDB startup probe command for image version 8.2.2.

The mongo CLI tool was deprecated and removed in MongoDB 8.0. The image mongo:8.2.2 requires mongosh instead. The current command will fail, preventing the pod from starting.

Apply this diff to fix the startup probe:

          startupProbe:
            exec:
-             command: ["mongo", "--eval", "db.adminCommand('ping')"]
+             command: ["mongosh", "--eval", "db.adminCommand('ping')"]
            initialDelaySeconds: 10
            timeoutSeconds: 15
            periodSeconds: 5
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
startupProbe:
exec:
command: ["mongo", "--eval", "db.adminCommand('ping')"]
initialDelaySeconds: 10
timeoutSeconds: 15
periodSeconds: 5
startupProbe:
exec:
command: ["mongosh", "--eval", "db.adminCommand('ping')"]
initialDelaySeconds: 10
timeoutSeconds: 15
periodSeconds: 5
🤖 Prompt for AI Agents
In infra/mongodb/templates/statefulset.yaml around lines 31 to 36 the
startupProbe uses the removed "mongo" CLI which fails on image mongo:8.2.2;
update the probe to call "mongosh" instead and keep the same eval string (e.g.
replace ["mongo", "--eval", "db.adminCommand('ping')"] with ["mongosh",
"--eval", "db.adminCommand('ping')"]) so the probe uses the supported shell;
ensure the image actually includes mongosh and adjust timeout/periods only if
needed for the new binary startup time.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
infra/mariadb/templates/init-database-with-user.yaml (1)

97-107: Consider: Add set -e to the main container script for consistency.

The init container now has set -e (line 62), but the main container script (lines 97-107) does not. If a mariadb command fails (e.g., CREATE DATABASE fails), the script will continue and attempt to create the user anyway, potentially leaving the database in an incomplete state. This is a minor concern for idempotent CREATE IF NOT EXISTS operations, but adding set -e here would improve robustness and fail-fast behavior.

If you want to ensure the script fails on any error:

  args:
    - |
      set -a
      . /config/env
      set +a
+     set -e
  
      mariadb -v --host=mariadb --user=root --password="${MARIADB_ROOT_PASSWORD}" -e "CREATE DATABASE IF NOT EXISTS \`${SYNC_DATABASE}\`;" 2>&1
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 999d993 and 554e97b.

📒 Files selected for processing (1)
  • infra/mariadb/templates/init-database-with-user.yaml (2 hunks)
🔇 Additional comments (2)
infra/mariadb/templates/init-database-with-user.yaml (2)

62-62: Good: set -e ensures init container fails on first error.

Adding set -e at the start of the init container script ensures the job fails fast if configuration loading fails (e.g., missing configmap/secret). This is a solid improvement for robustness.


101-103: The -v flag is valid and supported in the mariadb CLI.

According to official MariaDB documentation, -v / --verbose is a supported flag for the mariadb command-line client. It is repeatable (e.g., -vvv) to increase verbosity. The single -v usage in lines 101-103 is correct and appropriate for improving visibility during command execution.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
infra/mariadb/templates/init-database-with-user.yaml (1)

1-9: Remove profanity from comments.

The comment block uses language that is unprofessional for production code. Replace with neutral, descriptive text explaining the three-step pattern (ServiceAccount, RBAC RoleBinding, Job iteration).

Suggested replacement:

# =========================================
# Database and User Initialization
# =========================================
#
# 1. Uses the init-apps ServiceAccount
# 2. RoleBinding permits it to read ConfigMaps and Secrets in faf-apps namespace
# 3. Iterates over databasesAndUsers list and creates a Job for each database
#    a) initContainer: Load the ConfigMap and Secret into environment variables.
#       Cross-namespace access via kubectl requires explicit RBAC binding.
#    b) main container: Load env from file and create the database and user
infra/postgres/templates/statefulset.yaml (1)

36-36: Minor: Inconsistent template syntax for value access.

Line 36 uses $.Values.image.repository while line 20 uses index .Values "upgrade-image" "repository". Both are valid, but prefer consistent syntax within the same template. Use dot notation throughout if all values are top-level or nested.

infra/postgres/templates/init-database-with-user.yaml (1)

1-9: Remove profanity from comments.

The comment block uses language that is unprofessional for production code. Replace with neutral, descriptive text explaining the three-step pattern, similar to the mariadb template.

Suggested replacement:

# =========================================
# Database and User Initialization
# =========================================
#
# 1. Uses the init-apps ServiceAccount
# 2. RoleBinding permits it to read ConfigMaps and Secrets in faf-apps namespace
# 3. Iterates over databasesAndUsers list and creates a Job for each database
#    a) initContainer: Load the ConfigMap and Secret into environment variables.
#       Cross-namespace access via kubectl requires explicit RBAC binding.
#    b) main container: Load env from file and create the database and user
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 554e97b and 687f865.

📒 Files selected for processing (7)
  • infra/clusterroles/templates/role-binding.yaml (1 hunks)
  • infra/clusterroles/templates/service-account.yaml (1 hunks)
  • infra/mariadb/templates/init-database-with-user.yaml (2 hunks)
  • infra/postgres/templates/init-database-with-user.yaml (1 hunks)
  • infra/postgres/templates/statefulset.yaml (2 hunks)
  • infra/postgres/values.yaml (1 hunks)
  • scripts/init-postgres.sh (0 hunks)
💤 Files with no reviewable changes (1)
  • scripts/init-postgres.sh
🧰 Additional context used
🪛 YAMLlint (1.37.1)
infra/postgres/templates/statefulset.yaml

[error] 20-20: syntax error: expected , but found ''

(syntax)

infra/postgres/templates/init-database-with-user.yaml

[error] 11-11: syntax error: expected the node content, but found '-'

(syntax)

🔇 Additional comments (11)
infra/postgres/values.yaml (1)

1-24: LGTM!

The configuration structure is correct and properly supports templated image references and database initialization. The databasesAndUsers list entries align with the new init Job template expectations.

infra/clusterroles/templates/service-account.yaml (1)

1-4: LGTM!

The ServiceAccount is correctly defined and will serve as the identity for init-apps workflows.

infra/clusterroles/templates/role-binding.yaml (1)

1-13: Verify the read-cm-secrets ClusterRole exists.

The RoleBinding correctly binds the init-apps ServiceAccount across namespaces. Ensure the referenced read-cm-secrets ClusterRole is defined elsewhere in the cluster or in this PR.

infra/mariadb/templates/init-database-with-user.yaml (1)

39-39: Good: Enhanced error handling and verbosity.

The addition of set -e in the initContainer (line 39) and -v flags to mariadb commands (lines 78-80) improve observability and fail-fast semantics during initialization, which is valuable for debugging tilt.dev local development.

Also applies to: 78-80

infra/postgres/templates/statefulset.yaml (3)

20-20: YAMLlint false positive on Helm template syntax.

The reported syntax error is a false positive. The Helm {{ index ... }} notation is valid for nested value access. YAMLlint does not properly parse Helm templating directives; this file will validate correctly when processed by Helm.


25-29: Verify the postgres secret with key POSTGRES_PASSWORD exists.

The shift to sourcing POSTGRES_PASSWORD from a secret (instead of a ConfigMap or inline value) is more secure. Ensure the secret postgres with key POSTGRES_PASSWORD is created before the StatefulSet is deployed, or is managed by a secret operator (e.g., Infisical).


47-50: Good: startup probe improves readiness detection.

The startupProbe with pg_isready and a 5-second period is appropriate for PostgreSQL. This ensures the StatefulSet does not route traffic before the database is ready, which is beneficial for local development with tilt.

infra/postgres/templates/init-database-with-user.yaml (4)

11-11: YAMLlint false positive on Helm template syntax.

The reported syntax error is a false positive. The Helm {{- $wave := 1 }} syntax is valid for variable initialization with whitespace stripping. YAMLlint does not properly parse Helm templating; this file will validate correctly when processed by Helm.


29-29: LGTM: Proper RBAC usage and robust shell scripting.

The ServiceAccount reference, error handling (set -e), and environment variable export pattern (set -a/set +a) are correct. The idempotent database/user creation with existence checks (lines 80–95) is safe and prevents re-creation errors.

Also applies to: 39-39, 74-76


43-60: Verify image availability and secret existence.

Ensure that:

  1. The alpine/kubectl image (line 35) is available in your registry and supports the kubectl get operations for ConfigMaps and Secrets.
  2. The postgres secret with key POSTGRES_PASSWORD (referenced at line 78) exists in the cluster before the Job runs.
  3. The referenced ConfigMaps and Secrets (via {{ .configMapRef }} and {{ .secretRef }}) are created in the faf-apps namespace before the Job is applied.

The cross-namespace access pattern in the initContainer (lines 43–60) will fail silently if the RBAC RoleBinding is misconfigured or if the resources do not exist.

Also applies to: 80-95


80-80: Good: SQL injection protection via identifier quoting.

The use of double-quoted identifiers in psql commands (e.g., CREATE USER \"${SYNC_USERNAME}\") prevents SQL injection attacks when variable values contain special characters. This is a security best practice.

Also applies to: 87-87, 90-90, 94-94

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 687f865 and 94b23fe.

📒 Files selected for processing (8)
  • apps/rabbitmq/templates/config.yaml (1 hunks)
  • apps/rabbitmq/templates/init-rabbitmq-with-user.yaml (1 hunks)
  • apps/rabbitmq/templates/statefulset.yaml (1 hunks)
  • apps/rabbitmq/values.yaml (1 hunks)
  • infra/clusterroles/templates/role-binding.yaml (1 hunks)
  • infra/clusterroles/templates/service-account.yaml (1 hunks)
  • infra/mariadb/templates/init-database-with-user.yaml (2 hunks)
  • scripts/init-rabbitmq.sh (0 hunks)
💤 Files with no reviewable changes (1)
  • scripts/init-rabbitmq.sh
✅ Files skipped from review due to trivial changes (1)
  • apps/rabbitmq/templates/config.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
  • apps/rabbitmq/templates/statefulset.yaml
  • infra/clusterroles/templates/service-account.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
apps/rabbitmq/templates/init-rabbitmq-with-user.yaml

[error] 11-11: syntax error: expected the node content, but found '-'

(syntax)

🔇 Additional comments (8)
infra/mariadb/templates/init-database-with-user.yaml (2)

39-39: Good addition for fail-fast error handling.

Adding set -e ensures the initContainer fails immediately if any kubectl command fails, preventing invalid configuration from being written. This improves error visibility and prevents cascading failures in the main container.


78-80: LGTM on removing stderr redirection.

Removing 2>&1 allows stderr and stdout to flow naturally to their respective streams. Kubernetes will capture both streams in the container logs, and this change doesn't affect functionality.

infra/clusterroles/templates/role-binding.yaml (1)

1-29: RBAC dependencies are correctly defined and referenced.

The ClusterRole read-cm-secrets is defined in infra/clusterroles/templates/clusterrole-read-cm-secrets.yaml with appropriate rules for configmaps and secrets. The ServiceAccount init-apps exists in both the faf-infra and faf-apps namespaces in infra/clusterroles/templates/service-account.yaml. Both RoleBindings correctly reference these resources.

apps/rabbitmq/values.yaml (1)

8-28: LGTM!

The users list is well-structured with appropriate references to ConfigMaps and Secrets for each service. The key names align with the consuming template.

apps/rabbitmq/templates/init-rabbitmq-with-user.yaml (4)

11-24: Job metadata looks good; static analysis error is a false positive.

The YAMLlint error on line 11 can be safely ignored—it doesn't understand Helm template syntax. The ArgoCD hook configuration and wave-based sync are appropriate for sequential user provisioning.

Note: The wave-based naming means Job names will change if the users list is reordered, which could cause ArgoCD to recreate Jobs. This is acceptable for PostSync hooks with HookSucceeded delete policy.


33-58: InitContainer pattern is sound.

The initContainer correctly fetches credentials from the faf-apps namespace and prepares them for the main container. The use of base64 decode for secrets and file-based environment passing is appropriate for this Job pattern.


91-96: LGTM!

The volume mount configuration and restartPolicy: Never are appropriate for a Job. The wave increment ensures sequential execution of user provisioning.


25-32: The RBAC configuration is already properly defined in the codebase. The init-apps ServiceAccount exists in the faf-apps namespace with a RoleBinding granting access to the read-cm-secrets ClusterRole, which permits get and list operations on ConfigMaps and Secrets—exactly matching what the initContainer requires.

Comment on lines +1 to +10
# =======================================
# Jesus, what the fuck is happening here?
# =======================================
#
# 1. Create a service account
# 2. Permit it to read configmaps and secrets in the faf-apps namespace
# 3. Iterate over the databasesAndUsers list and create a job for each database
# a) initContainer: Load the configmap and secret into environment variables. This must happen via k8s api, as we can't directly reference cm/secrets cross-namespace.
# b) actual container: Load the env from file and create the database and user

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Replace unprofessional language in comments.

The comment on Line 2 contains profanity that is inappropriate for production code. Replace with professional language.

Apply this diff:

-# Jesus, what the fuck is happening here?
+# Complex cross-namespace RabbitMQ user provisioning pattern
🤖 Prompt for AI Agents
In apps/rabbitmq/templates/init-rabbitmq-with-user.yaml around lines 1 to 10,
the top comment contains profanity; remove the unprofessional language and
replace it with a concise, professional header that describes the file’s purpose
(e.g., "Initialization steps for RabbitMQ users and databases" or similar), then
ensure the numbered bullet list remains clear and formal; simply update the
comment text to be professional and descriptive without changing the functional
content below.

Comment on lines +70 to +90
- |
set -a
. /config/env
set +a
vhost="/faf-core"
if rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD list vhosts | grep -q "$vhost"; then
echo "$vhost vhost alerady exists"
else
rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD declare vhost name="$vhost"
fi
if rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD list users | grep -q "$SYNC_USERNAME"; then
echo "$SYNC_USERNAME user alerady exists"
else
rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD declare user name="$SYNC_USERNAME" password="$SYNC_PASSWORD" tags=""
fi
rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD declare permission vhost="$vhost" user="$SYNC_USERNAME" configure=".*" read=".*" write=".*"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix typos and consider refactoring for readability.

Two issues identified:

  1. Typo: Lines 78 and 84 contain "alerady" which should be "already"
  2. Repetitive code: The rabbitmqadmin command with --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD is repeated 5 times. Consider extracting these into a function or variable.

Apply this diff to fix the typos and improve readability:

               set -a
               . /config/env
               set +a
 
               vhost="/faf-core"
+              
+              # Helper function to reduce repetition
+              rabbitmq_admin() {
+                rabbitmqadmin --format=bash \
+                  --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local \
+                  --username=$ADMIN_USER \
+                  --password=$ADMIN_PASSWORD \
+                  "$@"
+              }
 
-              if rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD list vhosts | grep -q "$vhost"; then
-                echo "$vhost vhost alerady exists"
+              if rabbitmq_admin list vhosts | grep -q "$vhost"; then
+                echo "$vhost vhost already exists"
               else
-                rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD declare vhost name="$vhost" 
+                rabbitmq_admin declare vhost name="$vhost"
               fi
 
-              if rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD list users | grep -q "$SYNC_USERNAME"; then
-                echo "$SYNC_USERNAME user alerady exists"
+              if rabbitmq_admin list users | grep -q "$SYNC_USERNAME"; then
+                echo "$SYNC_USERNAME user already exists"
               else
-                rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD declare user name="$SYNC_USERNAME" password="$SYNC_PASSWORD" tags=""
+                rabbitmq_admin declare user name="$SYNC_USERNAME" password="$SYNC_PASSWORD" tags=""
               fi
 
-              rabbitmqadmin --format=bash --host=rabbitmq-0.rabbitmq.faf-apps.svc.cluster.local --username=$ADMIN_USER --password=$ADMIN_PASSWORD declare permission vhost="$vhost" user="$SYNC_USERNAME" configure=".*" read=".*" write=".*"
+              rabbitmq_admin declare permission vhost="$vhost" user="$SYNC_USERNAME" configure=".*" read=".*" write=".*"
🤖 Prompt for AI Agents
In apps/rabbitmq/templates/init-rabbitmq-with-user.yaml around lines 70 to 90,
correct the typos "alerady" to "already" on the two echo lines and refactor the
repeated rabbitmqadmin invocation into a single reusable base command: define a
variable (e.g., host and base_cmd) that includes the common --host, --username
and --password flags, then replace the five repeated rabbitmqadmin calls to use
"$base_cmd <subcommand...>" (or a small helper function) to improve readability
and reduce duplication while keeping the same logic for checking/declaring the
vhost, user and permissions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants