-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add granular control of SELinux labels for host mounts #19839
Conversation
0783286
to
d79c4cd
Compare
a1facca
to
ea9a103
Compare
34d4683
to
04c759f
Compare
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 for the jobspec addition. But I think we want to add support for this flag with the docker
driver (and maybe exec
/java
drivers too?) by making sure it's provided to the mounts the driver sets up (ex. driver.go#L1181-L1186
)
Also, I think this might close out #9123 too. Probably a good idea to do a quick search thru open issues to see if there are others.
demo/csi/hostpath/redis.nomad
Outdated
@@ -42,4 +42,4 @@ job "example" { | |||
} | |||
} | |||
} | |||
} | |||
} |
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.
Little bit of debris to cleanup 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.
I cant remove it :s
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.
You can remove it if you flatten the commits. Or you can update your editor config so that you make sure it's always adding the newline EOF (you want that for posix-compliance... run wc -l
on this file and you'll see it's one line short!)
err := tc.volMount.Validate() | ||
if !errors.Is(err, tc.expectedErr) { | ||
t.Fatalf("expected error %v, got %v", tc.expectedErr, err) | ||
} |
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.
can be shortened to something like must.ErrorIs(t, tc.expectedErr, err)
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.
must.ErrorIs doesn't work here, probably something with the unwarpping of the errors
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!
mErr.Errors = append(mErr.Errors, fmt.Errorf("Volume Mount (%d) references undefined volume %s", idx, vm.Volume)) | ||
} else if err := vm.Validate(); err != nil { | ||
mErr.Errors = append(mErr.Errors, fmt.Errorf("Volume Mount (%d) is invalid: \"%w\"", idx, err)) |
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.
Do you have an example of how this error looks like? I'm not sure why we do it in some many parts of the code, but appending directly to mErr.Errors
usually results in a bad error hierarchy.
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 is actually called in the task group validation test, it should not be a problem
nomad/structs/volumes.go
Outdated
mErr.Errors = append(mErr.Errors, errVolMountEmptyVol) | ||
} | ||
|
||
if !v.MountPropagationModeIsValid() { | ||
mErr.Errors = append(mErr.Errors, fmt.Errorf("%w: %q", errVolMountInvalidPropagationMode, v.PropagationMode)) | ||
} | ||
|
||
if !v.SELinuxLabelIsValid() { | ||
mErr.Errors = append(mErr.Errors, fmt.Errorf("%w: \"%s\"", errVolMountInvalidSELinuxLabel, v.SELinuxLabel)) |
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.
mErr.Errors = append(mErr.Errors, errVolMountEmptyVol) | |
} | |
if !v.MountPropagationModeIsValid() { | |
mErr.Errors = append(mErr.Errors, fmt.Errorf("%w: %q", errVolMountInvalidPropagationMode, v.PropagationMode)) | |
} | |
if !v.SELinuxLabelIsValid() { | |
mErr.Errors = append(mErr.Errors, fmt.Errorf("%w: \"%s\"", errVolMountInvalidSELinuxLabel, v.SELinuxLabel)) | |
mErr= multierror.Append(mErr, errVolMountEmptyVol) | |
} | |
if !v.MountPropagationModeIsValid() { | |
mErr = multierror.Append(mErr, fmt.Errorf("%w: %q", errVolMountInvalidPropagationMode, v.PropagationMode)) | |
} | |
if !v.SELinuxLabelIsValid() { | |
mErr= multierror.Append(mErr, fmt.Errorf("%w: \"%s\"", errVolMountInvalidSELinuxLabel, v.SELinuxLabel)) |
Since this is new code, we may as well use the nicer method 😄
Co-authored-by: Tim Gross <[email protected]>
Co-authored-by: Tim Gross <[email protected]>
770b2dc
to
1cb6ef0
Compare
Co-authored-by: Luiz Aoqui <[email protected]>
Currently when using the podman task driver plugin if the
selinuxlabel = "z"
option is present in the volume configuration, all the host volume mounts will be forced to use SELinux shared volume context.There is no way for operators to use the csi-hostpath plugin or any type of mount that is not compliant with the SELinux security, or mount a private volume. This PR introduces a new configuration option that can be set per volume mount, allowing to set different SELinux context to different volumes.
There will also be a PR on the nomad-driver-podman to read and apply the new configuration.
Closes: