address interpolation of artifact and template src/dest #9671

cgbaker · 2020-12-18T11:57:28Z

refactored #9668 for (imho) better abstraction

Resolves #9610
Resolves #9389
Resolves #6929

The main change is to use client-specific absolute paths in the environment maps for interpolating source for template and destination for template and artifact.

Because of this, it required adjusting the sandbox-escape checks to allow shared alloc dir, in addition to task working dir.

The result is that we should now support source and destination paths of the following forms:

well-known interpolated paths: ${NOMAD_ALLOC_DIR}, ${NOMAD_SECRETS_DIR}, ${NOMAD_TASK_DIR}
task-dir relative paths: local, secrets, ../alloc

The new e2e test shows the capability that we're targeting.

cgbaker · 2020-12-18T13:25:36Z

client/taskenv/env.go

+// If the interpolated result is a relative path, it is made absolute
+// wrt to the task working directory.
+// The result is checked to see whether it escapes both the task working
+// directory and the shared allocation directory.


this the result of the refactoring from #9668... it is potentially excessive refactoring. this method provides the functionality that we need for template and artifact path interpolation. namely, using the client-relative environment values to interpolate paths that we're using on the client: paths for consul-template and go-getter.

Whatever we can do to make it a "no brainer" for folks trying to handle client paths safely seems like a good thing to me. So I don't feel like it's excessive to pull this out to a centralized implementation! 😀

cgbaker · 2020-12-18T13:26:55Z

client/taskenv/env.go

