feat: wire the git sync wrapping secret and a /workspace volume - #1213
Conversation
aivong-openhands
left a comment
aivong-openhands
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable
Both gaps this PR identifies are real, and the shapes it chose are right. gitSyncSecretFromSecret matches the existing serviceKeyFromSecret / automationWebhookSecretFromSecret vocabulary, the new openhands-secrets template mirrors automation-webhook-secret.yaml line for line, the umbrella mirror is present, and ungating volumes: / volumeMounts: is the correct fix rather than nesting another conditional inside a conditional. I re-verified the rendering claims and they hold:
$ helm template t charts/openhands/charts/automation --set events.enabled=true | grep -A4 '^ volumes:'
volumes:
- name: workspace
emptyDir: {}
$ helm template t charts/openhands/charts/automation --set caBundle.enabled=true # workspace first, then ca-bundle ✓
$ helm lint charts/openhands/charts/automation # 1 linted, 0 failed ✓
Three things need attention before this ships. None of them ask you to change the approach.
1. The emptyDir is unbounded, and this repo already learned that lesson
charts/openhands/charts/runtime-api/templates/cleanup-cronjob.yaml gives its scratch emptyDir a sizeLimit driven by runtimeFileArchive.tmpSizeLimit (default 10Gi). This one gets emptyDir: {}.
What lands there is git clones of consumer-supplied repositories, one directory per org ({workspace_base}/git-sync/{org_id}, per OpenHands/automation#429). The total is a function of how many orgs enable sync and how large their repos are — nothing the chart can bound by construction. The automation pods declare memory and cpu limits but no ephemeral-storage request or limit, so an unbounded write path fills the node's ephemeral storage and the kubelet evicts the pod. That takes down the whole automation API, not just git sync. Inline comment below.
2. /workspace is hardcoded, decoupled from the value that actually decides it
The mount path is a literal /workspace in both templates, while the directory the service writes to is AUTOMATION_WORKSPACE_BASE — which this chart never sets, and which a consumer can set through the .Values.env passthrough that automation.env explicitly supports. Set it and the mount no longer covers the write path; the failure is silent until a clone lands on the container filesystem. Inline comment below.
3. Self-hosted gets a secret that reads as configured and isn't
charts/openhands-secrets now renders automation-git-sync-secret with git-sync-secret: "" whenever automation.enabled:
$ helm template s charts/openhands-secrets --set automation.enabled=true
...
name: automation-git-sync-secret
data:
git-sync-secret: ""
The template shape is right — that is exactly what automation-service-key.yaml and automation-webhook-secret.yaml do. The gap is that replicated/config.yaml, replicated/secrets.yaml, and the secretsChecksum list in replicated/openhands.yaml were not extended alongside it. So a Replicated install with automations on gets AUTOMATION_GIT_SYNC_SECRET="", which is falsy: the service falls through git_sync_secret → kv_secret (also unset by this chart) → is_local_mode is false → refuses with a 503 naming an env var the operator has no config option for.
Guideline 7 says defaults serve the self-hosted user. The description lists this as an ops follow-up, but unlike the SOPS secrets — which genuinely live in another repo — this is three edits in this repo, in the same three files the webhook secret already touches, and the established value: '{{repl RandomString 32}}' pattern generates-once-and-persists, which is exactly the semantics a wrapping key needs. Landing it here keeps the self-hosted default coherent.
[TESTING GAPS]
charts/openhands/charts/runtime-api/tests/cleanup_archive_volumes_test.yaml is a regression test for precisely the bug class this PR touches: a volume list item emitted without its volumes: key, silently merged into imagePullSecrets, yielding an invalid Pod spec that took out the staging reaper. That test exists because someone paid for it.
This PR ungates volumes: and volumeMounts: on two deployments and adds no test. The helm unittest … 14 tests passed in the description is the three pre-existing suites (events_pdb, events_topology, storage_env) — none of them assert anything about volumes, so that number is unchanged by this PR and proves nothing about it. A short suite asserting, on both deployment.yaml and events-deployment.yaml, that spec.template.spec.volumes[0].name == workspace with caBundle.enabled=false, that the mount is a real volumeMounts entry and not an env entry, and that both volumes render in order with caBundle.enabled=true, would lock this down.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Pod-spec surgery on both deployments of a live service, in the same template region that caused a prior staging outage, with no regression test. Mitigating factors are real: rendering is verified, optional: true means a missing secret cannot block pod startup, the values changes are purely additive so existing values files keep working, and automation.enabled defaults to false. The residual risk is operational rather than structural — unbounded node-local scratch on a data-bearing path, and a self-hosted configuration that looks wired and isn't.
VERDICT:
❌ Needs rework — the design is sound and I would approve it with items 1 and 3 addressed; the test is what stops the next volumes regression.
KEY INSIGHT:
The PR is right that this workspace data is disposable, but disposable bounds durability, not size — and the one chart in this repo that already mounts consumer-sized scratch into a pod caps it for exactly that reason.
This review was generated by an AI agent (OpenHands) on behalf of @enyst, running .agents/skills/code-review.md from main.
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing (e.g., "Security concerns about X do not apply here because Y"). See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
…he Replicated secret Address review feedback on the git sync chart change. - Cap the workspace emptyDir with a values-driven sizeLimit (10Gi by default, matching the runtime-api scratch volume). Its contents are clones of consumer repositories, one directory per organization, so an unbounded volume could exhaust the node's ephemeral storage and evict the pod. - Make the chart own the directory the service writes to: a single workspace.mountPath value renders AUTOMATION_WORKSPACE_BASE and the mount on both deployments, so the write path and the mount cannot drift apart. - Give Replicated installs a real wrapping key: add the generated-once automation_git_sync_secret config option, pass it through to the secrets chart, and include it in secretsChecksum so a rotation restarts the pods. Without it a self-hosted install rendered an empty secret that the service refused with a 503 naming an env var the operator could not configure. - Add a helm-unittest suite asserting, on both deployments, that the workspace volume and mount render as real volumes/volumeMounts entries with the cap, alongside the CA bundle when it is enabled, and that the env var, mount and cap follow values.workspace together.
|
Hello @aivong-openhands, I have updated the code based on your feedback. Thank you very much for taking the time to review and provide your valuable feedback! 🙏 |
aivong-openhands
left a comment
aivong-openhands
left a comment
There was a problem hiding this comment.
🟢 Taste Rating: Good taste — the change follows established Helm chart patterns, keeps the workspace env var and mount sourced from one values block, preserves override-friendly env de-duplication, and adds targeted helm-unittest coverage for the rendering regression shape.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This touches production deployment wiring, secret management, and Replicated packaging for automation git sync. The implementation is additive and simple, the new secret reference is optional for rollout sequencing, there are no dependency changes, and the PR checks cover Helm lint/render/unit-test paths.
VERDICT:
✅ Worth merging: Core chart logic is sound; I found no important issues to block approval.
KEY INSIGHT:
The important design choice is tying AUTOMATION_WORKSPACE_BASE and the emptyDir mount to the same workspace.mountPath value, which prevents runtime path drift while keeping the configuration consumer-friendly.
This review was generated by an AI agent (OpenHands) on behalf of the requester.
openhands-release-bot
Bot
added
the
released: openhands/0.70.0
|
🚀 Released in openhands/0.70.0. |
2 participants
Description
The automation service now syncs automations to git per organization (OpenHands/automation, OHE-3153). Two deployment gaps kept that from working in the cloud; this PR closes them (OHE-3230).
charts/openhands/charts/automation/templates/_env.yamlsetsAUTOMATION_GIT_SYNC_SECRETfrom.Values.gitSyncSecretFromSecret(automation-git-sync-secret/git-sync-secret) withoptional: true, so pods start before ops provisions the secret. Until it exists the service returns 503 fromPUT /v1/git-sync/confignaming the env var rather than falling back to a per-pod key file that the other replicas could not read. The value is added to the subchartvalues.yamland mirrored in the umbrellacharts/openhands/values.yaml.deployment.yamlandevents-deployment.yamlnow always declarevolumes/volumeMountswith anemptyDirnamedworkspacemounted at/workspace(the service'sAUTOMATION_WORKSPACE_BASEdefault, which the image does not create). The CA-bundle volume and mount stay conditional oncaBundle.enabled. Pods keep no state across restarts; a fresh pod re-clones on its first cycle.charts/openhands-secrets/templates/automation-git-sync-secret.yamlrenderingautomation-git-sync-secretfromconfig.automation_git_sync_secret, mirroring the webhook secret.Verification:
Helm Chart Checklist
Additional Notes
automation-git-sync-secret(keygit-sync-secret) ininfra/k8s/production/secrets/openhands/andsaas-deploy/secrets/staging/…and add it to the secret-generator lists; addgitSyncSecretFromSecrettosaas-deploy/app/automation/environments/*/values.yaml; add the option to the Replicated self-hosted config.