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

spec: Remove catalog from spec. #45

Closed
wants to merge 1 commit into from

Conversation

bsatlas
Copy link
Contributor

@bsatlas bsatlas commented Jan 17, 2019

This PR removes all references to the catalog endpoint from the distribution spec.

Maintainer approval

@bsatlas bsatlas changed the title spec: catalog from documentation. spec: Remove catalog from spec. Jan 17, 2019
@bsatlas bsatlas closed this Jan 22, 2019
spec.md Outdated

For details of the `Link` header, please see the [_Pagination_](#pagination) section.

#### Pagination
Copy link
Member

Choose a reason for hiding this comment

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

there's more detail in the catalog pagination section than in the pagination for tag results.. instead the tag pagination refers to this section.. so maybe filter through and see what can move down to the tags pagination section...

Copy link
Member

Choose a reason for hiding this comment

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

It looks like at least last and Link referred to this section from there

@bsatlas
Copy link
Contributor Author

bsatlas commented Jan 25, 2019

Alright, I'll take a look at it tomorrow.

@bsatlas bsatlas reopened this Jan 28, 2019
@jzelinskie
Copy link
Member

Did we vote on this yesterday? I think Stephen wanted to keep it in the spec as optional, but I honestly think that just not having it in OCI 1.0 is equivalent since most registries will still support it for backwards compatibility with v2.2.

@mikebrow
Copy link
Member

catalog should probably remain as optional, till it's been replaced

@vbatts
Copy link
Member

vbatts commented Jan 31, 2019 via email

@mikebrow
Copy link
Member

mikebrow commented Jan 31, 2019

From: #22 (comment)

@dmcgowan - I am +1 to not including the _catalog API at all. If we are going to have an OPTIONAL api, maybe we can have something that is more manageable, such as _list at any level, including /_list (lists first level namespaces if supported), //tags/_list, /manifests/_list, and //_list (lists repositories if supported). This is similar to the proposal you linked and we have more flexibility if we are not tied to _catalog.

I like what Derek proposed. I think being able to iterate over the contents is a fundamental operation. Simply removing catalog... without replacement, essentially removes our ability to iterate. I'd hate for that to be a limitation of the final 1.0, but agree we don't need cat or it's replacement for the first alpha just a TODO should suffice, maybe with an open issue tagged for v1.0? Just my opinion :-) Probably brought about by having to deal with a large list of said stuff.

@jonjohnsonjr
Copy link
Contributor

I think if we don't want people to rely on catalog, we should remove it from the spec. Removing it from the spec won't make registries that currently support it suddenly stop, and I certainly don't want anyone else to implement it. It seems that we all agree it's bad and needs to be replaced by something else.

Both _catalog and /tags/list aren't really sufficient to build real systems on top of, as outlined here: #22 (comment)

I would like to see some progress on a proposal for replacements to these things, but IMO both are out of scope for the distribution spec. If we want extensions for discovery and content management, we can do that separately.

Catalog (or something like it) is explicitly out of scope, per the proposal:

Managing the grouping of image repository names is considered part of distribution policy or content management, which are out of scope. For example, “which image repositories are under library/?” is out of scope for this project.

@bsatlas
Copy link
Contributor Author

bsatlas commented Jan 31, 2019

A solid compromise would be to keep /v2/_catalog optional and mark it as DEPRECATED with some kind of warning that talks about its flaws.

@bsatlas
Copy link
Contributor Author

bsatlas commented Jan 31, 2019

In order to make catalog usable in the future, I think are gonna have to have a long talk about how to logically group a collection of repositories into a spec-defined namespace. This will for sure be a breaking change so I don't forsee us being able to have a functional catalog endpoint until we release a V2.

My 2 cents.

@mikebrow
Copy link
Member

mikebrow commented Feb 1, 2019

Listing (and/or management) of repository names "is explicitly out of scope."

I want a recount on that original decision... throws up hands... :-)

@mikebrow
Copy link
Member

mikebrow commented Feb 1, 2019

@atlaskerr if you can just refactor the pagination text.. I think you'll have support for merging the PR. Cheers, Mike

@bsatlas
Copy link
Contributor Author

