Strong type libartifact #522
Conversation
|
it should be noted that in addition to what my commit comments are, this will replace #463 ... but only a portion of it. i will address the broader issues in subsequent prs. |
|
✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6565 |
| @@ -1,36 +1 @@ | |||
| github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2lLoLwho= | |||
There was a problem hiding this comment.
This looks drastic (and potentially affects our ability to detect malicious tag changes) — if tools were complaining, go work sync && go work vendor might be sufficient.
| if err != nil { | ||
| return ArtifactReference{}, err | ||
| } | ||
| ar.ref = named |
There was a problem hiding this comment.
(Absolutely non-blocking: return ArtifactReference{ref:named}, nil would work.)
| _, isTagged := named.(reference.Tagged) | ||
| _, isDigested := named.(reference.Digested) | ||
| if !isTagged && !isDigested { | ||
| named = reference.TagNameOnly(named) | ||
| } |
There was a problem hiding this comment.
reference.TagNameOnly already does the checks, it can be called unconditionally.
| // | ||
| // quay.io/podman/machine-os:latest | ||
| // quay.io/podman/machine-os | ||
| // quay.io/podman/machine-os@sha256:916ede4b2b9012f91f63100f8ba82d07ed81bf8a55d23c1503285a22a9759a1e |
There was a problem hiding this comment.
Please document whether repo:tag@digest references are valid (and if not, make sure they are rejected)
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestNewArtifactReference(t *testing.T) { |
There was a problem hiding this comment.
(Non-blocking: a table-driven test would be a bit shorter / easier to follow.)
| // validate and see if the input is a digest | ||
| // lookupArtifact looks up an artifact by name or by full or partial ID. | ||
| // note: lookupArtifact must be called while under a store lock | ||
| func (as ArtifactStore) lookupArtifact(ctx context.Context, asr ArtifactStoreReference) (*libartifact.Artifact, error) { |
There was a problem hiding this comment.
(I’d name this lookupArtifactLocked — but style in this package is up to you and others.)
| } | ||
| srcRef, err := alltransports.ParseImageName("docker://" + name) | ||
| func (as ArtifactStore) Pull(ctx context.Context, ref ArtifactReference, opts libimage.CopyOptions) (digest.Digest, error) { | ||
| srcRef, err := alltransports.ParseImageName("docker://" + ref.String()) |
There was a problem hiding this comment.
This is preexisting:
docker.NewReference(ref.ref)would do the same thing cheaper- Using either API, this rejects repo:tag@digest inputs. If they should be rejected, that should happen in
NewArtifactReference; if they should be accepted, that needs to be implemented somewhere around here. [FWIWlayout.NewReferencedoes allow repo:tag@digest strings.]
There was a problem hiding this comment.
great, i took bullet one and addressed bullet two in the other spots which should cover this I think?
| } | ||
| destRef, err := alltransports.ParseImageName("docker://" + dest) | ||
| func (as ArtifactStore) Push(ctx context.Context, src, dest ArtifactReference, opts libimage.CopyOptions) (digest.Digest, error) { | ||
| destRef, err := docker.NewReference(dest.ref) |
There was a problem hiding this comment.
(See Pull for repo:tag@digest, although that’s almost certainly a user error here and, I think, not worth handling specially here.)
There was a problem hiding this comment.
I think that is covered with a change to the type constructor.
| } | ||
|
|
||
| func getArtifactAndImageSource(ctx context.Context, as ArtifactStore, nameOrDigest string, options *libartTypes.FilterBlobOptions) (*libartifact.Artifact, types.ImageSource, error) { | ||
| func getArtifactAndImageSource(ctx context.Context, as ArtifactStore, asr ArtifactStoreReference, options *libartTypes.FilterBlobOptions) (*libartifact.Artifact, types.ImageSource, error) { |
There was a problem hiding this comment.
This should probably document that the store must be locked…
| func (as ArtifactStore) BlobMountPaths(ctx context.Context, nameOrDigest string, options *libartTypes.BlobMountPathOptions) ([]libartTypes.BlobMountPath, error) { | ||
| arty, imgSrc, err := getArtifactAndImageSource(ctx, as, nameOrDigest, &options.FilterBlobOptions) | ||
| func (as ArtifactStore) BlobMountPaths(ctx context.Context, asr ArtifactStoreReference, options *libartTypes.BlobMountPathOptions) ([]libartTypes.BlobMountPath, error) { | ||
| arty, imgSrc, err := getArtifactAndImageSource(ctx, as, asr, &options.FilterBlobOptions) |
There was a problem hiding this comment.
… and doesn’t this need to lock the store, then? Same for the other 2 callers below.
There was a problem hiding this comment.
im not going to deal with locks in this PR. I'm just trying to get the typing done so people (crio) can start consuming it.
|
Given the API change, do we need a Podman PR lined up before merging this, to minimize drift/breakage? |
Yup!. I have a working branch that will just need a little TLC with the recent changes. Ill probably have done so by the time this merges. |
Created two new types ArtifactReference and ArtifactStorageReference and switch the relevant exported functions to use these types. ArtifactReference is a theoretical reference to an artifact. It is essentially a named. As such, anything a named can be, so can this. This type is used in functions like pull and add. ArtifactStorageReference is also a theoretical reference to an artifact but to an artifact in the local store. The input to this type can be similar to an ArtifactReference. But it can also be a full or partial digest (sometimes called an ID) of the artifact in the store. It is used in functions like inspect and remove. GetByNameOrDigest was removed we now lookup artifacts using an unexported function (for now) as an object of the store. In my previous PR, issues outside the scope of typing were pointed out and while they are valid, I would like to address these issues in separate PRs. This PR does not address: * any locking issues that exist in artifacts * tests for the store or primary artifact functions Signed-off-by: Brent Baude <bbaude@redhat.com>
|
I think I have address all the review comments that are non-locking based, reserving those for later. |
3 participants
Created two new types ArtifactReference and ArtifactStorageReference and switch the relevant exported functions to use these types.
ArtifactReference is a theoretical reference to an artifact. It is essentially a named. As such, anything a named can be, so can this. This type is used in functions like pull and add.
ArtifactStorageReference is also a theoretical reference to an artifact but to an artifact in the local store. The input to this type can be similar to an ArtifactReference. But it can also be a full or partial digest (sometimes called an ID) of the artifact in the store. It is used in functions like inspect and remove.
GetByNameOrDigest was removed we now lookup artifacts using an unexported function (for now) as an object of the store.
In my previous PR, issues outside the scope of typing were pointed out and while they are valid, I would like to address these issues in separate PRs. This PR does not address: