Skip to content

spec: include optional port separator#498

Closed
artificial-intelligence wants to merge 1 commit intoopencontainers:mainfrom
artificial-intelligence:fix_tag_regex
Closed

spec: include optional port separator#498
artificial-intelligence wants to merge 1 commit intoopencontainers:mainfrom
artificial-intelligence:fix_tag_regex

Conversation

@artificial-intelligence
Copy link
Copy Markdown

This change makes the separators consistent
with the implementation in the docker distribution here

Relevant part from the code:

// optionalPort matches an optional port-number including the port separator
// (e.g. ":80").
optionalPort = `(?::[0-9]+)?`

Fix found by: 6ax here Related: docker/docker-py#3195

This change makes the separators consistent
with the implementation in the docker distribution
[here](https://github.com/distribution/distribution/blob/97b1d649c4938d0f608d96454d6a8326b1f96acd/reference/regexp.go#L65)

Relevant part from the code:

```
// optionalPort matches an optional port-number including the port separator
// (e.g. ":80").
optionalPort = `(?::[0-9]+)?`
```
Fix found by: [6ax](https://github.com/6ax) [here](docker/docker-py#3195 (comment))
Related: docker/docker-py#3195

Signed-off-by: Sven Kieske <kieske@osism.tech>
artificial-intelligence added a commit to artificial-intelligence/docker-py that referenced this pull request Dec 12, 2023
Closes: docker#3195
Related: opencontainers/distribution-spec#498

Signed-off-by: Sven Kieske <kieske@osism.tech>
Copy link
Copy Markdown
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.

This regexp is for the name component of the requests received by the registry and does not include the hostname or post. It's a path on the registry.

@artificial-intelligence
Copy link
Copy Markdown
Author

This regexp is for the name component of the requests received by the registry and does not include the hostname or post. It's a path on the registry.

So I'm really no expert on the spec, so it's totally possible I'm wrong but, according to:

distribution/distribution@bbd41f4

optionalPort is a part of the remote-name component which in turn is used interchangeably with path according to the linked commit message. That made me think this could also be a part of a tag, is that wrong? It looks like it, according to your comment?

But before docker-py 7.0.0 it was possible to use ":" in a tag at least, see the linked bug report.

I'm not sure if this is or is not allowed by the spec, because I honestly find the wording in
the Pulling manifests paragraph quite confusing.

My main complaint would be the sentence:

Throughout this document, `<reference>` as a tag MUST be at most 128 characters in length and MUST match the following regular expression:

does this mean, that every tag must match this regex, or only a <reference> must match this regex when used as a tag? Can there be something else as a tag, that is not a reference? That's not 100% clear to me.

I think the following changes could it make more clear to the reader, but this is just my reading as a drive-by-contributor, so ymmv 🙂

  • there is no dedicated paragraph for the correct syntax of things, it's thrown into an unrelated paragraph about pulling manifests (well there is a dedicated paragraph for this stuff, but it doesn't contain the needed information for tags, names, references etc.).
  • clearly define what a tag is and is not, and what it's syntax it is supposed to match.
  • it's not explained which kind of regex is used here, python-regex, golang regex, PCRE, etc.
  • explanations that actually don't explain anything like name refers to the namespace of the repository but it seems to be not explained what a namespace nor a repository are in this context in this paragraph, it's also not explained in the paragraph about Definitions where I would expect those things to be defined (line 59 ff). Of course I have a rough understanding of the concept of namespacing and repositories in general, but their meaning can vary slightly depending on the context, so imho it would be nice to clarify that, if possible.

Maybe I just missed this information somewhere in the docs, or in some of the linked docs.

Sorry if this maybe got a little bit off topic.

Thanks for your work and help on this!

@sudo-bmitch
Copy link
Copy Markdown
Contributor

Distribution and distribution-spec are separate projects. There's a lot of overlap in the terms, but also inconsistent usage between the two. The concept of a reference from the distribution project doesn't exist in the distribution-spec. At least not yet.

The distribution-spec only deals with the http API, and the name field referenced here is within the Path of that request. It does not cover the Host field in the http API, that isn't specified in the spec.

milas added a commit to docker/docker-py that referenced this pull request Jan 3, 2024
Update the regex and add test cases.

(There are some xfails here for cases that the regex is not currently
handling. It's too strict for IPv6 domains at the moment.)

Closes: #3195
Related: opencontainers/distribution-spec#498

Signed-off-by: Sven Kieske <kieske@osism.tech>
Signed-off-by: Milas Bowman <milas.bowman@docker.com>
Co-authored-by: Milas Bowman <milas.bowman@docker.com>
Copy link
Copy Markdown
Contributor

@rchincha rchincha left a comment

Choose a reason for hiding this comment

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

lgtm

@sudo-bmitch
Copy link
Copy Markdown
Contributor

Closing this one for now because we don't define a reference in OCI, yet.

@sudo-bmitch sudo-bmitch closed this Apr 4, 2024
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.

3 participants