tarfs: follow hardlinks in ReadFile

[BradLugo]

Description

In ReadFile, we either follow symlinks or attempt to read the amount of data from the tar as specified by the Size field in tar.Header. When we come across a hardlink, however, we essentially tell the caller that the file is empty since the Size is 0.

These changes to ReadFile are largely based on the changes @hdonnay made to Open for handling hardlinks: 48317f8

[BradLugo]

When we come across a hardlink, however, we essentially tell the caller that the file is empty since the Size is 0.

I wanted to note that I don't know if this behavior is consistent. For example, perhaps there's a flag you can pass to tar when creating the archive that sets the size header for hardlinks.

[codecov]

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 55.76%. Comparing base (b28ae8c) to head (e8f9aff).
Report is 2 commits behind head on main.

Files	Patch %	Lines
pkg/tarfs/tarfs.go	61.53%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1305      +/-   ##
==========================================
- Coverage   55.79%   55.76%   -0.04%     
==========================================
  Files         265      265              
  Lines       16552    16562      +10     
==========================================
  Hits         9236     9236              
- Misses       6357     6365       +8     
- Partials      959      961       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[BradLugo]

The more I think about this PR, the more I wonder if this is the right approach. Open and ReadFile share a lot of the same logic, and the os library's ReadFile calls its Open (https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/os/file.go;l=768).

It makes me wonder if we should move the datasize logic to Open then call Open in ReadFile. The main problem with Open (IIUC) is that hardlinks still aren't technically implemented correctly, i.e., they return an inode with a tar.Header that has a Size of 0. That's not hugely problematic since the caller can follow the link themselves, but I think it goes against the idea of an inode

[RTann]

nit: filepath.Base unless there's a specific reason path is used?

[BradLugo]

It was copied from the run function in TestSymlinks 🤷

[hdonnay]

I think I used path throughout and swapped specific uses because io/fs mandates forward-slash paths and go just handles forward-slash paths, so it's moot unless the path could be absolute.

[RTann]

curious would all this just be equivalent to return f.ReadFile(i.h.Linkname)? If so, should we just combine this case and the symlink check into a single case?

[RTann]

The more I think about this PR, the more I wonder if this is the right approach. Open and ReadFile share a lot of the same logic, and the os library's ReadFile calls its Open (https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/os/file.go;l=768).

It makes me wonder if we should move the datasize logic to Open then call Open in ReadFile. The main problem with Open (IIUC) is that hardlinks still aren't technically implemented correctly, i.e., they return an inode with a tar.Header that has a Size of 0. That's not hugely problematic since the caller can follow the link themselves, but I think it goes against the idea of an inode

I like the idea of implementing this as closely to os.ReadFile as possible, especially since Open and ReadFile, as implemented here, share a lot of the same logic

Also, Open uses the hardlink's "target" size, no?

[BradLugo]

The more I think about this PR, the more I wonder if this is the right approach. Open and ReadFile share a lot of the same logic, and the os library's ReadFile calls its Open (https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/os/file.go;l=768).
It makes me wonder if we should move the datasize logic to Open then call Open in ReadFile. The main problem with Open (IIUC) is that hardlinks still aren't technically implemented correctly, i.e., they return an inode with a tar.Header that has a Size of 0. That's not hugely problematic since the caller can follow the link themselves, but I think it goes against the idea of an inode

I like the idea of implementing this as closely to os.ReadFile as possible, especially since Open and ReadFile, as implemented here, share a lot of the same logic

Also, Open uses the hardlink's "target" size, no?

Ya know... When I was digging into this before, I saw the note at the bottom of https://pkg.go.dev/archive/tar#Reader.Read and assumed the tarfs.file.h (i.e., the *tar.Header of the hardlink) we were setting was problematic, but after looking more thoroughly when writing my first response, I think you're right. Since we get *tar.Reader with the correct offset and sz, store it in tarfs.file.h, which (IIUC) is what tar.Reader.Read ultimately looks at, I think we avoid the case of getting (0, io.EOF) on hardlinks.

So never mind. Now I want to use Open even more

---

I guess it really comes down to the hint at the top of ReadFile:

claircore/pkg/tarfs/tarfs.go

Lines 452 to 454 in 813e7cc

	// ReadFileFS is implemented because it can avoid allocating an intermediate
	// "file" struct and can immediately allocate a byte slice of the correct
	// size.

I'm open to having my mind changed, but maintaining these two functions that are very similar but diverge due to the allocation of a tarfs.file struct in such a critical code path isn't worth it. The PR proves it. Maybe I'm missing something though (like maybe tarfs.file.Read would cause us to allocate the data twice? not sure)

[hdonnay]

LGTM

On the rebase, can I trouble you to rewrite the commit summary line to not have fix(tarfs) but rather just tarfs? We try to follow something closer to the go commit style than "conventional commits".

[hdonnay]

I'll look though how this is handled in #669

[BradLugo]

LGTM

On the rebase, can I trouble you to rewrite the commit summary line to not have fix(tarfs) but rather just tarfs? We try to follow something closer to the go commit style than "conventional commits".

Yup, will do!
Is this documented anywhere? If not, I can open up another PR and add it somewhere.

---

Before we merge this, do you mind giving your thoughts on #1305 (comment)? I wonder if we can/should remove ReadFile altogether

[crozzy]

LGTM
On the rebase, can I trouble you to rewrite the commit summary line to not have fix(tarfs) but rather just tarfs? We try to follow something closer to the go commit style than "conventional commits".

Yup, will do! Is this documented anywhere? If not, I can open up another PR and add it somewhere.

Before we merge this, do you mind giving your thoughts on #1305 (comment)? I wonder if we can/should remove ReadFile altogether

https://github.com/quay/claircore/blob/main/docs/contributor/commit-style.md

[hdonnay]

/fast-forward