tazjin / nixery

Container registry which transparently builds images using the Nix package manager. Canonical repository is https://cs.tvl.fyi/depot/-/tree/tools/nixery
https://nixery.dev/
Apache License 2.0
1.83k stars 69 forks source link

feat(storage): Store blob content-type in extended attributes #127

Closed jpetazzo closed 3 years ago

jpetazzo commented 3 years ago

After the discussion in #116, this stores the blob content types in extended attributes when using the filesystem backend.

If the underlying filesystem doesn't support extended attributes, storing blobs won't work; also, if extended attributes get removed, blobs won't be served anymore. We can relax this behavior if needed (i.e. log errors but still accept to store or serve blobs). However, since the Docker Engine (and possibly other container engines) won't accept to pull images from a registry that doesn't use correct content types for manifest files, it could be argued that it's better to give a hard fail. (Otherwise, the container engine gives cryptic error messages like "missing signature key".)

I can change that behavior (and log errors but still store/serve blobs to the filesystem) if you think it's better.

One last thing: I have very little knowledge of Go dependencies and Nix, so I hope I did the right thing here to add the dependency on pkg/xattr; but please double-check as it's my first time doing this :sweat_smile:

jpetazzo commented 3 years ago

Welp, the CI system doesn't support xattrs. :upside_down_face:

Should we try to change the CI system, or?

flokli commented 3 years ago

@jpetazzo I did some quick tinkering by manually spinning up a ubuntu:latest container on my machine:

❯ docker run --mount type=tmpfs,destination=/foo -it ubuntu:latest
root@152f82d17d81:/# apt update && apt install attr &> /dev/null
[…]
root@152f82d17d81:/# touch /foo/bar;setfattr -n "user.mime_type" -v "value" /foo/bar
setfattr: /foo/bar: Operation not supported
root@152f82d17d81:/# touch /root/bar;setfattr -n "user.mime_type" -v "value" /root/bar

Apparently, user xattrs on tmpfs aren't supported (see tmpfs(5)), and for other filesystems, the answer is "it depends" (xattr(7)).

I tinkered around with the CI a bit, and luckily the CI itself is backed by something supporting xattrs, it's just tmpfs doesn't support it.

I made https://github.com/google/nixery/commit/d2c007060c32882e12a2b0c044fd6dda7db6ab17.patch, which fixes CI, as can be seen in https://github.com/flokli/nixery/runs/2861162544.

Feel free to cherry-pick this on top.

flokli commented 3 years ago

I pushed that CI change to a PR at https://github.com/google/nixery/pull/128. If that is merged, this PR should become green after a rebase.

tazjin commented 3 years ago

Rebasing this PR on master should now resolve the CI issue, thanks to @flokli!

flokli commented 3 years ago

@jpetazzo can you press the rebase button?

jpetazzo commented 3 years ago

Thanks for the work on the CI! 👍🏻 I would have been happy to try and fix it, but I saw that there had been changes in the CI recently so I wasn't sure what was the current state. Looks like everything passes now, yay! 🎉

... And sorry for the slower turnaround on my end :)

tazjin commented 3 years ago

... And sorry for the slower turnaround on my end :)

I feel like I should write that sentence :sweat_smile: Thank you for the contribution!

I guess we don't need 7db252f36a68d875429a25e06d88fbfc804d84fd anymore after this and can probably revert it once merged?

jpetazzo commented 3 years ago

I guess we don't need 7db252f anymore after this and can probably revert it once merged?

Indeed! Would you like me to submit a PR that does both (i.e. with these commits + reverting the other one)?

tazjin commented 3 years ago

Feel free to send a separate CL reverting the old commit. Thanks for your contribution! :)