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 bootc build commit #216

Open
cgwalters opened this issue Dec 4, 2023 · 13 comments · Fixed by #381
Open

Add bootc build commit #216

cgwalters opened this issue Dec 4, 2023 · 13 comments · Fixed by #381
Labels
enhancement New feature or request

Comments

@cgwalters
Copy link
Collaborator

This should take over the role of ostree container commit, however following up on ostreedev/ostree-rs-ext#569 we can change our thoughts on /var.

@cgwalters cgwalters added the enhancement New feature or request label Dec 4, 2023
@vrothberg
Copy link
Member

Can you sketch out where bootc build commit would be called? Dockerfile/Buildah?

@cgwalters
Copy link
Collaborator Author

Yes as part of a RUN invocation

@jmarrero
Copy link
Member

jmarrero commented Dec 5, 2023

@cgwalters any reason for this not to be a pass-thru to ostree-ext for now?

@cgwalters
Copy link
Collaborator Author

One reason is we kind of just did a pivot around our handling of /var since ostreedev/ostree-rs-ext#569 - and while this needs more testing and validation - we may want to degrade the /var bits to a warning for example.

A tricky thing about making it a literal re-exec style passthrough is right now ostree container is shipped by rpm-ostree, and ultimately we want to support bootc-only images, so we probably want to expose the container commit processing as a Rust API. Or possibly we split it out into a separate component/package.

@cgwalters
Copy link
Collaborator Author

This issue also relates to #215 in that ultimately what we'd do here wouldn't involve ostree per se. But we'd need to then figure out something around the thorny issue of how we represent these xattrs (xref discussion in containers/storage#1608 )

@jmarrero
Copy link
Member

jmarrero commented Feb 28, 2024

Just a heads up that we are working this issue on our mobbing sessions. Specially based on this comment:
#217 (comment)

I am not sure if this is intended to intersect with: #365

@cgwalters
Copy link
Collaborator Author

I am not sure if this is intended to intersect with: #365

They're very different things, which argues for using different verbs.

@cgwalters
Copy link
Collaborator Author

I'm a bit uncertain about doing this versus offering a container image that can work as part of a multi-stage build and do this stuff. That's inherently maximally flexible and avoids shipping potentially all sorts of tools in smaller images.

i.e. we'd document:

FROM quay.io/someuser/useros:latest as rootfs
FROM quay.io/exampleos/bootc-image-tool
RUN --mount=type=bind,from=rootfs,target=/sysroot bootc-image-scan /sysroot

Now don't get me wrong, it does seem quite convenient to ship basic scanning stuff as part of the image itself so you can just stick a RUN bootc build-scan or whatever as part of your single image flow...but on the flip side, this isn't something that exists in the podman/docker ecosystem by default.

If we take the "bootc does one thing and does it well" philosophy then this may end up being outside it, right?

I could go either way...

BTW one thing that we should have a basic lint checker for is having a single kernel (like rpm-ostree does).

@jmarrero
Copy link
Member

jmarrero commented Mar 1, 2024

If we take the "bootc does one thing and does it well" philosophy then this may end up being outside it, right?

I can always be persuaded by the original Unix philosophy. I say we close this then.

@cgwalters
Copy link
Collaborator Author

I keep going back and forth honestly 😄

One thing here is that while we have a stance that bootc is not a build system (you can use any container build system) there are in reality some rules it enforces specifically (such as the "one kernel" rule).

Even if we had an external tool, it could call out to bootc build-lint or whatever we call it for that set of rules.

But we should probably think about what other things we need to lint for. There are potentially a lot...but I don't know how much is bootc specific in the end, versus "generic Linux" stuff.

prestist added a commit to prestist/bootc that referenced this issue Mar 7, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
prestist added a commit to prestist/bootc that referenced this issue Mar 7, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
prestist added a commit to prestist/bootc that referenced this issue Mar 14, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
prestist added a commit to prestist/bootc that referenced this issue Mar 14, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
prestist added a commit to prestist/bootc that referenced this issue Mar 21, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>
jmarrero added a commit to jmarrero/bootc that referenced this issue May 3, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>
jmarrero added a commit to jmarrero/bootc that referenced this issue May 3, 2024
Add initial lint functionality, the first lint
checks if there are multiple kernels in the image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>
Co-authored-by: Renata Ravanelli <[email protected]>
Co-authored-by: Adam Piasecki <[email protected]>
@cgwalters
Copy link
Collaborator Author

cgwalters commented May 5, 2024

OK another one we hit recently is where a user has a restrictive umask 077, and is doing a container build from a git clone that has a Dockerfile that is doing

COPY usr /usr

Since the copy will preserve permissions (without --chmod) then we end up overwriting /usr with permissions 0700 which will just break running any code as non-root.

I think we should probably lint against this by default. There's not a really good use case for having /usr be permissions 0700.

As for a larger fix for this case...well, it's probably not going to be viable in reality to force everyone doing git clones to reset the umask to 022.

A COPY --chmod=0755 will work, but will also make all files unnecessarily executable. The simplest thing may be to show a pattern of splitting up data and executables via e.g.:

COPY --chmod=0755 usr-exe /usr
COPY --chmod=0644 usr-data /usr

Where usr-exe holds executables, and usr-data holds non-executables.

There is also https://docs.docker.com/reference/dockerfile/#copy---link - but we still need to canonicalize the permissions in that case.

Hmm...COPY should probably have something like COPY --mode-inherit that has a semantic like:

  • Take the base read-write permissions from the parent directory of the upper layers by default
  • If the executable bit is set on user in the source, it is propagated to other and group by default

This would give us sane umask-independent semantics that hit the 95% case. There are of course use cases for things like setuid binaries being only executable by a specific user/group, but if you're doing that type of stuff you really should be explicitly setting the mode at install time for that specific binary.

prestist added a commit to prestist/bootc that referenced this issue May 7, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>
prestist added a commit to prestist/bootc that referenced this issue May 7, 2024
add simple lint to check that we only have one kernel in
image.

fixes: containers#216
Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>

Signed-off-by: Steven Presti <[email protected]>
cgwalters pushed a commit to prestist/bootc that referenced this issue May 30, 2024
Add an entrypoint that basically everyone can start
adding to their builds which performs some basic
static analysis for known problems.

Closes : containers#216

Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>

Signed-off-by: Steven Presti <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
cgwalters pushed a commit to prestist/bootc that referenced this issue May 30, 2024
Add an entrypoint that basically everyone can start
adding to their builds which performs some basic
static analysis for known problems.

Closes : containers#216

Co-authored-by: Joseph Marrero <[email protected]>
Co-authored-by: Huijing Hei <[email protected]>
Co-authored-by: Yasmin de Souza <[email protected]>

Signed-off-by: Steven Presti <[email protected]>
Signed-off-by: Colin Walters <[email protected]>
@vrothberg
Copy link
Member

@cgwalters, I feel this issue isn't actionable yet. Shall we groom it or is it a WIP?

@travier travier reopened this Jun 6, 2024
@travier
Copy link
Member

travier commented Jun 6, 2024

Looks like #381 helps but does not fully fixes this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants