Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 2 commits into from
Apr 9, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f876c4b
Vendoring github.com/docker/docker/registry
ricardomaraschini Mar 13, 2020
b1de89b
Leveraging node pull secrets
ricardomaraschini Mar 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ require (
github.com/Microsoft/go-winio v0.4.14 // indirect
github.com/davecgh/go-spew v1.1.1
github.com/docker/distribution v2.7.1+incompatible
github.com/docker/docker v1.4.2-0.20190327010347-be7ac8be2ae0 // indirect
github.com/docker/docker v1.4.2-0.20190327010347-be7ac8be2ae0
github.com/docker/go-units v0.4.0
github.com/emicklei/go-restful v2.9.5+incompatible
github.com/fsouza/go-dockerclient v0.0.0-20171004212419-da3951ba2e9e
Expand Down
130 changes: 0 additions & 130 deletions pkg/image/apiserver/importer/credentials.go
View file
Open in desktop

This file was deleted.

115 changes: 0 additions & 115 deletions pkg/image/apiserver/importer/credentials_test.go
View file
Open in desktop

This file was deleted.

9 changes: 4 additions & 5 deletions pkg/image/apiserver/importer/import_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ import (
restclient "k8s.io/client-go/rest"
kapi "k8s.io/kubernetes/pkg/apis/core"

"github.com/openshift/library-go/pkg/image/registryclient"
imageapi "github.com/openshift/openshift-apiserver/pkg/image/apis/image"
"github.com/openshift/openshift-apiserver/pkg/image/apiserver/importer"
dockerregistry "github.com/openshift/openshift-apiserver/pkg/image/apiserver/importer/dockerv1client"
Expand Down Expand Up @@ -93,7 +92,7 @@ func retryWhenUnreachable(t *testing.T, f func() error, errorPatterns ...string)

func TestImageStreamImportDockerHub(t *testing.T) {
rt, _ := restclient.TransportFor(&restclient.Config{})
importCtx := registryclient.NewContext(rt, nil)
importCtx := importer.NewStaticCredentialsContext(rt, nil, nil)

imports := &imageapi.ImageStreamImport{
Spec: imageapi.ImageStreamImportSpec{
Expand Down Expand Up @@ -154,7 +153,7 @@ func TestImageStreamImportDockerHub(t *testing.T) {

func TestImageStreamImportQuayIO(t *testing.T) {
rt, _ := restclient.TransportFor(&restclient.Config{})
importCtx := registryclient.NewContext(rt, nil)
importCtx := importer.NewStaticCredentialsContext(rt, nil, nil)

repositoryName := quayRegistryName + "/coreos/etcd"
imports := &imageapi.ImageStreamImport{
Expand Down Expand Up @@ -207,7 +206,7 @@ func TestImageStreamImportQuayIO(t *testing.T) {

func TestImageStreamImportRedHatRegistry(t *testing.T) {
rt, _ := restclient.TransportFor(&restclient.Config{})
importCtx := registryclient.NewContext(rt, nil)
importCtx := importer.NewStaticCredentialsContext(rt, nil, nil)

repositoryName := pulpRegistryName + "/rhel7"
// test without the client on the context
Expand Down Expand Up @@ -244,7 +243,7 @@ func TestImageStreamImportRedHatRegistry(t *testing.T) {
},
}
context := gocontext.WithValue(gocontext.Background(), importer.ContextKeyV1RegistryClient, dockerregistry.NewClient(20*time.Second, false))
importCtx = registryclient.NewContext(rt, nil)
importCtx = importer.NewStaticCredentialsContext(rt, nil, nil)
err := retryWhenUnreachable(t, func() error {
i = importer.NewImageStreamImporter(importCtx, nil, 3, nil, nil)
if err := i.Import(context, imports, &imageapi.ImageStream{}); err != nil {
Expand Down
10 changes: 5 additions & 5 deletions pkg/image/apiserver/importer/importer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ type Interface interface {

// RepositoryRetriever fetches a Docker distribution.Repository.
type RepositoryRetriever interface {
// Repository returns a properly authenticated distribution.Repository for the given registry, repository
// name, and insecure toleration behavior.
Repository(ctx gocontext.Context, registry *url.URL, repoName string, insecure bool) (distribution.Repository, error)
// Repository returns a properly authenticated distribution.Repository for the given
// docker image reference and insecure toleration behavior.
Repository(ctx gocontext.Context, ref imageref.DockerImageReference, insecure bool) (distribution.Repository, error)
}

// ImageStreamImport implements an import strategy for container images. It keeps a cache of images
Expand Down Expand Up @@ -472,7 +472,7 @@ func (isi *ImageStreamImporter) getManifestFromSource(ctx gocontext.Context, ret
return nil, nil, nil, fmt.Errorf("unable to parse reference %q: %v", ref.String(), err)
}

repo, err := retriever.Repository(ctx, imageRef.RegistryURL(), imageRef.RepositoryName(), insecure)
repo, err := retriever.Repository(ctx, imageRef, insecure)
if err != nil {
return nil, nil, nil, formatPingError(imageRef, insecure, err)
}
Expand Down Expand Up @@ -667,7 +667,7 @@ func (isi *ImageStreamImporter) importRepositoryFromDocker(ctx gocontext.Context
// if repository import is requested (MaximumTags), attempt to load the tags, sort them, and request the first N
if count := repository.MaximumTags; count > 0 || count == -1 {
// retrieve the repository
repo, err := retriever.Repository(ctx, repository.Registry, repository.Name, repository.Insecure)
repo, err := retriever.Repository(ctx, repository.Ref, repository.Insecure)
if err != nil {
klog.V(5).Infof("unable to access repository %#v: %#v", repository, err)
if strings.HasSuffix(err.Error(), "does not support v2 API") {
Expand Down
19 changes: 14 additions & 5 deletions pkg/image/apiserver/importer/importer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,25 @@ import (
godigest "github.com/opencontainers/go-digest"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/kubernetes/pkg/api/legacyscheme"
kapi "k8s.io/kubernetes/pkg/apis/core"

"github.com/openshift/library-go/pkg/image/registryclient"
userapi "github.com/openshift/api/user/v1"
imageref "github.com/openshift/library-go/pkg/image/reference"
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))
Copy link
Contributor

@sttts sttts Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ricardomaraschini ricardomaraschini Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sttts sttts Mar 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, fine for me as a follow-up.

}

type mockRetrieverFunc func(registry *url.URL, repoName string, insecure bool) (distribution.Repository, error)

func (r mockRetrieverFunc) Repository(ctx context.Context, registry *url.URL, repoName string, insecure bool) (distribution.Repository, error) {
return r(registry, repoName, insecure)
func (r mockRetrieverFunc) Repository(ctx context.Context, ref imageref.DockerImageReference, insecure bool) (distribution.Repository, error) {
return r(ref.RegistryURL(), ref.RepositoryName(), insecure)
}

type mockRetriever struct {
Expand All @@ -40,7 +47,7 @@ type mockRetriever struct {
err error
}

func (r *mockRetriever) Repository(ctx context.Context, registry *url.URL, repoName string, insecure bool) (distribution.Repository, error) {
func (r *mockRetriever) Repository(ctx context.Context, ref imageref.DockerImageReference, insecure bool) (distribution.Repository, error) {
r.insecure = insecure
return r.repo, r.err
}
Expand Down Expand Up @@ -218,7 +225,9 @@ func TestDockerV1Fallback(t *testing.T) {
}

func TestImportNothing(t *testing.T) {
ctx := registryclient.NewContext(http.DefaultTransport, http.DefaultTransport).WithCredentials(registryclient.NoCredentials)
ctx := NewStaticCredentialsContext(
http.DefaultTransport, http.DefaultTransport, nil,
)
isi := &imageapi.ImageStreamImport{}
i := NewImageStreamImporter(ctx, nil, 5, nil, nil)
if err := i.Import(nil, isi, nil); err != nil {
Expand Down
Loading