This repository has been archived by the owner on Feb 24, 2020. It is now read-only.
stage0: use rootfs image permissions with overlay #1607
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@krnowak can you explain the mentioned future changes in "We hardcode the permissions for now because this part of the code will be removed soon."? |
We hardcode the permissions for now because this part of the code will be removed soon as part of building multiple stage1 images simultaneously (rkt#1420).
|
In |
When we use overlay, the permissions that end up in "rootfs/" are the ones in the upper directory. To fix it, we need to set them as they are in the image.
It was actually me who wrote that :D. @krnowak told me that code will be gone with the fix for #1420. I'll add it to the commit message.
Yes, I'll do that. |
iaguis
force-pushed
the
fix-overlay-rootfs-permissions
branch
from
a002132
to
f6ad96c
Compare
iaguis
force-pushed
the
fix-overlay-rootfs-permissions
branch
3 times, most recently
from
c80a1d3
to
1030e6f
Compare
| @@ -29,7 +29,8 @@ FTST_EMPTY_IMAGE_MANIFEST := $(FTST_EMPTY_IMAGE_DIR)/manifest | |||
|
|
|||
| TOPLEVEL_CHECK_STAMPS += $(FTST_FUNCTIONAL_TESTS_STAMP) | |||
| INSTALL_FILES += $(FTST_IMAGE_MANIFEST_SRC):$(FTST_IMAGE_MANIFEST):- $(FTST_INSPECT_BINARY):$(FTST_ACI_INSPECT):- $(FTST_EMPTY_IMAGE_MANIFEST_SRC):$(FTST_EMPTY_IMAGE_MANIFEST):- $(FTST_ACE_MAIN_IMAGE_MANIFEST_SRC):$(FTST_ACE_MAIN_IMAGE_MANIFEST):- $(FTST_ACE_SIDEKICK_IMAGE_MANIFEST_SRC):$(FTST_ACE_SIDEKICK_IMAGE_MANIFEST):- $(FTST_ECHO_SERVER_BINARY):$(FTST_ACI_ECHO_SERVER):- | |||
| CREATE_DIRS += $(FTST_IMAGE_DIR) $(FTST_IMAGE_ROOTFSDIR) $(FTST_EMPTY_IMAGE_DIR) $(FTST_EMPTY_IMAGE_ROOTFSDIR) $(FTST_IMAGE_TEST_DIRS) $(FTST_TEST_TMP) | |||
| CREATE_DIRS += $(FTST_IMAGE_DIR) $(FTST_EMPTY_IMAGE_DIR) $(FTST_EMPTY_IMAGE_ROOTFSDIR) $(FTST_IMAGE_TEST_DIRS) $(FTST_TEST_TMP) | |||
| INSTALL_DIRS += $(FTST_IMAGE_ROOTFSDIR):0755 | |||
There was a problem hiding this comment.
Isn't 0755 a default for directories anyway?
There was a problem hiding this comment.
Aha, sounds like yet another reason for deprecating the CREATE_DIRS variable.
|
One question above. The build system part is "ew!", but other parts LFAD. |
I read
|
|
I tested with overlay enabled on my machine (removing the It'd be great to run all the tests with and without overlay but Semaphore doesn't support overlay... |
We create the rkt-inspect.aci with a rootfs with permissions 755 and then check that those are 755 at execution time. Also, we check stage1's rootfs permissions, they should always be 750.
iaguis
force-pushed
the
fix-overlay-rootfs-permissions
branch
from
1030e6f
to
b154431
Compare
|
Added a comment about testing with overlay fs. |
|
LGTM but can you add a follow-up issue/PR for the overlay fs test (even if it is skipped on Semaphore)? |
|
Thanks! |
iaguis
added a commit
that referenced
this pull request
Oct 15, 2015
stage0: use rootfs image permissions with overlay
alban
added a commit
to alban/rkt
that referenced
this pull request
Oct 21, 2015
Setting the group of the rootfs to 'rkt' causes problems with user namespaces when the group is not mapped in the user namespace. It prevents the container from doing a 'mkdir' or a 'lstat' on /proc. See systemd/systemd#1585 Setting the group ownership of the rootfs to 'rkt' was done in rkt#1452 so that the command 'rkt status' could work as non-root. However, the rootfs should now be r+x by others since rkt#1607 so setting the group is not necessary anymore. This patch reverts a part of rkt#1602, therefore fixing the regression with user namespaces. 'rkt status' as non-root still works. Fixes rkt#1602
alban
added a commit
to alban/rkt
that referenced
this pull request
Oct 21, 2015
Setting the group of the rootfs to 'rkt' causes problems with user namespaces when the group is not mapped in the user namespace. It prevents the container from doing a 'mkdir' or a 'lstat' on /proc. See systemd/systemd#1585 Setting the group ownership of the rootfs to 'rkt' was done in rkt#1452 so that the command 'rkt status' could work as non-root. However, the rootfs should now be r+x by others since rkt#1607 so setting the group is not necessary anymore. This patch reverts a part of rkt#1602, therefore fixing the regression with user namespaces. 'rkt status' as non-root still works. Fixes rkt#1602
alban
added a commit
to alban/rkt
that referenced
this pull request
Oct 21, 2015
Setting the group of the rootfs to 'rkt' causes problems with user namespaces when the group is not mapped in the user namespace. It prevents the container from doing a 'mkdir' or a 'lstat' on /proc. See systemd/systemd#1585 Setting the group ownership of the rootfs to 'rkt' was done in rkt#1452 so that the command 'rkt status' could work as non-root. However, if the rootfs is r-x for others, setting the group should not be necessary. r-x for others was removed by rkt#1607 but I am adding it back. This patch reverts a part of rkt#1602, therefore fixing the regression with user namespaces. 'rkt status' as non-root still works. Fixes rkt#1602
alban
added a commit
to alban/rkt
that referenced
this pull request
Oct 21, 2015
Setting the group of the rootfs to 'rkt' causes problems with user namespaces when the group is not mapped in the user namespace. It prevents the container from doing a 'mkdir' or a 'lstat' on /proc. See systemd/systemd#1585 Setting the group ownership of the rootfs to 'rkt' was done in rkt#1452 so that the command 'rkt status' could work as non-root. However, if the rootfs is r-x for others, setting the group should not be necessary. r-x for others was removed by rkt#1607 but I am adding it back. This patch reverts a part of rkt#1602, therefore fixing the regression with user namespaces. 'rkt status' as non-root still works. Fixes rkt#1602
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we use overlay, the permissions that end up in "rootfs/" are the
ones in the upper directory. To fix it, we need to set them as they are
in the image.
Fixes #1581