From 56af60ad24302a721582e1a2731d55561464d0b9 Mon Sep 17 00:00:00 2001 From: Richard Scothern Date: Wed, 10 Feb 2016 18:07:28 -0800 Subject: [PATCH 1/2] To avoid any network use unless necessary, delay establishing authorization challenges with the upstream until any proxied data is found not to be local. Implement auth challenges behind an interface and add to unit tests. Also, remove a non-sensical unit test. Signed-off-by: Richard Scothern --- registry/proxy/proxyauth.go | 9 +-- registry/proxy/proxyblobstore.go | 9 +++ registry/proxy/proxyblobstore_test.go | 6 ++ registry/proxy/proxymanifeststore.go | 9 ++- registry/proxy/proxymanifeststore_test.go | 37 +++++++++-- registry/proxy/proxyregistry.go | 80 ++++++++++++++++++----- registry/proxy/proxytagservice.go | 27 +++++--- registry/proxy/proxytagservice_test.go | 23 ++++++- 8 files changed, 156 insertions(+), 44 deletions(-) diff --git a/registry/proxy/proxyauth.go b/registry/proxy/proxyauth.go index e4bec75a5..bcfa7aab0 100644 --- a/registry/proxy/proxyauth.go +++ b/registry/proxy/proxyauth.go @@ -8,6 +8,7 @@ import ( ) const tokenURL = "https://auth.docker.io/token" +const challengeHeader = "Docker-Distribution-Api-Version" type userpass struct { username string @@ -24,12 +25,8 @@ func (c credentials) Basic(u *url.URL) (string, string) { return up.username, up.password } -// ConfigureAuth authorizes with the upstream registry -func ConfigureAuth(remoteURL, username, password string, cm auth.ChallengeManager) (auth.CredentialStore, error) { - if err := ping(cm, remoteURL+"/v2/", "Docker-Distribution-Api-Version"); err != nil { - return nil, err - } - +// ConfigureAuth stores credentials for challenge responses +func configureAuth(username, password string) (auth.CredentialStore, error) { creds := map[string]userpass{ tokenURL: { username: username, diff --git a/registry/proxy/proxyblobstore.go b/registry/proxy/proxyblobstore.go index 1d7dfbc66..5f1a9c504 100644 --- a/registry/proxy/proxyblobstore.go +++ b/registry/proxy/proxyblobstore.go @@ -22,6 +22,7 @@ type proxyBlobStore struct { remoteStore distribution.BlobService scheduler *scheduler.TTLExpirationScheduler repositoryName reference.Named + authChallenger authChallenger } var _ distribution.BlobStore = &proxyBlobStore{} @@ -121,6 +122,10 @@ func (pbs *proxyBlobStore) ServeBlob(ctx context.Context, w http.ResponseWriter, return nil } + if err := pbs.authChallenger.tryEstablishChallenges(ctx); err != nil { + return err + } + mu.Lock() _, ok := inflight[dgst] if ok { @@ -162,6 +167,10 @@ func (pbs *proxyBlobStore) Stat(ctx context.Context, dgst digest.Digest) (distri return distribution.Descriptor{}, err } + if err := pbs.authChallenger.tryEstablishChallenges(ctx); err != nil { + return distribution.Descriptor{}, err + } + return pbs.remoteStore.Stat(ctx, dgst) } diff --git a/registry/proxy/proxyblobstore_test.go b/registry/proxy/proxyblobstore_test.go index 3054ef0b8..4d63aa423 100644 --- a/registry/proxy/proxyblobstore_test.go +++ b/registry/proxy/proxyblobstore_test.go @@ -168,6 +168,7 @@ func makeTestEnv(t *testing.T, name string) *testEnv { remoteStore: truthBlobs, localStore: localBlobs, scheduler: s, + authChallenger: &mockChallenger{}, } te := &testEnv{ @@ -242,6 +243,11 @@ func TestProxyStoreStat(t *testing.T) { if (*remoteStats)["stat"] != remoteBlobCount { t.Errorf("Unexpected remote stat count") } + + if te.store.authChallenger.(*mockChallenger).count != len(te.inRemote) { + t.Fatalf("Unexpected auth challenge count, got %#v", te.store.authChallenger) + } + } func TestProxyStoreServeHighConcurrency(t *testing.T) { diff --git a/registry/proxy/proxymanifeststore.go b/registry/proxy/proxymanifeststore.go index 0b5532d47..b81096672 100644 --- a/registry/proxy/proxymanifeststore.go +++ b/registry/proxy/proxymanifeststore.go @@ -19,6 +19,7 @@ type proxyManifestStore struct { remoteManifests distribution.ManifestService repositoryName reference.Named scheduler *scheduler.TTLExpirationScheduler + authChallenger authChallenger } var _ distribution.ManifestService = &proxyManifestStore{} @@ -31,7 +32,9 @@ func (pms proxyManifestStore) Exists(ctx context.Context, dgst digest.Digest) (b if exists { return true, nil } - + if err := pms.authChallenger.tryEstablishChallenges(ctx); err != nil { + return false, err + } return pms.remoteManifests.Exists(ctx, dgst) } @@ -41,6 +44,10 @@ func (pms proxyManifestStore) Get(ctx context.Context, dgst digest.Digest, optio var fromRemote bool manifest, err := pms.localManifests.Get(ctx, dgst, options...) if err != nil { + if err := pms.authChallenger.tryEstablishChallenges(ctx); err != nil { + return nil, err + } + manifest, err = pms.remoteManifests.Get(ctx, dgst, options...) if err != nil { return nil, err diff --git a/registry/proxy/proxymanifeststore_test.go b/registry/proxy/proxymanifeststore_test.go index 00f9daf93..e16fa6f51 100644 --- a/registry/proxy/proxymanifeststore_test.go +++ b/registry/proxy/proxymanifeststore_test.go @@ -2,6 +2,7 @@ package proxy import ( "io" + "sync" "testing" "github.com/docker/distribution" @@ -64,6 +65,20 @@ func (sm statsManifest) Put(ctx context.Context, manifest distribution.Manifest, } */ +type mockChallenger struct { + sync.Mutex + count int +} + +// Called for remote operations only +func (mc *mockChallenger) tryEstablishChallenges(context.Context) error { + mc.Lock() + defer mc.Unlock() + + mc.count++ + return nil +} + func newManifestStoreTestEnv(t *testing.T, name, tag string) *manifestStoreTestEnv { nameRef, err := reference.ParseNamed(name) if err != nil { @@ -120,6 +135,7 @@ func newManifestStoreTestEnv(t *testing.T, name, tag string) *manifestStoreTestE remoteManifests: truthManifests, scheduler: s, repositoryName: nameRef, + authChallenger: &mockChallenger{}, }, } } @@ -198,6 +214,10 @@ func TestProxyManifests(t *testing.T) { t.Errorf("Unexpected exists count : \n%v \n%v", localStats, remoteStats) } + if env.manifests.authChallenger.(*mockChallenger).count != 1 { + t.Fatalf("Expected 1 auth challenge, got %#v", env.manifests.authChallenger) + } + // Get - should succeed and pull manifest into local _, err = env.manifests.Get(ctx, env.manifestDigest) if err != nil { @@ -212,6 +232,10 @@ func TestProxyManifests(t *testing.T) { t.Errorf("Expected local put") } + if env.manifests.authChallenger.(*mockChallenger).count != 2 { + t.Fatalf("Expected 2 auth challenges, got %#v", env.manifests.authChallenger) + } + // Stat - should only go to local exists, err = env.manifests.Exists(ctx, env.manifestDigest) if err != nil { @@ -225,17 +249,18 @@ func TestProxyManifests(t *testing.T) { t.Errorf("Unexpected exists count") } - // Get - should get from remote, to test freshness + if env.manifests.authChallenger.(*mockChallenger).count != 2 { + t.Fatalf("Expected 2 auth challenges, got %#v", env.manifests.authChallenger) + } + + // Get proxied - won't require another authchallenge _, err = env.manifests.Get(ctx, env.manifestDigest) if err != nil { t.Fatal(err) } - if (*remoteStats)["get"] != 2 && (*remoteStats)["exists"] != 1 && (*localStats)["put"] != 1 { - t.Errorf("Unexpected get count") + if env.manifests.authChallenger.(*mockChallenger).count != 2 { + t.Fatalf("Expected 2 auth challenges, got %#v", env.manifests.authChallenger) } -} - -func TestProxyTagService(t *testing.T) { } diff --git a/registry/proxy/proxyregistry.go b/registry/proxy/proxyregistry.go index 6ea79ff6e..ae7086b5a 100644 --- a/registry/proxy/proxyregistry.go +++ b/registry/proxy/proxyregistry.go @@ -1,10 +1,11 @@ package proxy import ( + "fmt" "net/http" "net/url" + "sync" - "fmt" "github.com/docker/distribution" "github.com/docker/distribution/configuration" "github.com/docker/distribution/context" @@ -19,13 +20,10 @@ import ( // proxyingRegistry fetches content from a remote registry and caches it locally type proxyingRegistry struct { - embedded distribution.Namespace // provides local registry functionality - - scheduler *scheduler.TTLExpirationScheduler - - remoteURL string - credentialStore auth.CredentialStore - challengeManager auth.ChallengeManager + embedded distribution.Namespace // provides local registry functionality + scheduler *scheduler.TTLExpirationScheduler + remoteURL string + authChallenger authChallenger } // NewRegistryPullThroughCache creates a registry acting as a pull through cache @@ -93,18 +91,20 @@ func NewRegistryPullThroughCache(ctx context.Context, registry distribution.Name return nil, err } - challengeManager := auth.NewSimpleChallengeManager() - cs, err := ConfigureAuth(config.RemoteURL, config.Username, config.Password, challengeManager) + cs, err := configureAuth(config.Username, config.Password) if err != nil { return nil, err } return &proxyingRegistry{ - embedded: registry, - scheduler: s, - challengeManager: challengeManager, - credentialStore: cs, - remoteURL: config.RemoteURL, + embedded: registry, + scheduler: s, + remoteURL: config.RemoteURL, + authChallenger: &remoteAuthChallenger{ + remoteURL: config.RemoteURL, + challengeManager: auth.NewSimpleChallengeManager(), + credentialStore: cs, + }, }, nil } @@ -117,8 +117,13 @@ func (pr *proxyingRegistry) Repositories(ctx context.Context, repos []string, la } func (pr *proxyingRegistry) Repository(ctx context.Context, name reference.Named) (distribution.Repository, error) { + hcm, ok := pr.authChallenger.(*remoteAuthChallenger) + if !ok { + return nil, fmt.Errorf("unexpected challenge manager type %T", pr.authChallenger) + } + tr := transport.NewTransport(http.DefaultTransport, - auth.NewAuthorizer(pr.challengeManager, auth.NewTokenHandler(http.DefaultTransport, pr.credentialStore, name.Name(), "pull"))) + auth.NewAuthorizer(hcm.challengeManager, auth.NewTokenHandler(http.DefaultTransport, hcm.credentialStore, name.Name(), "pull"))) localRepo, err := pr.embedded.Repository(ctx, name) if err != nil { @@ -145,6 +150,7 @@ func (pr *proxyingRegistry) Repository(ctx context.Context, name reference.Named remoteStore: remoteRepo.Blobs(ctx), scheduler: pr.scheduler, repositoryName: name, + authChallenger: pr.authChallenger, }, manifests: &proxyManifestStore{ repositoryName: name, @@ -152,15 +158,53 @@ func (pr *proxyingRegistry) Repository(ctx context.Context, name reference.Named remoteManifests: remoteManifests, ctx: ctx, scheduler: pr.scheduler, + authChallenger: pr.authChallenger, }, name: name, tags: &proxyTagService{ - localTags: localRepo.Tags(ctx), - remoteTags: remoteRepo.Tags(ctx), + localTags: localRepo.Tags(ctx), + remoteTags: remoteRepo.Tags(ctx), + authChallenger: pr.authChallenger, }, }, nil } +// authChallenger encapsulates a request to the upstream to establish credential challenges +type authChallenger interface { + tryEstablishChallenges(context.Context) error +} + +type remoteAuthChallenger struct { + remoteURL string + sync.Mutex + challengeManager auth.ChallengeManager + credentialStore auth.CredentialStore +} + +// tryEstablishChallenges will attempt to get a challenge types for the upstream if none currently exist +func (hcm *remoteAuthChallenger) tryEstablishChallenges(ctx context.Context) error { + hcm.Lock() + defer hcm.Unlock() + + remoteURL := hcm.remoteURL + "/v2/" + challenges, err := hcm.challengeManager.GetChallenges(remoteURL) + if err != nil { + return err + } + + if len(challenges) > 0 { + return nil + } + + // establish challenge type with upstream + if err := ping(hcm.challengeManager, remoteURL, challengeHeader); err != nil { + return err + } + + context.GetLogger(ctx).Infof("Challenge established with upstream : %s %s", remoteURL, hcm.challengeManager) + return nil +} + // proxiedRepository uses proxying blob and manifest services to serve content // locally, or pulling it through from a remote and caching it locally if it doesn't // already exist diff --git a/registry/proxy/proxytagservice.go b/registry/proxy/proxytagservice.go index c52460c44..a8273030d 100644 --- a/registry/proxy/proxytagservice.go +++ b/registry/proxy/proxytagservice.go @@ -7,8 +7,9 @@ import ( // proxyTagService supports local and remote lookup of tags. type proxyTagService struct { - localTags distribution.TagService - remoteTags distribution.TagService + localTags distribution.TagService + remoteTags distribution.TagService + authChallenger authChallenger } var _ distribution.TagService = proxyTagService{} @@ -17,16 +18,19 @@ var _ distribution.TagService = proxyTagService{} // tag service first and then caching it locally. If the remote is unavailable // the local association is returned func (pt proxyTagService) Get(ctx context.Context, tag string) (distribution.Descriptor, error) { - desc, err := pt.remoteTags.Get(ctx, tag) + err := pt.authChallenger.tryEstablishChallenges(ctx) if err == nil { - err := pt.localTags.Tag(ctx, tag, desc) - if err != nil { - return distribution.Descriptor{}, err + desc, err := pt.remoteTags.Get(ctx, tag) + if err == nil { + err := pt.localTags.Tag(ctx, tag, desc) + if err != nil { + return distribution.Descriptor{}, err + } + return desc, nil } - return desc, nil } - desc, err = pt.localTags.Get(ctx, tag) + desc, err := pt.localTags.Get(ctx, tag) if err != nil { return distribution.Descriptor{}, err } @@ -46,9 +50,12 @@ func (pt proxyTagService) Untag(ctx context.Context, tag string) error { } func (pt proxyTagService) All(ctx context.Context) ([]string, error) { - tags, err := pt.remoteTags.All(ctx) + err := pt.authChallenger.tryEstablishChallenges(ctx) if err == nil { - return tags, err + tags, err := pt.remoteTags.All(ctx) + if err == nil { + return tags, err + } } return pt.localTags.All(ctx) } diff --git a/registry/proxy/proxytagservice_test.go b/registry/proxy/proxytagservice_test.go index 8d9518c03..a446645cb 100644 --- a/registry/proxy/proxytagservice_test.go +++ b/registry/proxy/proxytagservice_test.go @@ -69,8 +69,9 @@ func testProxyTagService(local, remote map[string]distribution.Descriptor) *prox remote = make(map[string]distribution.Descriptor) } return &proxyTagService{ - localTags: &mockTagStore{mapping: local}, - remoteTags: &mockTagStore{mapping: remote}, + localTags: &mockTagStore{mapping: local}, + remoteTags: &mockTagStore{mapping: remote}, + authChallenger: &mockChallenger{}, } } @@ -87,6 +88,10 @@ func TestGet(t *testing.T) { t.Fatal(err) } + if proxyTags.authChallenger.(*mockChallenger).count != 1 { + t.Fatalf("Expected 1 auth challenge call, got %#v", proxyTags.authChallenger) + } + if d != remoteDesc { t.Fatal("unable to get put tag") } @@ -112,6 +117,10 @@ func TestGet(t *testing.T) { t.Fatal(err) } + if proxyTags.authChallenger.(*mockChallenger).count != 2 { + t.Fatalf("Expected 2 auth challenge calls, got %#v", proxyTags.authChallenger) + } + if d != newRemoteDesc { t.Fatal("unable to get put tag") } @@ -142,7 +151,11 @@ func TestGet(t *testing.T) { t.Fatal("untagged tag should be pulled through") } - // Add another tag. Ensure both tags appear in enumerate + if proxyTags.authChallenger.(*mockChallenger).count != 3 { + t.Fatalf("Expected 3 auth challenge calls, got %#v", proxyTags.authChallenger) + } + + // Add another tag. Ensure both tags appear in 'All' err = proxyTags.remoteTags.Tag(ctx, "funtag", distribution.Descriptor{Size: 42}) if err != nil { t.Fatal(err) @@ -161,4 +174,8 @@ func TestGet(t *testing.T) { if all[0] != "funtag" && all[1] != "remote" { t.Fatalf("Unexpected tags returned from All() : %v ", all) } + + if proxyTags.authChallenger.(*mockChallenger).count != 4 { + t.Fatalf("Expected 4 auth challenge calls, got %#v", proxyTags.authChallenger) + } } From 4ce15476bd44b37d5a84ffebc8cde16a4ff2f798 Mon Sep 17 00:00:00 2001 From: Richard Scothern Date: Wed, 17 Feb 2016 10:42:34 -0800 Subject: [PATCH 2/2] Extend authChallenger interface to remove type cast. Signed-off-by: Richard Scothern --- registry/proxy/proxyauth.go | 2 +- registry/proxy/proxymanifeststore_test.go | 17 ++++++--- registry/proxy/proxyregistry.go | 43 +++++++++++++---------- 3 files changed, 39 insertions(+), 23 deletions(-) diff --git a/registry/proxy/proxyauth.go b/registry/proxy/proxyauth.go index bcfa7aab0..6f0eb0050 100644 --- a/registry/proxy/proxyauth.go +++ b/registry/proxy/proxyauth.go @@ -25,7 +25,7 @@ func (c credentials) Basic(u *url.URL) (string, string) { return up.username, up.password } -// ConfigureAuth stores credentials for challenge responses +// configureAuth stores credentials for challenge responses func configureAuth(username, password string) (auth.CredentialStore, error) { creds := map[string]userpass{ tokenURL: { diff --git a/registry/proxy/proxymanifeststore_test.go b/registry/proxy/proxymanifeststore_test.go index e16fa6f51..312eb343d 100644 --- a/registry/proxy/proxymanifeststore_test.go +++ b/registry/proxy/proxymanifeststore_test.go @@ -11,6 +11,7 @@ import ( "github.com/docker/distribution/manifest" "github.com/docker/distribution/manifest/schema1" "github.com/docker/distribution/reference" + "github.com/docker/distribution/registry/client/auth" "github.com/docker/distribution/registry/proxy/scheduler" "github.com/docker/distribution/registry/storage" "github.com/docker/distribution/registry/storage/cache/memory" @@ -71,11 +72,19 @@ type mockChallenger struct { } // Called for remote operations only -func (mc *mockChallenger) tryEstablishChallenges(context.Context) error { - mc.Lock() - defer mc.Unlock() +func (m *mockChallenger) tryEstablishChallenges(context.Context) error { + m.Lock() + defer m.Unlock() - mc.count++ + m.count++ + return nil +} + +func (m *mockChallenger) credentialStore() auth.CredentialStore { + return nil +} + +func (m *mockChallenger) challengeManager() auth.ChallengeManager { return nil } diff --git a/registry/proxy/proxyregistry.go b/registry/proxy/proxyregistry.go index ae7086b5a..e25fe783c 100644 --- a/registry/proxy/proxyregistry.go +++ b/registry/proxy/proxyregistry.go @@ -101,9 +101,9 @@ func NewRegistryPullThroughCache(ctx context.Context, registry distribution.Name scheduler: s, remoteURL: config.RemoteURL, authChallenger: &remoteAuthChallenger{ - remoteURL: config.RemoteURL, - challengeManager: auth.NewSimpleChallengeManager(), - credentialStore: cs, + remoteURL: config.RemoteURL, + cm: auth.NewSimpleChallengeManager(), + cs: cs, }, }, nil } @@ -117,13 +117,10 @@ func (pr *proxyingRegistry) Repositories(ctx context.Context, repos []string, la } func (pr *proxyingRegistry) Repository(ctx context.Context, name reference.Named) (distribution.Repository, error) { - hcm, ok := pr.authChallenger.(*remoteAuthChallenger) - if !ok { - return nil, fmt.Errorf("unexpected challenge manager type %T", pr.authChallenger) - } + c := pr.authChallenger tr := transport.NewTransport(http.DefaultTransport, - auth.NewAuthorizer(hcm.challengeManager, auth.NewTokenHandler(http.DefaultTransport, hcm.credentialStore, name.Name(), "pull"))) + auth.NewAuthorizer(c.challengeManager(), auth.NewTokenHandler(http.DefaultTransport, c.credentialStore(), name.Name(), "pull"))) localRepo, err := pr.embedded.Repository(ctx, name) if err != nil { @@ -172,22 +169,32 @@ func (pr *proxyingRegistry) Repository(ctx context.Context, name reference.Named // authChallenger encapsulates a request to the upstream to establish credential challenges type authChallenger interface { tryEstablishChallenges(context.Context) error + challengeManager() auth.ChallengeManager + credentialStore() auth.CredentialStore } type remoteAuthChallenger struct { remoteURL string sync.Mutex - challengeManager auth.ChallengeManager - credentialStore auth.CredentialStore + cm auth.ChallengeManager + cs auth.CredentialStore } -// tryEstablishChallenges will attempt to get a challenge types for the upstream if none currently exist -func (hcm *remoteAuthChallenger) tryEstablishChallenges(ctx context.Context) error { - hcm.Lock() - defer hcm.Unlock() +func (r *remoteAuthChallenger) credentialStore() auth.CredentialStore { + return r.cs +} - remoteURL := hcm.remoteURL + "/v2/" - challenges, err := hcm.challengeManager.GetChallenges(remoteURL) +func (r *remoteAuthChallenger) challengeManager() auth.ChallengeManager { + return r.cm +} + +// tryEstablishChallenges will attempt to get a challenge type for the upstream if none currently exist +func (r *remoteAuthChallenger) tryEstablishChallenges(ctx context.Context) error { + r.Lock() + defer r.Unlock() + + remoteURL := r.remoteURL + "/v2/" + challenges, err := r.cm.GetChallenges(remoteURL) if err != nil { return err } @@ -197,11 +204,11 @@ func (hcm *remoteAuthChallenger) tryEstablishChallenges(ctx context.Context) err } // establish challenge type with upstream - if err := ping(hcm.challengeManager, remoteURL, challengeHeader); err != nil { + if err := ping(r.cm, remoteURL, challengeHeader); err != nil { return err } - context.GetLogger(ctx).Infof("Challenge established with upstream : %s %s", remoteURL, hcm.challengeManager) + context.GetLogger(ctx).Infof("Challenge established with upstream : %s %s", remoteURL, r.cm) return nil }