Skip to content

chore: Integrate k8s setup#110

Merged
alexandreferris merged 2 commits into
mainfrom
chore/integrate_k8s_setup
Jun 24, 2026
Merged

chore: Integrate k8s setup#110
alexandreferris merged 2 commits into
mainfrom
chore/integrate_k8s_setup

Conversation

@alexandreferris

Copy link
Copy Markdown
Member

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?

Solutions

Add values and helper files

* Add values and helper files
@alexandreferris alexandreferris requested a review from a team as a code owner June 24, 2026 11:17
@alexandreferris alexandreferris self-assigned this Jun 24, 2026
@claude

claude Bot commented Jun 24, 2026

Copy link
Copy Markdown

PR Review: chore: Integrate k8s setup

Thanks for adding the Helm chart! 🎉 The structure follows the standard helm create layout closely and the probes/labels/helpers are well done. Most of my feedback comes from cross-checking the chart against what the application actually reads at runtime (util/EnvironmentVariables.kt, Routing.kt, docker-compose.yml) — there are a few mismatches that would break a real deploy.

🔴 Blocking — env var name mismatches (app will not start / misbehave)

  1. Crypto password secret key is wrong → startup crash.
    templates/statefulset.yaml injects WIRE_SDK_CRYPTO_STORAGE_PASSWORD, but the app reads WIRE_SDK_CRYPTOGRAPHY_STORAGE_PASSWORD:

    // util/EnvironmentVariables.kt
    val ENV_VAR_CRYPTOGRAPHY_STORAGE_PASSWORD: String =
        System.getenv("WIRE_SDK_CRYPTOGRAPHY_STORAGE_PASSWORD")

    This getenv has no default, so a wrong/missing name returns null and the non-null Kotlin String will throw at boot. docker-compose.yml already uses the ...CRYPTOGRAPHY... spelling. Please rename both the env var name and the secretKeyRef.key in the statefulset (and the secret) to WIRE_SDK_CRYPTOGRAPHY_STORAGE_PASSWORD.

  2. WIRE_SDK_APPLICATION_ID is never read.
    values.yaml sets WIRE_SDK_APPLICATION_ID, but the app reads WIRE_SDK_APP_ID (see ENV_VAR_APPLICATION_ID). Because that one does have a default (UUID.randomUUID()), it won't crash — it will silently start with a random application identity, which is arguably worse than crashing. Rename to WIRE_SDK_APP_ID.

  3. Listen port vs. probe/service port are decoupled.
    The app binds to GHAPP_SERVER_PORT (ENV_VAR_PORT), but the container/service/probes are hardcoded to 8080 via service.targetPort. Meanwhile values.yaml ships GHAPP_SERVER_PORT: "" and an unused PORT: "8080". Two problems:

    • "".toInt() throws NumberFormatException at startup if the empty default is ever used.
    • The app could listen on a port the Service/probes don't target.
      Suggest driving the env var from service.targetPort (e.g. set GHAPP_SERVER_PORT to {{ .Values.service.targetPort }}) and dropping the unused PORT var.

🟠 Functional issues

  1. ServiceMonitor scrapes /metrics, which doesn't exist. Routing.kt only exposes GET /health and the webhook POST route — there's no metrics endpoint (no Micrometer/Prometheus registry in the codebase). The ServiceMonitor will scrape 404s. Either add a metrics endpoint to the app first, or drop servicemonitor.yaml until metrics exist. Also consider gating it behind a serviceMonitor.enabled value, since the monitoring.coreos.com/v1 CRD isn't present on every cluster.

  2. Empty ConfigMap. templates/configmap.yaml renders only metadata with no data:. Nothing references it (env comes straight from values.yaml into the StatefulSet), and NOTES.txt even tells users to inspect it. Either populate it and envFrom it, or remove it.

  3. persistence.enabled is ignored. pvc.yaml and the StatefulSet's data volume/mount are always rendered regardless of .Values.persistence.enabled. So persistence.enabled: false has no effect (yet NOTES.txt branches on it). Wrap the PVC and the volume/volumeMount in {{- if .Values.persistence.enabled }}.

  4. serviceAccount.create is ignored. serviceaccount.yaml always renders even when create: false. Wrap it in {{- if .Values.serviceAccount.create }}. Also serviceAccount.annotations in values.yaml is never emitted into the SA metadata.annotations.

  5. autoscaling block is dead config. There's no hpa.yaml, so the autoscaling.* values do nothing. Note an HPA on a StatefulSet backed by a single RWO PVC wouldn't scale anyway (see chore: Add docker #WPB-18182 #10) — either remove the block or add the template intentionally.

🟡 StatefulSet / storage design

  1. kubectl logs deployment/... in NOTES.txt is wrong — the workload is a StatefulSet, so that command fails. Use statefulset/{{ include "githubapp.fullname" . }} (or a label-selector based logs command).

  2. External PVC instead of volumeClaimTemplates. A StatefulSet normally manages per-replica storage via volumeClaimTemplates. Here a single shared RWO PVC is mounted, so replicaCount > 1 can never schedule (all pods contend for one RWO volume). At replicaCount: 1 it works, but if multi-replica is ever intended, switch to volumeClaimTemplates. If single-replica is the design, a Deployment may be the simpler fit.

  3. Verify the mount path matches where the SDK writes. data mounts at /opt/githubapp/storage, but the container WORKDIR is /opt/github-app (note the differing name) and the Wire SDK chooses its own crypto-DB directory. If the SDK writes elsewhere (e.g. relative to CWD), the crypto state won't actually land on the PV and would be lost on restart — which the code comment warns is unrecoverable. Worth confirming the SDK's storage directory and pointing the volume there.

🔐 Security

  1. podSecurityContext / securityContext are empty. Given the app handles API tokens and a crypto storage password, consider hardening defaults: runAsNonRoot: true, a non-root runAsUser/fsGroup (the fsGroup also fixes PV write permissions for a non-root user), readOnlyRootFilesystem, allowPrivilegeEscalation: false, and capabilities: { drop: [ALL] }.

  2. Secrets volume mount appears unused. Secrets are consumed via secretKeyRef env vars (the app reads them through System.getenv), yet they're also mounted at /etc/secrets. Nothing reads files there — drop the mount unless something needs the file form.

🧪 Tests / validation

  • No CI step runs helm lint / helm template on the chart. A quick helm lint && helm template with a sample values file in CI would have caught the empty ConfigMap and the unhonored toggles. Consider adding helm template ... | kubeconform to the pipeline.
  • A values.schema.json would catch the empty-string GHAPP_SERVER_PORT and required secrets before they reach a cluster.

Nits

  • image.tag isn't set in values.yaml (falls back to Chart.AppVersion: "0.0.1") — fine if the ArgoCD ImageUpdater overrides it, as DEPLOYMENT.md describes; otherwise everything pins to 0.0.1.
  • storageClass: "gp3-automode-nodepool" is an AWS-specific default baked into the base chart. Per DEPLOYMENT.md, env-specific values live in the argocd repo, so consider leaving this "" (cluster default) in the chart.

Overall a solid first pass — items #1#3 are the must-fixes before this can deploy successfully. Happy to help with any of the template tweaks. 👍

* update values.yaml and statefulset.yaml based on Claude review
@alexandreferris alexandreferris merged commit 5e108f2 into main Jun 24, 2026
3 checks passed
@alexandreferris alexandreferris deleted the chore/integrate_k8s_setup branch June 24, 2026 11:38
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