luxolus
/
open-nomad
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

artifact: enable inheriting environment variables from client (#15514) 

* artifact: enable inheriting environment variables from client

This PR adds client configuration for specifying environment variables that
should be inherited by the artifact sandbox process from the Nomad Client agent.

Most users should not need to set these values but the configuration is provided
to ensure backwards compatability. Configuration of go-getter should ideally be
done through the artifact block in a jobspec task.

e.g.

```hcl
client {
  artifact {
    set_environment_variables = "TMPDIR,GIT_SSH_OPTS"
  }
}
```

Closes #15498

* website: update set_environment_variables text to mention PATH
This commit is contained in:
Seth Hoenig 2022-12-09 15:46:07 -06:00 committed by GitHub
parent da5ee8731c
commit be3f89b5f9
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
14 changed files with 192 additions and 78 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
client/allocrunner/taskrunner/getter/params.go
View File

@ -28,6 +28,7 @@ type parameters struct {
HgTimeout time.Duration `json:"hg_timeout"`
S3Timeout time.Duration `json:"s3_timeout"`
DisableFilesystemIsolation bool `json:"disable_filesystem_isolation"`
SetEnvironmentVariables string `json:"set_environment_variables"`
// Artifact
Mode getter.ClientMode `json:"artifact_mode"`
@ -88,6 +89,8 @@ func (p *parameters) Equal(o *parameters) bool {
return false
case p.DisableFilesystemIsolation != o.DisableFilesystemIsolation:
return false
case p.SetEnvironmentVariables != o.SetEnvironmentVariables:
return false
case p.Mode != o.Mode:
return false
case p.Source != o.Source:

1
client/allocrunner/taskrunner/getter/params_test.go
View File

@ -20,6 +20,7 @@ const paramsAsJSON = `
"hg_timeout": 4000000000,
"s3_timeout": 5000000000,
"disable_filesystem_isolation": true,
"set_environment_variables": "",
"artifact_mode": 2,
"artifact_source": "https://example.com/file.txt",
"artifact_destination": "local/out.txt",

1
client/allocrunner/taskrunner/getter/sandbox.go
View File

@ -47,6 +47,7 @@ func (s *Sandbox) Get(env interfaces.EnvReplacer, artifact *structs.TaskArtifact
HgTimeout: s.ac.HgTimeout,
S3Timeout: s.ac.S3Timeout,
DisableFilesystemIsolation: s.ac.DisableFilesystemIsolation,
SetEnvironmentVariables: s.ac.SetEnvironmentVariables,
// artifact configuration
Mode: mode,

25
client/allocrunner/taskrunner/getter/util.go
View File

@ -5,9 +5,12 @@ import (
"fmt"
"net/http"
"net/url"
"os"
"os/exec"
"path/filepath"
"sort"
"strings"
"unicode"
"github.com/hashicorp/go-getter"
"github.com/hashicorp/nomad/client/interfaces"
@ -95,6 +98,26 @@ func getTaskDir(env interfaces.EnvReplacer) string {
return filepath.Dir(p)
}
// environment merges the default minimal environment per-OS with the set of
// environment variables configured to be inherited from the Client
func environment(taskDir string, inherit string) []string {
chomp := func(s string) []string {
return strings.FieldsFunc(s, func(c rune) bool {
return c == ',' || unicode.IsSpace(c)
})
}
env := defaultEnvironment(taskDir)
for _, name := range chomp(inherit) {
env[name] = os.Getenv(name)
}
result := make([]string, 0, len(env))
for k, v := range env {
result = append(result, fmt.Sprintf("%s=%s", k, v))
}
sort.Strings(result)
return result
}
func (s *Sandbox) runCmd(env *parameters) error {
// find the nomad process
bin := subproc.Self()
@ -106,7 +129,7 @@ func (s *Sandbox) runCmd(env *parameters) error {
// start the subprocess, passing in parameters via stdin
output := new(bytes.Buffer)
cmd := exec.CommandContext(ctx, bin, SubCommand)
cmd.Env = minimalVars(env.TaskDir)
cmd.Env = environment(env.TaskDir, env.SetEnvironmentVariables)
cmd.Stdin = env.reader()
cmd.Stdout = output
cmd.Stderr = output

13
client/allocrunner/taskrunner/getter/util_default.go
View File

@ -3,7 +3,6 @@
package getter
import (
"fmt"
"path/filepath"
"syscall"
)
@ -27,13 +26,13 @@ func credentials() (uint32, uint32) {
return uint32(uid), uint32(gid)
}
// minimalVars returns the minimal environment set for artifact
// downloader sandbox
func minimalVars(taskDir string) []string {
// defaultEnvironment is the default minimal environment variables for Unix-like
// operating systems.
func defaultEnvironment(taskDir string) map[string]string {
tmpDir := filepath.Join(taskDir, "tmp")
return []string{
fmt.Sprintf("PATH=/usr/local/bin:/usr/bin:/bin"),
fmt.Sprintf("TMPDIR=%s", tmpDir),
return map[string]string{
"PATH": "/usr/local/bin:/usr/bin:/bin",
"TMPDIR": tmpDir,
}
}

12
client/allocrunner/taskrunner/getter/util_linux.go
View File

@ -3,7 +3,6 @@
package getter
import (
"fmt"
"path/filepath"
"syscall"
@ -49,13 +48,12 @@ func credentials() (uint32, uint32) {
}
}
// minimalVars returns the minimal environment set for artifact
// downloader sandbox
func minimalVars(taskDir string) []string {
// defaultEnvironment is the default minimal environment variables for Linux.
func defaultEnvironment(taskDir string) map[string]string {
tmpDir := filepath.Join(taskDir, "tmp")
return []string{
"PATH=/usr/local/bin:/usr/bin:/bin",
fmt.Sprintf("TMPDIR=%s", tmpDir),
return map[string]string{
"PATH": "/usr/local/bin:/usr/bin:/bin",
"TMPDIR": tmpDir,
}
}

70
client/allocrunner/taskrunner/getter/util_test.go
View File

@ -2,15 +2,18 @@ package getter
import (
"errors"
"runtime"
"testing"
"github.com/hashicorp/go-getter"
"github.com/hashicorp/nomad/ci"
"github.com/hashicorp/nomad/client/testutil"
"github.com/hashicorp/nomad/nomad/structs"
"github.com/shoenig/test/must"
)
func TestUtil_getURL(t *testing.T) {
ci.Parallel(t)
cases := []struct {
name string
artifact *structs.TaskArtifact
@ -61,6 +64,8 @@ func TestUtil_getURL(t *testing.T) {
}
func TestUtil_getDestination(t *testing.T) {
ci.Parallel(t)
env := noopTaskEnv("/path/to/task")
t.Run("ok", func(t *testing.T) {
result, err := getDestination(env, &structs.TaskArtifact{
@ -80,6 +85,8 @@ func TestUtil_getDestination(t *testing.T) {
}
func TestUtil_getMode(t *testing.T) {
ci.Parallel(t)
cases := []struct {
mode string
exp getter.ClientMode
@ -99,6 +106,8 @@ func TestUtil_getMode(t *testing.T) {
}
func TestUtil_getHeaders(t *testing.T) {
ci.Parallel(t)
env := upTaskEnv("/path/to/task")
t.Run("empty", func(t *testing.T) {
@ -123,26 +132,53 @@ func TestUtil_getHeaders(t *testing.T) {
}
func TestUtil_getTaskDir(t *testing.T) {
ci.Parallel(t)
env := noopTaskEnv("/path/to/task")
result := getTaskDir(env)
must.Eq(t, "/path/to/task", result)
}
func TestUtil_minimalVars(t *testing.T) {
var exp []string
switch runtime.GOOS {
case "windows":
exp = []string{
`PATH=C:\WINDOWS\system32;C:\WINDOWS;C:\WINDOWS\System32\Wbem;C:\WINDOWS\System32\WindowsPowerShell\v1.0\;`,
`TMP=C:\path\to\task\tmp`,
`TEMP=C:\path\to\task\tmp`,
}
default:
exp = []string{
func TestUtil_environment(t *testing.T) {
// not parallel
testutil.RequireLinux(t)
t.Run("default", func(t *testing.T) {
result := environment("/a/b/c", "")
must.Eq(t, []string{
"PATH=/usr/local/bin:/usr/bin:/bin",
"TMPDIR=/path/to/task/tmp",
}
}
result := minimalVars("/path/to/task")
must.Eq(t, exp, result)
"TMPDIR=/a/b/c/tmp",
}, result)
})
t.Run("append", func(t *testing.T) {
t.Setenv("ONE", "1")
t.Setenv("TWO", "2")
result := environment("/a/b/c", "ONE,TWO")
must.Eq(t, []string{
"ONE=1",
"PATH=/usr/local/bin:/usr/bin:/bin",
"TMPDIR=/a/b/c/tmp",
"TWO=2",
}, result)
})
t.Run("override", func(t *testing.T) {
t.Setenv("PATH", "/opt/bin")
t.Setenv("TMPDIR", "/scratch")
result := environment("/a/b/c", "PATH,TMPDIR")
must.Eq(t, []string{
"PATH=/opt/bin",
"TMPDIR=/scratch",
}, result)
})
t.Run("missing", func(t *testing.T) {
result := environment("/a/b/c", "DOES_NOT_EXIST")
must.Eq(t, []string{
"DOES_NOT_EXIST=",
"PATH=/usr/local/bin:/usr/bin:/bin",
"TMPDIR=/a/b/c/tmp",
}, result)
})
}

18
client/allocrunner/taskrunner/getter/util_windows.go
View File

@ -3,7 +3,6 @@
package getter
import (
"fmt"
"os"
"path/filepath"
"syscall"
@ -24,14 +23,15 @@ func lockdown(string) error {
return nil
}
func minimalVars(taskDir string) []string {
// defaultEnvironment is the default minimal environment variables for Windows.
func defaultEnvironment(taskDir string) map[string]string {
tmpDir := filepath.Join(taskDir, "tmp")
return []string{
fmt.Sprintf("HOMEPATH=%s", os.Getenv("HOMEPATH")),
fmt.Sprintf("HOMEDRIVE=%s", os.Getenv("HOMEDRIVE")),
fmt.Sprintf("USERPROFILE=%s", os.Getenv("USERPROFILE")),
fmt.Sprintf("PATH=%s", os.Getenv("PATH")),
fmt.Sprintf("TMP=%s", tmpDir),
fmt.Sprintf("TEMP=%s", tmpDir),
return map[string]string{
"HOMEPATH": os.Getenv("HOMEPATH"),
"HOMEDRIVE": os.Getenv("HOMEDRIVE"),
"USERPROFILE": os.Getenv("USERPROFILE"),
"PATH": os.Getenv("PATH"),
"TMP": tmpDir,
"TEMP": tmpDir,
}
}

2
client/config/artifact.go
View File

@ -20,6 +20,7 @@ type ArtifactConfig struct {
S3Timeout time.Duration
DisableFilesystemIsolation bool
SetEnvironmentVariables string
}
// ArtifactConfigFromAgent creates a new internal readonly copy of the client
@ -63,6 +64,7 @@ func ArtifactConfigFromAgent(c *config.ArtifactConfig) (*ArtifactConfig, error)
HgTimeout: hgTimeout,
S3Timeout: s3Timeout,
DisableFilesystemIsolation: *c.DisableFilesystemIsolation,
SetEnvironmentVariables: *c.SetEnvironmentVariables,
}, nil
}

74
client/config/artifact_test.go
View File

@ -7,22 +7,22 @@ import (
"github.com/hashicorp/nomad/ci"
"github.com/hashicorp/nomad/helper/pointer"
"github.com/hashicorp/nomad/nomad/structs/config"
"github.com/stretchr/testify/require"
"github.com/shoenig/test/must"
)
func TestArtifactConfigFromAgent(t *testing.T) {
ci.Parallel(t)
testCases := []struct {
name string
config *config.ArtifactConfig
expected *ArtifactConfig
expectedError string
name string
config *config.ArtifactConfig
exp *ArtifactConfig
expErr string
}{
{
name: "from default",
config: config.DefaultArtifactConfig(),
expected: &ArtifactConfig{
exp: &ArtifactConfig{
HTTPReadTimeout: 30 * time.Minute,
HTTPMaxBytes: 100_000_000_000,
GCSTimeout: 30 * time.Minute,
@ -41,7 +41,7 @@ func TestArtifactConfigFromAgent(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
},
expectedError: "error parsing HTTPReadTimeout",
expErr: "error parsing HTTPReadTimeout",
},
{
name: "invalid http max size",
@ -53,7 +53,7 @@ func TestArtifactConfigFromAgent(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
},
expectedError: "error parsing HTTPMaxSize",
expErr: "error parsing HTTPMaxSize",
},
{
name: "invalid gcs timeout",
@ -65,7 +65,7 @@ func TestArtifactConfigFromAgent(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
},
expectedError: "error parsing GCSTimeout",
expErr: "error parsing GCSTimeout",
},
{
name: "invalid git timeout",
@ -77,7 +77,7 @@ func TestArtifactConfigFromAgent(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
},
expectedError: "error parsing GitTimeout",
expErr: "error parsing GitTimeout",
},
{
name: "invalid hg timeout",
@ -89,7 +89,7 @@ func TestArtifactConfigFromAgent(t *testing.T) {
HgTimeout: pointer.Of("invalid"),
S3Timeout: pointer.Of("30m"),
},
expectedError: "error parsing HgTimeout",
expErr: "error parsing HgTimeout",
},
{
name: "invalid s3 timeout",
@ -101,7 +101,7 @@ func TestArtifactConfigFromAgent(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("invalid"),
},
expectedError: "error parsing S3Timeout",
expErr: "error parsing S3Timeout",
},
}
@ -109,12 +109,12 @@ func TestArtifactConfigFromAgent(t *testing.T) {
t.Run(tc.name, func(t *testing.T) {
got, err := ArtifactConfigFromAgent(tc.config)
if tc.expectedError != "" {
require.Error(t, err)
require.Contains(t, err.Error(), tc.expectedError)
if tc.expErr != "" {
must.Error(t, err)
must.StrContains(t, err.Error(), tc.expErr)
} else {
require.NoError(t, err)
require.Equal(t, tc.expected, got)
must.NoError(t, err)
must.Eq(t, tc.exp, got)
}
})
}
@ -123,18 +123,20 @@ func TestArtifactConfigFromAgent(t *testing.T) {
func TestArtifactConfig_Copy(t *testing.T) {
ci.Parallel(t)
config := &ArtifactConfig{
HTTPReadTimeout: time.Minute,
HTTPMaxBytes: 1000,
GCSTimeout: 2 * time.Minute,
GitTimeout: time.Second,
HgTimeout: time.Hour,
S3Timeout: 5 * time.Minute,
ac := &ArtifactConfig{
HTTPReadTimeout: time.Minute,
HTTPMaxBytes: 1000,
GCSTimeout: 2 * time.Minute,
GitTimeout: time.Second,
HgTimeout: time.Hour,
S3Timeout: 5 * time.Minute,
DisableFilesystemIsolation: true,
SetEnvironmentVariables: "FOO,BAR",
}
// make sure values are copied.
configCopy := config.Copy()
require.Equal(t, config, configCopy)
configCopy := ac.Copy()
must.Eq(t, ac, configCopy)
// modify copy and make sure original doesn't change.
configCopy.HTTPReadTimeout = 5 * time.Minute
@ -143,13 +145,17 @@ func TestArtifactConfig_Copy(t *testing.T) {
configCopy.GitTimeout = 3 * time.Second
configCopy.HgTimeout = 2 * time.Hour
configCopy.S3Timeout = 10 * time.Minute
configCopy.DisableFilesystemIsolation = false
configCopy.SetEnvironmentVariables = "BAZ"
require.Equal(t, &ArtifactConfig{
HTTPReadTimeout: time.Minute,
HTTPMaxBytes: 1000,
GCSTimeout: 2 * time.Minute,
GitTimeout: time.Second,
HgTimeout: time.Hour,
S3Timeout: 5 * time.Minute,
}, config)
must.Eq(t, &ArtifactConfig{
HTTPReadTimeout: time.Minute,
HTTPMaxBytes: 1000,
GCSTimeout: 2 * time.Minute,
GitTimeout: time.Second,
HgTimeout: time.Hour,
S3Timeout: 5 * time.Minute,
DisableFilesystemIsolation: true,
SetEnvironmentVariables: "FOO,BAR",
}, ac)
}

16
nomad/structs/config/artifact.go
View File

@ -39,6 +39,11 @@ type ArtifactConfig struct {
// artifact downloader can write only to the task sandbox directory, and can
// read only from specific locations on the host filesystem.
DisableFilesystemIsolation *bool `hcl:"disable_filesystem_isolation"`
// SetEnvironmentVariables is a comma-separated list of environment
// variable names to inherit from the Nomad Client and set in the artifact
// download sandbox process.
SetEnvironmentVariables *string `hcl:"set_environment_variables"`
}
func (a *ArtifactConfig) Copy() *ArtifactConfig {
@ -53,6 +58,7 @@ func (a *ArtifactConfig) Copy() *ArtifactConfig {
HgTimeout: pointer.Copy(a.HgTimeout),
S3Timeout: pointer.Copy(a.S3Timeout),
DisableFilesystemIsolation: pointer.Copy(a.DisableFilesystemIsolation),
SetEnvironmentVariables: pointer.Copy(a.SetEnvironmentVariables),
}
}
@ -71,6 +77,7 @@ func (a *ArtifactConfig) Merge(o *ArtifactConfig) *ArtifactConfig {
HgTimeout: pointer.Merge(a.HgTimeout, o.HgTimeout),
S3Timeout: pointer.Merge(a.S3Timeout, o.S3Timeout),
DisableFilesystemIsolation: pointer.Merge(a.DisableFilesystemIsolation, o.DisableFilesystemIsolation),
SetEnvironmentVariables: pointer.Merge(a.SetEnvironmentVariables, o.SetEnvironmentVariables),
}
}
}
@ -94,6 +101,8 @@ func (a *ArtifactConfig) Equal(o *ArtifactConfig) bool {
return false
case !pointer.Eq(a.DisableFilesystemIsolation, o.DisableFilesystemIsolation):
return false
case !pointer.Eq(a.SetEnvironmentVariables, o.SetEnvironmentVariables):
return false
}
return true
}
@ -161,6 +170,10 @@ func (a *ArtifactConfig) Validate() error {
return fmt.Errorf("disable_filesystem_isolation must be set")
}
if a.SetEnvironmentVariables == nil {
return fmt.Errorf("set_environment_variables must be set")
}
return nil
}
@ -192,5 +205,8 @@ func DefaultArtifactConfig() *ArtifactConfig {
// Toggle for disabling filesystem isolation, where available.
DisableFilesystemIsolation: pointer.Of(false),
// No environment variables are inherited from Client by default.
SetEnvironmentVariables: pointer.Of(""),
}
}

21
nomad/structs/config/artifact_test.go
View File

@ -42,6 +42,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
DisableFilesystemIsolation: pointer.Of(false),
SetEnvironmentVariables: pointer.Of(""),
},
other: &ArtifactConfig{
HTTPReadTimeout: pointer.Of("5m"),
@ -51,6 +52,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("3m"),
S3Timeout: pointer.Of("4m"),
DisableFilesystemIsolation: pointer.Of(true),
SetEnvironmentVariables: pointer.Of("FOO,BAR"),
},
expected: &ArtifactConfig{
HTTPReadTimeout: pointer.Of("5m"),
@ -60,6 +62,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("3m"),
S3Timeout: pointer.Of("4m"),
DisableFilesystemIsolation: pointer.Of(true),
SetEnvironmentVariables: pointer.Of("FOO,BAR"),
},
},
{
@ -73,6 +76,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("3m"),
S3Timeout: pointer.Of("4m"),
DisableFilesystemIsolation: pointer.Of(true),
SetEnvironmentVariables: pointer.Of("FOO,BAR"),
},
expected: &ArtifactConfig{
HTTPReadTimeout: pointer.Of("5m"),
@ -82,6 +86,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("3m"),
S3Timeout: pointer.Of("4m"),
DisableFilesystemIsolation: pointer.Of(true),
SetEnvironmentVariables: pointer.Of("FOO,BAR"),
},
},
{
@ -94,6 +99,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
DisableFilesystemIsolation: pointer.Of(true),
SetEnvironmentVariables: pointer.Of("FOO,BAR"),
},
other: nil,
expected: &ArtifactConfig{
@ -104,6 +110,7 @@ func TestArtifactConfig_Merge(t *testing.T) {
HgTimeout: pointer.Of("30m"),
S3Timeout: pointer.Of("30m"),
DisableFilesystemIsolation: pointer.Of(true),
SetEnvironmentVariables: pointer.Of("FOO,BAR"),
},
},
}
@ -339,6 +346,20 @@ func TestArtifactConfig_Validate(t *testing.T) {
},
expErr: "s3_timeout not a valid duration",
},
{
name: "fs isolation not set",
config: func(a *ArtifactConfig) {
a.DisableFilesystemIsolation = nil
},
expErr: "disable_filesystem_isolation must be set",
},
{
name: "env not set",
config: func(a *ArtifactConfig) {
a.SetEnvironmentVariables = nil
},
expErr: "set_environment_variables must be set",
},
}
for _, tc := range testCases {

5
website/content/docs/configuration/client.mdx
View File

@ -382,6 +382,11 @@ see the [drivers documentation](/docs/drivers).
isolation should be disabled for artifact downloads. Applies only to systems
where filesystem isolation via [landlock] is possible (Linux kernel 5.13+).
- `set_environment_variables` `(string:"")` - Specifies a comma separated list
of environment variables that should be inherited by the artifact sandbox from
the Nomad client's environment. By default a minimal environment is set including
a `PATH` appropriate for the operating system.
### `template` Parameters
- `function_denylist` `([]string: ["plugin", "writeToFile"])` - Specifies a

9
website/content/docs/upgrade/upgrade-specific.mdx
View File

@ -55,10 +55,12 @@ USERPROFILE=<inherit $USERPROFILE>
```
Configuration of the artifact downloader should happen through the [`options`][artifact_params]
and [`headers`][artifact_params] fields of the `artifact` block.
and [`headers`][artifact_params] fields of the `artifact` block. For backwards
compatibility, the sandbox can be configured to inherit specified environment variables
from the Nomad client by setting [`set_environment_variables`][artifact_env].
The use of filesystem isolation can be disabled in Client configuration by
setting [`disable_filesystem_isolation`][fs_isolation].
setting [`disable_filesystem_isolation`][artifact_fs_isolation].
## Nomad 1.4.0
@ -1580,4 +1582,5 @@ deleted and then Nomad 0.3.0 can be launched.
[gh_issue]: https://github.com/hashicorp/nomad/issues/new/choose
[upgrade process]: /docs/upgrade#upgrade-process
[landlock]: https://docs.kernel.org/userspace-api/landlock.html
[fs_isolation]: /docs/configuration/client#disable_filesystem_isolation
[artifact_fs_isolation]: /docs/configuration/client#disable_filesystem_isolation
[artifact_env]: /docs/configuration/client#set_environment_variables