Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document Conditional Requests #251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jonjohnsonjr
Copy link
Contributor

Here's a first stab at #250

This isn't complete, but I wanted to get something together so we can start bikeshedding.

A couple other use cases we should add are deleting tags (untagging) and interactions with caches.

@jonjohnsonjr
Copy link
Contributor Author

jonjohnsonjr commented Mar 17, 2021

@sargun had a comment about registry support for this. Many registries supply ETags today, but it's unclear if they already support conditional requests.

One simple idea would be defining an HTTP header such that registries can positively indicate support for conditional requests.

E.g. something dumb like:

OCI-Distribution-RFC7232: yes

If a client sees that header, they could rely on conditional request semantics. If they don't see that header, they could choose to alter their behavior to work around potential race conditions (if possible), log a warning, return an error, etc.


[RFC 7232](https://tools.ietf.org/html/rfc7232) defines the semantics of Conditional Requests in great detail, so this document will simply highlight the relevant portions of the RFC as applied to the Distribution Specification.

### Safely Mutating Manifests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this limited to only mutating a manifest for a given tag?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really think of anywhere else this is needed, but I might be overlooking something. There are races when creating, moving, and deleting tags, but everything else should be safe in the face of concurrency... right?

I think it's fine for this to apply to e.g. a manifest PUT, by digest, but it seems not very useful. You could say "don't PUT this manifest if it already exists", but PUT should be idempotent regardless, and you either want the manifest to exist or not.

Copy link
Contributor Author

@jonjohnsonjr jonjohnsonjr Mar 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could also use this when uploading blobs?

Instead of always doing a HEAD request to check for blob existence in a registry, you could just start with a POST that includes the If-None-Match header (if you know the blob digest). The registry returning a 412 would indicate that the blob already exists, and we could do both the existence check and the upload initiation in a single request.

Unfortunately, this would require us to have deterministic Etags, as @awakecoding is suggesting, which I like, but I haven't thought completely through.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that having deterministic etags would be great to have, but that is already something mostly managed by a registry themself, since any client would not particularly expect portability of a given etag across registries, would they?

@sargun
Copy link
Contributor

sargun commented Mar 19, 2021

@sargun had a comment about registry support for this. Many registries supply ETags today, but it's unclear if they already support conditional requests.

One simple idea would be defining an HTTP header such that registries can positively indicate support for conditional requests.

E.g. something dumb like:

OCI-Distribution-RFC7232: yes

If a client sees that header, they could rely on conditional request semantics. If they don't see that header, they could choose to alter their behavior to work around potential race conditions (if possible), log a warning, return an error, etc.

What API would the registry have to add this header for -- all? It seems like to me, the client can be optimistic, and if the registry doesn't support it, it can return some known error value.

@jonjohnsonjr
Copy link
Contributor Author

jonjohnsonjr commented Mar 24, 2021

What API would the registry have to add this header for -- all?

I would say anywhere that includes an Etag header? It would indicate that the client can rely on that Etag for conditional requests. Its absence would (likely) indicate a lack of support, so clients would have to proceed with caution.

It seems like to me, the client can be optimistic, and if the registry doesn't support it, it can return some known error value.

I don't think that works. If a registry doesn't implement conditional requests, the registry would happily just allow any requests to go through and not return any errors for conflicts. We need the registry to signal positive support, because by default there is no way to determine the lack of support beyond probing the registry's behavior by sending a request you would expect to fail.

Signed-off-by: Jon Johnson <jonjohnson@google.com>
Copy link
Member

@vbatts vbatts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the whole I'm good with this. I couldn't remember, but now am surprised that we completely leave etag usage as an exercise for the reader. :-\


[RFC 7232](https://tools.ietf.org/html/rfc7232) defines the semantics of Conditional Requests in great detail, so this document will simply highlight the relevant portions of the RFC as applied to the Distribution Specification.

### Safely Mutating Manifests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that having deterministic etags would be great to have, but that is already something mostly managed by a registry themself, since any client would not particularly expect portability of a given etag across registries, would they?

@@ -98,6 +98,11 @@ When process B attempts to upload the layer, the registry indicates that its not
If process A and B upload the same layer at the same time, both operations will proceed and the first to complete will be stored in the registry.
Even in the case where both uploads are accepted, the registry may securely only store one copy of the layer since the computed digests match.

### Conditional Requests

A container engine would like to safely push a new tag or modify an existing tag without racing against another container engine acting upon the registry at the same time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*registry/repo:tag

If-None-Match: *
```

If there already exists a manifest in the registry with a matching tag identifier, the registry MUST return a `412 Precondition Failed` response.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a nitpick, but maybe we should include this in the spec and not have a bunch of files. I feel like the other files we've had have rotted over time because they were not being reviewed alongside everything else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah. That's a fair point.

@jdolitsky
Copy link
Member

@jonjohnsonjr - can you address @vbatts and @jzelinskie comments above and we can merge?

Also - are these things we can realistically add to the conformance suite?

Copy link
Contributor

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the whole idea. Might need to work out a few nits.

@imjasonh
Copy link
Member

Friendly ping. Some of the recent discussion in the reference types WG has bumped into this as a blocker, and even regardless of how/whether that work uses and updates indexes, it would be great to have this formally spelled out.

@opencontainers/distribution-spec-maintainers any more comments and/or approvals?

cc @chris-crone @tonistiigi

@tonistiigi
Copy link

If a registry doesn't implement conditional requests, the registry would happily just allow any requests to go through and not return any errors for conflicts.

Can we be sure that there isn't a case where the current registry only uses etags for GET and fails on all If-Match because it doesn't understand the condition(eg. PUT/GET are managed by different internal services)? I guess even in that case client could do a GET again, and see that the etag has not changed so the server must be misbehaving.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants