Skip to content

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Oct 31, 2024

This is just a straight fork with some minor changes;

  • replaced distribution's uuid package for github.com/google/uuid in tests
  • removed uses of distribution's "context" package for a regular context (only used in tests)
  • fix some linting issues

I will do some follow-ups;

Note that this client should be replaced with something better altogether, but having a local fork allows us to make some granular changes ahead of that.

fork distribution client v2

This forks the distribution's registry client (v2) at v2.8.3;
distribution/distribution@4772604

This is the result of the following steps:

# install filter-repo (https://github.com/newren/git-filter-repo/blob/main/INSTALL.md)
brew install git-filter-repo

# create a temporary clone of docker
cd ~/Projects
git clone https://github.com/distribution/distribution.git distribution_temp
cd distribution_temp

# switch to v2.8.3 (git-filter doesn't like multiple branches, so reset)
git reset --hard v2.8.3

# commit taken from
git rev-parse --verify HEAD
4772604ae973031ab32dd9805a4bccf61d94909f

git filter-repo --analyze

# remove all code, except for 'registry/client', 'registry/storage/cache',
# and 'testutil', and rename to 'internal/registryclient', 'internal/registryclient/cache'
# and 'internal/registryclient/testutil'
git filter-repo \
  --force \
  --path 'registry/client' \
  --path 'registry/storage/cache/cache.go' \
  --path 'registry/storage/cache/cachedblobdescriptorstore.go' \
  --path 'registry/storage/cache/cachecheck/suite.go' \
  --path 'registry/storage/cache/memory' \
  --path 'testutil/handler.go' \
  --path-rename registry/client:internal/registryclient \
  --path-rename registry/storage/cache:internal/registryclient/cache \
  --path-rename internal/registryclient/cache/cachecheck/suite.go:internal/registryclient/cache/memory/memory_utils_test.go \
  --path-rename testutil:internal/registryclient/testutil

# go to the target github.com/docker/docker repository
cd ~/go/src/github.com/docker/docker

# create a branch to work with
git checkout -b fork_registryclient_v2

# add the temporary repository as an upstream and make sure it's up-to-date
git remote add distribution_temp ~/Projects/distribution_temp
git fetch distribution_temp

# merge the upstream code
git merge --allow-unrelated-histories --signoff -S distribution_temp/main

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

stevvooe and others added 30 commits February 11, 2015 12:42
Signed-off-by: Stephen J Day <stephen.day@docker.com>
After all of the perl refactoring, some import orderings were left asunder.
This commit corrects that.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
Signed-off-by: Stephen J Day <stephen.day@docker.com>
This changeset defines the interface for layer info caches. Layer info caches
speed up access to layer meta data accessed in storage driver backends. The
two main operations are tests for repository membership and resolving path and
size information for backend blobs.

Two implementations are available. The main implementation leverages redis to
store layer info. An alternative implementation simply caches layer info in
maps, which should speed up resolution for less sophisticated implementations.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
This PR refactors the blob service API to be oriented around blob descriptors.
Identified by digests, blobs become an abstract entity that can be read and
written using a descriptor as a handle. This allows blobs to take many forms,
such as a ReadSeekCloser or a simple byte buffer, allowing blob oriented
operations to better integrate with blob agnostic APIs (such as the `io`
package). The error definitions are now better organized to reflect conditions
that can only be seen when interacting with the blob API.

The main benefit of this is to separate the much smaller metadata from large
file storage. Many benefits also follow from this. Reading and writing has
been separated into discrete services. Backend implementation is also
simplified, by reducing the amount of metadata that needs to be picked up to
simply serve a read. This also improves cacheability.

"Opening" a blob simply consists of an access check (Stat) and a path
calculation. Caching is greatly simplified and we've made the mapping of
provisional to canonical hashes a first-class concept. BlobDescriptorService
and BlobProvider can be combined in different ways to achieve varying effects.

Recommend Review Approach
-------------------------

This is a very large patch. While apologies are in order, we are getting a
considerable amount of refactoring. Most changes follow from the changes to
the root package (distribution), so start there. From there, the main changes
are in storage. Looking at (*repository).Blobs will help to understand the how
the linkedBlobStore is wired. One can explore the internals within and also
branch out into understanding the changes to the caching layer. Following the
descriptions below will also help to guide you.

To reduce the chances for regressions, it was critical that major changes to
unit tests were avoided. Where possible, they are left untouched and where
not, the spirit is hopefully captured. Pay particular attention to where
behavior may have changed.

Storage
-------

The primary changes to the `storage` package, other than the interface
updates, were to merge the layerstore and blobstore. Blob access is now
layered even further. The first layer, blobStore, exposes a global
`BlobStatter` and `BlobProvider`. Operations here provide a fast path for most
read operations that don't take access control into account. The
`linkedBlobStore` layers on top of the `blobStore`, providing repository-
scoped blob link management in the backend. The `linkedBlobStore` implements
the full `BlobStore` suite, providing access-controlled, repository-local blob
writers. The abstraction between the two is slightly broken in that
`linkedBlobStore` is the only channel under which one can write into the global
blob store. The `linkedBlobStore` also provides flexibility in that it can act
over different link sets depending on configuration. This allows us to use the
same code for signature links, manifest links and blob links.  Eventually, we
will fully consolidate this storage.

The improved cache flow comes from the `linkedBlobStatter` component
of `linkedBlobStore`. Using a `cachedBlobStatter`, these combine together to
provide a simple cache hierarchy that should streamline access checks on read
and write operations, or at least provide a single path to optimize. The
metrics have been changed in a slightly incompatible way since the former
operations, Fetch and Exists, are no longer relevant.

The fileWriter and fileReader have been slightly modified to support the rest
of the changes. The most interesting is the removal of the `Stat` call from
`newFileReader`. This was the source of unnecessary round trips that were only
present to look up the size of the resulting reader. Now, one must simply pass
in the size, requiring the caller to decide whether or not the `Stat` call is
appropriate. In several cases, it turned out the caller already had the size
already. The `WriterAt` implementation has been removed from `fileWriter`,
since it is no longer required for `BlobWriter`, reducing the number of paths
which writes may take.

Cache
-----

Unfortunately, the `cache` package required a near full rewrite. It was pretty
mechanical in that the cache is oriented around the `BlobDescriptorService`
slightly modified to include the ability to set the values for individual
digests. While the implementation is oriented towards caching, it can act as a
primary store. Provisions are in place to have repository local metadata, in
addition to global metadata. Fallback is implemented as a part of the storage
package to maintain this flexibility.

One unfortunate side-effect is that caching is now repository-scoped, rather
than global. This should have little effect on performance but may increase
memory usage.

Handlers
--------

The `handlers` package has been updated to leverage the new API. For the most
part, the changes are superficial or mechanical based on the API changes. This
did expose a bug in the handling of provisional vs canonical digests that was
fixed in the unit tests.

Configuration
-------------

One user-facing change has been made to the configuration and is updated in
the associated documentation. The `layerinfo` cache parameter has been
deprecated by the `blobdescriptor` cache parameter. Both are equivalent and
configuration files should be backward compatible.

Notifications
-------------

Changes the `notification` package are simply to support the interface
changes.

Context
-------

A small change has been made to the tracing log-level. Traces have been moved
from "info" to "debug" level to reduce output when not needed.

Signed-off-by: Stephen J Day <stephen.day@docker.com>
Adds functionality to create a Repository client which connects to a remote endpoint.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Layer upload moved to its own file with its own unit tests

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Refactory authorizer to take a set of authentication handlers for different authentication schemes returned by an unauthorized HTTP requst.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Wrapping the reader in a NopCloser is necessary to prevent the http library from closing the input reader.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Repository creation now just takes in an http.RoundTripper. Authenticated requests or requests which require additional headers should use the NewTransport function along with a request modifier (such an an authentication handler).

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Added use of cache blob statter

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Update comments and TODOs
Fix switch style
Updated parse http response to take in reader
Add Cancel implementation
Update blobstore variable name

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Each type no longer requires holding a reference to repository.
Added implementation for signatures get.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Add check for unauthorized error code and explicitly set the error code if the content could not be parsed.
Updated repository test for unauthorized tests and nit feedback.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
The transport package no longer requires importing distribution for the ReadSeekCloser, instead declares its own.
Added comments on the Authenication handler in session.
Added todo on http seek reader to highlight its lack of belonging to the client transport.

Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
@thaJeztah thaJeztah modified the milestones: 28.0.0, 29.0.0 Jan 17, 2025
This forks the distribution's registry client (v2) at v2.8.3;
distribution/distribution@4772604

