Skip to content

feat: Production k8s deploy #WPB-27713 - #134

Merged
spoonman01 merged 1 commit into
mainfrom
feat/production-k8s-deploy-WPB-27713
Aug 11, 2026
Merged

feat: Production k8s deploy #WPB-27713#134
spoonman01 merged 1 commit into
mainfrom
feat/production-k8s-deploy-WPB-27713

Conversation

@spoonman01

Copy link
Copy Markdown
Contributor

PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Missing config for prod deploy

Solutions

Add Helm setup, copying from other prod setups and the Github App staging setup for Redis


PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

* Add values-prod.yaml
* Double check how env vars are fetched from secrets for Redis and the SDK
@spoonman01
spoonman01 requested a review from a team as a code owner August 11, 2026 10:20
@spoonman01 spoonman01 self-assigned this Aug 11, 2026
@claude

claude Bot commented Aug 11, 2026

Copy link
Copy Markdown

Review: feat: Production k8s deploy #WPB-27713

Small, focused change — one new helm/githubapp/values-prod.yaml. I checked it against the chart templates and the app itself:

✅ The four env vars match exactly what src/main/kotlin/com/wire/github/util/EnvironmentVariables.kt reads (GHAPP_SERVER_PORT, GHAPP_API_HOST, WIRE_SDK_APP_ID, WIRE_SDK_API_HOST); the three secret-backed ones (WIRE_SDK_API_TOKEN, WIRE_SDK_CRYPTOGRAPHY_STORAGE_PASSWORD, GHAPP_REDIS_URL) come from the statefulset template, so nothing is missing.
GHAPP_SERVER_PORT: "8080" lines up with service.targetPort: 8080 and the http-named probe ports.
GHAPP_API_HOST matches the ingress host, which matters because that value is what gets registered as the GitHub webhook URL.
sha-6bd8f51 is a real tag produced by .github/workflows/staging.yml (type=sha,format=short) for the current main HEAD.

Below are things worth resolving or at least confirming before this actually serves prod traffic. Most are questions/ops concerns rather than defects in the YAML.


1. This contradicts helm/githubapp/DEPLOYMENT.md

DEPLOYMENT.md says:

Helm charts are here, while values for each environment are in the argocd-integrations repo.

Consistent with that, there's no values-staging.yaml in this repo. So either:

  • the ArgoCD Application doesn't point at this path and the file is inert (worst case: it looks like prod config but nothing reads it), or
  • the convention has changed, in which case DEPLOYMENT.md should be updated in this PR and it's worth asking why staging's values still live elsewhere.

Please clarify which it is — the answer also determines whether item 2 matters.

2. Pinned image tag vs. ArgoCD ImageUpdater

tag: "sha-6bd8f51" is hardcoded. CI pushes a new sha-<short> on every push to main, and that's the only tag stream — staging and prod would draw from the same set of commit builds. Three possible outcomes, all worth being explicit about:

  • ImageUpdater writes back to argocd-integrations → this pin never changes and silently drifts from reality.
  • ImageUpdater writes back to this file → expect bot commits churning it (staging.yml already carries an if: github.actor != 'github-actions[bot]' guard, which suggests write-back commits happen somewhere).
  • Neither → every prod release requires a PR here. Legitimate, but should be a stated choice.

Suggestion: add a one-line comment above tag: recording the intent, and consider whether prod should track release/semver tags rather than the same sha-* stream staging follows. Otherwise "promote to prod" and "merge to main" are only distinguished by who remembers to bump this line.

3. Prod inherits staging's secret names — please confirm namespace isolation

Not overridden here, so prod inherits from values.yaml:

secrets.secretName: "githubapp-secrets"
redis.secretName:   "githubapp-valkey-secrets"
  • The chart has no Secret/ExternalSecret template — these must already exist in the prod namespace or the pod will sit in CreateContainerConfigError. Worth adding to the PR description as a pre-deploy step.
  • Since WIRE_SDK_APP_ID differs for prod, the token and crypto password must too. Please confirm prod is a separate namespace/cluster so these identically-named secrets can't resolve to staging's Wire token or staging's Valkey instance. That Valkey holds the per-conversation webhook secrets (SignatureValidator reads them), so a cross-wire would be a real security issue, not just a config mixup.
  • The PR description mentions "the Github App staging setup for Redis", but there's no redis: block in the file — I assume that means "inherit the chart default", which is fine; just flagging that the description reads as if something was added.

4. Data durability — the crypto storage password and the PVC are load-bearing