-	nodeAttrs := make(map[string]string)
+// buildEnv returns the environment variables and device environment
+// variables with respect to the task directories passed in the arguments.
+func (b *Builder) buildEnv(allocDir, localDir, secretsDir string,


there was a lot of interpolation that happened while creating these environment variables, so i did a back-of-the-envelope dependency analysis and broke out the code for creating the env vars and the device env vars into this method.

cgbaker · 2020-12-18T14:34:48Z

client/allocrunner/taskrunner/task_dir_hook.go

@@ -93,11 +100,10 @@ func setEnvvars(envBuilder *taskenv.Builder, fsi drivers.FSIsolation, taskDir *a
 	// Set the host environment variables for non-image based drivers
 	if fsi != drivers.FSIsolationImage {
 		// COMPAT(1.0) using inclusive language, blacklist is kept for backward compatibility.
-		denylist := conf.ReadAlternativeDefault(
+		filter := strings.Split(conf.ReadAlternativeDefault(


just making this consistent with its clone in plugins:
https://github.com/hashicorp/nomad/blob/master/plugins/drivers/testutils/testing.go#L252

tgross

The overall approach here LGTM. I think this will be a big improvement in terms of comprehensibility for both the operators and us developers.

tgross · 2020-12-18T14:48:25Z

client/taskenv/env.go

+// If the interpolated result is a relative path, it is made absolute
+// wrt to the task working directory.
+// The result is checked to see whether it escapes both the task working
+// directory and the shared allocation directory.


Whatever we can do to make it a "no brainer" for folks trying to handle client paths safely seems like a good thing to me. So I don't feel like it's excessive to pull this out to a centralized implementation! 😀

tgross · 2020-12-18T14:55:03Z

e2e/consultemplate/consultemplate.go

+// is supported as a destination for artifact and template blocks, and
+// that it is properly interpolated for task drivers with varying
+// filesystem isolation
+func (tc *ConsulTemplateTest) TestTemplatePathInterpolation_SharedAllocDir(f *framework.F) {


This test is great 👍

e2e/consultemplate/consultemplate.go

tgross · 2020-12-18T15:16:26Z

client/taskenv/env.go

+	envMap, deviceEnvs := b.buildEnv(b.allocDir, b.localDir, b.secretsDir, nodeAttrs)
+	envMapClient, _ := b.buildEnv(b.clientAllocDir, b.clientLocalDir, b.clientSecretsDir, nodeAttrs)
+
+	return NewTaskEnv(envMap, envMapClient, deviceEnvs, nodeAttrs, b.clientTaskDir, b.clientAllocDir)


It looks like this is the only non-test caller? I'm wondering if we should make this internal to the package to force test callers to always use the full safe path?

cgbaker · 2020-12-18T16:27:44Z

okay, it looks like i've got a problem... there is a use case that i didn't capture in this PR which may send this back to the drawing board:
https://github.com/hashicorp/nomad/pull/9671/files#diff-f40e85b67f576552c2d39989b55c983267bf19524185cc5f60d4716be0f71359R1683-L1686

it looks like the previous behavior of always joining against taskdir meant that absolute paths in the template destination would map to the same absolute path in the container:
https://github.com/hashicorp/nomad/pull/9671/files#diff-90e77cdb4f5439161bf5a053bf2d0b74d3ac3ee65cfce482bbd8e02646a14a64L143
this is because something like ${taskDir}//etc became ${taskDir}/etc which was mounted on top of /

... except for docker, where this only worked if the absolute path was one of the bind-mounted paths:

/secrets/...
/local/...
but not /alloc (which would go into {taskdir}/alloc which was not bind-mounted)

so, i need to figure out whether this invalidates the approach or not and deal with it. it may be as simple as handling absolute paths a little better...

schmichael

Not sure I follow the hiccup you've hit. It's definitely awfully confusing as we have 3 "FSIsolation" modes that affect this:

None (raw exec) - ClientPath == TaskPath but behavior is like Chroot (exec) in that you can write anywhere in the task's filesystem.
Chroot (exec/java) - ClientPath != TaskPath. Runtime paths are like Image but you can write to anywhere in the task's filesystem like None (raw exec) above.
Image (docker/podman) - ClientPath != TaskPath. Runtime paths are like Chroot but you can only write to the 3 bind-mounted paths (secrets, local, alloc).

For simplicities sake I think we should always be bind mounting <alloc-dir>/<alloc-id>/alloc to <alloc-dir>/<alloc-id>/<task>/alloc and doing all operations on that path. I'm not sure we ever need to reference the "real" .../alloc.

Image drivers like Docker and Podman should have .../alloc bind-mounted into their container filesystem as well.

I think that double bind mounting (once to create the task directory on the host, and the second time for Image based drivers) should make it so we never need to special case where the "alloc dir" is? It's the same as local and secrets.

While the ability to write anywhere on a Chroot driver's filesystem was likely an unintentional side effect, it should be considered a unique feature at this point distinct from how Image behaves. There's no point in trying to rememdy the discrepancy between the two now. In the future I hope Chroot goes away entirely in favor of Image, but obviously totally removing the Chroot behavior will take a long time.

client/taskenv/env.go

cgbaker · 2020-12-22T13:48:29Z

For simplicities sake I think we should always be bind mounting <alloc-dir>/<alloc-id>/alloc to <alloc-dir>/<alloc-id>/<task>/alloc and doing all operations on that path. I'm not sure we ever need to reference the "real" .../alloc.

We used to do this, but (if I'm understanding the PR correctly) we stopped because it required root for the Nomad client (which some docker-only users didn't need/want):
0785109#diff-0626141dbc5bf9ed895f1d35410b05e96fdbe3aaf1f734c2dcdad9b3cab48552R92

cgbaker · 2020-12-22T14:44:17Z

rebased against master and re-enabled the disabled test from #9383
ready for review

schmichael

We used to do this, but [...] we stopped because it required root for the Nomad client (which some docker-only users didn't need/want)

Argh of course, good catch. I even recognize that face on the PR 😬

An alternate fix for #2178 would have been to soft-fail on that linkDir call. I think that would make raw_exec behave more reasonably when Nomad is run as root.

I think this is a better solution all around thouhg? Distinctly rendering client paths vs task paths. Great work.

client/taskenv/env.go

schmichael · 2021-01-04T17:14:45Z

client/taskenv/env.go

+// If joinEscape, an interpolated path that escapes will be joined with the
+// task dir.


Can you add an example usage here? Maybe it's the holidays, but I don't want to admit how long I stared at this and the relevant 2 lines of code to figure out what was going on. 😅

yeah, will do. this is the hiccup that i ran up against, where previous behavior allowed for absolute destination paths for artifact and template:
https://github.com/hashicorp/nomad/pull/9671/files#diff-90e77cdb4f5439161bf5a053bf2d0b74d3ac3ee65cfce482bbd8e02646a14a64L140-L144

i'll add a documenting test.

documenting test here:
https://github.com/hashicorp/nomad/pull/9671/files#diff-6c8db153b63f572b0e0d0f447c3071b491c157fee3fe7c2c1040aaa70b40bf76R895-R903

schmichael · 2021-01-04T17:21:26Z

client/taskenv/env.go

+func (b *Builder) SetClientSharedAllocDir(dir string) *Builder {
+	b.mu.Lock()
+	b.clientSharedAllocDir = dir
+	b.mu.Unlock()
+	return b
+}
+
+func (b *Builder) SetClientTaskRoot(dir string) *Builder {
+	b.mu.Lock()
+	b.clientTaskRoot = dir
+	b.mu.Unlock()
+	return b
+}
+
+func (b *Builder) SetClientTaskLocalDir(dir string) *Builder {
+	b.mu.Lock()
+	b.clientTaskLocalDir = dir
+	b.mu.Unlock()
+	return b
+}
+
+func (b *Builder) SetClientTaskSecretsDir(dir string) *Builder {
+	b.mu.Lock()
+	b.clientTaskSecretsDir = dir
+	b.mu.Unlock()
+	return b
+}


Since these are reqiured for Build()'s proper function it's a shame we can't enforce those dependencies through the New func or similar, but I can see how that gets awkward given the way Builder's are built up vs where FSIsolation is handled. A problem for another time perhaps.

resolves #9839 resolves #6929 resolves #6910 e2e: template env interpolation path testing

Co-authored-by: Michael Schurter <mschurter@hashicorp.com>

github-actions · 2022-12-05T02:17:06Z

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

cgbaker mentioned this pull request Dec 18, 2020

WIP: better interpolation support for template and artifact source/destination #9668

Closed

cgbaker marked this pull request as draft December 18, 2020 11:58

cgbaker changed the title ~~refactor of 9668~~ WIP: refactor of 9668 Dec 18, 2020

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 18, 2020 12:42 Inactive

vercel bot temporarily deployed to Preview – nomad December 18, 2020 12:42 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 18, 2020 13:16 Inactive

vercel bot temporarily deployed to Preview – nomad December 18, 2020 13:16 Inactive

cgbaker force-pushed the b-9610-template-interpolation-paths-refactor branch from 0962139 to 1215201 Compare December 18, 2020 13:17

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 18, 2020 13:17 Inactive

vercel bot temporarily deployed to Preview – nomad December 18, 2020 13:17 Inactive

cgbaker force-pushed the b-9610-template-interpolation-paths-refactor branch from 1215201 to d286454 Compare December 18, 2020 13:22

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 18, 2020 13:22 Inactive

vercel bot temporarily deployed to Preview – nomad December 18, 2020 13:22 Inactive

cgbaker commented Dec 18, 2020

View reviewed changes

vercel bot temporarily deployed to Preview – nomad December 18, 2020 14:01 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 18, 2020 14:01 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 18, 2020 14:11 Inactive

vercel bot temporarily deployed to Preview – nomad December 18, 2020 14:11 Inactive

cgbaker requested review from schmichael and tgross December 18, 2020 14:30

cgbaker commented Dec 18, 2020

View reviewed changes

cgbaker changed the title ~~WIP: refactor of 9668~~ address interpolation of artifact and template src/dest Dec 18, 2020

tgross reviewed Dec 18, 2020

View reviewed changes

schmichael reviewed Dec 18, 2020

View reviewed changes

client/taskenv/env.go Outdated Show resolved Hide resolved

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 22, 2020 13:39 Inactive

vercel bot temporarily deployed to Preview – nomad December 22, 2020 13:39 Inactive

vercel bot temporarily deployed to Preview – nomad December 22, 2020 13:49 Inactive

vercel bot temporarily deployed to Preview – nomad December 22, 2020 14:26 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui December 22, 2020 14:26 Inactive

cgbaker marked this pull request as ready for review December 22, 2020 14:45

schmichael approved these changes Jan 4, 2021

View reviewed changes

hashicorp deleted a comment from wirat112535 Jan 4, 2021

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 4, 2021 17:50 Inactive

vercel bot temporarily deployed to Preview – nomad January 4, 2021 17:50 Inactive

vercel bot temporarily deployed to Preview – nomad January 4, 2021 18:52 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 4, 2021 18:52 Inactive

vercel bot temporarily deployed to Preview – nomad January 4, 2021 21:05 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 4, 2021 21:05 Inactive

vercel bot temporarily deployed to Preview – nomad January 4, 2021 22:01 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 4, 2021 22:01 Inactive

cgbaker and others added 6 commits January 4, 2021 22:25

update template and artifact interpolation to use client-relative paths

11f0fed

resolves #9839 resolves #6929 resolves #6910 e2e: template env interpolation path testing

enabled broken test that is no longer broken

6419966

Update client/taskenv/env.go

1a607e1

Co-authored-by: Michael Schurter <mschurter@hashicorp.com>

changelog

4791333

added documenting unit tests for new TaskEnv.ClientPath method

46060e4

modified e2e test so that it explicitly tested the use case in #6929

196f550

cgbaker force-pushed the b-9610-template-interpolation-paths-refactor branch from e70f08d to 196f550 Compare January 4, 2021 22:26

vercel bot temporarily deployed to Preview – nomad January 4, 2021 22:26 Inactive

vercel bot temporarily deployed to Preview – nomad-storybook-and-ui January 4, 2021 22:26 Inactive

cgbaker merged commit 30603ca into master Jan 4, 2021

cgbaker deleted the b-9610-template-interpolation-paths-refactor branch January 4, 2021 23:00

tgross mentioned this pull request Jan 19, 2021

Using artifact stanza errors with relative paths require a module with a pwd #8969

Closed

cgbaker mentioned this pull request Jan 27, 2021

docs: update filesystem docs about artifact/template interpolation #9905

Closed

github-actions bot locked as resolved and limited conversation to collaborators Dec 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

address interpolation of artifact and template src/dest #9671

address interpolation of artifact and template src/dest #9671

cgbaker commented Dec 18, 2020 •

edited

Loading

cgbaker Dec 18, 2020 •

edited

Loading

tgross Dec 18, 2020

cgbaker Dec 18, 2020

cgbaker Dec 18, 2020 •

edited

Loading

tgross left a comment

tgross Dec 18, 2020

tgross Dec 18, 2020

tgross Dec 18, 2020

cgbaker commented Dec 18, 2020 •

edited

Loading

schmichael left a comment

cgbaker commented Dec 22, 2020

cgbaker commented Dec 22, 2020 •

edited

Loading

schmichael left a comment

schmichael Jan 4, 2021

cgbaker Jan 4, 2021

cgbaker Jan 4, 2021

schmichael Jan 4, 2021

github-actions bot commented Dec 5, 2022

		// If joinEscape, an interpolated path that escapes will be joined with the
		// task dir.

address interpolation of artifact and template src/dest #9671

address interpolation of artifact and template src/dest #9671

Conversation

cgbaker commented Dec 18, 2020 • edited Loading

cgbaker Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgbaker Dec 18, 2020 • edited Loading

Choose a reason for hiding this comment

tgross left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgbaker commented Dec 18, 2020 • edited Loading

schmichael left a comment

Choose a reason for hiding this comment

cgbaker commented Dec 22, 2020

cgbaker commented Dec 22, 2020 • edited Loading

schmichael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2022

cgbaker commented Dec 18, 2020 •

edited

Loading

cgbaker Dec 18, 2020 •

edited

Loading

cgbaker Dec 18, 2020 •

edited

Loading

cgbaker commented Dec 18, 2020 •

edited

Loading

cgbaker commented Dec 22, 2020 •

edited

Loading