Document manifest size limit recommendation

imjasonh commented 3 years ago

Fixes https://github.com/opencontainers/distribution-spec/issues/260

cc @sudo-bmitch @jonjohnsonjr

jzelinskie commented 3 years ago

We'll probably also want to reach out to see what all the major registries have for existing limits to avoid any potential issues with the 4MB number.

imjasonh commented 3 years ago

We'll probably also want to reach out to see what all the major registries have for existing limits to avoid any potential issues with the 4MB number.

Yeah, I've attempted to start a survey of existing registries here: https://github.com/opencontainers/distribution-spec/issues/260#issuecomment-893069598

quay.io supports manifests up to 1MB, after which it fails with 413 Request Entity Too Large (an nginx error, not a spec error)

Maybe the recommendation should be 1MB? 😄

mikebrow commented 3 years ago

We'll probably also want to reach out to see what all the major registries have for existing limits to avoid any potential issues with the 4MB number.

Yeah, I've attempted to start a survey of existing registries here: #260 (comment)

quay.io supports manifests up to 1MB, after which it fails with 413 Request Entity Too Large (an nginx error, not a spec error)

Maybe the recommendation should be 1MB? 😄

The more I think about it.. the more I think we should just recommend that they SHOULD have a limit, refer to a table stored somewhere on here showing survey results for existing limits for registries and certain clients. I think the error returned should be a MUST..

jonjohnsonjr commented 3 years ago

the more I think we should just recommend that they SHOULD have a limit

I don't think it makes sense to suggest registries impose an artificial limit. This is kind of philosophical, but I'd lean towards Postel's Law:

be conservative in what you send, be liberal in what you accept

It is expected that registries will have some practical limit in terms of what it can accept. They should have predictable behavior for that case by returning a 413, but I don't see the point in having a registry reject something that it's otherwise able to handle just fine. If they want to impose some artificial limitation to give themselves flexibility in the future, that's fine, but I don't think we should be recommending that.

On the other hand, clients are actually in control of the artifacts they're producing, so they have the flexibility to fix any problems caused by the size of the manifest they produce. If I know that my runtime can support arbitrarily large manifests, then there's not any benefit from a registry imposing an arbitrary limit.

For example, imagine this situation:

Registry supports up to 4MB manifests.
Client pulling only supports 1MB manifests.

We can successfully push a 2MB manifest, but it will fail when the client tries to pull it. There might be some benefit to shifting this failure "left" by imposing a 1MB limit in the registry, but what happens if eventually the client gets upgraded to remove that limitation?

Registry supports only up to 1MB (trying to be helpful)
Client pulling supports up to 4MB now (fixed!)

Well... now we're still stuck at 1MB, even though we would prefer to be pushing a 2MB manifest :/

mikebrow commented 3 years ago

the more I think we should just recommend that they SHOULD have a limit It is expected that registries will have some practical limit in terms of what it can accept.

nod.. MAY enforce a practical limit... see reference table for current known size limits. The should part is as obvious as it is expected.

Cheers, Mike

sudo-bmitch commented 3 years ago

Maybe the recommendation should be 1MB? 😄

The more I think about it.. the more I think we should just recommend that they SHOULD have a limit, refer to a table stored somewhere on here showing survey results for existing limits for registries and certain clients. I think the error returned should be a MUST..

@mikebrow are you suggesting we don't provide a specific size? From the server, I understand this, it gives flexibility, and we have a clear API communication to the client when the registry specific limit is exceeded.

But from a client, it means that I don't know if my manifest can be pushed to any OCI compliant registry. An image building tool will not know which registry the image may be retagged and pushed to in the future.

I'll end up pushing the problem off to the user, which means some users may push images designed for their specific registry. That could create a registry lock-in where those images can't be easily copied to another registry.

mikebrow commented 3 years ago

Maybe the recommendation should be 1MB? 😄

The more I think about it.. the more I think we should just recommend that they SHOULD have a limit, refer to a table stored somewhere on here showing survey results for existing limits for registries and certain clients. I think the error returned should be a MUST..

@mikebrow are you suggesting we don't provide a specific size? From the server, I understand this, it gives flexibility, and we have a clear API communication to the client when the registry specific limit is exceeded.

