Cleanup: Replace ResolveRepositoryName with RepositoryInfo{} #8456

dkjer · 2014-10-07T20:31:14Z

Reason for cleanup

Currently, we breakdown a repository name using a combination of ResolveRepositoryName, and NewEndpoint as follows:

  debian
  \____/
     |
  repoName  =>  ResolveRepositoryName()
                      |
               ______/ \_____________________
              /                              \
  https://index.docker.io/v1/              debian
  \_________________________/              \____/
        |                                     |
     hostname  =>  NewEndpoint()          remoteName
                       |                      |
                    endpoint              ___/\______
                       |                 /           \
 https://index.docker.io/v1/      library/debian    debian
 \______/\_____________/\__/      \____________/     \___/
     |          |         |              |             |
 URL.Scheme  URL.Host  Version      remoteName~    localName



  127.0.0.1:5000/myapp _________________________________
  \__________________/                                  \
     |                                                  |
  repoName  => ResolveRepositoryName()                  |
                      |                                 |
               ______/ \______________________          |
              /                               \         |
       127.0.0.1:5000                       myapp       |
       \____________/                       \____/      |
         |                                    |         |
     hostname  =>  NewEndpoint()              |         |
                       |                      |         |
      GET https://127.0.0.1:5000/v1/_ping     |         |
                       |                      |         |
                       x (Error)              |         |
                       |                      |         |
      GET http://127.0.0.1:5000/v1/_ping      |         |
                       |                      |         |
                    endpoint                  |         |
                       |                      |         |
 http://127.0.0.1:5000/v1/                    |         |
 \_____/\____________/\__/                    |         |
     |        |         |                     |         |
 URL.Scheme  URL.Host  Version          remoteName  localName

However, policies around whether or not repository is using the 'official index' or an 'official repository' are sprinkled throughout the code. Also, rules regarding remoteName for the official repositories ('library/ prefix'), localName, which mirrors to use, etc, are also sprinkled throughout the code.

This results in awkward follow-up checks against repository names, such as

if len(strings.SplitN(name, "/", 2)) == 1 { ... "You cannot push a \"root\" repository." ...}

if endpoint.VersionString(1) == registry.IndexServerAddress() { ... [clean up names and set up mirrors] ... }

Having to track down these pieces of logic throughout the code makes modifications to repository name parsing fragile, and hard to test.

Refactors made

ResolveRepositoryName(repoName) deprecated.

Replaced with the following:

ValidateRepositoryName(repoName)
- Used when a RepositoryInfo breakdown is not needed, just a validation check.
ParseRepositoryInfo(repoName)
- Used when RegistryServiceConfig is not available, and just performs the breakdown (above)
NormalizeLocalName
- Transforms a name for use as a key into TagStore.Repositories.
- Used to avoid multiple official image names referring to the same image (e.g. 'library/debian' vs 'debian')
ResolveRepositoryInfo
- Calls a Job("resolve_repository") to fully resolve a repository name into a RepositoryInfo
- Result includes index info.
ResolveIndexInfo
- Calls a Job("resolve_index") to resolve an index name into an IndexInfo
- Result includes repository configuration (for now, only Mirrors are defined).

RepositoryInfo

type IndexInfo struct {
        Name       string   `json:"name"`
        Mirrors    []string `json:"mirrors"`
        IsSecure   bool     `json:"is_secure"`
        IsOfficial bool     `json:"is_official"`
}

type RepositoryInfo struct {
        Index         *IndexInfo `json:"index"`
        RemoteName    string     `json:"remote_name"`
        LocalName     string     `json:"local_name"`
        CanonicalName string     `json:"canonical_name"`
        IsOfficial    bool       `json:"is_official"`
}

Returned by ParseRepositoryInfo, or ResolveRepositoryInfo.

Index.Name: Replaces 'hostname'.
- Examples: 'docker.io' (official), '127.0.0.1:8000' (private)
Index.Mirrors: List of mirrors for use in a pull.
- Only configurable for official index, currently.
Index.IsSecure:
- True for official index
- Value for private index depends on --insecure-registry flag
Index.IsOfficial: Whether or not repository refers to the official docker.io index.
RemoteName: Remote repository name
- Examples: 'library/debian', 'user/repo'
- Policies for official repo names are resolved.
LocalName: Name to use in TagStore.Repositories
- Examples: 'debian', '127.0.0.1:8000/user/repo'
- Policies for official vs private repo names are resolved.
CanonicalName: 'global' name of repository.
- Displayed to the user in information messages, e.g.: "Pulling image from "
- For now, the CanonicalName matches LocalName rules.
IsOfficial: Whether or not repository refers to a library repository on the official docker.io index.

