-
Notifications
You must be signed in to change notification settings - Fork 421
Auto-detect ARC/DinD and emit AWF --docker-host-path-prefix in generated workflows
#31614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
9d1533b
2599fa0
14d6e52
3391b4b
79fba3c
92ef155
ba3ecc9
4a67e52
8378b3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,15 @@ import ( | |
|
|
||
| var awfHelpersLog = logger.New("workflow:awf_helpers") | ||
|
|
||
| const ( | ||
| awfArcDindPrefixArgsVarName = "GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS" | ||
| // Bash regex used in [[ ... =~ ... ]] to detect localhost TCP Docker hosts. | ||
| // Keep this in bash-compatible syntax (escaped dots) because it is emitted directly | ||
| // into generated shell scripts. | ||
| awfArcDindDockerHostRegex = `^tcp://(localhost|127\.0\.0\.1)(:[0-9]+)?$` | ||
| awfArcDindHostPathPrefixFlag = "--docker-host-path-prefix /tmp/gh-aw" | ||
| ) | ||
|
|
||
| // AWFCommandConfig contains configuration for building AWF commands. | ||
| // This struct centralizes all the parameters needed to construct an AWF-wrapped command. | ||
| type AWFCommandConfig struct { | ||
|
|
@@ -88,6 +97,24 @@ func BuildAWFCommand(config AWFCommandConfig) string { | |
| // and --mount "${RUNNER_TEMP}/...") are appended raw below so that shell variable | ||
| // expansion is not suppressed by single-quoting. | ||
| awfArgs := BuildAWFArgs(config) | ||
| firewallConfig := getFirewallConfig(config.WorkflowData) | ||
|
|
||
| // Auto-detect ARC/DinD split daemon topology at runtime and emit | ||
| // --docker-host-path-prefix when supported by the selected AWF version. | ||
| // This avoids requiring workflow-authored sandbox.agent.args for standard ARC DinD setups. | ||
| arcDindPrefixProbe := "" | ||
| arcDindPrefixArgsRef := "" | ||
| if awfSupportsDockerHostPathPrefix(firewallConfig) { | ||
| arcDindPrefixProbe = fmt.Sprintf(`%s="" | ||
| if [[ "${DOCKER_HOST:-}" =~ %s ]]; then | ||
| %s="%s" | ||
| fi`, | ||
| awfArcDindPrefixArgsVarName, | ||
| awfArcDindDockerHostRegex, | ||
| awfArcDindPrefixArgsVarName, | ||
| awfArcDindHostPathPrefixFlag) | ||
| arcDindPrefixArgsRef = fmt.Sprintf("${%s}", awfArcDindPrefixArgsVarName) | ||
| } | ||
|
Comment on lines
+115
to
+117
|
||
|
|
||
| // Build the expandable args string for args that need shell variable expansion. | ||
| // These MUST be appended as raw (unescaped) strings because single-quoting would | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/zoom-out] The Consider extracting the command assembly into a small helper that accepts the optional preamble lines as a slice and joins them, so the ARC/DinD probe (and any future injections) only needs to be added once: func assemblePipefailCommand(preambles []string, probe, awfCmd, expandableArgs, arcDindRef, awfArgs, wrapped, logFile string) string {
body := strings.Join(append(preambles, probe), "\n")
return fmt.Sprintf("set -o pipefail\n%s\n# shellcheck disable=SC1003\n%s %s %s %s \\\n -- %s 2>&1 | tee -a %s", body, awfCmd, expandableArgs, arcDindRef, awfArgs, wrapped, logFile)
}This would make future version-gated injections trivial and keep the logic in one place. |
||
|
|
@@ -166,14 +193,17 @@ func BuildAWFCommand(config AWFCommandConfig) string { | |
| %s | ||
| %s | ||
| %s | ||
| %s | ||
| # shellcheck disable=SC1003 | ||
| %s %s %s \ | ||
| %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| config.PathSetup, | ||
| preCreateLog, | ||
| configFileSetup, | ||
| arcDindPrefixProbe, | ||
| awfCommand, | ||
| expandableArgs, | ||
| arcDindPrefixArgsRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
|
|
@@ -182,39 +212,48 @@ func BuildAWFCommand(config AWFCommandConfig) string { | |
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| %s | ||
| # shellcheck disable=SC1003 | ||
| %s %s %s \ | ||
| %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| config.PathSetup, | ||
| preCreateLog, | ||
| arcDindPrefixProbe, | ||
| awfCommand, | ||
| expandableArgs, | ||
| arcDindPrefixArgsRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
| } else if configFileSetup != "" { | ||
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| %s | ||
| # shellcheck disable=SC1003 | ||
| %s %s %s \ | ||
| %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| preCreateLog, | ||
| configFileSetup, | ||
| arcDindPrefixProbe, | ||
| awfCommand, | ||
| expandableArgs, | ||
| arcDindPrefixArgsRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
| } else { | ||
| command = fmt.Sprintf(`set -o pipefail | ||
| %s | ||
| %s | ||
| # shellcheck disable=SC1003 | ||
| %s %s %s \ | ||
| %s %s %s %s \ | ||
| -- %s 2>&1 | tee -a %s`, | ||
| preCreateLog, | ||
| arcDindPrefixProbe, | ||
| awfCommand, | ||
| expandableArgs, | ||
| arcDindPrefixArgsRef, | ||
| shellJoinArgs(awfArgs), | ||
| shellWrappedCommand, | ||
| shellEscapeArg(config.LogFile)) | ||
|
|
@@ -684,3 +723,21 @@ func awfSupportsAllowHostPorts(firewallConfig *FirewallConfig) bool { | |
| minVersion := string(constants.AWFAllowHostPortsMinVersion) | ||
| return semverutil.Compare(versionStr, minVersion) >= 0 | ||
| } | ||
|
|
||
| // awfSupportsDockerHostPathPrefix returns true when the effective AWF version supports | ||
| // --docker-host-path-prefix. | ||
| func awfSupportsDockerHostPathPrefix(firewallConfig *FirewallConfig) bool { | ||
| var versionStr string | ||
| if firewallConfig != nil && firewallConfig.Version != "" { | ||
| versionStr = firewallConfig.Version | ||
| } else { | ||
| versionStr = string(constants.DefaultFirewallVersion) | ||
| } | ||
|
|
||
| if strings.EqualFold(versionStr, "latest") { | ||
| return true | ||
| } | ||
|
|
||
| minVersion := string(constants.AWFDockerHostPathPrefixMinVersion) | ||
| return semverutil.Compare(versionStr, minVersion) >= 0 | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,6 +44,17 @@ func TestFirewallArgsInCopilotEngine(t *testing.T) { | |
| t.Error("Expected command to contain '--log-level'") | ||
| } | ||
|
|
||
| initSnippet := `GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS=""` | ||
| conditionSnippet := `if [[ "${DOCKER_HOST:-}" =~ ^tcp://(localhost|127\.0\.0\.1)(:[0-9]+)?$ ]]; then` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The Reference the constant directly so the test always stays in sync: conditionSnippet := fmt.Sprintf(`if [[ "${DOCKER_HOST:-}" =~ %s ]]; then`, awfArcDindDockerHostRegex) |
||
| flagAssignmentSnippet := `GH_AW_DOCKER_HOST_PATH_PREFIX_ARGS="--docker-host-path-prefix /tmp/gh-aw"` | ||
|
|
||
| initIdx := strings.Index(stepContent, initSnippet) | ||
| conditionIdx := strings.Index(stepContent, conditionSnippet) | ||
| flagIdx := strings.Index(stepContent, flagAssignmentSnippet) | ||
| if initIdx == -1 || conditionIdx == -1 || flagIdx == -1 || !(initIdx < conditionIdx && conditionIdx < flagIdx) { | ||
| t.Error("Expected command to initialize ARC/DinD probe variable, then evaluate DOCKER_HOST condition, then assign docker-host-path-prefix flag") | ||
| } | ||
|
|
||
| // Verify that --log-dir is included in copilot args for log collection | ||
| if !strings.Contains(stepContent, "--log-dir /tmp/gh-aw/sandbox/agent/logs/") { | ||
| t.Error("Expected copilot command to contain '--log-dir /tmp/gh-aw/sandbox/agent/logs/' for log collection in firewall mode") | ||
|
|
@@ -211,6 +222,29 @@ func TestFirewallArgsInCopilotEngine(t *testing.T) { | |
| } | ||
| }) | ||
|
|
||
| t.Run("skips docker-host-path-prefix probe when AWF version is too old", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
| EngineConfig: &EngineConfig{ | ||
| ID: "copilot", | ||
| }, | ||
| NetworkPermissions: &NetworkPermissions{ | ||
| Firewall: &FirewallConfig{ | ||
| Enabled: true, | ||
| Version: "v0.25.42", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| engine := NewCopilotEngine() | ||
| steps := engine.GetExecutionSteps(workflowData, "test.log") | ||
| stepContent := requireCopilotExecutionStep(t, steps) | ||
|
|
||
| if strings.Contains(stepContent, "--docker-host-path-prefix /tmp/gh-aw") { | ||
| t.Error("Expected command to skip --docker-host-path-prefix for unsupported AWF versions") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The "skips docker-host-path-prefix probe" test only asserts that On an old AWF version the probe block should be entirely omitted — not just the flag value. Adding: if strings.Contains(stepContent, awfArcDindPrefixArgsVarName) {
t.Error("Expected no probe variable for unsupported AWF versions")
}would give the test full coverage of the negative path and guard against accidental partial injection. |
||
| } | ||
| }) | ||
|
|
||
| t.Run("AWF command includes ssl-bump flag when enabled", func(t *testing.T) { | ||
| workflowData := &WorkflowData{ | ||
| Name: "test-workflow", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] When
awfSupportsDockerHostPathPrefixreturnsfalse,arcDindPrefixProbeis set to""andarcDindPrefixArgsRefis also"". Each format string has%s\n%s\n— two consecutive%sslots that will both expand to empty strings, producing a stray blank line in the generated script on older AWF versions.A test that captures the full command output for the old-version path and asserts there are no consecutive blank lines (or that the probe variable name does not appear at all) would pin this behavior explicitly and catch any future regression.