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

prepare-root: allow sysroot.readonly=true with kernel cmdline ro #3316

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

ruihe774
Copy link
Contributor

As described in #3315.

Copy link

openshift-ci bot commented Sep 28, 2024

Hi @ruihe774. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@github-actions github-actions bot added the area/prepare-root Issue relates to ostree-prepare-root label Sep 28, 2024
@cgwalters
Copy link
Member

I'm not opposed to this, but I'd like to try a bit harder to reach another design where we can mount the root writable in the initramfs. Let's debate that in the issue #3315 I think to keep it in once place.

@ruihe774 ruihe774 changed the title prepare-root: add kernel cmdline ostree.prepare-root.readonly prepare-root: allow sysroot.readonly=true with kernel cmdline ro Oct 10, 2024
@cgwalters cgwalters enabled auto-merge October 10, 2024 12:45
@ruihe774 ruihe774 disabled auto-merge October 10, 2024 12:48
@ruihe774
Copy link
Contributor Author

Does MS_BIND | MS_REMOUNT create readonly bind-mount for readonly source dir? If not, it is necessary to check sysroot_currently_writable and add MS_RDONLY when performing bind-mounts.

@ruihe774
Copy link
Contributor Author

Does MS_BIND | MS_REMOUNT create readonly bind-mount for readonly source dir? If not, it is necessary to check sysroot_currently_writable and add MS_RDONLY when performing bind-mounts.

Ok. It just works. Feel free to merge it.

@ruihe774
Copy link
Contributor Author

ruihe774 commented Oct 10, 2024

Are there test cases that cover things in initramfs? I think I'd better add some tests to prevent regression. Especially, I have not tried ro with root_transient, etc_transient, and using_composefs yet.

@cgwalters
Copy link
Member

Ok. It just works. Feel free to merge it.

Yeah, bind mounts will inherit the writability by default.

Are there test cases that cover things in initramfs? I think I'd better add some tests to prevent regression.

There's definitely a coverage gap in some cases; we only test some of this implicitly. I've been meaning to try to make it easier to run ostree-prepare-root outside of a proper initramfs (and boot process), testing it as a container which would dramatically simplify complexity for testing.

Especially, I have not tried ro with root_transient, etc_transient, and using_composefs yet.

I think though your use case in the future is going to be better addressed by adding integrity and measurement at the OS level and not trying to enforce it via drive-specific writability - which gets into the composefs path specifically so we'd effectively be obsoleting the case of "ro with composefs we mount writable later" for example.

@cgwalters
Copy link
Member

/ok-to-test

@cgwalters cgwalters enabled auto-merge October 10, 2024 18:11
@cgwalters cgwalters merged commit f7018d8 into ostreedev:main Oct 10, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prepare-root Issue relates to ostree-prepare-root ok-to-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants