allow to use context for getting signatures #316

mfojtik · 2017-07-28T20:50:09Z

@mtrmac @runcom since we plan to use the containers/image for downloading signatures from external location, we want to wrap every HTTP request to any location with context.Context that allows to set timeouts and deadlines for the request (so we won't open a request to a black hole, etc.)

i don't want to sweep all containers/image code and address all cases for now (but somebody should ;-) just plumb the code we currently need for openshift/origin#15494

@smarterclayton FYI

mtrmac · 2017-07-28T20:55:21Z

ACK, something like that needs to happen, but context.Context is weird to have in SystemContext, I’d prefer to do that sweep to changing the API later. I'll see what I can do.

You only care about ImageSource for now, is that correct?

mfojtik · 2017-07-28T20:56:57Z

@mtrmac yes, that is correct. I can add this as new argument for reference.NewImageSource but I didn't want to break the API.

mtrmac · 2017-07-28T21:00:48Z

(BTW if you are planning to carry this patch, there are more places that need to be patched: makeRequestToResolvedURL and getBearerToken.)

mtrmac · 2017-07-28T21:08:08Z

I can add this as new argument for reference.NewImageSource but I didn't want to break the API.

https://golang.org/pkg/context/ says

Do not store Contexts inside a struct type; instead, pass a Context explicitly to each function that needs it.

so this would be an even bigger API break.

Oh well, we can do it, or perhaps add …WithContext methods to the interface — but I guess I’d prefer the API break for now; we don’t have a documented ABI commitment (@runcom WDYT?).

(For reference, previous stab at this: #43 )

runcom · 2017-07-29T08:03:32Z

Right we don't have an API commitment yet so I'm fine adding CTX to the first argument of interfaces so we'll be more idiomatic. I had started doing this somewhere in another PR also but not sure where it went.

I'm +1 here

mfojtik · 2017-07-31T09:56:05Z

i'm fine with adding .WithContext() into interfaces

mfojtik · 2017-07-31T09:56:32Z

(for now we only care about signatures, so I can plumb that, leaving you guys with the rest ;-)

mfojtik · 2017-07-31T10:46:50Z

@mtrmac @runcom my last commit add context.Context to the codepath we need in origin to get the autoimport of signatures done. I left bunch of TODO's for methods that have to be plumbed.

PTAL

runcom

Seems ok but I want to explicitly use context.TODO or context.Background when you pass nil as context

runcom · 2017-07-31T10:49:47Z

directory/directory_test.go

 	require.NoError(t, err)
 	defer src.Close()
-	sigs, err := src.GetSignatures()
+	sigs, err := src.GetSignatures(nil)


I believe we want to pass context.TODO here even if this is a test (or context.Background better)

runcom · 2017-07-31T10:50:27Z

docker/docker_client.go

 // The host name and schema is taken from the client or autodetected, and the path is relative to it, i.e. the path usually starts with /v2/.
-func (c *dockerClient) makeRequest(method, path string, headers map[string][]string, stream io.Reader) (*http.Response, error) {
+func (c *dockerClient) makeRequest(ctx context.Context, method, path string, headers map[string][]string, stream io.Reader) (*http.Response, error) {
 	if err := c.detectProperties(); err != nil {


This function also needs ctx

runcom · 2017-07-31T10:51:48Z

docker/docker_client.go

 	if err != nil {
 		return nil, err
 	}
+	if ctx != nil {


We should make sure context is either todo or background here so I'll drop this if.
req.WithContext can accept nil?

req.WithContext can't accept nil unfortunately but context.Background seems ok here?

Yes please :)

Why should this accept nil? nil is an invalid Context value, and callers who pass it should just crash. This PR is already touching all callers, so it can ensure that nil is never used.

Or am I missing anything?

runcom · 2017-07-31T10:52:28Z

docker/docker_client.go

 		url := fmt.Sprintf(resolvedPingV2URL, scheme, c.registry)
-		resp, err := c.makeRequestToResolvedURL("GET", url, nil, nil, -1, true)
+		// FIXME: Pass the context.Context
+		resp, err := c.makeRequestToResolvedURL(nil, "GET", url, nil, nil, -1, true)


Use context.TODO or Background

runcom · 2017-07-31T10:52:50Z

docker/docker_image.go

 	path := fmt.Sprintf(tagsPath, reference.Path(i.src.ref.ref))
-	res, err := i.src.c.makeRequest("GET", path, nil, nil)
+	// FIXME: Pass the context.Context
+	res, err := i.src.c.makeRequest(nil, "GET", path, nil, nil)


TODO or Background

mfojtik · 2017-07-31T11:20:17Z

@runcom all fixed :)

mfojtik · 2017-07-31T14:14:51Z

@mtrmac @runcom why are the travis tests broken? ;-) doesn't seems this broken them

runcom · 2017-07-31T14:16:55Z

@mfojtik you need to sign your commits (git commit -s)

mfojtik · 2017-07-31T14:17:34Z

@runcom i'm sure they are signed :-) (github shows "verified") i guess you checking the commit message?

runcom · 2017-07-31T14:18:36Z

@mfojtik Verified means GPG signed, you need "Signed-by"

mfojtik · 2017-07-31T14:47:06Z

@runcom the bot still does not like me :/

runcom · 2017-07-31T14:48:29Z

seems like everything's ok now

runcom · 2017-07-31T14:48:43Z

you just need to rebase this I guess

mfojtik · 2017-07-31T14:49:14Z

@runcom already did :)

mfojtik · 2017-07-31T14:50:45Z

ok weird... i must have the link to travis cached... all green now. guess this is ready for shipping?

runcom · 2017-07-31T14:55:24Z

LGTM but we also need @mtrmac review :)

mtrmac · 2017-07-31T19:07:46Z

storage/storage_image.go

-func (s *storageImageSource) GetSignatures() (signatures [][]byte, err error) {
+func (s *storageImageSource) GetSignatures(ctx context.Context) (signatures [][]byte, err error) {
 	var offset int
+	// FIXME: pass the context.Context


containers/storage is generally dealing with local files, ignoring the ctx here should be fine.

mtrmac · 2017-07-31T19:09:34Z

openshift/openshift.go

 }

-func (s *openshiftImageSource) GetSignatures() ([][]byte, error) {
+func (s *openshiftImageSource) GetSignatures(ctx context.Context) ([][]byte, error) {


Could this one be implemented now?

I’m perfectly fine with having context support missing in most of the API, but when it exists, it would be better for users if it reliably worked.

I think containers/image should switch to use the client-go from openshift as external dependency once we cut that out. In that case the context will be available.
This change was just to satisfy the interface.

In the mean time we would still have unreliable API, that’s not great. I’ll take a stab at this.

(As for using the OpenShift client, that would be great (I extracting those thousand lines in openshift-copies.go was annoying and by now it’s probably pretty behind the original, and I do hate the duplication), but the two times this was attempted, it added 60? MB to the size of skopeo, which is just too much.)

@mfojtik added in https://github.com/mtrmac/image/tree/add-context-openshift , can you add that into this PR?

(Disclaimer: I’m still on Go 1.6, so this is untested. I’ll have to update ~immediately, I know.)

added your commit.

mtrmac · 2017-07-31T19:11:51Z

docker/docker_client.go

 	if err != nil {
 		return nil, err
 	}
+	if ctx != nil {


Why should this accept nil? nil is an invalid Context value, and callers who pass it should just crash. This PR is already touching all callers, so it can ensure that nil is never used.

Or am I missing anything?

mtrmac · 2017-07-31T19:13:21Z

docker/docker_client.go

 		url := fmt.Sprintf(resolvedPingV2URL, scheme, c.registry)
-		resp, err := c.makeRequestToResolvedURL("GET", url, nil, nil, -1, true)
+		// FIXME: Pass the context.Context
+		resp, err := c.makeRequestToResolvedURL(ctx, "GET", url, nil, nil, -1, true)


The FIXME seems to already have been resolved.

mtrmac · 2017-07-31T19:15:22Z

docker/docker_client.go

+		req = req.WithContext(ctx)
+	} else {
+		req = req.WithContext(context.Background())
+	}


Below this, setupRequestAuth and getBearerToken also need Context support.

mtrmac · 2017-07-31T19:30:41Z

docker/docker_client.go

 			url := fmt.Sprintf(resolvedPingV1URL, scheme, c.registry)
-			resp, err := c.makeRequestToResolvedURL("GET", url, nil, nil, -1, true)
+			// FIXME: Pass the context.Context
+			resp, err := c.makeRequestToResolvedURL(ctx, "GET", url, nil, nil, -1, true)


The FIXME seems to already have been resolved.

mtrmac · 2017-07-31T19:31:44Z

docker/docker_image_dest.go

 	uploadPath := fmt.Sprintf(blobUploadPath, reference.Path(d.ref.ref))
 	logrus.Debugf("Uploading %s", uploadPath)
-	res, err := d.c.makeRequest("POST", uploadPath, nil, nil)
+	res, err := d.c.makeRequest(context.Background(), "POST", uploadPath, nil, nil)


All of the .Background() in this file should be .TODO() AFAICT.

mtrmac · 2017-07-31T19:31:58Z

docker/docker_image_dest.go

 	// but the X-Registry-Supports-Signatures API extension does not support that yet.

-	existingSignatures, err := d.c.getExtensionsSignatures(d.ref, d.manifestDigest)
+	existingSignatures, err := d.c.getExtensionsSignatures(nil, d.ref, d.manifestDigest)


context.TODO(), not nil, please.

mtrmac · 2017-07-31T19:33:05Z

docker/docker_image_src.go

 // which is not nil.
-func (s *dockerImageSource) getSignaturesFromLookaside() ([][]byte, error) {
+func (s *dockerImageSource) getSignaturesFromLookaside(ctx context.Context) ([][]byte, error) {
 	if err := s.ensureManifestIsLoaded(); err != nil {


ensureManifestIsLoaded also needs to handle ctx.

mtrmac · 2017-07-31T19:33:47Z

docker/docker_image_src.go

+		if err != nil {
+			return nil, false, err
+		}
+		if ctx != nil {


Drop this conditional please; ctx == nil is invalid.

mfojtik · 2017-08-01T09:01:20Z

@mtrmac all comments addressed

TomSweeneyRedHat · 2017-08-01T12:52:47Z

@mfojtik looks like a rebase is needed.

mtrmac · 2017-08-01T14:36:39Z

docker/docker_image_src.go

 	)
 	for _, url := range urls {
-		resp, err = s.c.makeRequestToResolvedURL("GET", url, nil, nil, -1, false)
+		resp, err = s.c.makeRequestToResolvedURL(context.Background(), "GET", url, nil, nil, -1, false)


context.TODO(), not .Background(), everywhere in this file

mtrmac · 2017-08-01T14:40:34Z

openshift/openshift.go

 }

-func (s *openshiftImageSource) GetSignatures() ([][]byte, error) {
+func (s *openshiftImageSource) GetSignatures(ctx context.Context) ([][]byte, error) {


In the mean time we would still have unreliable API, that’s not great. I’ll take a stab at this.

(As for using the OpenShift client, that would be great (I extracting those thousand lines in openshift-copies.go was annoying and by now it’s probably pretty behind the original, and I do hate the duplication), but the two times this was attempted, it added 60? MB to the size of skopeo, which is just too much.)

mfojtik · 2017-08-02T10:15:41Z

@mtrmac updated, PTAL

TomSweeneyRedHat · 2017-08-02T12:28:34Z

LGTM fwiw.

mtrmac · 2017-08-02T15:00:50Z

👍

@mfojtik Thanks; one last update after #320 , please.

Signed-off-by: Michal Fojtik <[email protected]>

Signed-off-by: Miloslav Trmač <[email protected]>

mfojtik · 2017-08-02T19:17:04Z

@mtrmac updated, lets wait for tests to pass

mfojtik · 2017-08-03T08:10:32Z

@runcom guess this one is now good for shipping

mfojtik mentioned this pull request Jul 28, 2017

image: automatically import images signatures openshift/origin#15494

Closed

mfojtik force-pushed the add-context branch 2 times, most recently from 8316870 to f6b7b85 Compare July 31, 2017 10:45

runcom reviewed Jul 31, 2017

View reviewed changes

mfojtik force-pushed the add-context branch 3 times, most recently from d054424 to 61bb862 Compare July 31, 2017 11:19

mfojtik force-pushed the add-context branch from 61bb862 to 023b4a7 Compare July 31, 2017 14:26

mtrmac reviewed Jul 31, 2017

View reviewed changes

mfojtik force-pushed the add-context branch from 023b4a7 to fd07e7b Compare August 1, 2017 09:00

mtrmac reviewed Aug 1, 2017

View reviewed changes

mfojtik force-pushed the add-context branch 2 times, most recently from 00973d5 to 8305c27 Compare August 2, 2017 10:15

mfojtik and others added 2 commits August 2, 2017 21:16

pass context.Context to signature retrieval http calls

1ef1350

Signed-off-by: Michal Fojtik <[email protected]>

Implement context.Context in openshiftImageSource.GetSignatures

deb05ba

Signed-off-by: Miloslav Trmač <[email protected]>

mfojtik force-pushed the add-context branch from 8305c27 to deb05ba Compare August 2, 2017 19:16

runcom merged commit dbd0a4c into containers:master Aug 3, 2017

allow to use context for getting signatures #316

allow to use context for getting signatures #316

Uh oh!

Conversation

mfojtik commented Jul 28, 2017

Uh oh!

mtrmac commented Jul 28, 2017

Uh oh!

mfojtik commented Jul 28, 2017

Uh oh!

mtrmac commented Jul 28, 2017

Uh oh!

mtrmac commented Jul 28, 2017

Uh oh!

runcom commented Jul 29, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

runcom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

runcom commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

runcom commented Jul 31, 2017

Uh oh!

runcom commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

mfojtik commented Jul 31, 2017

Uh oh!

runcom commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mfojtik Jul 31, 2017 •

edited

Loading

mfojtik commented Jul 31, 2017 •

edited

Loading

runcom commented Jul 31, 2017 •

edited

Loading

mtrmac Jul 31, 2017 •

edited

Loading

mtrmac commented Aug 2, 2017 •

edited by runcom

Loading