IR-21: Leveraging image config for "insecure" registries during ImageStreamImport #111

ricardomaraschini · 2020-06-02T07:35:46Z

By leveraging images.config.openshift.io/cluster "insecure" registry configuration we now can flag a registry as insecure for all image stream imports. Without this patch users had to set the registry as "insecure" on the ImageStreamImport request, regardless of what was defined on images.config.openshift.io/cluster.

I have chosen to include here some other housekeeping changes:

Standardized the receiver name for "ImageStreamImporter" struct (linter warning)
Migrating from golang.org/x/net/context to context
Removed unnecessary function parameters (using struct "properties" instead)
Vendoring sysregistriesv2 so we can remove a own copy of this package we had laying on our repository

All four changes above were constricted into their own commits (plus commits where we vendor stuff) in an attempt to make the review easier.

Tests for this feature were implemented through openshift/origin#25058

ricardomaraschini · 2020-06-03T12:32:03Z

/test e2e-aws

ricardomaraschini · 2020-06-03T14:56:24Z

/test e2e-aws

ricardomaraschini · 2020-06-03T16:27:59Z

/test e2e-aws-upgrade

ricardomaraschini · 2020-06-04T08:00:13Z

/test e2e-aws

ricardomaraschini · 2020-06-04T10:00:35Z

/retest

ricardomaraschini · 2020-06-04T10:09:36Z

/assign @dmage @mfojtik

dmage · 2020-06-04T12:22:18Z

pkg/image/apiserver/sysregistriesv2/system_registries_v2.go

@@ -116,6 +116,39 @@ func (config *V2RegistriesConf) Nonempty() bool {
 		len(config.UnqualifiedSearchRegistries) != 0)
 }

+// Insecure returns if access to registry at location is flagged as insecure, i.e. invalid


This implementation doesn't match the registries.conf documentation.

Please keep it in sync with https://github.com/containers/image/blob/master/pkg/sysregistriesv2/system_registries_v2.go

You may want to import this package directly, AFAIK they fixed their "types" package and it shouldn't have 100500 dependencies anymore.

I did not know this package was copied from somewhere else. I have updated this PR to vendor the package and remove the copy&pasta we had.

ricardomaraschini · 2020-06-04T13:21:57Z

/assign @dmage

ricardomaraschini · 2020-06-04T13:22:12Z

/assign @mfojtik

dmage · 2020-06-15T12:37:36Z

/lgtm
/unassign
/assign @mfojtik

dmage · 2020-06-30T12:27:46Z

/assign @sttts

sttts · 2020-06-30T13:17:00Z

pkg/image/apiserver/importer/importer.go

