-
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
Intial implementation for #11807 #11813
Conversation
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.
Overall this is looking really good @apollo13! I've left some comments based on our internal discussion of the RFC.
nomad/structs/structs.go
Outdated
// NamespaceCapabilities represents a set of capabilities allowed for this | ||
// namespace, to be checked at job submission time. | ||
type NamespaceCapabilities struct { | ||
EnabledTaskDrivers []string | ||
} |
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.
From our internal discussion of the RFC, I think we're going to want to have both a allowlist and a denylist. So can we add a DisabledTaskDrivers
here as well?
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.
Sure, how will those interact? exclusive or will allowlist allow and then denylist will deny from that set?
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 feels like the place where Apache's confusing order by allow,deny allow from all
logic fits. I think it should be you allow first, and then prune out of that set.
@tgross is that more in line with what you were thinking? (HCL parsing is weird at best, but it seems to work :D) |
Btw, this could also nicely save #6554 -- but we'd have to modify the structs a bit. Ie:
That said, given how dangerous most drivers can be I do not see much point in disabled drivers and would just keep an "allow list". I'd really love to fix #6554 with this as well |
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.
Example HCL spec:
name = "dev"
description = "Namespace for developers"
capabilities {
enabled_task_drivers = ["docker", "exec"]
disabled_task_drivers = ["raw_exec"]
}
Taking it for a spin:
$ nomad namespace apply ./namespace.hcl
Successfully applied namespace specification "dev"!
$ nomad namespace list
Name Description EnabledDrivers DisabledDrivers
default Default shared namespace * <none>
dev Namespace for developers docker,exec raw_exec
$ nomad namespace status dev
Name = dev
Description = Namespace for developers
Quota = <none>
EnabledDrivers = docker,exec
DisabledDrivers = raw_exec
So far so good!
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.
So I've had more time to think about this in relation to #6554 and I think a really elegant solution would just be to dispense the task driver plugins multiple times. As I understand it, you can kind of put whatever you want as the HCL block identifier for plugin configs. For drivers that have a concept of lower privilege modes, you could just dispense the driver twice with two different configs (thinking mostly of container drivers here like docker, podman, and containerd, where the driver is aware of flags that need to be monitored in the config). You could then only allow use of the privileged drivers in specific namespaces. This approach would also let you do this in a mostly opaque way, since Nomad doesn't need to know which machines or drivers provide privileged access, from its perspective its just another driver. |
Hm, yeah I like where you're going with that. (That work would be separate from this PR though, so that we can ship this feature independently.) |
For sure would be a distinct item. I was wanting to raise it here because I think that once this is merged #6605 can be solved almost entirely by existing features and functionality. |
@tgross Pushed a few more changes. There is one failing test (at least) in https://app.circleci.com/pipelines/github/hashicorp/nomad/19158/workflows/fddc3c0b-1b53-467e-87d5-b6aa50d77b9d/jobs/193573 -- I am not sure how to fix it? One way to fix it would be to adjust my error message about non-existing namespace to match the one |
And now I killed semgrep:
|
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 is looking great, @apollo13
And now I killed semgrep:
Weird! We're still experimenting with semgrep a bit so we won't consider that blocking this PR. I'll open an issue with upstream on that run. Edit: already done by @shoenig https://github.com/returntocorp/semgrep-action/issues/505
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.
I've left a few more comments. Thanks for your patience in refining this!
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.
LGTM!
I don't know why the website preview is skipping this build, but I've got our docs folks looking at it. In the meantime, I pulled the PR down locally and looked at the doc page updates and that looks good too.
Thanks for this contribution, @apollo13! This will ship in Nomad 1.3.0.
Wow this is great, thank you so much! |
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. |
Looks good so far:
@tgross Do you mind giving it an initial quick review -- if I am on the right track I will continue with it.
Refs #11807