fix(alluxio): build the metadata query command as an argv vector - #6146
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6146 +/- ##
==========================================
+ Coverage 65.10% 65.13% +0.02%
==========================================
Files 485 485
Lines 34015 34039 +24
==========================================
+ Hits 22147 22171 +24
Misses 10127 10127
Partials 1741 1741 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
c8ee6d9 to
1bb0fb9
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes metadata-restore command execution for Alluxio (and aligns JuiceFS) by eliminating shell-wrapped sed invocations and instead constructing sed commands as plain argv vectors, allowing them to pass cmdguard validation and execute reliably in engine pods.
Changes:
- Refactor metadata query command building to return
[]string{"sed","-n",<line>,<file>}(nobash -c) and add unit tests asserting the argv shape. - Tighten behavior for unsupported metadata keys to return an error instead of attempting a fallback
sedinvocation. - Add
cmdguard.ValidateArgand apply it inParseBackupRestorePathto reject shell-metacharacter/control-character input early, with accompanying tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/databackup.go | Validates backup/restore path strings early via cmdguard.ValidateArg before parsing. |
| pkg/utils/databackup_test.go | Adds tests ensuring shell metacharacters/control chars are rejected and ordinary paths still parse. |
| pkg/utils/cmdguard/exec.go | Introduces ValidateArg with a stricter character/control-character policy for single arguments. |
| pkg/utils/cmdguard/arg_test.go | Adds unit coverage for accepted/rejected cases in ValidateArg. |
| pkg/ddc/alluxio/operations/base.go | Builds metadata query sed command as argv vector via metaDataQueryCommand; errors on unknown keys. |
| pkg/ddc/alluxio/operations/base_test.go | Updates existing tests and adds coverage for metaDataQueryCommand argv construction. |
| pkg/ddc/juicefs/operations/base.go | Mirrors Alluxio change: correct sed argv and error on unknown keys for consistency. |
| pkg/ddc/juicefs/operations/base_test.go | Updates existing tests and adds coverage for metaDataQueryCommand argv construction. |
Suppressed comments (1)
pkg/utils/cmdguard/exec.go:143
- The control-character error currently reports an "index" from a range loop, which is a byte index in Go strings. Calling this out (and quoting the arg) avoids confusion when debugging.
if r < 0x20 || r == 0x7f {
return fmt.Errorf("arg %s has illegal control character %q at index %d", arg, r, i)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The parsed path is used to build commands run inside engine pods and is rendered into | ||
| // Helm values, so reject anything a shell would act on here rather than relying on every | ||
| // consumer to quote it correctly. |
| if strings.ContainsRune(arg, illegalChar) { | ||
| return fmt.Errorf("arg %s has illegal access with illegalChar %c", arg, illegalChar) | ||
| } |
|
|
||
| // A file name carrying characters that are significant to a shell must stay in a single | ||
| // argv element, and the command must never be wrapped in a shell. | ||
| pathWithSpecialChars := "/pvc/tmp/a b'c;d/metadata.yaml" |
|
|
||
| // A file name carrying characters that are significant to a shell must stay in a single | ||
| // argv element. | ||
| pathWithSpecialChars := "/pvc/tmp/a b'c;d/metadata.yaml" |
QueryMetaDataInfoIntoFile assembled its command as a shell script:
str = "sed -n '" + line + "' " + filename
command = []string{"bash", "-c", str}
Every container exec passes through cmdguard.ValidateCommandSlice
(pkg/utils/kubeclient/exec.go:125). A `bash -c` command takes the shell-script
validation path, whose illegalSequences list contains `'`, so the command was
rejected before reaching the master pod:
unsafe shell script sed -n '3p' /host/metadata.yaml,
illegal sequence detected: '
The single quotes belong to the sed script itself, so this failed on every
call. RestoreMetadataInternal could not read the backup, and a dataset with
DataRestoreLocation set never had Status.UfsTotal or Status.FileNum restored.
Build the command as an argv vector instead. exec() passes the slice straight
to ExecCommandInContainerWithTimeoutContext, so no shell is involved: the
command now takes cmdguard's plain-argv path, the quotes are gone, and the
file name stays in its own element where it is handed to sed verbatim. A path
containing spaces now works too, where joining it into a shell string would
have word-split it into several sed operands.
The construction moves into metaDataQueryCommand so the resulting argv can be
asserted in a unit test. An unrecognized key now returns an error instead of
logging one and then running sed with an empty script, which would have
returned the whole file as the value.
Add cmdguard.ValidateArg for a single argument such as a path, and call it from
ParseBackupRestorePath. The characters it rejects are kept in cmdguard next to
the existing lists instead of being restated by each caller: argIllegalChars
extends illegalChars with the glob, brace, home-expansion, comment and quoting
characters that carry no meaning in a path, and control characters are rejected
as well. Whitespace stays allowed, since spaces occur in real paths and are
harmless once the value is its own argv element. Validating where the path is
parsed also covers the consumers that never reach ValidateCommandSlice, such as
transform.go rendering the path into Helm values for a command that runs inside
the pod. checkCommandArgs is left untouched, so the behaviour of every existing
exec, helm and CSI call site is unchanged.
The JuiceFS copy of QueryMetaDataInfoIntoFile has no production caller, but its
argv was malformed - `sed -n "'3p' /file"` makes sed treat the whole argument
as its script. It is corrected the same way to keep the two engines consistent.
Signed-off-by: cheyang <cheyang.cy@alibaba-inc.com>
1bb0fb9 to
921abbd
Compare
|
Thanks for the review — all four inline comments were valid and are addressed in the force-pushed update. 1. 2. Without 3 & 4. Test path was not reachable. Correct, and I verified it: Both tests now use the spaces-only path. This makes the assertion stronger rather than weaker: a space is the shell-significant character a restore path can actually carry, and it is exactly what the previous Also addressed the Codecov gap. The uncovered lines were the unknown-key branch of On the SonarCloud duplication (32.2% on new code): that is the two near-identical |
|
What this PR does
AlluxioFileUtils.QueryMetaDataInfoIntoFileassembled its command as a shell script:Every container exec passes through
cmdguard.ValidateCommandSlice(pkg/utils/kubeclient/exec.go:125). Abash -ccommand takes the shell-script validation path, whoseillegalSequenceslist contains', so the command was rejected before reaching the master pod:The single quotes belong to the sed script itself, so this failed on every call, not just on unusual input.
RestoreMetadataInternalcould never read the backup file, so a Dataset withDataRestoreLocationset never hadStatus.UfsTotalorStatus.FileNumrestored.The change
Build the command as an argv vector:
exec()hands the slice straight toExecCommandInContainerWithTimeoutContext, so no shell is involved. The command now takes cmdguard's plain-argv path, the quotes are gone, and the file name stays in its own element where it reaches sed verbatim.Also included:
metaDataQueryCommand, so the resulting argv is directly assertable in a unit test.sed -n '' <file>, which would have returned the whole file as the value.cmdguard.ValidateArg, for validating a single argument such as a path, called fromParseBackupRestorePath. The character lists stay incmdguardnext to the existing ones rather than being restated by each caller: it reusesillegalChars, adds the glob / brace / home-expansion / comment / quoting characters that carry no meaning in a path, and rejects control characters. Validating at the point where the path is parsed also covers the consumers that never reachValidateCommandSlice—transform.gorenders this path into Helm values for a command that runs inside the pod.checkCommandArgsis deliberately left unchanged, so the behaviour of every existing exec, helm and CSI call site is identical to before.QueryMetaDataInfoIntoFilehas no production caller, but its argv was malformed:sed -n "'3p' /file"makes sed treat the whole argument as its script. Corrected the same way to keep the two engines consistent.Testing
New unit tests are pure functions over the command builder, the new validator and the path parser — no
gomonkeypatching, so they are deterministic:TestMetaDataQueryCommand(alluxio + juicefs): exact argv per key, unknown key errors, and a file name containing shell-significant characters stays in a single argv element with no shell wrapper.TestValidateArg(cmdguard): ordinary paths accepted; one case per rejected character class, including the control characters.TestParseBackupRestorePathRejectsShellMetacharacters: rejection through the parser plus a set of ordinary paths that must keep working.go build ./...andgo vetare clean.One note for reviewers:
pkg/ddc/alluxio/operationshas a number of pre-existing test failures on darwin/arm64 caused bygomonkeypatch re-application, unrelated to this change. I compared the set of failing test names before and after the change and it is unchanged, so there is no regression from this PR — but CI on Linux is the authoritative check.