Skip to content
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 RFC requirement for mountLabel #934

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mashimiao
Copy link

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

Copy link
Contributor

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is adding a new runtime MUST, but I think it's still a patch-level change because our semver only protects config authors. The semantics of mountLabel have not changed, and there would be no need for config authors to adjust configurations to account for this PR. The only thing that changes is that authors of compliant runtimes will have to add mountLabel support if they were missing it already (or begin erroring out in create for configurations that set mountLabel if they choose to continue to not support it, #813, #927).

config-linux.md Outdated
@@ -629,7 +629,8 @@ The following parameters can be specified to set up seccomp:

## <a name="configLinuxMountLabel" />Mount Label

**`mountLabel`** (string, OPTIONAL) will set the Selinux context for the mounts in the container.
**`mountLabel`** (string, OPTIONAL) specifies SELinux context for the mounts in the container.
If `mountLabel` is set, the runtime MUST set SELinux context of the mounts in the container to the given value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to keep “the” before “SELinux context”.

@Mashimiao
Copy link
Author

ping @opencontainers/runtime-spec-maintainers

@hqhq
Copy link
Contributor

hqhq commented Nov 30, 2017

LGTM

Approved with PullApprove

config-linux.md Outdated
@@ -629,7 +629,8 @@ The following parameters can be specified to set up seccomp:

## <a name="configLinuxMountLabel" />Mount Label

**`mountLabel`** (string, OPTIONAL) will set the Selinux context for the mounts in the container.
**`mountLabel`** (string, OPTIONAL) specifies the SELinux context for the mounts in the container.
If `mountLabel` is set, the runtime MUST set the SELinux context of the mounts in the container to the given value.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You want to drop this indent (I've made that mistake before, #936 ;).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, it seems we should submit a PR to fix other places.

Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao
Copy link
Author

@opencontainers/runtime-spec-maintainers PTAL

wking added a commit to wking/opencontainer-runtime-spec that referenced this pull request Dec 1, 2017
Like 5d9bbad (config: Dedent root paragraphs, since they aren't a
list entry, 2017-11-08, opencontainers#936).  Ma Shimiao pointed out that there may
be more instances of this [1], and this commit fixes all instances
turned up with:

  $ git grep -B1 '^    [^ ]' | grep -A1 '[:-]\*\*'

[1]: opencontainers#934 (comment)

Signed-off-by: W. Trevor King <wking@tremily.us>
@TomSweeneyRedHat
Copy link

LGTM

Copy link
Contributor

@wking wking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this change in the short term, although I'd prefer a more comprehensive fix for this issue. I think we need something like #680 for properties, something that addresses #813 for values (#927 is a partial fix), something that tightens the “cannot apply a property as specified” loophole, and per-property docs that point out exactly what is being checked for that property.

The new line here doesn't add more detail about what is being checked for this property. It seems like the new line is a per-property alternative to #680?

@Mashimiao
Copy link
Author

@opencontainers/runtime-spec-maintainers PTAL

@vbatts
Copy link
Member

vbatts commented Dec 17, 2019

NACK
I'm 👍 with @wking #934 (review) here.
This would be a conditional MUST (the config generated should've accounted for whether selinux on the host is even supported). Which is needed for the trust model of handing down to the container runtime.
But to add a MUST here now, would be a revision in the spec and would be a compatibility issue for the community to need to update to using this newer version of the spec.

@h-vetinari
Copy link
Contributor

@vbatts: But to add a MUST here now, would be a revision in the spec and would be a compatibility issue for the community to need to update to using this newer version of the spec.

Wouldn't that compatibility issue arise anyway due to deprecating the prestart hooks?

@h-vetinari h-vetinari mentioned this pull request Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants