Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import #83

ricardomaraschini · 2020-03-13T10:40:39Z

With this PR we start to use node's pull secrets during image stream import process as described on the enhancement proposal. We prioritize namespace secrets over node pull secrets.

Note: Please review openshift/cluster-openshift-apiserver-operator#284 as well.

ricardomaraschini · 2020-03-13T10:49:39Z

/retest

ricardomaraschini · 2020-03-13T11:34:57Z

/test verify

ricardomaraschini · 2020-03-13T11:49:48Z

/test unit

ricardomaraschini · 2020-03-13T11:56:09Z

/test e2e-aws

ricardomaraschini · 2020-03-13T13:34:25Z

/retest

ricardomaraschini · 2020-03-13T13:42:19Z

/test images

ricardomaraschini · 2020-03-13T13:50:30Z

/test e2e-aws-serial

ricardomaraschini · 2020-03-14T12:54:59Z

/retest

ricardomaraschini · 2020-03-14T14:50:17Z

/retest

ricardomaraschini · 2020-03-14T17:08:52Z

/test e2e-aws

ricardomaraschini · 2020-03-14T20:49:15Z

/retest

ricardomaraschini · 2020-03-15T12:44:32Z

/retest

ricardomaraschini · 2020-03-15T15:18:27Z

/test e2e-aws

ricardomaraschini · 2020-03-15T18:02:19Z

/test e2e-aws

ricardomaraschini · 2020-03-15T20:55:33Z

/assign @dmage @adambkaplan @mfojtik

ricardomaraschini · 2020-03-18T15:02:20Z

/assign @sttts

sttts · 2020-03-19T08:25:42Z

pkg/image/apiserver/registry/imagestreamimport/rest.go

@@ -258,24 +257,6 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		}
 	}

-	// if we encountered an error loading credentials and any images could not be retrieved with an access


this code was there to get proper error messages from the API. How is this done now?

This is part of a behavioral change. Now if we can't load the secrets we return an API error to the user and don't create any image streams. With that out of the way(error retrieving secrets) future StatusReasonUnauthorized and StatusReasonForbidden would be correct so no need to overwrite them by Unable to load secrets for this image error.

Let me know what you reckon and I will adjust appropriately.

This commit vendors github.com/docker/docker/registry as we are going to start to leverage type registry.StaticCredentialStore when creating a registryclient.Context.

sttts · 2020-03-19T12:16:29Z

pkg/image/apiserver/importer/importer_test.go

 	imageapi "github.com/openshift/openshift-apiserver/pkg/image/apis/image"
 	dockerregistry "github.com/openshift/openshift-apiserver/pkg/image/apiserver/importer/dockerv1client"
 	"github.com/openshift/openshift-apiserver/pkg/image/apiserver/sysregistriesv2"
 )