This is the result of the following steps:

    # install filter-repo (https://github.com/newren/git-filter-repo/blob/main/INSTALL.md)
    brew install git-filter-repo

    # create a temporary clone of docker
    cd ~/Projects
    git clone https://github.com/distribution/distribution.git distribution_temp
    cd distribution_temp

    # switch to v2.8.3 (git-filter doesn't like multiple branches, so reset)
    git reset --hard v2.8.3

    # commit taken from
    git rev-parse --verify HEAD
    4772604ae973031ab32dd9805a4bccf61d94909f

    git filter-repo --analyze

    # remove all code, except for 'registry/client', 'registry/storage/cache',
    # and 'testutil', and rename to 'internal/registryclient', 'internal/registryclient/cache'
    # and 'internal/registryclient/testutil'
    git filter-repo \
      --force \
      --path 'registry/client' \
      --path 'registry/storage/cache/cache.go' \
      --path 'registry/storage/cache/cachedblobdescriptorstore.go' \
      --path 'registry/storage/cache/cachecheck/suite.go' \
      --path 'registry/storage/cache/memory' \
      --path 'testutil/handler.go' \
      --path-rename registry/client:internal/registryclient \
      --path-rename registry/storage/cache:internal/registryclient/cache \
      --path-rename internal/registryclient/cache/cachecheck/suite.go:internal/registryclient/cache/memory/memory_utils_test.go \
      --path-rename testutil:internal/registryclient/testutil

    # go to the target github.com/docker/docker repository
    cd ~/go/src/github.com/docker/docker

    # create a branch to work with
    git checkout -b fork_registryclient_v2

    # add the temporary repository as an upstream and make sure it's up-to-date
    git remote add distribution_temp ~/Projects/distribution_temp
    git fetch distribution_temp

    # merge the upstream code
    git merge --allow-unrelated-histories --signoff -S distribution_temp/main

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the fork_registryclient_v2 branch 4 times, most recently from 2475d68 to f24ec17 Compare March 23, 2025 17:42
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the fork_registryclient_v2 branch 5 times, most recently from 2f35c11 to d7c9a44 Compare March 23, 2025 20:20
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    internal/registryclient/auth/session_test.go:69:15: G107: Potential HTTP request made with variable url (gosec)
        resp, err := http.Get(endpoint)
                     ^
    internal/registryclient/transport/http_reader.go:13:23: use of `regexp.MustCompile` forbidden because "Use internal/lazyregexp.New instead." (forbidigo)
        contentRangeRegexp = regexp.MustCompile(`bytes ([0-9]+)-([0-9]+)/([0-9]+|\\*)`)
                             ^
    internal/registryclient/repository.go:194:6: shadow: declaration of "desc" shadows declaration at line 179 (govet)
            _, desc, err := distribution.UnmarshalManifest(ctHeader, bytes)
               ^
    internal/registryclient/transport/transport.go:133:2: naked return in func `Read` with 6 lines of code (nakedret)
        return
        ^
    internal/registryclient/auth/challenge/authchallenge.go:159:3: naked return in func `parseValueAndParams` with 27 lines of code (nakedret)
            return
            ^
    internal/registryclient/auth/challenge/authchallenge.go:167:4: naked return in func `parseValueAndParams` with 27 lines of code (nakedret)
                return
                ^
    internal/registryclient/auth/challenge/authchallenge.go:170:4: naked return in func `parseValueAndParams` with 27 lines of code (nakedret)
                return
                ^
    internal/registryclient/auth/challenge/authchallenge.go:175:4: naked return in func `parseValueAndParams` with 27 lines of code (nakedret)
                return
                ^
    internal/registryclient/auth/challenge/authchallenge.go:181:2: naked return in func `parseValueAndParams` with 27 lines of code (nakedret)
        return
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Looks like no logging would happen for a cache without a tracker set;
update the code to use context-logger.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Alternatively, we can use "github.com/docker/go-metrics" directly.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
These functions were only used in tests; move them to a _test.go file
to indicate they're not used elsewhere.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This interface references distribution.TagService, which provides
methods that are not implemented, so now require stubbing out in
some places.

Start with copying the interface as-is; then we can start removing
parts that are not used.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
We only use the "Get" and "List" methods; decouple from distribution,
and return our concrete implementation that only provides those two.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
We don't provide, nor need, the extra functionality of the
blobstore.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This branch is importing many historical commits, not all of which
have a proper DCO.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies area/distribution Image Distribution kind/refactor PR's that refactor, or clean-up code status/2-code-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.