Create --append flag to add file to existing artifact using podman artifact add command

[Honny1]

This PR creates a --append flag to add a file to an existing artifact using the podman artifact add command.

Fixes: https://issues.redhat.com/browse/RUN-2444

Does this PR introduce a user-facing change?

`podman artifact add` has new option: `--append`

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Honny1
Once this PR has been reviewed and has the lgtm label, please assign ashley-cui for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[Honny1]

@baude After the rebase, I reworked the code and extended the tests to test the --append option with the --annotation option and made the --append option incompatible with the artifact --type option.

[Honny1]

cc @baude @Luap99 @mtrmac

[Luap99]

just some minor comments, mostly LGTM

[Luap99]

This should be put into a const block, and maybe name it ManifestSchemaVersion so we now want schema it is talking about.

[mtrmac]

I’m not truly convinced this is worth making a constant. For specV1.Manifest.Version, the only valid value is 2, and that’s unlikely to change (a change would certainly mean a new MIME type and probably a new Go type).

It’s not a parameter that can be tuned without changing the type references.

The clean location where this constant should exist is somewhere in the opencontainers/image-spec repo.

All that said, now that the code to declare a constant exists, I’m not going to ask that it should be replaced by a hard-coded 2.

[Luap99]

Suggested change

	return nil, fmt.Errorf("file: %s already exist in artifact", fileName)
	return nil, fmt.Errorf("file %q already exist in artifact", fileName)

[Luap99]

Do we need to read the index? AFAICT artifact.GetDigest() should return the same digest.

That said it seems GetDigest() reads the full manifest again and calculates the digest so using the index like you did might actually be the best thing here

[Honny1]

Yes, you're right when I tried to explain why I'm doing it there. I've found that my approach is not a good one.

[Luap99]

Not blocking but generally error checks should be combined with ExitWithError(code, "msg") so it only takes one line and so it is not forgotten to check the error message.

same in the other places below

[Luap99]

pre existing, what I just noticed in my extract PR is that this must copy the map here.
Reason is the map is shared so each iteration is overwriting the title and then all files end up with the same title. Looks like like we have a testing gap there, I am happy to push at as separate PR after this though.

[Honny1]

I moved the cloning of annotations to the loop where we insert files into the store. I'll add a check to test this case.

[Honny1]

https://github.com/containers/podman/pull/25179/files#diff-3b46c282a493fbd1962639614c6f4f023bda2836ae663ccd51d4be8b2fd96e03R223

[mtrmac]

If two artifacts happen to have the same contents, and the same manifest (but differ in AnnotationRefName), this would delete both, not just the one being updated.

AFAICS the NewReference call always uses an AnnotationRefName-based reference, and in that case PutManifest replaces the previous manifest with the same name, already.

So, I think this only does something if there are two manifests with the same digest (== if this loses data). If the only manifest with oldDigest was replaced in the index, it will not be found by this List.

= I think this never does what we want.

---

I do think there is something we need to do — the original manifest blob should be deleted. But that should be an automatic part of imageDest.PutManifest (or Commit?), not something higher-level callers should need to do explicitly.

[Honny1]

PutManifest takes the instanceDigest argument. With this argument, we can replace the contents of the previous manifest. Still, the file will not be renamed so that the filename will be the original digest and the original digest will remain in the index.json repository.

In the case of adding multiple files to the artifact as separate runs of the command, the digest will need to be retrieved from index.json.

I think this is not a good approach. WDYT? @mtrmac

---

Current implementation: PutManifest with nil instanceDigest creates a new manifest file and writes a new entry to index.json. If the name of the new item already exists in index.json, specV1.AnnotationRefName is removed from the existing entry, and a new item with the name is inserted into index.json. This causes two artifacts to exist, one with a name (the latest version) and one without a name (the previous name). Therefore, I decided to remove the artifact according to the digest. I will extend this by checking if an empty name is present. To ensure that there are no duplicate artifacts with different names.

[mtrmac]

PutManifest takes the instanceDigest argument. With this argument, we can replace the contents of the previous manifest.

I don’t understand this at all. (The instanceDigest parameter exists to support multi-platform images, typically so that when writing “to a tag”, the tag is not moved by writes of the per-platform manifests, but only by the write of the final top-level one. It is both used by the generic c/image/copy code, and a part of the public stable API; we can’t just redefine its meaning for one transport. [We could add a new transport-specific API, the way PutBlobFromLocalFile was added for artifacts.])

Anyway, the implementation of PutManifest knows whether/what it is replacing.

Still, the file will not be renamed so that the filename will be the original digest

That doesn’t work. The OCI data storage does not (apart from the top level) differentiate between manifests and data, it is possible for the same file (same digest) to be both a layer in one manifest, and a manifest elsewhere. The contents of the blob must match the digest.

---

Current implementation: PutManifest with nil instanceDigest creates a new manifest file and writes a new entry to index.json. If the name of the new item already exists in index.json, specV1.AnnotationRefName is removed from the existing entry, and a new item with the name is inserted into index.json. This causes two artifacts to exist, one with a name (the latest version) and one without a name (the previous name). Therefore, I decided to remove the artifact according to the digest. I will extend this by checking if an empty name is present. To ensure that there are no duplicate artifacts with different names.

That might be correct enough for the artifact store, but it is incorrect in general (the OCI layout store is not prohibited from containing N entries pointing to the same digest, some with no name).

Really, c/image/oci/layout is responsible for keeping the layout consistent. And it, nowadays, has code to reference-count the blobs and to tell whether there are any other users, i.e. whether the overwritten manifest can/should be deleted.