Skip to content

image/registryclient: set insecure and return early in blobMirroredRepository #1096

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 4 commits into from
Jun 9, 2021
Merged

image/registryclient: set insecure and return early in blobMirroredRepository #1096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
973b589
Replace golang.org/x/net/context with stdlib's context
soltysh Jun 8, 2021
6d5b747
image/registryclient: set insecure blobMirroredRepository
soltysh Jun 7, 2021
e949186
image/registryclient: return proper error when no alternates found
soltysh Jun 7, 2021
c899fd5
image/registryclient: add tests for alternate image sources
soltysh Jun 8, 2021
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
1 change: 0 additions & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ require (
github.com/xlab/handysort v0.0.0-20150421192137-fb3537ed64a1 // indirect
go.etcd.io/etcd v0.5.0-alpha.5.0.20200910180754-dd1b699fc489
golang.org/x/crypto v0.0.0-20210220033148-5ea612d1eb83
golang.org/x/net v0.0.0-20210224082022-3d97a244fca7
golang.org/x/sys v0.0.0-20210225134936-a50acf3fe073
golang.org/x/time v0.0.0-20210220033141-f8bda1e9f3ba
gopkg.in/asn1-ber.v1 v1.0.0-20181015200546-f715ec2f112d // indirect
Expand Down
3 changes: 2 additions & 1 deletion pkg/image/registryclient/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package registryclient

import (
"context"
"fmt"
"hash"
"io"
Expand All @@ -12,7 +13,6 @@ import (
"sync"
"time"

"golang.org/x/net/context"
"golang.org/x/time/rate"

"k8s.io/klog/v2"
Expand Down Expand Up @@ -236,6 +236,7 @@ func (c *Context) Repository(ctx context.Context, registry *url.URL, repoName st
}
return &blobMirroredRepository{
locator: locator,
insecure: insecure,
strategy: c.Alternates,
retriever: c,
}, nil
Expand Down
5 changes: 4 additions & 1 deletion pkg/image/registryclient/client_mirrored.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package registryclient

import (
"context"
"fmt"
"net/http"
"net/url"
Expand All @@ -9,7 +10,6 @@ import (
"github.com/docker/distribution"
"github.com/opencontainers/go-digest"
"github.com/openshift/library-go/pkg/image/reference"
"golang.org/x/net/context"
"k8s.io/klog/v2"

distributionreference "github.com/docker/distribution/reference"
Expand Down Expand Up @@ -216,6 +216,9 @@ func (r *blobMirroredRepository) alternates(ctx context.Context, fn func(r Repos
if err != nil {
return err
}
if len(alternates) == 0 {
Copy link
Author

@soltysh soltysh Jun 7, 2021

Choose a reason for hiding this comment

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

@smarterclayton alternatively we could check for nil strategy in https://github.com/openshift/library-go/blob/master/pkg/image/registryclient/client_mirrored.go#L207 similarly to initialRepos or errorRepos

return attemptErr
}
Copy link
Contributor

@smarterclayton smarterclayton Jun 7, 2021

Choose a reason for hiding this comment

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

This looks correct but now I want to see a unit test for it. This is the real loop we need to have unit tests on anyway.

if alternateErr := r.attemptRepos(ctx, alternates, fn); alternateErr != nil {
return attemptErr
}
Expand Down
132 changes: 124 additions & 8 deletions pkg/image/registryclient/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package registryclient

import (
"bytes"
"context"
"encoding/hex"
"flag"
"fmt"
"io/ioutil"
"net/http"
Expand All @@ -17,7 +17,6 @@ import (
"golang.org/x/time/rate"

"k8s.io/client-go/rest"
klog "k8s.io/klog/v2"

"github.com/docker/distribution"
"github.com/docker/distribution/manifest/manifestlist"
Expand All @@ -27,7 +26,6 @@ import (
registryclient "github.com/docker/distribution/registry/client"
"github.com/opencontainers/go-digest"
imagereference "github.com/openshift/library-go/pkg/image/reference"
"golang.org/x/net/context"
)

type mockRetriever struct {
Expand Down Expand Up @@ -921,9 +919,6 @@ func TestMirroredRegistry_ManifestGet(t *testing.T) {
schema2.MediaTypeManifest,
})

klog.InitFlags(flag.CommandLine)
//cliflag.InitFlags()
flag.CommandLine.Lookup("v").Value.Set("10")
rt, err := rest.TransportFor(&rest.Config{})
if err != nil {
t.Fatal(err)
Expand All @@ -934,6 +929,8 @@ func TestMirroredRegistry_ManifestGet(t *testing.T) {
}
c := NewContext(rt, insecureRT)

t.Logf("original request")

r, err := c.Repository(context.Background(), &url.URL{Host: "registry-1.docker.io"}, "library/postgres", false)
if err != nil {
t.Fatal(err)
Expand All @@ -950,14 +947,13 @@ func TestMirroredRegistry_ManifestGet(t *testing.T) {
t.Fatal("Expected data to be present")
}

t.Logf("next request")
t.Logf("alternate FirstRequest")

c.Alternates = &fakeAlternateBlobStrategy{
FirstAlternates: []imagereference.DockerImageReference{
{Registry: "quay.io", Namespace: "library", Name: "postgres"},
{Registry: "docker.io", Namespace: "library", Name: "postgres"},
},
FirstErr: nil,
}
r, err = c.Repository(context.Background(), &url.URL{Host: "quay.io"}, "test/other", false)
if err != nil {
Expand Down Expand Up @@ -997,4 +993,124 @@ func TestMirroredRegistry_ManifestGet(t *testing.T) {
if !reflect.DeepEqual(ref, imagereference.DockerImageReference{Registry: "docker.io", Namespace: "library", Name: "postgres"}) {
t.Fatalf("Unexpected reference: %#v", ref)
}

t.Logf("alternate OnFailure")

c.Alternates = &fakeAlternateBlobStrategy{
FailureAlternates: []imagereference.DockerImageReference{
{Registry: "quay.io", Namespace: "library", Name: "postgres"},
{Registry: "docker.io", Namespace: "library", Name: "postgres"},
},
}
r, err = c.Repository(context.Background(), &url.URL{Host: "quay.io"}, "test/other", false)
if err != nil {
t.Fatal(err)
}
m, err = r.Manifests(context.Background())
if err != nil {
t.Fatal(err)
}
mwl, ok = m.(ManifestWithLocationService)
if !ok {
t.Fatalf("Expected service to implement location retrieval")
}
secondManifest, secondRef, err := mwl.GetWithLocation(context.Background(), digest.Digest("sha256:b94ab3a31950e7d25654d024044ac217c2b3a94eff426e3415424c1c16ca3fe6"), opt)
if err != nil {
t.Fatal(err)
}

if secondManifest == nil {
t.Fatal("Expected data to be present")
}
if !reflect.DeepEqual(manifest, secondManifest) {
t.Fatalf("Mirror and non mirror request did not match")
}
if !reflect.DeepEqual(secondRef, imagereference.DockerImageReference{Registry: "docker.io", Namespace: "library", Name: "postgres"}) {
t.Fatalf("Unexpected reference: %#v", ref)
}
}

func TestMirroredRegistry_InvalidManifest(t *testing.T) {
opt := distribution.WithManifestMediaTypes([]string{
manifestlist.MediaTypeManifestList,
schema2.MediaTypeManifest,
})

rt, err := rest.TransportFor(&rest.Config{})
if err != nil {
t.Fatal(err)
}
insecureRT, err := rest.TransportFor(&rest.Config{TLSClientConfig: rest.TLSClientConfig{Insecure: true}})
if err != nil {
t.Fatal(err)
}
c := NewContext(rt, insecureRT)

r, err := c.Repository(context.Background(), &url.URL{Host: "registry-1.docker.io"}, "library/postgres", false)
if err != nil {
t.Fatal(err)
}
m, err := r.Manifests(context.Background())
if err != nil {
t.Fatal(err)
}
_, err = m.Get(context.Background(), digest.Digest("@sha256:deadbeef00000000000000000000000000000000000000000000000000000000"), opt)
if err == nil {
t.Fatal("Expected manifest reading error")
}
}

func TestMirroredRegistry_AlternateErrors(t *testing.T) {
opt := distribution.WithManifestMediaTypes([]string{
manifestlist.MediaTypeManifestList,
schema2.MediaTypeManifest,
})

rt, err := rest.TransportFor(&rest.Config{})
if err != nil {
t.Fatal(err)
}
insecureRT, err := rest.TransportFor(&rest.Config{TLSClientConfig: rest.TLSClientConfig{Insecure: true}})
if err != nil {
t.Fatal(err)
}

c := NewContext(rt, insecureRT)
c.Alternates = &fakeAlternateBlobStrategy{
FirstErr: fmt.Errorf("icsp error"),
}

t.Logf("alternate FirstRequest error")

r, err := c.Repository(context.Background(), &url.URL{Host: "quay.io"}, "test/other", false)
if err != nil {
t.Fatal(err)
}
m, err := r.Manifests(context.Background())
if err != nil {
t.Fatal(err)
}
_, err = m.Get(context.Background(), digest.Digest("sha256:b94ab3a31950e7d25654d024044ac217c2b3a94eff426e3415424c1c16ca3fe6"), opt)
if err == nil || !strings.Contains(err.Error(), "icsp error") {
t.Fatalf("Expected 'icsp error', got %v", err)
}

t.Logf("alternate OnFailure error")

c.Alternates = &fakeAlternateBlobStrategy{
FailureErr: fmt.Errorf("icsp failure error"),
}

r, err = c.Repository(context.Background(), &url.URL{Host: "quay.io"}, "test/other", false)
if err != nil {
t.Fatal(err)
}
m, err = r.Manifests(context.Background())
if err != nil {
t.Fatal(err)
}
_, err = m.Get(context.Background(), digest.Digest("sha256:b94ab3a31950e7d25654d024044ac217c2b3a94eff426e3415424c1c16ca3fe6"), opt)
if err == nil || !strings.Contains(err.Error(), "icsp failure error") {
t.Fatalf("Expected 'icsp failure error' got %v", err)
}
}
4 changes: 2 additions & 2 deletions pkg/operator/staticpod/installerpod/copy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package installerpod

import (
"golang.org/x/net/context"
"k8s.io/klog/v2"
"context"

v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"

"github.com/openshift/library-go/pkg/operator/resource/retry"
)
Expand Down
1 change: 0 additions & 1 deletion vendor/modules.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,6 @@ golang.org/x/crypto/openpgp/s2k
golang.org/x/crypto/poly1305
golang.org/x/crypto/salsa20/salsa
# golang.org/x/net v0.0.0-20210224082022-3d97a244fca7
## explicit
golang.org/x/net/context
golang.org/x/net/context/ctxhttp
golang.org/x/net/http/httpguts
Expand Down