LK4D4 · 2014-10-07T20:49:05Z

Pretty big commit :/

LK4D4 · 2014-10-07T20:59:26Z

Also TestSearchOnCentralRegistry failed

dkjer · 2014-10-07T21:07:43Z

Fix for TestSearchOnCentralRegistry committed

LK4D4 · 2014-10-07T21:20:20Z

I hope that +700 lines of code is mostly because of tests :)

dkjer · 2014-10-07T21:37:30Z

+522 -53 lines are from tests ;)

LK4D4 · 2014-10-08T02:33:19Z

@dkjer Hehe, now we have panic in tests :)

jessfraz · 2014-10-08T02:38:20Z

@dkjer @LK4D4 this is something else, disregard for now

jessfraz · 2014-10-17T18:36:14Z

integration-cli/docker_cli_pull_test.go

+	for _, name := range names {
+		pullCmd := exec.Command(dockerBinary, "pull", name)
+		out, exitCode, err := runCommandWithOutput(pullCmd)
+		errorOut(err, t, fmt.Sprintf("%s %s", out, err))


just a note, we removed the errorOut() function in a cleanup PR

Thanks @jfrazelle I have updated the PR

jessfraz · 2014-10-20T17:24:46Z

I like this a lot btw, it's a lot more readable now :) pinging @dmp42 and @dmcgowan because of the registry code cleanup

tiborvass · 2014-10-21T16:16:26Z

@dkjer needs a rebase :)

dkjer · 2014-10-21T20:05:23Z

@tiborvass rebased

dmcgowan · 2014-10-22T04:34:47Z

I like this change as well after a quick glance through. I will give it a more thorough look later in the week.

dmcgowan · 2014-10-22T04:38:18Z

registry/registry.go

@@ -35,7 +35,7 @@ const (
 	ConnectTimeout
 )

