-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
WIP: better interpolation support for template and artifact source/destination #9668
Conversation
clientEnv := make(map[string]string, len(req.TaskEnv.EnvMap)) | ||
for k, v := range req.TaskEnv.EnvMap { | ||
clientEnv[k] = v | ||
} | ||
clientEnv[taskenv.AllocDir] = req.TaskDir.SharedAllocDir | ||
clientEnv[taskenv.TaskLocalDir] = req.TaskDir.Dir | ||
clientTaskEnv := taskenv.NewTaskEnv( | ||
clientEnv, | ||
req.TaskEnv.DeviceEnv(), | ||
req.TaskEnv.NodeAttrs, | ||
) |
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.
not happy with this deconstruction, but it was either that or change a lot of plumbing to get the taskEnv.Builder
into here.
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.
in hindsight, i'm thinking that a cleaner abstraction is to pass around a single TaskEnv
/EnvReplacer
with two methods: ReplaceEnv
and ReplaceEnvClient
. Could even augment it with all of the interpolation/join/sandbox-check logic and clean this stuff up.
Will take a pass at that now.
94347f3
to
4d7d89d
Compare
escapes := helper.PathEscapesSandbox(taskDir, dest) | ||
dest := clientTaskEnv.ReplaceEnv(artifact.RelativeDest) | ||
// if it was a relative path (like 'local' or '../alloc', join it with the task working directory) | ||
if !filepath.IsAbs(dest) { |
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.
we don't always join... interpolation of ${NOMAD_*_PATH}
will provide absolute paths, so join is only necessary if using a relative path.
if !filepath.IsAbs(dest) { | ||
dest = filepath.Join(taskDir.Dir, dest) | ||
} | ||
dest = filepath.Clean(dest) |
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.
stole this Clean
from somewhere else... not sure that it's necessary.
e2e/consultemplate/consultemplate.go
Outdated
@@ -244,6 +244,16 @@ func (tc *ConsulTemplateTest) TestTemplatePathInterpolation_Ok(f *framework.F) { | |||
func(out string) bool { | |||
return len(out) > 0 | |||
}, nil), "expected file to have contents") | |||
|
|||
f.NoError(waitForTemplateRender(allocID, "task/alloc/shared.txt", |
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.
per #9610, this did not work for docker as of late
4d7d89d
to
4395da3
Compare
4395da3
to
82d630b
Compare
ggURL, err := getGetterUrl(taskEnv, artifact) | ||
// clientTaskEnv is used for interpolating the destination directory of the artifact in the client | ||
// workloadTaskEnv is used for other interpolation operations | ||
func GetArtifact(workloadTaskEnv, clientTaskEnv EnvReplacer, artifact *structs.TaskArtifact, taskDir, sharedAllocDir string) error { |
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.
not particularly happy with this either... but plumbing a taskenv.Builder
down here seems worse.
@@ -28,6 +28,13 @@ job "template-paths" { | |||
destination = "${NOMAD_SECRETS_DIR}/foo/dst" | |||
} | |||
|
|||
template { |
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.
this was most recently not-working with docker
driver
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. |
This is the first draft to address #9610 and #9389 and #6929. I wanted to get early feedback on the general approach.
UPDATE: I've refactored this in #9671 with (imho) better abstraction
The main change is to use client-specific absolute paths in the environment maps for interpolating
source
fortemplate
anddestination
fortemplate
andartifact
.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 destination paths of the following forms:
${NOMAD_ALLOC_DIR}
,${NOMAD_SECRETS_DIR}
,${NOMAD_TASK_DIR}
local
,secrets
,../alloc
I'm in the process of adding new tests. The new e2e test shows the capability that we're targeting.
Also need to fix the failing test. And I need to read through pre-hook and make sure that I haven't broken anything in there wrt to pre-hooks and the task env.