+func init() {
+	runtime.Must(userapi.Install(legacyscheme.Scheme))


why is the legacy scheme used? Installing anything into that is suspicious. Just create a new scheme object and install into it what you need.

Functions[1] such as schema1ToImage or schema2ToImage on this package rely on the legacyscheme, not sure what the options were when they got developed. I don't mind at all fixing this to use a new scheme but it goes over the scope of this feature and I would address it as technical debt. If you are OK with leaving this as is for now I will create a new front to address this issue. You may even see TODO comments[2] talking about the legacyscheme.

[1] https://github.com/openshift/openshift-apiserver/blob/master/pkg/image/apiserver/importer/image.go#L88
[2] https://github.com/openshift/openshift-apiserver/blob/master/pkg/image/apiserver/importer/dockerv1client/client.go#L34

ok, fine for me as a follow-up.

dmage · 2020-03-19T12:33:45Z

pkg/image/apiserver/importer/staticcredentials.go

+		})
+	}
+
+	importCtx := registryclient.NewContext(


By recreating Context all the time, you are losing all caching features. Essentially you'll make a lot of ping requests to the /v2/ endpoint for every tag in ImageStreamImport.

I can lay down a cache layer here and reuse them, what do you think? Ideally I would add this logic on library-go but unfortunately i can't as it includes k8s keyrings and I am not supposed to add these dependencies there.

Could you PTAL ? I have added a code to reuse contexts.

getting credentials using ref and then caching them by regurl definitely needs to be covered by tests. It can cause authentication problems.

/hold

I am investigating what is the best way of achieving this.

@dmage I have updated the indexing to use the repository name instead of the URL. Could you PTAL?

ricardomaraschini · 2020-03-19T19:25:35Z

/retest

dmage · 2020-03-20T16:47:28Z

pkg/image/apiserver/importer/importer.go

@@ -273,7 +273,7 @@ func (i *ImageStreamImporter) importFromRepository(ctx gocontext.Context, retrie

 	key := repositoryKey{url: *registryURL, name: repoName}
 	repo := &importRepository{
-		Ref:         ref,
+		Ref:         defaultRef,


what is implication of this change?

As far as I checked the only side effect is some error strings, see here for an example.

What changes is that:

If no registry is present, docker.io is used.

If no Namespace is set on the registry being imported and the repository is docker hub, library is used on the repository URL.

If no tag is set, latest is used.

Do you see any problem with this approach? Let me know if you have an use case where this can cause issues and I will make sure it works.

If it's for error strings, then it should contain the name as it was entered by the user

Yes, you are right. I have reverted this change and now the error string is the same as it used to be. Please let me know if it is OK for you.

dmage · 2020-03-20T17:07:43Z

pkg/image/apiserver/importer/staticcredentials.go

+		})
+	}
+
+	importCtx := registryclient.NewContext(


getting credentials using ref and then caching them by regurl definitely needs to be covered by tests. It can cause authentication problems.

dmage · 2020-03-23T13:50:55Z

Looks good, we'll need to create BZs to track follow-up tech debt addressing.

/lgtm

ricardomaraschini · 2020-03-23T15:05:38Z

Looks good, we'll need to create BZs to track follow-up tech debt addressing.

/lgtm

I have already opened a JIRA card for the tech debt, thanks for the heads-up.

Starting to merge node pull secrets with kubernetes docker secrets during image stream import. Implementation details may be found at: openshift/enhancements/node-pull-credentials/pull-credentials.md

ricardomaraschini · 2020-03-24T13:35:25Z

/retest

dmage · 2020-03-25T09:40:02Z

/retitle Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import

openshift-ci-robot · 2020-03-25T09:40:28Z

@ricardomaraschini: This pull request references Bugzilla bug 1816534, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

dmage · 2020-03-25T09:42:10Z

/lgtm

ricardomaraschini · 2020-03-29T10:53:07Z

/unhold

dmage · 2020-04-07T15:34:20Z

@mfojtik @sttts please take a look

sttts · 2020-04-09T12:48:38Z

/approve

openshift-ci-robot · 2020-04-09T12:49:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dmage, ricardomaraschini, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-04-09T14:40:41Z

@ricardomaraschini: All pull requests linked via external trackers have merged: openshift/openshift-apiserver#83. Bugzilla bug 1816534 has been moved to the MODIFIED state.

In response to this:

Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 13, 2020

openshift-ci-robot requested review from bparees and mfojtik March 13, 2020 10:41

ricardomaraschini changed the title ~~WIP: Levaraging node credentials~~ WIP: Leveraging node credentials Mar 13, 2020

ricardomaraschini mentioned this pull request Mar 15, 2020

DEVEXP-424: Leveraging node pull secrets during image stream import #64

Closed

ricardomaraschini changed the title ~~WIP: Leveraging node credentials~~ DEVEXP-424: Leveraging node credentials Mar 15, 2020

ricardomaraschini changed the title ~~DEVEXP-424: Leveraging node credentials~~ DEVEXP-424: Leveraging node credentials during image stream import. Mar 15, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2020

openshift-ci-robot assigned adambkaplan, dmage and mfojtik Mar 15, 2020

gabemontero mentioned this pull request Mar 18, 2020

remove pull secret copy, defer to image registry openshift/cluster-samples-operator#249

Merged

openshift-ci-robot assigned sttts Mar 18, 2020

sttts reviewed Mar 19, 2020

View reviewed changes

Vendoring github.com/docker/docker/registry

f876c4b

This commit vendors github.com/docker/docker/registry as we are going to start to leverage type registry.StaticCredentialStore when creating a registryclient.Context.

sttts reviewed Mar 19, 2020

View reviewed changes

dmage reviewed Mar 19, 2020

View reviewed changes

ricardomaraschini changed the title ~~DEVEXP-424: Leveraging node credentials during image stream import.~~ DEVEXP-424: Leveraging node credentials during image stream import Mar 19, 2020

dmage reviewed Mar 20, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 23, 2020

Leveraging node pull secrets

b1de89b

Starting to merge node pull secrets with kubernetes docker secrets during image stream import. Implementation details may be found at: openshift/enhancements/node-pull-credentials/pull-credentials.md

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2020

openshift-ci-robot changed the title ~~DEVEXP-424: Leveraging node credentials during image stream import~~ Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import Mar 25, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Mar 25, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 25, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 29, 2020

ricardomaraschini mentioned this pull request Mar 30, 2020

Can't pull from registry.redhat.io in builder pod openshift/builder#145

Closed

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2020

openshift-merge-robot merged commit c41ece2 into openshift:master Apr 9, 2020

csrwng mentioned this pull request Sep 21, 2020

Mount pull secret in openshift api-server pod openshift/ibm-roks-toolkit#63

Closed

Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import #83

Bug 1816534: DEVEXP-424: Leveraging node credentials during image stream import #83

Conversation

ricardomaraschini commented Mar 13, 2020 • edited Loading

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 13, 2020

ricardomaraschini commented Mar 14, 2020

ricardomaraschini commented Mar 14, 2020

ricardomaraschini commented Mar 14, 2020

ricardomaraschini commented Mar 14, 2020

ricardomaraschini commented Mar 15, 2020

ricardomaraschini commented Mar 15, 2020

ricardomaraschini commented Mar 15, 2020

ricardomaraschini commented Mar 15, 2020

ricardomaraschini commented Mar 18, 2020

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

ricardomaraschini Mar 20, 2020 • edited Loading

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

ricardomaraschini commented Mar 19, 2020

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

dmage commented Mar 23, 2020

ricardomaraschini commented Mar 23, 2020

ricardomaraschini commented Mar 24, 2020

dmage commented Mar 25, 2020

openshift-ci-robot commented Mar 25, 2020

dmage commented Mar 25, 2020

ricardomaraschini commented Mar 29, 2020

dmage commented Apr 7, 2020

sttts commented Apr 9, 2020

openshift-ci-robot commented Apr 9, 2020

openshift-ci-robot commented Apr 9, 2020

ricardomaraschini commented Mar 13, 2020 •

edited

Loading

ricardomaraschini Mar 20, 2020 •

edited

Loading