-func newClient(jar http.CookieJar, roots *x509.CertPool, cert *tls.Certificate, timeout TimeoutType) *http.Client {
+var newClient = func(jar http.CookieJar, roots *x509.CertPool, cert *tls.Certificate, timeout TimeoutType) *http.Client {


Is this intentional?

Yes, that was needed for wrapping newClient calls during tests.

See registry_mock_test.go func useInsecureHttpsClient()

k, thanks for pointing me there

dmcgowan · 2014-10-29T21:49:28Z

registry/auth.go

 	// First try the happy case
-	if c, found := config.Configs[hostname]; found {
+	if c, found := config.Configs[configKey]; found || repoInfo.IsOfficialIndex {


When is found false while IsOfficialIndex is true and why return an empty config in that case?

This was just to preserve the existing behavior. If we are using the IndexServerAddress we return Configs[IndexServerAddress()] even if it is empty:

func (config *ConfigFile) ResolveAuthConfig(hostname string) AuthConfig { if hostname == IndexServerAddress() || len(hostname) == 0 { // default to the index server return config.Configs[IndexServerAddress()] }

I thought perhaps you understood why that is the behavior. I agree there is no behavior change and no need to address further.

dmcgowan · 2014-10-29T23:46:45Z

Completed my review and testing, the code and new code layout looks good. Just need to address some of the questions I couldn't answer for myself.

Ping @vbatts and @jlhawn this might interest you guys as well

jlhawn · 2014-10-30T00:09:08Z

This is great. The RepositoryInfo struct should be very helpful going forward. I wonder though if there has yet been a proposal issue opened about separating image naming from transport, and another formal design proposal on how registry mirroring should work. @dmp42 should we open some proposal issues now to start discussing ideas openly?

dmp42 · 2014-10-30T00:54:42Z

Yes, we should.

Separation of naming and transport: what was floated around before was going with something like docker pull foo/bar --from http://somewhereregistry/ - or a similar way that would put the accent on what you are expecting to pull ("name"), and from where you are pulling it ("transport").

In that scenario, mirroring suddenly becomes trivial (from a client perspective).

Also, this opens the door for different transport types, eg:

docker pull foo/bar --from file:///somefolder (or docker pull foo/bar --from otherprotocol:///thing)

Indeed, let's get to a proposal.

dmcgowan · 2014-10-30T20:35:31Z

There is a pull behavioral change with this PR. Before the name as given on the command line is used as the local name of the repository, now the name is always canonicalized and that is used locally. The effect being git pull library/ubuntu pulls the image as ubuntu and not library/ubuntu. I am not convinced this is a bad thing but I think it should be discussed. @dmp42 initial thoughts on this?

dkjer · 2014-11-02T00:33:33Z

This PR had a lot of conflicts with 380c832 and afade42 . I ended up splitting index-related info from RepositoryInfo into IndexInfo, and reimplementing --insecure-registry.

dmp42 · 2014-11-03T16:12:56Z

@dkjer careful with --insecure-registry - it's a problematic flag in its current state (ping @tiborvass @proppy )

ping @dmcgowan for additional review

dmp42 · 2014-11-03T16:17:27Z

@dkjer #8898

dmp42 · 2014-11-03T16:47:20Z

Also relevant: #8887

dkjer · 2014-11-03T19:03:53Z

@dmp42 Yeah, --insecure-registry was also missing tests (everything passed, before reimplementing). I tried to just preserve existing functionality; however this sets me up to resolve conflicts for each --insecure-registry change that happens

dmcgowan · 2014-11-03T22:53:29Z

docker/daemon.go

-		log.Fatal(err)
-	}
-
-	// load registry service


Could the original flow stay the same, Register + Install and rather just update the Install call to
registry.NewService(registryCfg).Install(eng). This would keep builtins from needing to be changed and match the refactor change from the linked commit.

LK4D4 · 2014-12-23T05:21:57Z

@jfrazelle @tiborvass wanted to test this thoroughly because we almost have no tests for registry.

jessfraz · 2014-12-23T05:23:44Z

ah cool carry on :)

On Mon, Dec 22, 2014 at 9:22 PM, Alexander Morozov <notifications@github.com

wrote:

@jfrazelle https://github.com/jfrazelle @tiborvass
https://github.com/tiborvass wanted to test this thoroughly because we
almost have no tests for registry.

—
Reply to this email directly or view it on GitHub
#8456 (comment).

tiborvass · 2015-01-06T20:55:13Z

registry/registry_test.go

@@ -232,6 +738,7 @@ func TestSearchRepositories(t *testing.T) {
 	assertEqual(t, results.Results[0].StarCount, 42, "Expected 'fakeimage' a ot hae 42 stars")
 }

+<<<<<<< HEAD


@dkjer bad rebase :)

Passing RepositoryInfo to ResolveAuthConfig, pullRepository, and pushRepository Moving --registry-mirror configuration to registry config Created resolve_repository job Repo names with 'index.docker.io' or 'docker.io' are now synonymous with omitting an index name. Adding test for RepositoryInfo Adding tests for opts.StringSetOpts and registry.ValidateMirror Fixing search term use of repoInfo Adding integration tests for registry mirror configuration Normalizing LookupImage image name to match LocalName parsing rules Normalizing repository LocalName to avoid multiple references to an official image Removing errorOut use in tests Removing TODO comment gofmt changes golint comments cleanup. renaming RegistryOptions => registry.Options, and RegistryServiceConfig => registry.ServiceConfig Splitting out builtins.Registry and registry.NewService calls Stray whitespace cleanup Moving integration tests for Mirrors and InsecureRegistries into TestNewIndexInfo unit test Factoring out ValidateRepositoryName from NewRepositoryInfo Removing unused IndexServerURL Allowing json marshaling of ServiceConfig. Exposing ServiceConfig in /info Switching to CamelCase for json marshaling PR cleanup; removing 'Is' prefix from boolean members. Removing unneeded json tags. Removing non-cleanup related fix for 'localhost:[port]' in splitReposName Merge fixes for gh9735 Fixing integration test Reapplying #9754 Adding comment on config.IndexConfigs use from isSecureIndex Remove unused error return value from isSecureIndex Signed-off-by: Don Kjer <don.kjer@gmail.com> Adding back comment in isSecureIndex Signed-off-by: Don Kjer <don.kjer@gmail.com>

…nfig.go Signed-off-by: Don Kjer <don.kjer@gmail.com>

tiborvass · 2015-01-08T20:43:14Z

LGTM

Tested locally, cause drone doesn't like go1.3 syntax since it's using go1.4 gofmt.

Signed-off-by: Don Kjer <don.kjer@gmail.com>

LK4D4 · 2015-01-08T21:57:42Z

Haha, drone is happy.

Cleanup: Replace ResolveRepositoryName with RepositoryInfo{}

tiborvass · 2015-01-08T22:19:49Z

Thanks so much @dkjer for your awesome contribution and infinite patience! Happy new year :)

dkjer mentioned this pull request Oct 8, 2014

Proposal: Allow configuration of the docker daemon to disallow unqualified repositories #8326

Open

jessfraz reviewed Oct 17, 2014
View reviewed changes

dmcgowan reviewed Oct 22, 2014
View reviewed changes

dmp42 added the Distribution label Oct 22, 2014

dkjer mentioned this pull request Oct 25, 2014

--registry-mirror is ignored in new V2 pull codepath #8688

Closed

dmcgowan reviewed Oct 29, 2014
View reviewed changes

dmcgowan reviewed Nov 3, 2014
View reviewed changes

SvenDowideit added the /project/docs/needs edit label Dec 27, 2014

tiborvass reviewed Jan 6, 2015
View reviewed changes

dkjer added 2 commits January 8, 2015 20:14

Moving NewIndexInfo, NewRepositoryInfo and associated helpers into co…

6f0068f

…nfig.go Signed-off-by: Don Kjer <don.kjer@gmail.com>

Adding workaround to suppress gofmt issues with api/client/utils.go

33b931e

Signed-off-by: Don Kjer <don.kjer@gmail.com>

tiborvass added a commit that referenced this pull request Jan 8, 2015

Merge pull request #8456 from lindenlab/cleanup-repository-info

6870bde

Cleanup: Replace ResolveRepositoryName with RepositoryInfo{}

tiborvass merged commit 6870bde into moby:master Jan 8, 2015

dkjer deleted the cleanup-repository-info branch January 8, 2015 22:40

stevenschlansker mentioned this pull request Jan 9, 2015

Proposal: Mirroring Support for Private Registries #9161

Closed

SvenDowideit mentioned this pull request Jan 27, 2015

Post 1.4.1 docs update 2 #9739

Merged

dmcgowan mentioned this pull request Jan 29, 2015

Allow single name component repository names #10423

Merged

mmdriley mentioned this pull request Apr 22, 2015

fix issue #12281 #12527

Closed

tiborvass removed their assignment Nov 3, 2015

shin- mentioned this pull request Nov 23, 2015

Can't pull images with . In name. docker/docker-py#860

Closed

Cleanup: Replace ResolveRepositoryName with RepositoryInfo{} #8456

Cleanup: Replace ResolveRepositoryName with RepositoryInfo{} #8456

Conversation

dkjer commented Oct 7, 2014

Reason for cleanup

Refactors made

ResolveRepositoryName(repoName) deprecated.

RepositoryInfo

LK4D4 commented Oct 7, 2014

LK4D4 commented Oct 7, 2014

dkjer commented Oct 7, 2014

LK4D4 commented Oct 7, 2014

dkjer commented Oct 7, 2014

LK4D4 commented Oct 8, 2014

jessfraz commented Oct 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessfraz commented Oct 20, 2014

tiborvass commented Oct 21, 2014

dkjer commented Oct 21, 2014

dmcgowan commented Oct 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmcgowan commented Oct 29, 2014

jlhawn commented Oct 30, 2014

dmp42 commented Oct 30, 2014

dmcgowan commented Oct 30, 2014

dkjer commented Nov 2, 2014

dmp42 commented Nov 3, 2014

dmp42 commented Nov 3, 2014

dmp42 commented Nov 3, 2014

dkjer commented Nov 3, 2014

Choose a reason for hiding this comment

LK4D4 commented Dec 23, 2014

jessfraz commented Dec 23, 2014

Choose a reason for hiding this comment

tiborvass commented Jan 8, 2015

LK4D4 commented Jan 8, 2015

tiborvass commented Jan 8, 2015