But from a client, it means that I don't know if my manifest can be pushed to any OCI compliant registry. An image building tool will not know which registry the image may be retagged and pushed to in the future.

I'll end up pushing the problem off to the user, which means some users may push images designed for their specific registry. That could create a registry lock-in where those images can't be easily copied to another registry.

I don't think we should be mandating physical resource size requirements/limits on the client or the server. Reason being someone might want the limit to be 1GB another might want it to be 1KB still another 1MB and all three may have a perfectly valid reason.

I think we should provide a location for a table of known current limits (client/server) and let the market solutions decide. Registries with larger maximums holding a lock on the accounts exploiting said maximum.. I'd suggest clients consider using the table as a benchmark to warn users when their manifest are so large as to have a limited number of registries where they can currently be stored. We could provide just such a suggestion for clients maybe even an api. Clients could also provide a default limit with an override option or what not and certainly registries may have various limits based on account type, storage service, etc.

Cheers, Mike

imjasonh commented 3 years ago

Picking this back up, I think a reasonable first cut of this change would be:

registries MAY impose a manifest size limit (this serves as both as permission to registries, and as a warning to clients)
- if they do impose a limit, the error MUST be HTTP 413
link to some table (in this repo?) with observed limits among some known registries, and the most recent date they were tested

I think we could improve on this over time, but this seems relatively uncontroversial subset of the change that we can iterate on.

wdyt?

vbatts commented 3 years ago

@imjasonh sounds good.

Something that would likely be nice is if there was an HTTP response from the registry where this limit could be hinted in a header. That could be an eventual SHOULD language. But is not diversion from this PR for now.

jonjohnsonjr commented 3 years ago

Something that would likely be nice is if there was an HTTP response from the registry where this limit could be hinted in a header.

Agreed. I can imagine two reasonable ways to handle this:

Maintain a list of features and/or limits in the spec that registries can advertise via some common mechanism (headers or some a new endpoint).
Return structured, actionable error messages when appropriate.

Three are more interesting use cases for the second option, but for the issue at hand, we could do something like:

    {
        "errors": [
            {
                "code": "MANIFEST_TOO_LARGE",
                "message": "boot too big",
                "detail": {
                   "limit": 1048576
                }
            }
        ]
    }

There's room for this in the spec:

The detail field is OPTIONAL and MAY contain arbitrary JSON data providing information the client can use to resolve the issue.

As an aside: I noticed that specs-go has Detail as a string, but it should be interface{} or maybe []byte given the wording in the spec. In distribution/distribution, this is an interface{}: https://github.com/opencontainers/distribution-spec/blob/13fa739db32c13f816c68cabfd67badc576615d7/specs-go/v1/error.go#L39

Was this change deliberate?

joaodrp commented 3 years ago

We shouldn't try to mandate a minimum manifest size that all registries should accept. Especially not if picking X because that's the lowest we know (e.g., 1MB because that's that maximum that Quay accepts...). There are many registry implementations out there, each with its own technical limitations (either by design or not), and these are likely to change over time.

The approach that @imjasonh proposes in https://github.com/opencontainers/distribution-spec/pull/293#issuecomment-897641988 sounds like the best to me. I also agree that it should be clear to clients whenever they hit such limits, and it should be possible to determine what that limit is programmatically. I would be in favor of a custom error code like @jonjohnsonjr suggested in https://github.com/opencontainers/distribution-spec/pull/293#issuecomment-897738682.

rchincha commented 2 years ago

Also note that "maximum" may be dependent on the resources made available to the registry. For example, docker registry with a 1GB disk will have a different maximum than the same docker registry with a 1TB disk. Unless this scenario out of scope of this issue.

jdolitsky commented 2 years ago

I suppose we add this to 1.1 then?

tianon commented 1 year ago

Just to make sure this is written down somewhere (because "megabytes" is a legally disputed unit that's either base 2 or base 10 depending on whether you're in/bound by the hard drive industry), I'm going to assume from @jonjohnsonjr's example in https://github.com/opencontainers/distribution-spec/pull/293#issuecomment-897738682 that this was intended to be base 2 (and thus 4194304 bytes, not 4000000). :+1:

opencontainers / distribution-spec

Document manifest size limit recommendation #293