-
Notifications
You must be signed in to change notification settings - Fork 784
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
file sandbox: check but don't alter paths passed to 'file' #1254
Conversation
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254 [wip consul-template]
template/funcs.go
Outdated
// pathInSandbox determines whether a provided path falls within the sandbox. | ||
// returns an error only if the file can't be evaluated (missing, invalid | ||
// symlink, etc.) | ||
func pathInSandbox(sandbox, path string) (bool, 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.
Returning both a bool and an error is redundant here as you always return false with an error and true when there is no error. So when you check !ok
above, you would always use that error and ignore the returned one whenever this returned false.
I'd say skip the bool and just use the standard if err != nil {return err}
on the result.
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.
It was feeling semantically weird to return an error on a yes/no question. But I think you're right, let's do that.
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.
Done!
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.
Thanks. And yeah, I get the semantic itch for having it return a bool to a yes/no question-y function. Maybe with a different name or something it wouldn't feel wrong or if it could be reworked such that the error could be handled internally (just changing the bool result). But as is, I think simplifying it is better.
659167f
to
5af785d
Compare
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254
|
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254
5af785d
to
377c9aa
Compare
@tgross Yeah.. that TestRunner_quiescence/snooze test has a race in it somewhere. It hasn't been a priority as it is infrequent enough and I'm planning on reworking that runner code anyways. |
Are we ok to merge then? |
Yep. Going to take care of that in a few. |
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254 pulls in vault KVv2 read fixes from: hashicorp/consul-template#1253
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254 pulls in vault KVv2 read fixes from: hashicorp/consul-template#1253
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254 pulls in vault KVv2 read fixes from: hashicorp/consul-template#1253
pulls in configuration option for blacklisting template functions from: hashicorp/consul-template#1243 hashicorp/consul-template#1246 pulls in configuration option for file sandboxing from: hashicorp/consul-template#1249 hashicorp/consul-template#1254 pulls in vault KVv2 read fixes from: hashicorp/consul-template#1253
During a discussion with @notnoop about hashicorp/nomad#6075, we discovered that altering the path being passed into the
file
function was going to cut off a lot of existing uses of consul-template in Nomad, particularly around the use of theNOMAD_TASK_DIR
variable.This PR provides a check but doesn't prefix the path parameter.
cc @schmichael @eikenb