@@ -81,38 +89,68 @@ func NewImageStreamImporter(retriever RepositoryRetriever, regConf *sysregistrie
 		limiter:   limiter,
 		regConf:   regConf,

-		digestToRepositoryCache: make(map[gocontext.Context]map[manifestKey]*imageapi.Image),
+		digestToRepositoryCache: make(map[context.Context]map[manifestKey]*imageapi.Image),


that's interesting type. Which context ends up here? The one of the request?

I had the same impression when I came across this construction for the first time, seems rather interesting. I did some digging and I discovered that this is entangled with the client request's context. The next obvious question I had was: "what?!". I could not find where this was freed so I thought this would grow indefinitely, a real memory hoarder eating everything on its way. Well, this is not the case because the object holding it is created in a per request basis, so the next obvious question was "why?", if it is based on a per request basis why to index per context? My guess is that this object was planned to handle multiple requests at the same time but never got finished.

@dmage Am I missing something here? I think we could have a task to check(fix?) this as tech-debt, what do you think?

while we touch this, can you add a comment? This has big code smell.

I created a tech-debt card: https://issues.redhat.com/browse/IR-116

sttts · 2020-06-30T13:18:02Z

pkg/image/apiserver/importer/importer_test.go

@@ -11,7 +12,7 @@ import (
 	"testing"
 	"time"

-	"golang.org/x/net/context"
+	"github.com/containers/image/pkg/sysregistriesv2"



nit: remove empty line

sttts · 2020-06-30T13:19:49Z

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

@@ -223,7 +237,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 		r.transport, r.insecureTransport, secretsList.Items,
 	)
 	imports := r.importFn(importCtx, v2regConf)
-	if err := imports.Import(ctx.(gocontext.Context), isi, stream); err != nil {
+	if err := imports.Import(ctx.(context.Context), isi, stream); err != nil {


why this cast?

Nice catch, removed it.

sttts · 2020-06-30T13:20:00Z

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

@@ -246,7 +260,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation
 			r.transport, r.insecureTransport, nil,
 		)
 		imports := r.importFn(importCtx, v2regConf)
-		if err := imports.Import(ctx.(gocontext.Context), isi, stream); err != nil {
+		if err := imports.Import(ctx.(context.Context), isi, stream); err != nil {


why this cast?

Nice catch, removed it.

ricardomaraschini · 2020-07-01T10:39:09Z

/retest

ricardomaraschini · 2020-07-06T14:17:16Z

/retest

openshift-ci-robot · 2020-07-21T11:35:02Z

@dmage: dmage unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file.

In response to this:

/override ci/prow/e2e-cmd
this test is broken for now

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-07-21T11:36:14Z

@sttts can you override e2e-cmd? @soltysh is working on it, but it's broken for now.

openshift-bot · 2020-07-21T11:42:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

sttts · 2020-07-21T11:54:28Z

/override e2e-cmd

openshift-ci-robot · 2020-07-21T11:54:30Z

@sttts: /override requires a failed status context to operate on.
The following unknown contexts were given:

e2e-cmd

Only the following contexts were expected:

ci/prow/e2e-aws
ci/prow/e2e-aws-serial
ci/prow/e2e-aws-upgrade
ci/prow/e2e-cmd
ci/prow/images
ci/prow/unit
ci/prow/verify
tide

In response to this:

/override e2e-cmd

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.

sttts · 2020-07-21T11:54:43Z

/override ci/prow/e2e-cmd

openshift-ci-robot · 2020-07-21T11:54:58Z

@sttts: Overrode contexts on behalf of sttts: ci/prow/e2e-cmd

In response to this:

/override ci/prow/e2e-cmd

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-bot · 2020-07-21T13:26:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T13:53:01Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T15:49:51Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T18:12:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T18:38:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T21:01:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T21:14:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-21T23:37:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-22T01:08:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-22T02:26:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-22T04:10:57Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-22T06:47:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-22T08:43:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-07-22T08:56:55Z

/retest

Please review the full test history for this PR and help us cut down flakes.

petr-muller · 2020-07-22T10:07:02Z

/hold

If the test is broken then there's no point in retesting. Either button merge it or /override after a failure is reported and then unhold (but Tide may decide to re-run the tests before merge so button merge is probably better).

sttts · 2020-07-22T10:22:57Z

/override ci/prow/e2e-cmd

sttts · 2020-07-22T10:23:08Z

/hold cancel

openshift-ci-robot · 2020-07-22T10:23:18Z

@sttts: Overrode contexts on behalf of sttts: ci/prow/e2e-cmd

In response to this:

/override ci/prow/e2e-cmd

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 the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2020

openshift-ci-robot requested review from deads2k and miminar June 2, 2020 07:36

ricardomaraschini changed the title ~~WIP - IR21: Leveraging image config for "insecure" registries during ImageStreamImport~~ WIP - IR-21: Leveraging image config for "insecure" registries during ImageStreamImport Jun 2, 2020

ricardomaraschini mentioned this pull request Jun 3, 2020

Bug 1871080: IR-21: Adding tests for image stream imports on blocked/insecure registries openshift/origin#25058

Merged

ricardomaraschini changed the title ~~WIP - IR-21: Leveraging image config for "insecure" registries during ImageStreamImport~~ IR-21: Leveraging image config for "insecure" registries during ImageStreamImport Jun 4, 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 Jun 4, 2020

dmage reviewed Jun 4, 2020

View reviewed changes

openshift-ci-robot assigned dmage Jun 4, 2020

openshift-ci-robot assigned mfojtik Jun 4, 2020

openshift-ci-robot unassigned dmage Jun 15, 2020

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

openshift-ci-robot assigned sttts Jun 30, 2020

sttts reviewed Jun 30, 2020

View reviewed changes

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 1, 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 Jul 22, 2020

openshift deleted a comment from openshift-ci-robot Jul 22, 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 Jul 22, 2020

openshift-merge-robot merged commit 8ecba74 into openshift:master Jul 22, 2020

mtrmac mentioned this pull request Sep 24, 2020

Use openshift/runtime-utils/pkg/registries and containers/image instead local copies #41

Closed

IR-21: Leveraging image config for "insecure" registries during ImageStreamImport #111

IR-21: Leveraging image config for "insecure" registries during ImageStreamImport #111

Conversation

ricardomaraschini commented Jun 2, 2020 • edited Loading

ricardomaraschini commented Jun 3, 2020

ricardomaraschini commented Jun 3, 2020

ricardomaraschini commented Jun 3, 2020

ricardomaraschini commented Jun 4, 2020

ricardomaraschini commented Jun 4, 2020

ricardomaraschini commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ricardomaraschini commented Jun 4, 2020

ricardomaraschini commented Jun 4, 2020

dmage commented Jun 15, 2020

dmage commented Jun 30, 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

sttts Jun 30, 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 Jul 1, 2020

ricardomaraschini commented Jul 6, 2020

openshift-ci-robot commented Jul 21, 2020

dmage commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

sttts commented Jul 21, 2020

openshift-ci-robot commented Jul 21, 2020

sttts commented Jul 21, 2020

openshift-ci-robot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 21, 2020

openshift-bot commented Jul 22, 2020

openshift-bot commented Jul 22, 2020

openshift-bot commented Jul 22, 2020

openshift-bot commented Jul 22, 2020

openshift-bot commented Jul 22, 2020

openshift-bot commented Jul 22, 2020

petr-muller commented Jul 22, 2020

sttts commented Jul 22, 2020

sttts commented Jul 22, 2020

openshift-ci-robot commented Jul 22, 2020

ricardomaraschini commented Jun 2, 2020 •

edited

Loading

sttts Jun 30, 2020 •

edited

Loading