fix: Argo workflows parameters as env variables#2756
Conversation
|
kubernetes / argo init uses |
Greptile SummaryThis PR reworks Argo Workflows parameter passing from inline CLI arguments to environment variables (
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Argo as Argo Workflow Engine
participant Shell as Bash Shell (Container)
participant SetParams as set_parameters.py
participant File as Temp Export File
participant MF as Metaflow CLI (step)
Argo->>Shell: Inject workflow.parameters (base64-encoded)
Shell->>SetParams: python -m set_parameters <file> name,flag,b64val ...
SetParams->>SetParams: base64 decode + JSON parse each param
SetParams->>SetParams: Skip params where none_default=True and value="null"
SetParams->>File: Write export METAFLOW_INIT_<NAME>=<json_value>
Shell->>Shell: source temp file (. `pwd`/<file>)
Note over Shell: METAFLOW_INIT_* env vars now set
Shell->>MF: Execute bootstrap + metaflow step command
MF->>MF: Click reads METAFLOW_INIT_* for parameter values
Last reviewed commit: 4db34ef |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| f.write( | ||
| "export METAFLOW_INIT_%s=%s\n" | ||
| % ( | ||
| k.upper().replace("-", "_"), | ||
| parsed_value, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Unquoted shell export values risk expansion
The export statement writes parsed_value (a JSON-serialized string) without shell quoting. For string parameters, json.dumps wraps the value in double quotes (e.g., "hello world" becomes "hello world"), which provides some shell protection. However, shell metacharacters like $, backticks, and $(...) inside double quotes are still subject to variable expansion and command substitution when the file is sourced.
For example, a parameter value of $(whoami) would produce:
export METAFLOW_INIT_FOO="$(whoami)"
which executes the command substitution when sourced.
Consider using shlex.quote() to wrap the value in single quotes:
import shlex
f.write(
"export METAFLOW_INIT_%s=%s\n"
% (
k.upper().replace("-", "_"),
shlex.quote(parsed_value),
)
)Note: the existing Airflow set_parameters.py has the same pattern, but since this is a new file, it's worth fixing here.
There was a problem hiding this comment.
Same pattern is used in step-functions and airflow implementations so if there is an actual issue here, it needs to be fixed in all three.
Opting to go with the current implementation, as shlex.quote forces outer quotes to the value which completely breaks expectations.
| parsed_value, | ||
| ) | ||
| ) | ||
| os.chmod(output_file, 509) |
There was a problem hiding this comment.
Decimal literal obscures file permissions
Using 509 (decimal) makes it hard to understand the intended file permissions at a glance. This is 0o775 (rwxrwxr-x) in octal. Using the octal literal makes the intent immediately clear and is the standard convention for chmod calls.
| os.chmod(output_file, 509) | |
| os.chmod(output_file, 0o775) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Rework argo params handling to pass values as environment variables instead of part of the CLI. Allows overriding the param values via environment if necessary.