bsatlas commented Feb 1, 2019

My rebase messed up my branch and Github automatically closed it. Oops. I'll bring it back up when I fix the issue!

@bsatlas bsatlas reopened this Feb 1, 2019
This commit removes all references to the deprecated `_catalog` endpoint.

Pagination specific documentation has been merged into the pagination
section for listing tags.

Signed-off-by: Atlas Kerr <atlaskerr@gmail.com>
@bsatlas
Copy link
Contributor Author

bsatlas commented Feb 1, 2019

This PR is ready for review.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

See comment.

Images are stored in collections, known as a _repository_, which is keyed by a `name`, as seen throughout the API specification.
A registry instance MAY contain several repositories.
The list of available repositories is made available through the _catalog_.
### Listing Image Tags
Copy link
Member

Choose a reason for hiding this comment

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

?? I think you wanted to pick up the interesting description of how pagination works and merge that down into the pagination description in image tags. Not move some portions of the description of image tags into the description of listing repositories. See references to repositories in the pagination examples below.

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.

I'm not opposed to this, but I would only approve contingent that we have a workgroup for unified listing API


```HTTP
GET /v2/_catalog?n=<integer>
GET /v2/<name>/tags/list?n=<integer>
Copy link
Member

Choose a reason for hiding this comment

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

wait, is this new?

Copy link
Member

Choose a reason for hiding this comment

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

oh no, just consolidating examples ..

@dmcgowan
Copy link
Member

+1 on removal. Do we need a "reserved" endpoint function though to ensure this endpoint does not get redefined as something else in the future? I don't foresee it being reused but we should prevent collision through redefinition and ensure that repositories starting with _ is forbidden.

@stevvooe
Copy link
Contributor

We do need to reserve this. If a registry wants to implement _catalog, then go forth.

BTW, we also need to deprecate /list for /, in certain APIs.

@vbatts
Copy link
Member

vbatts commented Apr 26, 2019

ok y'all, I've been hearing churning from several sources that while the _catalog route was a pain, that it is a depended on feature. At this point it does not matter that "it should have never been used", but it does exist and will be a blocker for any migration from what was the Docker Registry API v2.s2 over to what will be the OCI distribution spec v1.
If we do add routes to facilitate listing (and I do like some of the proposals that have been surfaced), it sounds like they will have to be be additive and not replacing of the _catalog route.
I'm a NACK for removing this.

@SteveLasker
Copy link
Contributor

It would be good to understand who’s dependent upon this and how it would carry forward. Although ACR implements it, I’m still an advocate for replacing. Removing it from the spec doesn’t preclude some distribution implementations from keeping their apis. ACR would likely keep it until we complete the new API. We’d then start a deprecation process.
But, keeping it suggests others must implement it, and it doesn’t appear to be realistic so we’d have a spec that isn’t supportable.
Can we queue this up for discussion next week.

@cyphar
Copy link
Member

cyphar commented Apr 26, 2019

In my view, the plan for transition from Docker distribution to OCI distribution should be the same as the plan for transition from Docker images to OCI images -- the absolute minimum number of changes should be made that would require clients to be rewritten (in fairness, OCI images changed a lot but all of the change is metadata-level and to allow extensibility in the format).

This will allow us to have an OCI project (even though it might be imperfect) that users can easily switch to. After that, we can go through normal spec change iteration and have a 1.1 (or 2.0) which removes cruft we don't need and so on. Otherwise we will (further) needlessly delay the widespread adoption of OCI.

Removal of _catalog and replacing it with something else falls into a change that "would require clients to be rewritten".

@jzelinskie
Copy link
Member

jzelinskie commented Apr 26, 2019

while the _catalog route was a pain, that it is a depended on feature

Only for some definition of "depended on". Programs like Spinnaker and Twistlock use _catalog to discover all of the content in a registry. However, every registry implementation has caveats and doesn't expose all images via the endpoint. For example, public registries on the internet often don't want the ability for anyone to index their entire database for both privacy and performance reasons. Thus the programs depending on this endpoint are already broken.

The problem of integration with tools that want to index a registry efficiently needs to be solved at post-Distribution v1.

At this point it does not matter that "it should have never been used", but it does exist and will be a blocker for any migration from what was the Docker Registry API v2.s2 over to what will be the OCI distribution spec v1.

My argument is that because it is broken and people are mistakenly using it, the spec should have it as either optional or drop it under the assumption that every popular registry already implements Docker v2-2 in the meantime while a real solution can be created. Regardless of the spec, the popular registries are going to be stuck supporting this broken endpoint for a while.

@SteveLasker
Copy link
Contributor

i agree we want to make it easy for people to move to OCI. From what i understand, _catalog isn’t implemented by many existing distributions. There’s no client API in docker.exe or other common tooling that i'm aware of.
I’m not suggesting a distribution that implements this or any other additive api wouldn’t be OCI compliant, but requiring the _catalog api in the spec says it is required, yet won’t be implemented.
So, i’m just trying to understand why we would keep a spec on something that isn’t supported, yet can continue to be supported by those that have.
Removing it frees up the space for a new api. Which, we do need more diligence in completing. (myself included in this self recognition of lack in progress)

@SteveLasker
Copy link
Contributor

On the scanner comment from Jimmy; this is true. And while we want to standardize for scanners and other more generic tooling, each scanner company has already had to deal with each clouds unique implementation, including different apis altogether. Removing the catalog API just frees up the mental model to construct something that each of us can implement consistently, reducing the churn on the scanners and other tooling.

@amouat
Copy link
Contributor

amouat commented May 3, 2019

According to tweets I've seen from people at DockerCon, there is now a docker registry ls command. I can only assume this uses the catalog API (please correct me if anyone knows better!). This means that if we remove the catalog stuff, OCI registries will need to support a separate API to be fully compatible with the docker CLI.

There's still a strong argument to remove it, but I just wanted to flag this in case people weren't aware.

@jonjohnsonjr
Copy link
Contributor

It looks like docker registry ls is scoped to a repository, so it wouldn't need catalog, just tag listing.

@SteveLasker
Copy link
Contributor

it’s also a plug-in as part of Docker Enterprise 3.

@amouat
Copy link
Contributor

amouat commented May 3, 2019

it’s also a plug-in as part of Docker Enterprise 3.

What do you mean? It's not in the normal Docker CLI? Or it doesn't work against the regular docker distribution registry?

@SteveLasker
Copy link
Contributor

Apparently it’s only in Docker EE and only works with DTR, because the catalog and listing apis are so inconsistently implemented. ;). I’ve asked the docker folks to join us in helping define a new api we can all support these common scenarios: https://hackmd.io/s/BJPAUxDvV#OCI-Catalog-Listing-API---Workgroup

@dmcgowan
Copy link
Member

This seems stalled and perhaps we should put it to a vote of whether to include the catalog in 1.0 or not. I believe the maintainers are in agreement that we will not be adding any new endpoints for 1.0 for this functionality but wait for 1.1. I am still +1 for dropping in 1.0.

The docker registry plugin is something experimental to interact with DTR and it is not relevant to this conversation. However it would be good to have them (I am not involved in that) included in any discussion for creating a post 1.0 endpoint which could generically work for everyone.

@SteveLasker
Copy link
Contributor

While at KubeCon, several of us got together to discuss in person:
Notes:

Summary

As docker recently announced a new search api, implemented in DTR and Docker EE, because the _catalog api wasn't consistently supported, we agreed to remove _catalog from the spec. We agreed to reserve the endpoint name, to avoid conflict for registries that did implement _catalog

@dmcgowan
Copy link
Member

I put up a vote count in the comment. Let's get full approval then we can ask that this be rebased.

@mikebrow
Copy link
Member

/SGTM to remove the api.. and reserve it.
We can continue to work on the replacement(s).

@dmcgowan
Copy link
Member

@mikebrow sounds good, if everyone agrees we can carry this PR or make sure that gets included before merging

@vbatts
Copy link
Member

vbatts commented Aug 14, 2019

we came to a conclusion on the call today that removes and reserves catalog API, as even currently all the registries that implement a catalog endpoint all do it differently. A complaint registry does not need to implement this endpoint, but registries that have already implemented such can keep it. There is no expected behavior for this endpoint

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.

10 participants