docker: export functions and variables for a distribution-spec cert tests

[dongsupark]

Export the following functions and variables to be used by tests for certification of the distribution-spec.

• DockerReference
• MakeRequest
• NewDockerClientFromRef
• SendAuth, V2Auth, NoAuth
• TagOrDigest

Does it make sense? If not, please let me know.
See also my work-in-progress pull request: kinvolk-archives/ocicert#1

[runcom]

makes sense to me, where we could learn more about the certification of distribution-spec?

[dongsupark]

@runcom Recent discussions: opencontainers/distribution-spec#24

[runcom]

alright, it does makes sense to me. I just want to avoid exposing the low level of the docker client we use. Otherwise it's like using any other http client like curl. In case of docker/distribution, how are you dealing with that since they don't expose a raw client like us?

[dongsupark]

@runcom Ok, I see.
To avoid exposing the internal docker client, how about doing like this:

1. Define a new wrapper function and a context struct in docker/docker_client.go

type SendRequestContext struct {
	Ref     types.ImageReference
	Method  string
	ReqPath string
	Headers map[string][]string
	Stream  io.Reader
	Auth    SendAuth
}

func ClientSendRequest(ctx context.Context, sys *types.SystemContext, reqCtx *SendRequestContext) (*http.Response, error) {
	dr, ok := reqCtx.Ref.(DockerReference)
	if !ok {
		return nil, fmt.Errorf("cannot get docker reference from %v", reqCtx.Ref)
	}

	client, err := newDockerClientFromRef(sys, dr, false, "pull")
	if err != nil {
		return nil, fmt.Errorf("%v", err)
	}

	res, err := client.MakeRequest(ctx, reqCtx.Method, reqCtx.ReqPath, reqCtx.Headers, reqCtx.Stream, reqCtx.Auth)
	if err != nil {
		return nil, fmt.Errorf("%v", err)
	}
	defer res.Body.Close()

	return res, nil
}

2. Then a caller would call it like this:

reqPath := filepath.Join("/v2", reference.Path(dr.DockerReference()), "manifests", refTail)

res, err := docker.ClientSendRequest(ctx, sys,
	&docker.SendRequestContext{
		Ref:     img.Reference(),
		Method:  "GET",
		ReqPath: reqPath,
		Headers: nil,
		Stream:  nil,
		Auth:    docker.V2Auth,
	})

Though some functions and variables like DockerReference, V2Auth still need to be exposed.
Would it be better than the current one?

[mtrmac]

Thanks for the PR.

I am a bit unclear on what exactly the tests are supposed to verify, and depending on that the desired API changes significantly; and the linked discussion does not really say.

