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.

Sign up for GitHub

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

Jump to bottom

Make NormalizeLocalName to not reach the network to normalize names. #18014

Merged
merged 1 commit into from Nov 17, 2015
Merged

Make NormalizeLocalName to not reach the network to normalize names. #18014

merged 1 commit into from Nov 17, 2015

Conversation

calavera
Copy link
Contributor

Fixes #18003

Signed-off-by: David Calavera david.calavera@gmail.com

@calavera calavera added this to the 1.9.1 milestone Nov 16, 2015
@calavera calavera force-pushed the make_normalize_local_name_local branch from f5e895a to 3d4255c Compare November 16, 2015 15:31
tiborvass
tiborvass reviewed Nov 16, 2015
View reviewed changes
registry/config.go
return name
}

var oficialIndex bool
Copy link
Contributor

Choose a reason for hiding this comment

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

officialIndex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're in Barcelona, it's oficial 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks.

@calavera calavera force-pushed the make_normalize_local_name_local branch from 3d4255c to 757d812 Compare November 16, 2015 15:35
@calavera calavera force-pushed the make_normalize_local_name_local branch from 757d812 to 1c55be9 Compare November 16, 2015 15:41
aaronlehmann
aaronlehmann reviewed Nov 16, 2015
View reviewed changes
registry/config.go
func NormalizeLocalName(name string) string {
repoInfo, err := ParseRepositoryInfo(name)
indexName, remoteName := splitReposName(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a call to validateNoSchema to match the old behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel like I left it out for some reason. But I don't think it can harm. I've changed the code a little bit to reuse a validate function that we already implemented with the checks we need.

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 16, 2015

😭

@calavera calavera force-pushed the make_normalize_local_name_local branch 2 times, most recently from 870aae2 to 3396742 Compare November 16, 2015 18:40
@aaronlehmann
Copy link
Contributor

Should NewRepositoryInfo use loadRepositoryName too, to reduce code duplication? The only downside I see is a redundant call to ValidateIndexName, but that's not a big deal.

anusha-ragunathan
anusha-ragunathan reviewed Nov 16, 2015
View reviewed changes
registry/config.go

// normalizeLibraryRepoName removes the library prefix from
// the repository name for official repos.
func normalizeLibraryRepoName(name string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

normalizeLibraryRepoName can be used in NewRepositoryInfo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's already used there.

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 16, 2015

LGTM
I wonder what's with --net=none target on Jenkins. It should catch such things.

@calavera calavera force-pushed the make_normalize_local_name_local branch from 3396742 to 4277ee0 Compare November 16, 2015 22:27
@calavera
Copy link
Contributor Author

@aaronlehmann, @anusha-ragunathan I've removed that duplicated code. Please, take a look.

anusha-ragunathan
anusha-ragunathan reviewed Nov 16, 2015
View reviewed changes
registry/config.go
var err error
if err = validateNoSchema(reposName); err != nil {
return err
_, _, err := loadRepositoryName(reposName, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: ValidateRepositoryName is now a thin wrapper over loadRepositoryName. Given that, its better to give them similar names for better readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

loadRepositoryName is correct. I rather not change calls to ValidateRepositoryName in this patch.

@calavera calavera force-pushed the make_normalize_local_name_local branch from 4277ee0 to d77f23c Compare November 16, 2015 22:51
anusha-ragunathan
anusha-ragunathan reviewed Nov 16, 2015
View reviewed changes
registry/config.go
@@ -302,34 +315,22 @@ func splitReposName(reposName string) (string, string) {

// NewRepositoryInfo validates and breaks down a repository name into a RepositoryInfo
func (config *ServiceConfig) NewRepositoryInfo(reposName string, bySearch bool) (*RepositoryInfo, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: For the sake of code reuse/readability, is it possible to restructure NewRepositoryInfo to take a bool arg? This arg would indicate call from NormalizeLocalName. There are only 3 callers for NewRepositoryInfo, so changing the function signature would be okay. Meanwhile, in NewRepositoryInfo, since most of the code that we need is here, we can use the bool flag to navigate our way around NewIndexInfo (which causes the network lookup).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not here. I rather minimize the changes we need for the patch release. That's not necessary to fix the bug.

@tonistiigi
Copy link
Member

Can we also fix the isSecureIndex()? ParseRepositoryInfo always uses emptyServiceConfig and doesn't have any InsecureRegistryCIDRs. We should test this first and not try to resolve the host as there is no way any of the IPs could match anyway. Atm these host resolutions calls would still happen on requests from client side and I don't think there is any need for them. Meaning when for example DNS goes down for client they would get similar timeout for pull/push/build etc.

@calavera
Make NormalizeLocalName to not reach the network to normalize names.
b665730 
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the make_normalize_local_name_local branch from d77f23c to b665730 Compare November 17, 2015 08:31
@calavera
Copy link
Contributor Author

@tonistiigi I rather make the less changes necessaries as possible to fix the ps behavior. I agree that we should fix that isSecureIndex, but I want to minimize the changes going into the patch release.

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 17, 2015

@tonistiigi @anusha-ragunathan @aaronlehmann let's keep it small for 1.9.1 and merge it now if there is no significant problems.

@tonistiigi
Copy link
Member

LGTM

Although, I don't get the comment about simplicity. isSecureIndex is a 2 line fix so if only code size is the criteria, its much smaller than this patch.

@aaronlehmann
Copy link
Contributor

LGTM

I agree about not making bigger changes here. This code changes a lot in #17924, so it will be easier to make additional changes after that PR is merged.

@anusha-ragunathan
Copy link
Contributor

LGTM

LK4D4 added a commit that referenced this pull request Nov 17, 2015
@LK4D4
Merge pull request #18014 from calavera/make_normalize_local_name_local
e55274e 
Make NormalizeLocalName to not reach the network to normalize names.
@LK4D4 LK4D4 merged commit e55274e into moby:master Nov 17, 2015
@LK4D4
Copy link
Contributor

LK4D4 commented Nov 17, 2015

Okay, looks like 4 LGTMs is enough

@tiborvass tiborvass mentioned this pull request Nov 20, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/distribution
Projects
None yet
Milestone
1.9.1
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@calavera @LK4D4 @aaronlehmann @tonistiigi @anusha-ragunathan @tiborvass @GordonTheTurtle