chunked: honor the ForceMask setting #1971

giuseppe · 2024-06-13T10:33:10Z

make sure the ForceMask setting is honored for partial pulls. More details in each commit.

openshift-ci · 2024-06-13T10:33:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2024-06-15T01:36:13Z

LGTM

cgwalters

Looks sane to me, just one comment/question.

cgwalters · 2024-06-18T12:06:05Z

pkg/chunked/storage_linux.go

+			if r.Xattrs == nil {
+				r.Xattrs = make(map[string]string)
+			}
+			r.Xattrs[idtools.ContainersOverrideXattr] = base64.StdEncoding.EncodeToString([]byte(value))


It feels a bit unclean to mutate the input here; I'd trust you if you said it was safe, but still.

In ostree we have something very similar to the ContainersOverrideXattr in the "bare-user" mode vs "bare" and the code paths in the backend are pretty distinct. Doing something similar here would want instead for e.g. setFileAttrs to take an override parameter or so perhaps.

This should be safe because mergedEntries is an array of values (i.e. copies), created from scratch for this function. … ugh, except that a hash is a reference type, so AFAICS this might be affecting the c.toc field.

Ultimately, that is still safe because c.toc is written at most once, and read exactly once; but the number of assumptions is getting a bit large.

In Rust:

xattrs would likely be an Option<T> instead of a pointer, so the compiler would have made us check for its presence

We likely would have been passing a &TableOfContents or so, and the compiler would have errored out on an attempt to mutate it

And if we chose to pass instead &mut TableOfContents the compiler would verify that there weren't other threads (goroutines) that were concurrently reading it, etc.

But we can often do something better than mutation for something like this; Rust has efficient iterators over collections, and in this case instead of mutation it'd likely be better to leave the input read-only and instead pass impl Iterator<Item=Xattr> to the function that sets xattrs (instead of an array). It's efficient to do e.g. toc.xattrs.iter().chain(Xattr::new(idtools.CONTAINER_OVERRIDE_XATTR, value)) (although this would blend borrowed and owned values so we may need a Cow or so)

I’d sign up to be a platinum member of the Go haters club, but that’s not really under discussion in this PR. (If you wanted to make a case for rewriting the stack in Rust, I’d not oppose that at all.)

I do agree that this would be safer with a deep copy, but then … Go doesn’t have a language-native deep copy operation either. So it’s either risk of bugs due to overwriting data (if it ever turned from single use into multiple-use) or risk of bugs due to missing a field copy (if we ever added a field), and to me that feels like a wash, up to @giuseppe .

mtrmac

The ForceMask parts LGTM.

pkg/idtools/idtools.go

drivers/overlay/overlay.go

mtrmac · 2024-06-18T14:11:13Z

drivers/overlay/overlay.go

 	}
 	if idMappings == nil {
-		idMappings = &idtools.IDMappings{}
+		idMappings = idtools.NewIDMappingsFromMaps(d.uidMaps, d.gidMaps)


Warning: I don’t understand the whole picture at all.

Shouldn’t this happen at a (much?) higher level? Looking at store.putLayer, it is done there — and canUseShifting is involved in the decision.

I have no idea which of the two is correct, to be clear.

I never understood this part as well :-) I took the chance now to dig deeper into it...

I think we are carrying the feature from Docker (e5f58bf) where it was possible to configure a single uid/gid mapping for all the containers, so the mappings are configured only once for the driver.

In facts this is the setting we get when we configure remap-uids and remap-gids in the storage.conf file.

I'll drop this patch from the current PR and remove the remap-*ids logic completely in a different PR

opened a test PR to see how far the CI goes: #1976

If I read the code correctly, the d.[ug]idMaps fields are never used when applying tarballs; and the generic code which triggers shifting or per-container remapping does not know about this option; that makes it really hard to see a use case.

Dealing with that in a separate PR LGTM.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

mtrmac · 2024-06-19T14:11:02Z

