Enable argo hooks to inherit security context from start step - #3354
Enable argo hooks to inherit security context from start step#3354kevslinger wants to merge 1 commit into
Conversation
Greptile SummaryThis PR propagates the start step’s Kubernetes security context to Argo lifecycle hooks, error-message capture hooks, and the heartbeat daemon.
Confidence Score: 5/5The PR appears safe to merge with no actionable defects identified. The new fields use the same Kubernetes security-context construction and Argo container serialization pattern already used for ordinary step containers, while preserving the prior unset behavior when no context is configured.
|
| Filename | Overview |
|---|---|
| metaflow/plugins/argo/argo_workflows.py | Consistently adds the start step’s security context to all three generated auxiliary Argo container paths without an identified correctness issue. |
Reviews (1): Last reviewed commit: "enable argo hooks to inherit security co..." | Re-trigger Greptile
Shriprasad-P
left a comment
There was a problem hiding this comment.
The production change looks correct. These generated Argo templates already derive several Kubernetes settings from the start step, and using the same start-step security_context here is consistent with that model. V1SecurityContext(**security_context) also matches how ordinary step containers handle @kubernetes(security_context=...).
I do think one thing should be addressed before merge: this bug fix currently has no regression coverage, and the existing Argo tests do not assert securityContext for these generated templates.
A compile-to-JSON test should be enough:
- configure
security_contexton the start step - assert it appears on the lifecycle hook,
error-msg-capture-hook, andheartbeat-daemon - verify the field remains unset/null when no security context is configured
The implementation itself looks right; the missing regression test is the remaining gap.
There was a problem hiding this comment.
The production change looks correct. These three auxiliary templates already inherit start-step Kubernetes settings such as image, secrets, and service account, and ordinary step containers already convert security_context through V1SecurityContext. Propagating the same container security context to the lifecycle hook, error-capture hook, and heartbeat daemon therefore looks consistent with the existing Argo compilation model.
The blocker for me is regression coverage. There currently appears to be no test asserting securityContext on these generated templates, and Metaflow's contribution guidance requires a test for bug fixes.
Fixes #3353
Before this, generated hooks in flows on argo would have
"securityContext": null. After this change, a custom security context applied to the start step (via e.g. a@kubernetes(security_context={...})) will get picked up and included in the hook.Using the reproduce example from #3353, after this change, all 3 of
start,end, anderror-msg-capture-hookhave the same security context. A key benefit of this PR is that generated hooks will now not get blocked by security systems which expect a strict security context in all pods.