pkg/archive, pkg/tarsum: replace use of Xattrs with PAXRecords #46707

corhere · 2023-10-23T20:29:33Z

Fixes pkg/archive: replace use of Xattrs with PAXRecords #39924

- What I did
Replaced use of Xattrs with PAXRecords.

- How I did it
Extended the pkg/archive unit tests to verify that these changes do not introduce any regressions in xattr handling. And I checked that the tests fail as expected when the handling of extended attributes is deliberately broken. (No changes to the pkg/tarsum tests were required.)

- How to verify it
CI is green.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

The existing pkg/archive unit tests are primarily round-trip tests which assert that pkg/archive produces tarballs which pkg/archive can unpack. While these tests are effective at catching regressions in archiving or unarchiving, they have a blind spot for regressions in compatibility with the rest of the ecosystem. For example, a typo in the capabilities extended attribute constant would result in subtly broken image layer tarballs, but the existing tests would not catch the bug if both the archiving and unarchiving implementations have the same typo. Extend the test for archiving an overlay filesystem layer to assert that the overlayfs style whiteouts (extended attributes and device files) are transformed into AUFS-style whiteouts (magic file names). Extend the test for archiving files with extended attributes to assert that the extended attribute is encoded into the file's tar header in the standard, interoperable format compatible with the rest of the ecosystem. Signed-off-by: Cory Snider <csnider@mirantis.com>

Signed-off-by: Cory Snider <csnider@mirantis.com>

Fix a silly bug in the implementation which had the effect of len(h.Xattrs) blank entries being inserted in the middle of orderedHeaders. Luckily this is not a load-bearing bug: empty headers are ignored as the tarsum digest is computed by concatenating header keys and values without any intervening delimiter. Signed-off-by: Cory Snider <csnider@mirantis.com>

Signed-off-by: Cory Snider <csnider@mirantis.com>

thaJeztah · 2023-10-24T09:26:47Z

Thanks for working on this! I recall I started working on this (just found an old branch where I started some bits thaJeztah@400be67), but wasn't sure what the best way was to verify compatibility, and looks like I never returned to continuing that work 😓

thaJeztah

LGTM

thaJeztah · 2023-10-24T09:37:03Z

pkg/tarsum/versioning.go

