feat: Add OCI Artifact support to the Podman REST API #26111

ninja-quokka · 2025-05-12T12:08:17Z

This patch adds a new endpoint to the REST API called "artifacts" with the following methods:

Add
Extract
Inspect
List
Pull
Push
Remove

This API will be utilised by the Podman bindings to add OCI Artifact support to our remote clients.

Jira: https://issues.redhat.com/browse/RUN-2711

Does this PR introduce a user-facing change?

Added artifacts endpoint to Podman REST API

cmd/podman/artifact/add.go

Luap99 · 2025-05-13T10:01:11Z

pkg/api/handlers/libpod/artifacts.go

+		RetryDelay string `schema:"retryDelay"`
+		TLSVerify  bool   `schema:"tlsVerify"`
+	}{
+		TLSVerify: true,


What is the point to define a default here if you are going to check
if _, found := r.URL.Query()["tlsVerify"]; found { below anyway, seems unnecessary

It's an existing pattern from the images api, please confirm I understand what you mean.

You are saying that because we have a default value that will get overridden by the Decode() if a user sets the tlsVerify parameter we don't need to then check later with r.URL.Query() and can just set artifactsPushOptions.SkipTLSVerify = types.NewOptionalBool(!query.TLSVerify)

I've made this change, let me know what you think

Well actually that is hard coding the default to true. But if it is not set we should really be using undefined instead so the backend code can pick right defaults.

As of 0c4d023 you can actually just use types.OptionalBool directly in the query so not need to convert that then

Right! I get you now!

I still need to convert the true to false because the two vars we are mapping are not in sync sadly: InsecureSkipTLSVerify vs TLSVerify BUT if it's not defined by the user, I will leave it as OptionalBoolUndefined and let the backend code handle the default.

Luap99 · 2025-05-13T10:02:24Z

pkg/api/handlers/libpod/artifacts.go

+	if len(query.Name) == 0 {
+		utils.InternalServerError(w, errors.New("name parameter cannot be empty"))
+		return


That should not be a server error? Clearly the client gave us a invalid name so it should be a 400 error

Also why is this the only endpoint which such check? And why is this a query param? Should the name not be part of the URL like done on push and our other pull endpoints.

I think it would be more correct to have the name in the path rather than as a query but I'm matching up with imagePull endpoint for this one which has the "reference" in the query.

Happy to change it but would be best discussed on the Change Request IMO.

I fixed the return code and added the check to the other endpoints that take the name as a query.

pkg/api/server/register_artifacts.go

pkg/domain/infra/abi/artifact.go

Luap99 · 2025-05-13T10:20:26Z

pkg/libartifact/store/store.go

 		if err != nil {
 			return nil, err
 		}

 		// If we did not receive an override for the layer's mediatype, use
 		// detection to determine it.
 		if len(mediaType) < 1 {
-			mediaType, err = determineManifestType(path)
+			mediaType, err = determineManifestType(artifact.Blob)


How can that work? PutBlob() reads from the io.Reader which advances the point where it will read from. And since it will read until EOF there is nothing that could be read here again to get the filetype.

Not sure how to handle this, either we must pass in an ReadSeeker so we seek back to the beginning (which likely would not work for the artifact add API as we like to consume the stream directly) or wrap this into another reader where we can read the first 512 bytes, i.e. bufio.NewReader() then we can call .Peek() on that to get the first 512 bytes and store that. And of course that would need to happen before we call PutBlob()

Yeah, this is one of the biggest gotchas when using the io.Reader to stream the data.

This didn't work and was just using the fallback default each time.

I have fixed the function by using a bufio.NewReader() Peek on the 512 bytes.

I also added tested for both detecting a mediaType and using the fallback.

Luap99 · 2025-05-13T10:23:56Z

pkg/libartifact/store/store.go

+	reader, writer := io.Pipe()
+
+	go func() {


would it be better to accept an io.Writer as argument instead of returning a reader.

Then this function can directly write to the target (socket) and we don't have another read/write cycle in the caller. And it would mean we can remove the need for the goroutine here

Done, that's much cleaner. I initially implemented it that way but thought I needed to return an io.Reader.

Thanks for the heads up that Write calls WriteHeader(http.StatusOK) before writing the data. If the Header does not contain a Content-Type line, Write adds a Content-Type set to the result of passing the initial 512 bytes of written data to [DetectContentType].

Luap99 · 2025-05-13T10:28:05Z

pkg/libartifact/store/store.go

+	// Get io.Reader from src
+	f, ok := src.(*os.File)
+	if !ok {
+		return fmt.Errorf("failed to open blob: %w", err)
+	}


That is not really a API contract c/image gives us here, if the returned reader is no longer a file this will break us hard. I don't see why the file permissions we use in the layout store would be of interest. We should likely just hard code the same permission instead.

Hey @Luap99, I agree, what file permissions would you suggest to hard code here?

0600 as mode should be enough I guess.
Would we even want to set uid/gid and the timestmaps? Not sure if it is valid to just leave them empty.

I'll look into what's mandatory.

I see what you mean now, the API contract is for an io.ReadCloser, it's not promised to be an os.File in which case stat would fail etc.

So now I craft the tar header manually with the minimum information.

I add the timestamps because if not, it defaults to epoch and doesn't look very nice.

Please let me know what you think!

That seems fine.

Honny1 · 2025-05-13T15:35:59Z

pkg/api/handlers/libpod/artifacts.go

+		return
+	}
+
+	name := utils.GetName(r)


Could the name be part of the query structure? As in PullArtifact.

I'm happy to change it, the reason I did it like these is just to mimic the image pull and push endpoints. This might be something we can discuss on the Change Request document?

Luap99 · 2025-05-14T13:23:31Z

pkg/libartifact/store/store.go

+func determineManifestType(blob io.Reader) (io.Reader, string, error) {
+	maxBytes := 512
+
+	peekBuffer := bufio.NewReader(blob)


Logic wise I think it would be clearer if you accept blob *bufio.Reader as argument and don't reader the reader again. The the caller just has to call bufio.NewReader(blob)

Imposing a buffer on all future I/O, when we don’t have a particular opinion, seems unnecessary (IIRC the HTTP stack already uses a buffer internally, so this is pure overhead).

Other code uses io.MultiReader(bytes.NewReader(start), rest), eg. compression.DetectCompressionFormat.

I wasn't aware of the MultiReader() trick, sure we can use that.
That said assuming the caller already has a *bufio.Reader then bufio.NewReader() will see that and not create yet another buffer over it, well at least as long as the buffer size matches.

The HTTP stack uses a bufio internally, but the .Body is often wrapping it — in a LimitReader to support pipelining, or in a private type to support Transfer-Encoding: Chunked. So this would probably really allocate a new one.

Luap99 · 2025-05-14T13:30:24Z

pkg/libartifact/store/store.go

+		blobInfo, err := imageDest.PutBlob(ctx, artifact.Blob, blobInfo, none.NoCache, false)
+		if err != nil {
+			return nil, err
+		}


I just noticed switching to PutBlob() means it will be unable to reflink the file into the artifact store during add meaning we double the storage space. It is not clear how we can avoid that though if we accept a normal io.Reader API.

@mtrmac Any ideas?

… and determineManifestType will (either via bufio or via MultiReader break the visibility of the underlying io.File anyway.

I think it’s fair to have two code paths, one with PutBlobFromLocalFile, one with PutBlob, and there are various ways to structure this:

Accept an io.Reader, and do a type check on *io.File. “A transparent optimization” but it could break in invisible ways

Make this explicit in entities.ArtifactBlob — carry an io.Reader and *io.File side by side, documenting that exactly one of them must be set.

I’d prefer the latter. It looks ugly but it makes the trade-off / lost optimization explicitly visible.

Now, determineManifestType. If we have two code paths, we can obviously also two code paths for this autodetection, one working on seekable files and one on single-use streams.

But, I’m generally suspicious about heuristics. WDYT about making the MIME type a mandatory parameter on almost all layers of the stack (libartifact, infra/abi, infra/tunnel), and moving the guessing heuristics … well, all the way up to the CLI layer?

BTW determineManifestType uses http.DetectContentType. That’s … such a web-focused thing, that I don’t really see the point.

I mean, either users care about MIME types, or they don’t. If they don’t, slapping octet-stream on it might not be so bad.

On overall structure, I'm inclined to say we should have separate PutBlob and PutBlobFromLocalFile - it seems simpler to eliminate all questions as to whether we can apply optimizations like reflinks.
On MIME type: I think that sounds like a separate PR, but I would like to see it, overriding MIME type at addition via CLI parameter sounds valuable, but would also need to be added to local codepaths and this PR is big enough as it is.

Okay, I will update the code so a call from the CLI uses PutBlobFromLocalFile() and a call from the API uses PutBlob()

I will also update determineManifestType() so as to not break reflinking from a CLI call path.

@mtrmac Do you have any performance results on bufio VS MultiReader? They both need to return an io.Reader that can be used after the initial read is done however bufio's peek is specifically designed for tasks like we are attempting here so I would rather use it.

In a follow up PR I can add the option to set the MIME type of the file, with a fall back to determineManifestType() which in turn falls back to octet-stream

But, I’m generally suspicious about heuristics. WDYT about making the MIME type a mandatory parameter on almost all layers of the stack (libartifact, infra/abi, infra/tunnel), and moving the guessing heuristics … well, all the way up to the CLI layer?

I don't know, the thing is we should look it at the POV from an API only user. And making the client guess the mime type seems suboptimal compared to letting the server do it. Mandating the the mime type is mandatory makes the API harder to use for other clients.

overriding MIME type at addition via CLI parameter sounds valuable

That is what --file-type already does so there is already an option exposed.

@mtrmac Do you have any performance results on bufio VS MultiReader?

I’d be curious to see them, but… that feels obvious? The fast path of MultiReader is just an indirect call to the next underlying reader in line, about half a dozen pointer reads + comparisons.

bufio.Reader.Read can’t possibly do better than that. It could, in theory, do much worse (always copying to its internal buffer, and always copying out) — luckily, it is optimized to avoid that.

So, reading the code, I think the difference should round to ~negligible.

(To be fair, originally I was thinking that MultiReader must be clearly superior in all respects, and that’s not the case, because bufio.Reader might have better support for ReadFrom/WriteTo. But then again, that doesn’t apply here.)

But, I’m generally suspicious about heuristics. WDYT about making the MIME type a mandatory parameter on almost all layers of the stack (libartifact, infra/abi, infra/tunnel), and moving the guessing heuristics … well, all the way up to the CLI layer?

I don't know, the thing is we should look it at the POV from an API only user. And making the client guess the mime type seems suboptimal compared to letting the server do it.

Is it an API break if the servers’ heuristic (== net/http’s heuristic) changes? That’s… difficult, right?

Mandating the the mime type is mandatory makes the API harder to use for other clients.

Making it optional and hard-coding to application/octet-stream would make it just as easy for clients who just don’t care.

Making it optional and hard-coding to application/octet-stream would make it just as easy for clients who just don’t care.

Possibly yeah, I would not mind moving this detection out of libartifact code. @baude added this originally not sure if there was a specific reason. If cmd/podman does the work there it could simplify things.

Accept an io.Reader, and do a type check on *io.File. “A transparent optimization” but it could break in invisible ways

I think that is the somewhat idiomatic go way, if you look into the std the io code there are ton of type checks to try to use optimized interfaces whenever possible.

Using two arguments always runs into the issue what if there are both given, I miss a proper enum type for something like that in go to actually prefer that.

if you look into the std the io code there are ton of type checks to try to use optimized interfaces whenever possible.

The standard library also has various unit tests to ensure the fast paths get triggered.

Here … would we have noticed that determineManifestType wraps the *os.File and breaks the type check? By now that’s hypothetical, but I’m worried that without tools to enforce properties, we are going to miss things, or break them over time. (Of course, one possible answer is to keep the simpler API and add a unit test.)

pkg/api/server/register_artifacts.go

pkg/libartifact/store/store.go

mtrmac

Looking at the API, as the part that would be very hard to change … but I’ll fully defer to API experts, and being consistent with other APIs is valuable.

pkg/api/handlers/libpod/artifacts.go

pkg/libartifact/store/store.go

mheon · 2025-05-14T19:29:08Z

cmd/podman/artifact/add.go

+	for _, blobPath := range blobs {
+		b, err := os.Open(blobPath)
+		if err != nil {
+			return fmt.Errorf("error opening path %s: %w", blobPath, err)


Don't start errors with "error" as that will get added automatically when it's printed. "opening path %s: %w" should be more than sufficient.

mheon · 2025-05-14T19:31:31Z

pkg/api/handlers/libpod/artifacts.go

+
+	"github.com/containers/image/v5/types"
+	libartifact_types "github.com/containers/podman/v5/pkg/libartifact/types"
+


Prefer leaving out empty lines in imports except between standard library and everything else (IE line 9 should stay, 12, 15, 17 should go). That lets gofmt order all the imports.

Thanks, done.

pkg/api/handlers/libpod/artifacts.go

mheon · 2025-05-14T19:32:50Z

pkg/api/handlers/libpod/artifacts.go

+	if errors.Is(err, libartifact_types.ErrArtifactNotExist) {
+		utils.ArtifactNotFound(w, name, err)
+		return
+	}


Probably should else-if the err != nil bit here

mheon · 2025-05-14T19:35:04Z

pkg/api/handlers/libpod/artifacts.go

+	runtime := r.Context().Value(api.RuntimeKey).(*libpod.Runtime)
+	imageEngine := abi.ImageEngine{Libpod: runtime}
+
+	name := utils.GetName(r)


Probably worth a dedicated error for "name was not provided" if name == ""

I don't think that is needed here because name is provided on the path

mheon · 2025-05-14T19:36:27Z

pkg/api/handlers/libpod/artifacts.go

+		return
+	}
+
+	// FIX: Should we verify r.body isn't empty here? It's hard as it's streaming


Adding an empty file to an artifact should be valid, no? I think there's no need to worry

I agree this is valid, and it should work. (Maybe worth having as a test case.)

ack, thanks guys!

mheon · 2025-05-14T19:38:24Z

pkg/api/handlers/libpod/artifacts.go

+		ArtifactType: query.Type,
+	}
+
+	artifactBlobs := []entities.ArtifactBlob{{ // FIX: Should this be a pointer?


All Go arrays are reference types, so you ought to be fine

I agree this is clearly fine.

… Go slices are value types carrying shared references to the underlying array. That sharing is potentially very risky, when a slice is passed across API boundaries and both copies are then used to do modifications. (That includes the situation where both copies are independently passed to append.)

mheon · 2025-05-14T19:39:40Z

pkg/api/handlers/libpod/artifacts.go

+
+	imageEngine := abi.ImageEngine{Libpod: runtime}
+
+	// FIX: push currently just returns an empty struct, should we return the digest?


ninja-quokka · 2025-05-15T06:31:26Z

@mheon @mtrmac @Luap99 @Honny1 thanks for the reviews guys, I have implemented most changes but I didn't finish today so I will push tomorrow.

mtrmac · 2025-05-15T18:26:47Z

@ninja-quokka if you didn’t push the changes yet, please don’t mark review comments as resolved — it makes it harder to track what needs re-checking.

(I’m not sure about the PR author marking comments as resolved — I could go either way depending on situation, and anyway the opinion of regular Podman reviewers is much more important.)

pkg/domain/infra/abi/artifact.go

mtrmac · 2025-05-20T17:28:53Z

pkg/libartifact/store/store.go

+	if srcSize == -1 {
+		src.Close()
+		return fmt.Errorf("failed to get artifact blob size")
+	}
+	defer src.Close()


Well…

I agree that for reading blobs from regular files, the API doesn’t explicitly promise returning a value, but, in practice, it will always do so, and it would make no sense to make that conditional; I think that having a Podman test that exercises this code path at least once would ensure that c/image doesn’t change without us noticing, and it would be good enough. (Or we could formally add that commitment to c/image oci/layout’s GetBlob implementation.)

But, curiously enough, oci/layout.GetBlob can trigger a HTTP request if the manifest contains alternative URLs. And in that case the size might really be unavailable.

That’s… surprising, and something that the Podman artifacts code needs to handle somehow, and, also, unrelated to this PR. I guess c/image should provide an opt-out of oci/layout causing network traffic? We could also switch the order to check local files first; that would, pedantically, be an API break.

ninja-quokka · 2025-05-27T13:03:21Z

Hi folks,

Could I please get another review pass over this patch? I believe the current CI failures are flakes unrelated to this patch.

@mtrmac I can't respond to your feedback about srcSize directly, is that a change you would like made in this PR or in a follow up?

Adding @baude to the party as well 🎉

/cc @baude

mtrmac · 2025-05-27T16:43:23Z

@mtrmac I can't respond to your feedback about srcSize directly, is that a change you would like made in this PR or in a follow up?

The “trigger HTTP request” aspect of #26111 (comment) is separate, but I’d like that tracked, and fixed with a measure of urgency. I have, at least, filed #26215 so that it doesn’t immediately get forgotten.

baude · 2025-05-27T18:58:28Z

pkg/api/handlers/libpod/artifacts.go

+	case types.OptionalBoolTrue:
+		artifactsPullOptions.InsecureSkipTLSVerify = types.NewOptionalBool(false)
+	case types.OptionalBoolFalse:
+		artifactsPullOptions.InsecureSkipTLSVerify = types.NewOptionalBool(true)


should there be a fall through and something like an error here?

baude · 2025-05-27T18:59:44Z

pkg/api/handlers/libpod/artifacts.go

+	if err != nil {
+		var errcd errcode.ErrorCoder
+		if errors.As(err, &errcd) {
+			rc := errcd.ErrorCode().Descriptor().HTTPStatusCode


would love // comments in here as to these conditions. maybe im on the only like that.

Yeah I will add comments, it's not super clear.

WDYT about using named constants like http.StatusUnauthorized instead of hard-coding numbers? That might even be self-documenting enough that the separate comments wouldn’t be necessary.

Paying more attention to what is going on here, one thing to consider is that

c/image doesn’t quite promise that returned errors match github.com/docker/distribution/registry/api/errcode

just recently, github.com/docker/distribution released a new major version, and a version of these types will be github.com/distribution/distribution/v3/… .

Now, that puts us in a somewhat awkward position in that we either keep using an obsolete and unmaintained package, or break (an undocumented aspect of an) API.

I don’t know what we are going to do here longer-term; one thing I’d recommend doing right now is checking for c/image/docker.ErrUnauthorizedForCredentials instead of 401 via the ErrCode dependency. That’s not a comprehensive solution because c/image does not provide a stable API for “manifest not found” yet — that would need to be added there.

(Pedantically, ErrorCode().Descriptor().HTTPStatusCode documents status codes that a registry server should return in these situations. They don’t strictly speaking are defined to affect what a container engine HTTP API should return, although there is obviously some similarity.)

@mtrmac Great call the on the named constants, much more readable.

Regarding using c/image/docker.ErrUnauthorizedForCredentials, I don't see that error in the wrapped error tree returned from imageEngine.ArtifactPull() [1]

I used the errcode method because it's what we use in image pull. Not to say that changes anything, just to let you know the dependency is else where as well.

[1]

- *fmt.wrapError: initializing source docker://quay.io/libpod_secret/testartifact:latest: reading manifest latest in quay.io/libpod_secret/testartifact: unauthorized: access to the requested resource is not authorized - *fmt.wrapError: reading manifest latest in quay.io/libpod_secret/testartifact: unauthorized: access to the requested resource is not authorized - errcode.Error: unauthorized: access to the requested resource is not authorized

My mistake, ErrUnauthorizedForCredentials is returned when the credentials are invalid, i.e. when the user uses some credentials

The quoted error seems to be when the user did not try to log in at all, it’s just the repo’s access that is refused (and the same error is returned by Quay when the repo outright does not exist[1]). So, that’s a different case, and currently needs to be determined by looking for the errcode.Error value.

([1] Given this ambiguity, Podman might want to be careful about promising to return specific error types for “not found” vs. “not authorized” from its API, because various registries intentionally conflate the two. I suppose doing the same thing that the “image pull” code does is, by definition, acceptable … but that code, AFAICS, doesn’t make (or at least doesn’t fulfill) such a promise.)

baude · 2025-05-27T19:02:21Z

pkg/api/handlers/libpod/artifacts.go

+	case types.OptionalBoolTrue:
+		artifactsPushOptions.SkipTLSVerify = types.NewOptionalBool(false)
+	case types.OptionalBoolFalse:
+		artifactsPushOptions.SkipTLSVerify = types.NewOptionalBool(true)


do we need a fallthrough?

Maybe?

query.TLSVerify can only be OptionalBoolTrue, OptionalBoolFalse or OptionalBoolUndefined. We handle the first two because we have to switch the logic and we pass OptionalBoolUndefined to be handled by the backend code as suggested by Paul: #26111 (comment)

I looked into this some more and it's quite confusing where the default would be defined in the backend code.

If you would like I can add a switch for OptionalBoolUndefined and have it set artifactsPushOptions.SkipTLSVerify = types.NewOptionalBool(false) so the default from the handler would be having TLS enabled.

The reason we must pass undefined in the backed is because registries.conf allows user to specify insecure option on a per registry basis, for example in newDockerClient() in reads the config file and sets the options accordingly. If a caller always hards code true or false then this gets always overwritten and the config file is ignored which would be a bug.
As undefined is the default anyway for this option I don't think we must handle the case here explicitly

Yes, undefined must map to undefined, as Paul says.

Ideally, I’d like there to be a default: that explicitly fails, or explicitly turns on TLS, for any unexpected values. And I’d like the logic centralized into a single helper, not copy&pasted all over the place; it’s easy enough to miss a ! somewhere.

Compare #26111 (comment) .

That’s probably out of scope of this PR, or at least the “centralized logic” part is.

Thanks folks, I added a default case that will fall back to enabling TLS if unexpected handling occurred.

baude · 2025-05-27T19:04:27Z

when reviewing this, i wondered about adding events for artifacts. @Luap99 wdyt .... if images, containers, etc have them, should this as part of a follow on PR?

Luap99 · 2025-05-28T10:08:34Z

when reviewing this, i wondered about adding events for artifacts. @Luap99 wdyt .... if images, containers, etc have them, should this as part of a follow on PR?

Yes that is out of scope but given things like podman desktop, cockpit, etc... depend on such things to keep the UI up to date I guess we need it sooner than later. (That assume that they implement artifact support)

packit-as-a-service · 2025-06-01T22:10:10Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

mtrmac · 2025-06-02T18:13:10Z

pkg/domain/infra/abi/artifact.go

+	artifactDigest, err := artStore.Pull(ctx, name, *pullOptions)
+	if err != nil {
+		return nil, err
+	}
+
+	return &entities.ArtifactPullReport{
+		ArtifactDigest: &artifactDigest,
+	}, nil


This is rather late…

Shouldn’t the pull + push code be, from the start, designed for reporting progress in the body stream, instead of blocking and only returning a single success/failure indication at the end? Compare #24887 .

Hi @mtrmac, I planned for reporting the progress in the body stream to be a follow up PR to keep this one as simple as I could.

I know this sounds like a bad idea as changing APIs should be avoided but it should be okay as Podman artifacts are currently in an experimental stage.

That might be fine, this PR is already large enough as it is; I’ll defer to Podman experts to decide how they want to track/structure this.

I'm fine with multiple PRs. I'd prefer to have it cleaned up by 5.6, but there's plenty of time between now and August

I will create a card for adding body streaming to the push and pull API

baude · 2025-06-03T13:24:25Z

LGTM

Luap99 · 2025-06-03T13:48:57Z

pkg/api/handlers/libpod/artifacts.go

+	default: // Should never happen, require TLS, to be conservative
+		logrus.Errorf("Unexpected handling occurred for TLSVerify, falling back to requiring TLS")
+		artifactsPullOptions.InsecureSkipTLSVerify = types.NewOptionalBool(false)
+	}


This seems wrong and adds exactly the issue back I was mentioned before. The undefined case is not handled which means you hard code undefined to be false.
You would need an empty undefined case for this to make sense.

Then (a whole different topic) server side logs are basically never caught anywhere in CI tests which we won't notice this problem either. I think logically if we this cannot happen a panic() might be more reasonable to signal a invariant that should never happen.

Yes you are right, I added an empty undefined case and updated the default to now panic rather than log because it really should never happen.

Luap99 · 2025-06-03T13:50:22Z

pkg/api/handlers/libpod/artifacts.go

+		ArtifactMIMEType string   `schema:"artifactMIMEType"`
+		Append           bool     `schema:"append"`
+	}{
+		Append: false,


the default bool value is false anyway so this seems unnecessary

Luap99 · 2025-06-03T13:51:55Z

pkg/api/handlers/libpod/artifacts.go

+	default: // Should never happen, require TLS, to be conservative
+		logrus.Errorf("Unexpected handling occurred for TLSVerify, falling back to requiring TLS")
+		artifactsPushOptions.SkipTLSVerify = types.NewOptionalBool(false)
+	}


same comment as on the other part, if it is done twice I would argue it should be split into a helper function as well.

I will create the helper do this in a follow up PR and update the other occurrences of this pattern in code base to use it if suitable :)

Luap99 · 2025-06-03T13:55:56Z

pkg/libartifact/store/store.go

+		err = copyTrustedImageBlobToTarStream(ctx, imgSrc, digest, filename, tw)
+		if err != nil {
+			return err
+		}


isn't this missing an early return here,w e should only be copying that signle match not go over all layers below again

Yes, good catch, I added the early return here thank you!

This patch adds a new endpoint to the REST API called "artifacts" with the following methods: - Add - Extract - Inspect - List - Pull - Push - Remove This API will be utilised by the Podman bindings to add OCI Artifact support to our remote clients. Jira: https://issues.redhat.com/browse/RUN-2711 Signed-off-by: Lewis Roy <[email protected]>

Luap99

LGTM

openshift-ci · 2025-06-04T13:02:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, ninja-quokka

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99]

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

baude · 2025-06-04T14:31:00Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels May 12, 2025

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label May 12, 2025

ninja-quokka force-pushed the restful_art branch 9 times, most recently from 930c2ac to 16309c6 Compare May 13, 2025 04:54

Luap99 reviewed May 13, 2025

View reviewed changes

Honny1 reviewed May 13, 2025

View reviewed changes

ninja-quokka force-pushed the restful_art branch from 16309c6 to ce191f6 Compare May 14, 2025 13:04

Luap99 reviewed May 14, 2025

View reviewed changes

mtrmac reviewed May 14, 2025

View reviewed changes

pkg/api/server/register_artifacts.go Show resolved Hide resolved

pkg/libartifact/store/store.go Outdated Show resolved Hide resolved

mtrmac reviewed May 14, 2025

View reviewed changes

pkg/libartifact/store/store.go Outdated Show resolved Hide resolved

mtrmac reviewed May 14, 2025

View reviewed changes

pkg/api/handlers/libpod/artifacts.go Outdated Show resolved Hide resolved

pkg/api/handlers/libpod/artifacts.go Outdated Show resolved Hide resolved

mtrmac reviewed May 14, 2025

View reviewed changes

pkg/libartifact/store/store.go Outdated Show resolved Hide resolved

mheon reviewed May 14, 2025

View reviewed changes

pkg/api/handlers/libpod/artifacts.go Show resolved Hide resolved

mheon reviewed May 14, 2025

View reviewed changes

mtrmac reviewed May 21, 2025

View reviewed changes

ninja-quokka force-pushed the restful_art branch 2 times, most recently from ca3fb19 to 887f669 Compare May 22, 2025 11:41

ninja-quokka marked this pull request as ready for review May 22, 2025 11:44

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 22, 2025

openshift-ci bot requested a review from baude May 27, 2025 13:03

mtrmac mentioned this pull request May 27, 2025

OCI artifact pushes can trigger HTTP GET requests to remote locations #26215

Open

baude reviewed May 27, 2025

View reviewed changes

ninja-quokka force-pushed the restful_art branch from 887f669 to 9809b13 Compare May 28, 2025 05:14

ninja-quokka force-pushed the restful_art branch from 9809b13 to ee3af24 Compare June 1, 2025 21:34

mtrmac reviewed Jun 2, 2025

View reviewed changes

ninja-quokka force-pushed the restful_art branch from ee3af24 to cf0e51e Compare June 3, 2025 11:55

Luap99 reviewed Jun 3, 2025

View reviewed changes

ninja-quokka force-pushed the restful_art branch from cf0e51e to 99cfdc0 Compare June 4, 2025 05:49

Luap99 approved these changes Jun 4, 2025

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2025

openshift-ci bot assigned baude Jun 4, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 4, 2025

openshift-merge-bot bot merged commit badf6b8 into containers:main Jun 4, 2025
76 of 77 checks passed


		"github.com/containers/image/v5/types"
		libartifact_types "github.com/containers/podman/v5/pkg/libartifact/types"


		imageEngine := abi.ImageEngine{Libpod: runtime}

		// FIX: push currently just returns an empty struct, should we return the digest?

feat: Add OCI Artifact support to the Podman REST API #26111

feat: Add OCI Artifact support to the Podman REST API #26111

Uh oh!

Conversation

ninja-quokka commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Does this PR introduce a user-facing change?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ninja-quokka commented May 12, 2025 •

edited

Loading