• If the tests are intended to ensure that the tested registry implementation is interoperable with this containers/image library, as it is used by real-world callers, the tests should just use the public API (e.g., modulo error handling, containers/image.docker.NewReference().NewImageSource().GetManifest() as is; not the currently-private makeRequest that no real-world caller uses).
• If the tests are intended to ensure that a registry implementation conforms exactly to the HTTP API spec as written, then the tests should probably work directly at the HTTP API level; containers/image does, in general and where practical, accommodate / work around non-conforming server implementations (e.g. note how we don’t DELETE an aborted upload object, because that requires additional permissions with docker/distribution, or how deleteImage uses a not-correct-in-principle-but-working-in-practice action * instead of the probably right delete. And if further incompatibilities that are practical to work around arise in the future, containers/image could be extended to work around them as well, making some test results invalid.

If you want to test interoperability with c/image, that is very welcome, but just use the public API as it would be called in practice, please.

(It is quite reasonable to contemplate that the API should be extended in some way, e.g. to make error values easier to semantically process in callers so that they can, just for illustration, reliably distinguish e.g. a missing image from a network timeout error. Quite likely that could be useful for the test suite as well. But this would not go as far as exposing the whole HTTP communication so that a HTTP-level test suite could verify handling of corner cases, because real-world callers would not need that, and again, any error handling code would include workarounds for interoperability; a particular known pain point is that different registries report “image not found” in somewhat inconsistent ways, and c/image would probably prefer to report all of them as a single kind of Go error value so that the callers don’t have to care.)

Or, maybe — it somewhat looks as if what you really wanted was raw HTTP-level access for testing the behavior, but you’d like containers/image to handle authentication for you, so that each individual test can focus on its specific part of the API — and that you don’t care for the whole ImageReference abstraction, or for automatically looking up credentials and TLS configuration in /etc and elsewhere, because they make the test suite change behavior depending on where it’s run? That would be a fairly different conversation (more or less, expose setupRequestAuth).

[mtrmac]

That would be a fairly different conversation (more or less, expose setupRequestAuth).

… and while I can clearly see how that could allow code reuse, it would not quite be at the level of “using containers/image to test the registry”, because it would be using only a very small part of it.

Or, maybe (wildly speculating here), c/image/docker could get a new boolean in types.SystemContext.DockerNoWorkarounds that would be enabled in the test suite and disabled everywhere else? That might be practical to maintain.

Overall, though, opencontainers/distribution-spec#24 really needs to decide whether they want to test whether real-world interoperability with c/image, or strict spec conformance (or, sure, both independently).

[dongsupark]

@mtrmac

What I want to do is definitely the 2nd one, ensure that a registry implementation conforms exactly to the HTTP API spec, not the 1st one, test interoperability with c/image.

If the tests are intended to ensure that a registry implementation conforms exactly to the HTTP API spec as written, then the tests should probably work directly at the HTTP API level; containers/image does, in general and where practical, accommodate / work around non-conforming server implementations

Hmm, I thought that the containers/image would always work in a conformant way. Good to know.
Then maybe it would be better to directly communicate to the HTTP layer.

Or, maybe — it somewhat looks as if what you really wanted was raw HTTP-level access for testing the behavior, but you’d like containers/image to handle authentication for you, so that each individual test can focus on its specific part of the API — and that you don’t care for the whole ImageReference abstraction, or for automatically looking up credentials and TLS configuration in /etc and elsewhere, because they make the test suite change behavior depending on where it’s run? That would be a fairly different conversation (more or less, expose setupRequestAuth).

That's not exactly what I wanted to do. But that sounds like an option. If you say that's better, then I can do so.

Or, maybe (wildly speculating here), c/image/docker could get a new boolean in types.SystemContext.DockerNoWorkarounds that would be enabled in the test suite and disabled everywhere else? That might be practical to maintain.

That sounds like a good idea.

I'm going to write a comment in opencontainers/distribution-spec#24 as well.

[mtrmac]

That would be a fairly different conversation (more or less, expose setupRequestAuth).

Hum, that may not be quite what you want either, because setupRequestAuth does not use the scopes from the challenge, but client-provided ones:

image/docker/docker_client.go

Lines 416 to 423 in 4b6d9cf

	// we're using the challenges from the /v2/ ping response and not the one from the destination
	// URL in this request because:
	//
	// 1) docker does that as well
	// 2) gcr.io is sending 401 without a WWW-Authenticate header in the real request
	//
	// debugging: https://github.com/containers/image/pull/211#issuecomment-273426236 and follows up
	func (c *dockerClient) setupRequestAuth(req *http.Request) error {

So…

Or, maybe (wildly speculating here), c/image/docker could get a new boolean in types.SystemContext.DockerNoWorkarounds that would be enabled in the test suite and disabled everywhere else? That might be practical to maintain.

That sounds like a good idea.

… DockerNoWorkarounds, while possible in principle, is not trivial to pull off because it’s not immediately obvious which parts of the code are conformant and which parts are a workaround.

Sure, in this case there is a big comment, but your WIP work, as well as the SendRequestContext proposal above, has already accepted the compatibility-workaround API paradigm without noticing that it is a non-compliant workaround.

It can definitely be done but it will require extremely careful line-by-line review of the code vs. the spec.

[runcom]

please all take a look at my concerns here as well opencontainers/distribution-spec#24 (comment)

[dongsupark]

@mtrmac

That would be a fairly different conversation (more or less, expose setupRequestAuth).
Hum, that may not be quite what you want either, because setupRequestAuth does not use the scopes from the challenge, but client-provided ones:

image/docker/docker_client.go

Lines 416 to 423 in 4b6d9cf

	// we're using the challenges from the /v2/ ping response and not the one from the destination
	// URL in this request because:
	//
	// 1) docker does that as well
	// 2) gcr.io is sending 401 without a WWW-Authenticate header in the real request
	//
	// debugging: https://github.com/containers/image/pull/211#issuecomment-273426236 and follows up
	func (c *dockerClient) setupRequestAuth(req *http.Request) error {

Hmm, Interesting. The scope provided by client is used.
Looking into #211, I suppose it was done for supporting docker and gcr.
I wanted to use setupRequestAuth to avoid writing duplicated lots of auth code on my own. I wanted to do so even if the actual communication was done on HTTP layer.
But in this case, I'm not sure any more how it can be done.

… DockerNoWorkarounds, while possible in principle, is not trivial to pull off because it’s not immediately obvious which parts of the code are conformant and which parts are a workaround.

Thanks. Let me think.

[rhatdan]

@dongsupark Needs a rebase. Is this still something you are working on?

[dongsupark]

No, I'm not working on it any more. I came up with a pure HTTP-based approach.
Will close. Thanks!