WPB-24072 mount galley.yaml into background-worker#5180
WPB-24072 mount galley.yaml into background-worker#5180battermann wants to merge 8 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Moves Galley option/key handling into wire-subsystems and updates background-worker to load and reuse galley.yaml (plus related K8s config/secrets), removing duplicated Galley-related settings from background-worker configuration.
Changes:
- Rename
Galley.Options/Galley.KeystoWire.Options.Galley/Wire.Options.Keysand update imports/usages across Galley + tests. - Update background-worker to accept/load a separate Galley config file and derive Cassandra/Postgres/federation-domain settings from it.
- Update Helm chart/templates and integration harness to mount/pass Galley config/secrets to background-worker; remove redundant background-worker values.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| services/galley/test/integration/TestSetup.hs | Switch test imports from Galley.Options to Wire.Options.Galley. |
| services/galley/test/integration/TestHelpers.hs | Switch test imports from Galley.Options to Wire.Options.Galley. |
| services/galley/test/integration/Run.hs | Switch integration runner imports to Wire.Options.Galley. |
| services/galley/test/integration/Federation.hs | Switch imports to Wire.Options.Galley. |
| services/galley/test/integration/API/Util/TeamFeature.hs | Switch imports to Wire.Options.Galley. |
| services/galley/test/integration/API/Util.hs | Switch qualified options import to Wire.Options.Galley. |
| services/galley/test/integration/API/Teams/LegalHold/Util.hs | Switch imports to Wire.Options.Galley. |
| services/galley/test/integration/API/Teams.hs | Switch imports to Wire.Options.Galley. |
| services/galley/test/integration/API/SQS.hs | Switch imports to Wire.Options.Galley. |
| services/galley/test/integration/API/MLS/Util.hs | Switch imports from Galley.Options/Keys to Wire.Options.*. |
| services/galley/test/integration/API.hs | Switch imports to Wire.Options.Galley. |
| services/galley/src/Galley/Run.hs | Switch runtime options import to Wire.Options.Galley. |
| services/galley/src/Galley/Env.hs | Switch options import to Wire.Options.Galley. |
| services/galley/src/Galley/App.hs | Switch options/keys imports to Wire.Options.*. |
| services/galley/src/Galley/API/Update.hs | Switch options import to Wire.Options.Galley. |
| services/galley/src/Galley/API/Teams/Features.hs | Switch options import to Wire.Options.Galley. |
| services/galley/src/Galley/API/Teams.hs | Switch options import to Wire.Options.Galley. |
| services/galley/src/Galley/API/Message.hs | Switch options import to Wire.Options.Galley. |
| services/galley/src/Galley/API/LegalHold/Conflicts.hs | Switch options import to Wire.Options.Galley. |
| services/galley/src/Galley/API/Internal.hs | Switch options import and update lens chain to Wire.Options.Galley. |
| services/galley/src/Galley/API/Federation/Handlers.hs | Switch options import to Wire.Options.Galley. |
| services/galley/galley.cabal | Drop Galley.Options/Keys modules and crypto deps now housed in wire-subsystems. |
| services/galley/default.nix | Align Nix deps with moved options/keys implementation. |
| services/background-worker/src/Wire/BackgroundWorker/Options.hs | Remove duplicated Galley/Postgres/federation fields from background-worker options. |
| services/background-worker/src/Wire/BackgroundWorker/Env.hs | Build env using Galley opts for Cassandra/Postgres/federation-domain. |
| services/background-worker/src/Wire/BackgroundWorker.hs | Thread Galley opts through run. |
| services/background-worker/exec/Main.hs | Add CLI flag and config loading for a separate Galley config file. |
| services/background-worker/default.nix | Add optparse-applicative to executable deps. |
| services/background-worker/background-worker.integration.yaml | Remove duplicated Galley/Postgres/federation settings from sample integration config. |
| services/background-worker/background-worker.cabal | Add optparse-applicative dependency for the executable. |
| libs/wire-subsystems/wire-subsystems.cabal | Add deps + expose new Wire.Options.* modules. |
| libs/wire-subsystems/src/Wire/Options/Keys.hs | Rename module to Wire.Options.Keys. |
| libs/wire-subsystems/src/Wire/Options/Galley.hs | Rename module to Wire.Options.Galley and depend on Wire.Options.Keys. |
| libs/wire-subsystems/default.nix | Add deps for the moved keys/options modules. |
| libs/types-common/src/Util/Options.hs | Factor out decodeConfigFile helper and reuse it from getOptions. |
| integration/test/Testlib/ModService.hs | Start background-worker with both its config and a generated Galley config; clean up multiple temp paths. |
| hack/helmfile.yaml.gotmpl | Remove background-worker federationDomain value injection (now sourced from Galley). |
| hack/helm_vars/wire-server/values.yaml.gotmpl | Remove redundant background-worker Postgres/Galley Cassandra/federationDomain values. |
| charts/wire-server/values.yaml | Remove redundant background-worker Postgres/Galley Cassandra/federationDomain values and docs. |
| charts/wire-server/templates/background-worker/deployment.yaml | Mount Galley config/secret into background-worker pod; add checksums. |
| charts/wire-server/templates/background-worker/configmap.yaml | Remove Galley/Postgres/federationDomain config emission; adjust RabbitMQ CA cert path logic. |
| charts/wire-server/templates/background-worker/cassandra-secret.yaml | Remove background-worker Galley Cassandra secret (now sourced from Galley). |
| charts/wire-server/templates/_helpers.tpl | Remove background-worker-specific Galley TLS helper; add galley.tlsSecretRef. |
| changelog.d/0-release-notes/WPB-24072 | Release note describing the config unification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1 @@ | |||
| `background-worker` now reuses `galley`'s configmap and secrets for cassandra, postgres and federation domain settings. This removes redundant settings and keeps the two services aligned. No operator action is strictly required, however we advice to remove the value overrides of the `background-worker` for galley's cassandra, postgres, and the federation domain as they are duplicate an no longer needed. | |||
There was a problem hiding this comment.
The release note contains a few typos/grammar issues (e.g. “we advice” / “duplicate an”). Please correct to improve clarity for operators.
| `background-worker` now reuses `galley`'s configmap and secrets for cassandra, postgres and federation domain settings. This removes redundant settings and keeps the two services aligned. No operator action is strictly required, however we advice to remove the value overrides of the `background-worker` for galley's cassandra, postgres, and the federation domain as they are duplicate an no longer needed. | |
| `background-worker` now reuses `galley`'s configmap and secrets for cassandra, postgres and federation domain settings. This removes redundant settings and keeps the two services aligned. No operator action is strictly required; however, we advise removing the `background-worker` value overrides for galley's cassandra, postgres, and federation domain settings, as they are duplicated and no longer needed. |
| {{- if $.Values.galley.config.rabbitmq.tlsCaSecretRef }} | ||
| caCert: /etc/wire/galley/rabbitmq-ca/{{ $.Values.galley.config.rabbitmq.tlsCaSecretRef.key }} |
There was a problem hiding this comment.
caCert is now keyed off $.Values.galley.config.rabbitmq.tlsCaSecretRef, which ignores background-worker.config.rabbitmq.tlsCaSecretRef. This breaks setups where background-worker has its own RabbitMQ TLS CA configured (or where only background-worker uses TLS). Consider switching the condition/key back to the background-worker rabbitmq config, or explicitly documenting/removing support for background-worker-specific RabbitMQ TLS CA overrides.
| {{- if $.Values.galley.config.rabbitmq.tlsCaSecretRef }} | |
| caCert: /etc/wire/galley/rabbitmq-ca/{{ $.Values.galley.config.rabbitmq.tlsCaSecretRef.key }} | |
| {{- if .tlsCaSecretRef }} | |
| caCert: /etc/wire/galley/rabbitmq-ca/{{ .tlsCaSecretRef.key }} |
| {{- if .Values.galley.config.rabbitmq.tlsCaSecretRef }} | ||
| - name: "galley-rabbitmq-ca" | ||
| secret: | ||
| secretName: {{ $backgroundWorker.config.rabbitmq.tlsCaSecretRef.name }} | ||
| secretName: {{ .Values.galley.config.rabbitmq.tlsCaSecretRef.name }} | ||
| {{- end }} |
There was a problem hiding this comment.
The galley-rabbitmq-ca volume is only created when Values.galley.config.rabbitmq.tlsCaSecretRef is set. If background-worker is configured with a RabbitMQ TLS CA but galley is not, the CA secret will not be mounted and background-worker will fail to connect. Align the condition with the background-worker rabbitmq TLS CA configuration (or otherwise ensure the mount condition matches the path written into background-worker.yaml).
| secret: | ||
| secretName: "galley" |
There was a problem hiding this comment.
Mounting the entire galley Secret into the background-worker pod grants background-worker access to secrets it likely doesn't need (e.g. MLS private keys, galley RabbitMQ credentials). To follow least-privilege, consider creating a dedicated Secret (or a projected volume) containing only the shared items background-worker requires (e.g. Postgres password / Cassandra CA), and mount that instead of the full galley secret.
| secret: | |
| secretName: "galley" | |
| projected: | |
| sources: | |
| - secret: | |
| name: "galley" | |
| items: | |
| - key: "postgres-password" | |
| path: "postgres-password" |
| exists <- doesFileExist path | ||
| case (exists, mOpts) of | ||
| -- config file exists, take options from there | ||
| (True, _) -> do | ||
| decodeFileEither path >>= \case | ||
| Left e -> | ||
| fail $ | ||
| show e | ||
| <> " while attempting to decode " | ||
| <> path | ||
| Right o -> pure o | ||
| -- config doesn't exist, take options from command line | ||
| (True, _) -> decodeConfigFile path | ||
| (False, Just opts) -> pure opts | ||
| -- no config, no parser, just fail | ||
| (False, Nothing) -> | ||
| fail $ "Config file at " <> path <> " does not exist." | ||
| (False, Nothing) -> fail $ "Config file at " <> path <> " does not exist." |
There was a problem hiding this comment.
getOptions checks doesFileExist path and then immediately calls decodeConfigFile, which performs the same existence check again. Consider removing the pre-check in getOptions (and rely on decodeConfigFile for the error message) to avoid duplication and keep the code paths consistent.
| @@ -0,0 +1 @@ | |||
| `background-worker` now reuses `galley`'s configmap and secrets for cassandra, postgres and federation domain settings. This removes redundant settings and keeps the two services aligned. No operator action is strictly required, however we advice to remove the value overrides of the `background-worker` for galley's cassandra, postgres, and the federation domain as they are duplicate an no longer needed. | |||
There was a problem hiding this comment.
It would be nice to list all the options which are removed.
akshaymankar
left a comment
There was a problem hiding this comment.
I think we should keep the flexibility of configuring the hasql pool differently for the bcakground-worker. Other comments are nits.
| (BackgroundWorker, _) -> do | ||
| config <- getConfig | ||
| galleyConf <- getGalleyConf | ||
| tempFile <- writeTempFile "/tmp" (execName <> "-" <> domain <> "-" <> ".yaml") (cs $ Yaml.encode config) | ||
| galleyConfTemp <- writeTempFile "/tmp" (configName Galley <> "-" <> domain <> "-" <> ".yaml") (cs $ Yaml.encode galleyConf) | ||
| let params = ["-c", tempFile, "--galley-config-file", galleyConfTemp] | ||
| (_, Just stdoutHdl, Just stderrHdl, ph) <- createProcess (proc exe params) {cwd = cwd, std_out = CreatePipe, std_err = CreatePipe} | ||
| let colorize = fromMaybe id (lookup execName processColors) | ||
| void $ forkIO $ logToConsoleDebug (Just stdOut) colorize prefix stdoutHdl | ||
| void $ forkIO $ logToConsoleDebug (Just stdErr) colorize prefix stderrHdl | ||
| liftIO $ writeIORef phRef (Just ph) | ||
| pure $ ServiceInstance ph [tempFile, galleyConfTemp] |
There was a problem hiding this comment.
I'm wondering if it makes sense to write the config once before these processes start and then just use those configs as needed.
Also it'd be nice to dedup this code.
| hasqlPool <- initPostgresPool opts.postgresqlPool opts.postgresql opts.postgresqlPassword | ||
| hasqlPool <- initPostgresPool galleyOpts._postgresqlPool galleyOpts._postgresql galleyOpts._postgresqlPassword |
There was a problem hiding this comment.
This might be an exception that we'd want to keep as the pool requirements for the web process may need to be different from pool requirements for the background worker.
Checklist
changelog.d