drivers/overlay/overlay.go

 	if options != nil {
 		idMappings = options.Mappings
+		forceMask = options.ForceMask


Should this be conditioned on options.ForceMask != nil?

(One point is that the semantics is not documented. Another is that this field is never set when called by c/image, anyway. And yet again, if it is not used, why is it a field of ApplyDiffWithDifferOpts at all…)

AFAICS ApplyDiffOpts.ForceMask is never set when creating non-staged/chunked layers, so maybe we can just remove the filed and simplify?

(Running on macOS, apparently I can’t trust the IDE:) ApplyDiff does

if d.options.forceMask != nil { options.ForceMask = d.options.forceMask }

the opposite of this logic… but, also, the field is not used on that path at all, either.

Removing ApplyDiffOpts.ForceMask looks increasingly attractive to me.

thanks for spotting it. I'd say the options should have a higher precedence, but I've adapted it to follow what ApplyDiff does.

Maybe something more to simplify along the lines of #1976

mtrmac · 2024-06-19T14:15:36Z

pkg/chunked/storage_linux.go

+			if r.Xattrs == nil {
+				r.Xattrs = make(map[string]string)
+			}
+			r.Xattrs[idtools.ContainersOverrideXattr] = base64.StdEncoding.EncodeToString([]byte(value))


I’d sign up to be a platinum member of the Go haters club, but that’s not really under discussion in this PR. (If you wanted to make a case for rewriting the stack in Rust, I’d not oppose that at all.)

I do agree that this would be safer with a deep copy, but then … Go doesn’t have a language-native deep copy operation either. So it’s either risk of bugs due to overwriting data (if it ever turned from single use into multiple-use) or risk of bugs due to missing a field copy (if we ever added a field), and to me that feels like a wash, up to @giuseppe .

cgwalters · 2024-06-19T14:43:21Z

I’d sign up to be a platinum member of the Go haters club, but that’s not really under discussion in this PR.

In general, everyone should feel free to not respond to my Rust comments...it's partially just cathartic 😄, but also partially about tracking pain points we see in practice.

(If you wanted to make a case for rewriting the stack in Rust, I’d not oppose that at all.)

Now this is totally tangential but I think as you know I've been digging into this repo and also just thinking about how to sanely intersect the ostree/container stuff (which is all in Rust today, although speaking to a C library for substantial parts) with composefs and that led to containers/composefs#286 which could grow into a composefs-native container storage backend...but...yeah, a lot involved there. Rewriting anything is a big risky lift, I don't think it's the right time. But...we could do something like add a /usr/libexec/composefs-container-storage in Rust to composefs that gets forked from c/storage (much like how the composefs C binaries are today) and speak to it via IPC or so. Dunno, anyways: obviously very tangential.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

and also fix a segfault when the xattr map is nil Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

rhatdan · 2024-06-20T14:55:39Z

LGTM

mtrmac · 2024-06-20T15:16:06Z

drivers/overlay/overlay.go

+	forceMask := options.ForceMask
+
 	if options != nil {


Either the first line can crash, or the second one is unnecessary.

thanks, opened a PR: #1984

openshift-ci bot added the approved label Jun 13, 2024

giuseppe force-pushed the chunked-honors-force-mask branch from 10010bc to 1f2d694 Compare June 13, 2024 10:33

cgwalters reviewed Jun 18, 2024

View reviewed changes

mtrmac reviewed Jun 18, 2024

View reviewed changes

giuseppe added 2 commits June 19, 2024 11:22

idtools: move format override xattr to new function

f37aea9

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: use FormatContainersOverrideXattr

e717273

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the chunked-honors-force-mask branch from 1f2d694 to 116897a Compare June 19, 2024 10:03

mtrmac reviewed Jun 19, 2024

View reviewed changes

giuseppe added 2 commits June 19, 2024 23:20

overlay: pass down ForceMask to chunked

eec2a60

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

chunked: set correctly the xattr for ForceMask

f35833c

and also fix a segfault when the xattr map is nil Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>

giuseppe force-pushed the chunked-honors-force-mask branch from 116897a to f35833c Compare June 19, 2024 21:20

rhatdan merged commit 768258b into containers:main Jun 20, 2024
17 of 18 checks passed

mtrmac reviewed Jun 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chunked: honor the ForceMask setting #1971

chunked: honor the ForceMask setting #1971

giuseppe commented Jun 13, 2024

openshift-ci bot commented Jun 13, 2024

rhatdan commented Jun 15, 2024

cgwalters left a comment

cgwalters Jun 18, 2024

mtrmac Jun 18, 2024

cgwalters Jun 19, 2024

mtrmac Jun 19, 2024

mtrmac left a comment

mtrmac Jun 18, 2024

giuseppe Jun 19, 2024 •

edited

Loading

giuseppe Jun 19, 2024

mtrmac Jun 19, 2024

mtrmac Jun 19, 2024

mtrmac Jun 19, 2024

giuseppe Jun 19, 2024

mtrmac Jun 19, 2024

cgwalters commented Jun 19, 2024

rhatdan commented Jun 20, 2024

mtrmac Jun 20, 2024

giuseppe Jun 21, 2024

chunked: honor the ForceMask setting #1971

chunked: honor the ForceMask setting #1971

Conversation

giuseppe commented Jun 13, 2024

openshift-ci bot commented Jun 13, 2024

rhatdan commented Jun 15, 2024

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jun 19, 2024

rhatdan commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe Jun 19, 2024 •

edited

Loading