+	var xattrs [][2]string
+	for k, v := range h.PAXRecords {
+		if xattr, ok := strings.CutPrefix(k, paxSchilyXattr); ok {
+			xattrs = append(xattrs, [2]string{xattr, v})


Ah, smart to construct the key/value pair here instead of further down (I was looking here as to "what changed?" but now see this is to prevent the loop at line 134 👍

Interesting; looks like BuildKit forked this code, and now I'm wondering if the implementation should be shared (the tarsum specification looks to be in this package);

moby/vendor/github.com/moby/buildkit/cache/contenthash/tarsum.go

Lines 38 to 76 in 452ca90

func v1TarHeaderSelect(h *tar.Header) (orderedHeaders [][2]string) {

pax := h.PAXRecords

if len(h.Xattrs) > 0 { //nolint:staticcheck // field deprecated in stdlib

if pax == nil {

pax = map[string]string{}

for k, v := range h.Xattrs { //nolint:staticcheck // field deprecated in stdlib

pax["SCHILY.xattr."+k] = v

}

}

}

// Get extended attributes.

xAttrKeys := make([]string, 0, len(h.PAXRecords))

for k := range pax {

if strings.HasPrefix(k, "SCHILY.xattr.") {

k = strings.TrimPrefix(k, "SCHILY.xattr.")

if k == "security.capability" || !strings.HasPrefix(k, "security.") && !strings.HasPrefix(k, "system.") {

xAttrKeys = append(xAttrKeys, k)

}

}

}

sort.Strings(xAttrKeys)

// Make the slice with enough capacity to hold the 11 basic headers

// we want from the v0 selector plus however many xattrs we have.

orderedHeaders = make([][2]string, 0, 11+len(xAttrKeys))

// Copy all headers from v0 excluding the 'mtime' header (the 5th element).

v0headers := v0TarHeaderSelect(h)

orderedHeaders = append(orderedHeaders, v0headers[0:5]...)

orderedHeaders = append(orderedHeaders, v0headers[6:]...)

// Finally, append the sorted xattrs.

for _, k := range xAttrKeys {

orderedHeaders = append(orderedHeaders, [2]string{k, h.PAXRecords["SCHILY.xattr."+k]})

}

return

}

pkg/tarsum/versioning.go

thaJeztah · 2023-10-24T09:51:39Z

@tonistiigi PTAL (also curious what we should do with the tarsum package; would it make sense to make it its own module (together with the specification?)

thaJeztah · 2023-10-26T19:37:32Z

Discussed this in the maintainers call, and current implementation looks good, but perhaps Cory will do a follow-up to merge instead of replace

corhere added 4 commits October 23, 2023 16:21

pkg/archive: migrate to (tar.Header).PAXRecords

3cf409a

Signed-off-by: Cory Snider <csnider@mirantis.com>

hack/validate: stop suppressing Xattrs deprecation

52da882

Signed-off-by: Cory Snider <csnider@mirantis.com>

corhere added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Oct 23, 2023

corhere requested a review from thaJeztah October 23, 2023 20:29

corhere requested a review from tianon as a code owner October 23, 2023 20:29

thaJeztah approved these changes Oct 24, 2023

View reviewed changes

thaJeztah marked this pull request as draft October 26, 2023 19:36

thaJeztah marked this pull request as ready for review October 26, 2023 19:36

thaJeztah merged commit 7cabe08 into moby:master Oct 26, 2023
104 checks passed

corhere mentioned this pull request Oct 26, 2023

pkg/tarsum: handle xattrs like archive/tar does #46736

Merged

thaJeztah added this to the 25.0.0 milestone Nov 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/archive, pkg/tarsum: replace use of Xattrs with PAXRecords #46707

pkg/archive, pkg/tarsum: replace use of Xattrs with PAXRecords #46707

corhere commented Oct 23, 2023

thaJeztah commented Oct 24, 2023 •

edited

Loading

thaJeztah left a comment

thaJeztah Oct 24, 2023

thaJeztah Oct 24, 2023

thaJeztah commented Oct 24, 2023

thaJeztah commented Oct 26, 2023

	func v1TarHeaderSelect(h *tar.Header) (orderedHeaders [][2]string) {
	pax := h.PAXRecords
	if len(h.Xattrs) > 0 { //nolint:staticcheck // field deprecated in stdlib
	if pax == nil {
	pax = map[string]string{}
	for k, v := range h.Xattrs { //nolint:staticcheck // field deprecated in stdlib
	pax["SCHILY.xattr."+k] = v
	}
	}
	}

	// Get extended attributes.
	xAttrKeys := make([]string, 0, len(h.PAXRecords))
	for k := range pax {
	if strings.HasPrefix(k, "SCHILY.xattr.") {
	k = strings.TrimPrefix(k, "SCHILY.xattr.")
	if k == "security.capability" \|\| !strings.HasPrefix(k, "security.") && !strings.HasPrefix(k, "system.") {
	xAttrKeys = append(xAttrKeys, k)
	}
	}
	}
	sort.Strings(xAttrKeys)

	// Make the slice with enough capacity to hold the 11 basic headers
	// we want from the v0 selector plus however many xattrs we have.
	orderedHeaders = make([][2]string, 0, 11+len(xAttrKeys))

	// Copy all headers from v0 excluding the 'mtime' header (the 5th element).
	v0headers := v0TarHeaderSelect(h)
	orderedHeaders = append(orderedHeaders, v0headers[0:5]...)
	orderedHeaders = append(orderedHeaders, v0headers[6:]...)

	// Finally, append the sorted xattrs.
	for _, k := range xAttrKeys {
	orderedHeaders = append(orderedHeaders, [2]string{k, h.PAXRecords["SCHILY.xattr."+k]})
	}

	return
	}

pkg/archive, pkg/tarsum: replace use of Xattrs with PAXRecords #46707

pkg/archive, pkg/tarsum: replace use of Xattrs with PAXRecords #46707

Conversation

corhere commented Oct 23, 2023

thaJeztah commented Oct 24, 2023 • edited Loading

thaJeztah left a comment

Choose a reason for hiding this comment

thaJeztah Oct 24, 2023

Choose a reason for hiding this comment

thaJeztah Oct 24, 2023

Choose a reason for hiding this comment

thaJeztah commented Oct 24, 2023

thaJeztah commented Oct 26, 2023

thaJeztah commented Oct 24, 2023 •

edited

Loading