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 manifest size limit recommendation #293

Merged
merged 1 commit into from
Feb 17, 2022

Conversation

imjasonh
Copy link
Member

@imjasonh imjasonh commented Aug 5, 2021

Copy link
Contributor

@sudo-bmitch sudo-bmitch left a comment

Choose a reason for hiding this comment

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

Thanks for driving this! Few small suggestions, and the DCO is going to need a new push.

@@ -402,6 +402,10 @@ The `<location>` is a pullable manifest URL.

An attempt to pull a nonexistent repository MUST return response code `404 Not Found`

A registry SHOULD enforce some limit on the maximum manifest size that it can accept.
Copy link
Contributor

Choose a reason for hiding this comment

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

While this is recommended for public registries, I can see a case for private registries not wanting to implement this.

Suggested change
A registry SHOULD enforce some limit on the maximum manifest size that it can accept.
A registry MAY enforce limits on the maximum manifest size that it can accept.

@@ -402,6 +402,10 @@ The `<location>` is a pullable manifest URL.

An attempt to pull a nonexistent repository MUST return response code `404 Not Found`

A registry SHOULD enforce some limit on the maximum manifest size that it can accept.
A registry that enforces this limit SHOULD respond to a request to push a manifest over this limit with a response code `413 Payload Too Large`.
Client and registry implementations SHOULD expect to be able to support manifest pushes of at least 4 megabytes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest splitting this into two lines, one for clients, and one for registries:

Suggested change
Client and registry implementations SHOULD expect to be able to support manifest pushes of at least 4 megabytes.
A registry that implementations this SHOULD support manifest pushes of at least 4 megabytes.
A client SHOULD avoiding pushing a manifest larger than 4 megabytes and SHOULD support pulling a manifest of at least 4 megabytes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like:

Registries SHOULD NOT impose a limit less than 4 megabytes on the size of a manifest.
Clients SHOULD support handling manifest sizes of at least 4 megabytes.
Clients concerned with portability SHOULD NOT generate manifests larger than 4 megabytes.

FWIW, if quay has a 1MB limit, I would lean toward 1MB as the recommendation.

Copy link
Member

Choose a reason for hiding this comment

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

The wording from @jonjohnsonjr is more clear.

@jzelinskie
Copy link
Member

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
Copy link
Member Author

imjasonh commented Aug 5, 2021

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? 😄

@mikebrow
Copy link
Member

mikebrow commented Aug 5, 2021

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
Copy link
Contributor

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
Copy link
Member

mikebrow commented Aug 5, 2021

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
Copy link
Contributor

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
Copy link
Member

mikebrow commented Aug 9, 2021

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
Copy link
Member Author

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
Copy link
Member

vbatts commented Aug 12, 2021

@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
Copy link
Contributor

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:

  1. 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).
  2. 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{}:

Detail string `json:"detail"`

Was this change deliberate?

@joaodrp
Copy link
Contributor

joaodrp commented Aug 23, 2021

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 #293 (comment) 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 #293 (comment).

@rchincha
Copy link
Contributor

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 jdolitsky added this to the v1.2.0 milestone Feb 17, 2022
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

This provides solid guidance and doesn't impose any new requirements, other than explicitly requiring a status code.

@vbatts vbatts merged commit dd38b7e into opencontainers:main Feb 17, 2022
@jdolitsky
Copy link
Member

I suppose we add this to 1.1 then?

@jdolitsky jdolitsky removed this from the v1.2.0 milestone Feb 17, 2022
@jdolitsky jdolitsky added this to the v1.1.0 milestone Feb 17, 2022
imjasonh pushed a commit to imjasonh/distribution-spec that referenced this pull request Mar 3, 2022
Document manifest size limit recommendation

Signed-off-by: Jason Hall <[email protected]>
@jdolitsky jdolitsky mentioned this pull request Sep 15, 2022
@tianon
Copy link
Member

tianon commented Mar 3, 2023

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 #293 (comment) that this was intended to be base 2 (and thus 4194304 bytes, not 4000000). 👍

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.

Recommend enforcing limits on manifest/blob sizes