Make the blob storage in pkg/registry pluggable. #1158

mattmoor · 2021-10-26T23:03:48Z

With this change folks can use registry.WithBlobHandler(...) to substitute a custom blob handling layer.

codecov-commenter · 2021-10-26T23:05:30Z

Codecov Report

Merging #1158 (2a9295a) into main (e7cd6af) will decrease coverage by 0.25%.
The diff coverage is 58.00%.

@@            Coverage Diff             @@
##             main    #1158      +/-   ##
==========================================
- Coverage   75.22%   74.96%   -0.26%     
==========================================
  Files         108      108              
  Lines        7850     7982     +132     
==========================================
+ Hits         5905     5984      +79     
- Misses       1379     1424      +45     
- Partials      566      574       +8

Impacted Files	Coverage Δ
pkg/registry/registry.go	`92.98% <50.00%> (-7.02%)`	⬇️
pkg/registry/blobs.go	`77.34% <58.45%> (-11.18%)`	⬇️

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 e7cd6af...2a9295a. Read the comment docs.

With this change folks can use `registry.WithBlobHandler(...)` to substitute a custom blob handling layer.

imjasonh

This looks good to me, if you're interested we can do manifests next (roughly the same way), and just leave uploads for later when/if we ever need it.

imjasonh · 2021-10-27T17:58:47Z

pkg/registry/blobs.go

@@ -102,10 +151,20 @@ func (b *blobs) handle(resp http.ResponseWriter, req *http.Request) *regError {
 			}
 		}

-		resp.Header().Set("Content-Length", fmt.Sprint(len(b)))
+		b, ok := b.bh.Get(repo, h)


I wonder if we can wrap the returned ReadCloser in a internal/verify.ReadCloser to automagically check the digest matches before returning it.

If we do it in here, we'll need to be sure to check the error on defer b.Close().

I think there are a bunch of additional checks we could add, so I'm sorta reluctant to start pulling that particular thread in this PR. Another notable bit is that we don't check the possible err return value on io.Copy() below, which at one point was the source of a fun bug in the Docker client (around digest verification!).

I think my personal bias would be towards leaning on internal consistency and checking things on their way in (vs. on the way out), but I'd be happy to iterate with you on follow up PRs here.

mattmoor · 2021-11-02T04:20:22Z

@imjasonh I'll optimistically start to look at what we might be able to do with manifests tomorrow.

mattmoor · 2021-11-02T04:23:55Z

@jonjohnsonjr PTAL

jonjohnsonjr · 2021-11-02T17:18:50Z

pkg/registry/blobs.go

 	"math/rand"
 	"net/http"
 	"path"
 	"strings"
 	"sync"
+
+	"github.com/google/go-containerregistry/pkg/name"


I've been trying to keep this from having any dependencies outside the stdlib.

Too late:

go-containerregistry/pkg/registry/manifest.go

Lines 31 to 32 in 0dfbb56

v1 "github.com/google/go-containerregistry/pkg/v1"

"github.com/google/go-containerregistry/pkg/v1/types"

Generally, I agree with this goal for implementations of the interfaces, but it seems wasteful to reimplement validation that exists in the name and similar packages.

#798 (comment)

Jon beat me to it 🙈

My comment stands. It's foolish to reimplement the validation we have in name and other places that let us avoid dealing with stringly typed data, and I don't see the risk of a dependency cycle for these superficial things because they should absolutely NOT need a full-blown registry implementation to test interfaces.

I could absolutely get behind a depcheck style test that this never depend on pkg/v1/remote in particular.

pkg/registry/blobs.go

imjasonh · 2021-11-04T21:56:39Z

pkg/registry/blobs.go

+				Message: err.Error(),
+			}
+		}
+		repo, err := name.NewRepository(req.URL.Host + path.Join(elem[1:len(elem)-2]...))


(Resurrecting this because I'm sad we couldn't make it happen)

Aside from the question of whether we need to depend on pkg/name here, I wonder if req.URL.Host is adding anything besides letting the string pass through NewRepository. Really all blob storers want is the subrepo path.

I hear the concern about duplicating validation logic though. Would it be reasonable to export a method from pkg/name to validate a subrepo path just so pkg/registry could use it?

I'm sad we couldn't make it happen

Me too, but alas I've unblocked myself (and it is glorious 😉 ).

I hear the concern about duplicating validation logic though

We have a bigger issue here. This repo is now apparently governed by unwritten rules in one person's head, which also have had no rationale articulated anywhere that was arrived at by consensus of the stakeholders in this library.

I've clearly demonstrated a willingness to send PRs here (and elsewhere) to fix things, move things forward, and I'm happy to engage in productive discussions about them, but waiting a week to get a Nack based on unwritten rules (that are already violated with what is checked in), is frankly both aggravating and unsustainable.

go-containerregistry/pkg/registry/registry.go

Lines 15 to 23 in a0c4bd2

// Package registry implements a docker V2 registry and the OCI distribution specification.

//

// It is designed to be used anywhere a low dependency container registry is needed, with an

// initial focus on tests.

//

// Its goal is to be standards compliant and its strictness will increase over time.

//

// This is currently a low flightmiles system. It's likely quite safe to use in tests; If you're using it

// in production, please let us know how and send us CL's for integration tests.

In order for this to disqualify this change, "low dependency" must be interpreted to exclude what it currently imports. 🙃

