Copilot commented on code in PR #142:
URL:
https://github.com/apache/skywalking-infra-e2e/pull/142#discussion_r3004870425
##########
internal/components/collector/kind.go:
##########
@@ -151,6 +158,48 @@ func collectPodDescribe(kubeConfigPath, outputDir,
namespace, podName string) er
return nil
}
+// containsGlob reports whether the path contains glob metacharacters.
+func containsGlob(path string) bool {
+ return strings.ContainsAny(path, "*?[")
+}
+
+// expandPodGlob expands a glob pattern inside a pod. If the path has no glob
+// characters it is returned as-is. Otherwise kubectl exec runs sh to expand
+// the pattern and returns the matched paths.
+func expandPodGlob(kubeConfigPath, namespace, podName, container, pattern
string) ([]string, error) {
+ if !containsGlob(pattern) {
+ return []string{pattern}, nil
+ }
+
+ cmd := fmt.Sprintf("kubectl --kubeconfig %s -n %s exec %s",
kubeConfigPath, namespace, podName)
+ if container != "" {
+ cmd += fmt.Sprintf(" -c %s", container)
+ }
+ cmd += fmt.Sprintf(" -- sh -c 'ls -d %s 2>/dev/null'", pattern)
Review Comment:
`expandPodGlob` builds a shell command by interpolating `kubeConfigPath`,
`namespace`, `podName`, `container`, and especially `pattern` directly into a
string that is executed via `util.ExecuteCommand` (which runs `bash -ec`). If
`pattern` contains a single quote (or other shell-breaking characters), it can
break out of the quoted `sh -c '...'` and execute arbitrary commands on the
host running the collector. Please avoid string-concatenated shell commands
here (e.g., build an `exec.Command` with args), or strictly validate/escape the
interpolated values so they cannot terminate quoting or inject additional shell
syntax.
##########
internal/components/collector/kind.go:
##########
@@ -151,6 +158,48 @@ func collectPodDescribe(kubeConfigPath, outputDir,
namespace, podName string) er
return nil
}
+// containsGlob reports whether the path contains glob metacharacters.
+func containsGlob(path string) bool {
+ return strings.ContainsAny(path, "*?[")
+}
+
+// expandPodGlob expands a glob pattern inside a pod. If the path has no glob
+// characters it is returned as-is. Otherwise kubectl exec runs sh to expand
+// the pattern and returns the matched paths.
+func expandPodGlob(kubeConfigPath, namespace, podName, container, pattern
string) ([]string, error) {
+ if !containsGlob(pattern) {
+ return []string{pattern}, nil
+ }
+
+ cmd := fmt.Sprintf("kubectl --kubeconfig %s -n %s exec %s",
kubeConfigPath, namespace, podName)
+ if container != "" {
+ cmd += fmt.Sprintf(" -c %s", container)
+ }
+ cmd += fmt.Sprintf(" -- sh -c 'ls -d %s 2>/dev/null'", pattern)
+
+ stdout, stderr, err := util.ExecuteCommand(cmd)
+ if err != nil {
+ logger.Log.Warnf("failed to expand glob %s in pod %s/%s: %v,
stderr: %s", pattern, namespace, podName, err, stderr)
+ return nil, fmt.Errorf("glob expansion failed for %s: %v",
pattern, err)
Review Comment:
The `sh -c 'ls -d %s 2>/dev/null'` approach will exit non-zero when the glob
matches nothing (bash/sh leaves unmatched globs unexpanded, so `ls` fails). In
that case this function returns "glob expansion failed" and never reaches the
`len(paths)==0` branch intended for the "matched no files" case. Consider
changing the in-container script to return success and emit only existing
matches (e.g., a POSIX `for` loop with `test -e` and `printf`, or `ls ... ||
true` + `--`), then rely on empty stdout to signal no matches.
##########
internal/components/collector/kind.go:
##########
@@ -151,6 +158,48 @@ func collectPodDescribe(kubeConfigPath, outputDir,
namespace, podName string) er
return nil
}
+// containsGlob reports whether the path contains glob metacharacters.
+func containsGlob(path string) bool {
+ return strings.ContainsAny(path, "*?[")
+}
+
+// expandPodGlob expands a glob pattern inside a pod. If the path has no glob
+// characters it is returned as-is. Otherwise kubectl exec runs sh to expand
+// the pattern and returns the matched paths.
+func expandPodGlob(kubeConfigPath, namespace, podName, container, pattern
string) ([]string, error) {
+ if !containsGlob(pattern) {
+ return []string{pattern}, nil
+ }
+
+ cmd := fmt.Sprintf("kubectl --kubeconfig %s -n %s exec %s",
kubeConfigPath, namespace, podName)
+ if container != "" {
+ cmd += fmt.Sprintf(" -c %s", container)
+ }
+ cmd += fmt.Sprintf(" -- sh -c 'ls -d %s 2>/dev/null'", pattern)
+
Review Comment:
New glob expansion behavior (command construction + parsing of stdout into
paths + handling of "no matches" vs command failure) is not covered by unit
tests—current tests only exercise the no-glob passthrough. To make this
testable, consider abstracting the command execution behind a small
interface/function variable so tests can stub `util.ExecuteCommand` and
validate parsing and error mapping deterministically.
##########
internal/components/collector/compose.go:
##########
@@ -123,6 +130,37 @@ func collectContainerInspect(outputDir, service,
containerID string) error {
return nil
}
+// expandContainerGlob expands a glob pattern inside a Docker container.
+// If the path has no glob characters it is returned as-is.
+func expandContainerGlob(containerID, service, pattern string) ([]string,
error) {
+ if !containsGlob(pattern) {
+ return []string{pattern}, nil
+ }
+
+ cmd := fmt.Sprintf("docker exec %s sh -c 'ls -d %s 2>/dev/null'",
containerID, pattern)
+ stdout, stderr, err := util.ExecuteCommand(cmd)
+ if err != nil {
+ logger.Log.Warnf("failed to expand glob %s in service %s: %v,
stderr: %s", pattern, service, err, stderr)
+ return nil, fmt.Errorf("glob expansion failed for %s: %v",
pattern, err)
+ }
+
+ var paths []string
+ for _, line := range strings.Split(strings.TrimSpace(stdout), "\n") {
+ line = strings.TrimSpace(line)
+ if line != "" {
+ paths = append(paths, line)
+ }
+ }
+
+ if len(paths) == 0 {
+ logger.Log.Warnf("glob %s matched no files in service %s",
pattern, service)
+ return nil, fmt.Errorf("glob %s matched no files", pattern)
+ }
Review Comment:
Like `expandPodGlob`, this uses `ls -d <pattern>`; when the glob matches
nothing, `ls` exits non-zero and you return "glob expansion failed" rather than
the intended "matched no files" error. Adjust the in-container script to
succeed with empty output on no matches (and consider adding `--` to prevent
patterns starting with `-` being treated as flags), then treat empty stdout as
"no matches".
##########
internal/components/collector/compose.go:
##########
@@ -123,6 +130,37 @@ func collectContainerInspect(outputDir, service,
containerID string) error {
return nil
}
+// expandContainerGlob expands a glob pattern inside a Docker container.
+// If the path has no glob characters it is returned as-is.
+func expandContainerGlob(containerID, service, pattern string) ([]string,
error) {
+ if !containsGlob(pattern) {
+ return []string{pattern}, nil
+ }
+
+ cmd := fmt.Sprintf("docker exec %s sh -c 'ls -d %s 2>/dev/null'",
containerID, pattern)
+ stdout, stderr, err := util.ExecuteCommand(cmd)
+ if err != nil {
+ logger.Log.Warnf("failed to expand glob %s in service %s: %v,
stderr: %s", pattern, service, err, stderr)
+ return nil, fmt.Errorf("glob expansion failed for %s: %v",
pattern, err)
+ }
Review Comment:
`expandContainerGlob` interpolates `pattern` directly into a string executed
via `util.ExecuteCommand` (`bash -ec`). A pattern containing a single quote can
break out of the quoted `sh -c '...'` and inject additional host-side shell
commands. Please avoid building this as a shell string (prefer `exec.Command`
with args) or strictly validate/escape `pattern` (and any other interpolated
values) so it cannot terminate quoting or introduce shell metacharacters.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