EnvironmentVariables.kt documents WIRE_SDK_CRYPTOGRAPHY_STORAGE_PASSWORD as: "If lost or forgotten, there is no future access to the database." Combined with the inherited persistence: {size: 1Gi, storageClass: gp3-automode-nodepool}:

  • Confirm gp3-automode-nodepool exists in the prod cluster — a wrong storage class means a Pending PVC and a pod that never starts.
  • 1Gi on a StatefulSet-attached PVC is awkward to grow later. Fine if the SDK crypto DB stays small; worth a deliberate look for prod.
  • The PVC template has helm.sh/resource-policy: keep 👍, but confirm both the volume and that secret are covered by a backup policy. Losing either means re-registering the bot client.

5. No security hardening for a now-internet-facing prod service

podSecurityContext and securityContext are {} in values.yaml and aren't overridden here, so the container runs as root (the Dockerfile has no USER, and eclipse-temurin:21-jre defaults to root). For a service about to be publicly reachable, consider:

podSecurityContext:
  runAsNonRoot: true
  runAsUser: 10001
  fsGroup: 10001          # so the PVC at /opt/githubapp/storage stays writable
securityContext:
  allowPrivilegeEscalation: false
  capabilities:
    drop: [ALL]

readOnlyRootFilesystem: true would be nice too but needs a writable /tmp for the JVM — worth testing rather than assuming. If that's out of scope here, a follow-up ticket is reasonable; it's just easier to set before prod has traffic than after.

6. Ingress path: / exposes more than the webhook route

Not wrong — the webhook route is POST /{conversationId}/{conversationDomain}, so a prefix match is needed — but two consequences now that this is public:

  • /metrics doesn't exist. templates/servicemonitor.yaml scrapes port: http, path: /metrics, but there's no micrometer dependency and Routing.kt only defines /health and the webhook POST. Prod will emit continuous scrape failures/alerts. Pre-existing, but this PR is what makes it a prod problem.
  • Unauthenticated 500s. Routing.kt:53-55 does requireNotNull() on X-GitHub-Event / X-Hub-Signature / X-GitHub-Delivery, so any bare POST /a/b from the internet throws → 500 + log noise. Signature validation itself is correctly in place for real payloads, so this isn't an auth hole, just a hardening gap. Returning 400 plus an nginx rate-limit annotation (nginx.ingress.kubernetes.io/limit-rps) would be a cheap improvement.

Both are pre-existing and out of scope for a values file — worth follow-up tickets.

7. Pre-existing security follow-up worth tracking before prod (not a blocker)

SignatureValidator uses HMAC-SHA1 (X-Hub-Signature) with a plain == comparison. GitHub also sends X-Hub-Signature-256; moving to it plus a constant-time comparison (MessageDigest.isEqual) removes a timing side-channel on the per-conversation webhook secret. Fine as a separate ticket, but "we're going to prod" is the natural moment to file it.

8. Availability nit

replicaCount: 1 + ReadWriteOnce PVC + StatefulSet means every deploy has a short window with no pod. GitHub does not retry failed webhook deliveries, so redeploys can silently drop events. Probably acceptable for this service — just make sure it's a conscious tradeoff, and note a PDB won't help with a single replica.

9. Style / testing

  • Indentation differs from values.yaml, which indents sequences under their parent key (hosts: - host:); this file puts them at column 0. Both are valid YAML, but given commit 6bd8f51 was literally "Manually fix staging.yml indentation problem", matching the existing style keeps future diffs clean.
  • Nothing in CI validates the chart — no helm reference anywhere under .github/. A helm lint helm/githubapp + helm template helm/githubapp -f helm/githubapp/values-prod.yaml step in pull-request.yml is a few lines and would catch typos in a file nobody renders locally. Since a values file can't really be unit-tested, this is the closest equivalent to test coverage and I'd recommend it as part of this change.

Overall: the YAML itself is correct and consistent with the chart and the app's env contract — nothing here is broken. My main asks are (1) resolve the DEPLOYMENT.md contradiction so it's clear this file is actually used, (2) state the image-tag/ImageUpdater intent, and (3) confirm prod secret/namespace isolation. The rest can be follow-ups. Nice, tight PR otherwise 👍

@bbaarriiss bbaarriiss left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks Luca 🚀

@spoonman01
spoonman01 merged commit f1954c4 into main Aug 11, 2026
5 checks passed
@spoonman01
spoonman01 deleted the feat/production-k8s-deploy-WPB-27713 branch August 11, 2026 12:46
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.

2 participants