Also, I'd recommend a README: https://github.com/google/go-containerregistry#philosophy

We have a bigger issue here. This repo is now apparently governed by unwritten rules in one person's head, which also have had no rationale articulated anywhere that was arrived at by consensus of the stakeholders in this library.

Hoping to address this in #1166

Taking the issue of dependency philosophy aside, I think I'm still convinced that the dependency on pkg/name is unnecessary, or at least having a full name.Repository in the interface is. If not for validation of the repository path, you don't actually need a full name.Repository, and could just get by with Get(string path, h v1.Hash), and not add any new dependencies. Path validation itself is fairly simple, and could be moved to internal/something and shared by pkg/name and pkg/registry.

You are assuming details of the blob layer by stating that it can only care about path. Every single cloud vendor registry uses host to route storage requests, Amazon even gives each customer their own hostname per region. I doubt this is how you wrote kontain.me, but….

Hopefully you get my point.

Ah I see what you're saying. The same code depending on pkg/registry with some blob storage impl, running at foo.registry.example and bar.registry.example, might decide to store those blobs differently based on the host.

Can we document this expectation in the doc comments for BlobHandler? It wasn't immediately clear to me (because I'm a big dumb dummy) why the host was included, except to make path validation easier.

Can we document this expectation in the doc comments for BlobHandler

Absolutely.

imjasonh · 2021-11-12T14:33:22Z

pkg/registry/blobs.go

+
+	// Get returns true and a reader for consuming the blob specified with the hash,
+	// if it exists.  It now, it returns (nil, error).
+	Get(repo name.Repository, h v1.Hash) (io.ReadCloser, error)


Something occurred to me about this API, thinking through how I could use it with kontain.me -- this API would require kontain.me to proxy and serve blobs from GCS, instead of HTTP redirecting like I do today, which is a lot faster and cheaper than proxying through Cloud Run.

This might just mean that pkg/registry isn't going to be a good fit for kontain.me, and that's fine, but I wanted to surface that here since I think it's a pretty nice use case that isn't supported by this proposal.

mattmoor force-pushed the pluggable-blob-storage branch from 6767eef to c09136c Compare October 26, 2021 23:19

Make the blob storage in pkg/registry pluggable.

130a80c

With this change folks can use `registry.WithBlobHandler(...)` to substitute a custom blob handling layer.

mattmoor force-pushed the pluggable-blob-storage branch from c09136c to 130a80c Compare October 26, 2021 23:21

imjasonh reviewed Oct 27, 2021

View reviewed changes

jonjohnsonjr reviewed Nov 2, 2021

View reviewed changes

Have blob handler methods return errors

2a9295a

mattmoor force-pushed the pluggable-blob-storage branch from 5e9e18e to 2a9295a Compare November 2, 2021 18:08

mattmoor closed this Nov 2, 2021

imjasonh reviewed Nov 4, 2021

View reviewed changes

This was referenced Nov 5, 2021

Document philosophy and expectations of pkg/registry #1166

Open

Add pkg/registry/README.md documenting expectations #1167

Merged

joaopapereira mentioned this pull request Nov 8, 2021

Same registry different credentials carvel-dev/imgpkg#295

Merged

imjasonh reviewed Nov 12, 2021

View reviewed changes

imjasonh mentioned this pull request Dec 23, 2021

Pluggable registry storage for uploads #1216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the blob storage in pkg/registry pluggable. #1158

Make the blob storage in pkg/registry pluggable. #1158

mattmoor commented Oct 26, 2021

codecov-commenter commented Oct 26, 2021 •

edited

Loading

imjasonh left a comment

imjasonh Oct 27, 2021

mattmoor Nov 2, 2021

mattmoor commented Nov 2, 2021

mattmoor commented Nov 2, 2021

jonjohnsonjr Nov 2, 2021

mattmoor Nov 2, 2021

mattmoor Nov 2, 2021

jonjohnsonjr Nov 2, 2021

imjasonh Nov 2, 2021

mattmoor Nov 2, 2021

imjasonh Nov 4, 2021

mattmoor Nov 4, 2021

jonjohnsonjr Nov 4, 2021

mattmoor Nov 4, 2021

mattmoor Nov 4, 2021

imjasonh Nov 5, 2021

mattmoor Nov 5, 2021

imjasonh Nov 5, 2021

mattmoor Nov 5, 2021

imjasonh Nov 12, 2021

	v1 "github.com/google/go-containerregistry/pkg/v1"
	"github.com/google/go-containerregistry/pkg/v1/types"

	// Package registry implements a docker V2 registry and the OCI distribution specification.
	//
	// It is designed to be used anywhere a low dependency container registry is needed, with an
	// initial focus on tests.
	//
	// Its goal is to be standards compliant and its strictness will increase over time.
	//
	// This is currently a low flightmiles system. It's likely quite safe to use in tests; If you're using it
	// in production, please let us know how and send us CL's for integration tests.

Make the blob storage in pkg/registry pluggable. #1158

Make the blob storage in pkg/registry pluggable. #1158

Conversation

mattmoor commented Oct 26, 2021

codecov-commenter commented Oct 26, 2021 • edited Loading

Codecov Report

imjasonh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Nov 2, 2021

mattmoor commented Nov 2, 2021

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

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

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

codecov-commenter commented Oct 26, 2021 •

edited

Loading