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

Fix bug in parsing domain from repository reference #2979

Merged
merged 1 commit into from
Mar 1, 2021

Conversation

avtakkar
Copy link

regexp.DomainRegexp matches mixed-case and uppercase strings without . as well. For example, in /v2/SAMPLE/reponame/manifests/latest, the route handler would match SAMPLE as regexp.DomainRegexp and reponame as regexp.nameComponentRegexp. To parse the normalized name, an uppercase or mixed-case string should be considered a valid domain.

Fixes #2858
Redis cache enabled path invokes ParseNormalizedName which produces an error because rather than matching TESTNAME to the domain like the router does, it matches TESTNAME/reponame to the name component.

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "avtakkar/fix-domain-split" git@github.com:avtakkar/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@codecov
Copy link

codecov bot commented Aug 14, 2019

Codecov Report

Merging #2979 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2979      +/-   ##
==========================================
- Coverage   60.69%   60.64%   -0.05%     
==========================================
  Files         102      102              
  Lines        8044     8044              
==========================================
- Hits         4882     4878       -4     
- Misses       2511     2514       +3     
- Partials      651      652       +1
Flag Coverage Δ
#linux 60.64% <100%> (-0.05%) ⬇️
Impacted Files Coverage Δ
reference/normalize.go 82.02% <100%> (ø) ⬆️
registry/handlers/app.go 48.16% <0%> (-0.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fb7fff...e89db8d. Read the comment docs.

@caervs caervs self-requested a review October 9, 2019 23:58
Copy link
Contributor

@caervs caervs left a comment

Choose a reason for hiding this comment

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

Yeah. Another way to say it is when the first part of a ref is mixed case then you know that it must be a domain since repo names can't be mixed case.

LGTM

@caervs caervs requested a review from dmcgowan October 21, 2019 17:32
@dmcgowan
Copy link
Collaborator

Change looks good,

Can we get a test added to TestParseRepositoryInfo which explicitly matches the mixed case value as the domain?

Signed-off-by: Aviral Takkar <aviral26@users.noreply.github.com>
Base automatically changed from master to main January 27, 2021 15:51
@milosgajdos milosgajdos added the bug label Feb 2, 2021
Copy link
Collaborator

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

When redis cache is enabled for blobdescriptor, accepted repository name doesn't match with the route regex
6 participants