func: Allow custom paths to be added the the getter landlock #20349
Conversation
|
|
There was a problem hiding this comment.
This is excellent @astudentofblake! Just had a few minor nitpicks to clear up and then we can get this merged
client/config/artifact.go
Outdated
| @@ -9,6 +9,7 @@ import ( | |||
|
|
|||
| "github.com/dustin/go-humanize" | |||
| "github.com/hashicorp/nomad/nomad/structs/config" | |||
| "golang.org/x/exp/slices" | |||
There was a problem hiding this comment.
This can just be "slices" now; as of Go 1.21 I think
There was a problem hiding this comment.
I've fixed these.
"golang.org/x/exp/slices" still exists in 2 other places outside this review, there seems to be a semgrep rule to catch these, but I didn't notice an error
nomad/structs/config/artifact.go
Outdated
| "github.com/hashicorp/nomad/helper/pointer" | ||
| "github.com/shoenig/go-landlock" | ||
| "golang.org/x/exp/slices" |
| "p:a/b:r", | ||
| "p:a/c/drw", | ||
| "f:x/y:rw", |
There was a problem hiding this comment.
It's helpful for future maintainers when test data is at least plausible - in this case these paths are malformed (would trigger an error in landlock.ParsePath). Let's make these something that would be valid input (and in other tests).
There was a problem hiding this comment.
These should all now contain reasonable data
| - `filesystem_isolation_extra_paths` `([]string: nil)` - Allow extra paths | ||
| in the filesystem isolation. Paths are specified in the form "[kind]:[mode]:[path]" | ||
| e.g. "f:r:/dev/urandom" |
There was a problem hiding this comment.
We should clarify what are kind and mode - kind must be f or d (file or directory), mode must be r, w, c, x (read, write, create, execute). And then let's add a second example of a directory and different permissions, like d:rx:/opt/bin
There was a problem hiding this comment.
I've updated this with the extra info and tried to match the formatting.
nomad/structs/config/artifact.go
Outdated
| @@ -209,6 +227,12 @@ func (a *ArtifactConfig) Validate() error { | |||
| return fmt.Errorf("disable_filesystem_isolation must be set") | |||
| } | |||
|
|
|||
| for _, p := range a.FilesystemIsolationExtraPaths { | |||
| if _, err := landlock.ParsePath(p); err != nil { | |||
| return fmt.Errorf("filesystem_isolation_extra_paths contains invalid lockdown path %s", p) | |||
There was a problem hiding this comment.
let's swap the %s for %q so the user input stands out in the error
fix: more meaningful examples fix: improve documentation fix: quote error output
* func: Allow custom paths to be added the the getter landlock Fixes: 20315 * fix: slices imports fix: more meaningful examples fix: improve documentation fix: quote error output
Fixes: #20315