From 5855f79156f7bcd6331d00f718c4d53f49f92f1b Mon Sep 17 00:00:00 2001 From: Josh Wolf Date: Wed, 1 Dec 2021 20:57:40 -0700 Subject: [PATCH] allow reference string to be passed to AddArtifact instead of name.ParseReference for ease of use, move reference validation within AddArtifact --- cmd/hauler/cli/store/add.go | 35 ++++-------- pkg/artifact/oci.go | 3 +- pkg/collection/chart/chart.go | 27 ++------- pkg/collection/k3s/k3s.go | 28 ++-------- pkg/content/chart/chart_test.go | 72 ------------------------ pkg/content/image/image_test.go | 99 --------------------------------- pkg/store/store.go | 15 ++++- pkg/store/store_test.go | 20 ++++--- 8 files changed, 49 insertions(+), 250 deletions(-) diff --git a/cmd/hauler/cli/store/add.go b/cmd/hauler/cli/store/add.go index 0b5b9ac..589796e 100644 --- a/cmd/hauler/cli/store/add.go +++ b/cmd/hauler/cli/store/add.go @@ -2,8 +2,10 @@ package store import ( "context" + "fmt" "github.com/google/go-containerregistry/pkg/name" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/spf13/cobra" "github.com/rancherfederal/hauler/pkg/apis/hauler.cattle.io/v1alpha1" @@ -94,17 +96,13 @@ func storeFile(ctx context.Context, s *store.Store, fi v1alpha1.File) error { l := log.FromContext(ctx) f := file.NewFile(fi.Ref) - ref, err := name.ParseReference(f.Name(fi.Ref), name.WithDefaultRegistry("")) + + desc, err := s.AddArtifact(ctx, f, f.Name(fi.Ref)) if err != nil { return err } - desc, err := s.AddArtifact(ctx, f, ref) - if err != nil { - return err - } - - l.With(log.Fields{"type": s.Identify(ctx, desc)}).Infof("added [%s] to store", ref.Name()) + l.With(log.Fields{"type": s.Identify(ctx, desc)}).Infof("added [%s] to store", desc.Annotations[ocispec.AnnotationRefName]) return nil } @@ -133,17 +131,12 @@ func storeImage(ctx context.Context, s *store.Store, i v1alpha1.Image) error { return err } - ref, err := name.ParseReference(i.Ref) + desc, err := s.AddArtifact(ctx, oci, i.Ref) if err != nil { return err } - desc, err := s.AddArtifact(ctx, oci, ref) - if err != nil { - return err - } - - l.With(log.Fields{"type": s.Identify(ctx, desc)}).Infof("added [%s] to store", ref.Name()) + l.With(log.Fields{"type": s.Identify(ctx, desc)}).Infof("added [%s] to store", i.Ref) return nil } @@ -171,29 +164,25 @@ func AddChartCmd(ctx context.Context, o *AddChartOpts, s *store.Store, chartName return storeChart(ctx, s, cfg) } -func storeChart(ctx context.Context, s *store.Store, ch v1alpha1.Chart) error { +func storeChart(ctx context.Context, s *store.Store, cfg v1alpha1.Chart) error { l := log.FromContext(ctx) - oci, err := chart.NewChart(ch.Name, ch.RepoURL, ch.Version) + oci, err := chart.NewChart(cfg.Name, cfg.RepoURL, cfg.Version) if err != nil { return err } - tag := ch.Version + tag := cfg.Version if tag == "" { tag = name.DefaultTag } - ref, err := name.ParseReference(ch.Name, name.WithDefaultRegistry(""), name.WithDefaultTag(tag)) - if err != nil { - return err - } - + ref := fmt.Sprintf("%s:%s", cfg.Name, tag) desc, err := s.AddArtifact(ctx, oci, ref) if err != nil { return err } - l.With(log.Fields{"type": s.Identify(ctx, desc)}).Infof("added [%s] to store", ref.Name()) + l.With(log.Fields{"type": s.Identify(ctx, desc)}).Infof("added [%s] to store", ref) return nil } diff --git a/pkg/artifact/oci.go b/pkg/artifact/oci.go index e6fecad..8faf43d 100644 --- a/pkg/artifact/oci.go +++ b/pkg/artifact/oci.go @@ -1,7 +1,6 @@ package artifact import ( - "github.com/google/go-containerregistry/pkg/name" "github.com/google/go-containerregistry/pkg/v1" ) @@ -20,5 +19,5 @@ type OCI interface { type Collection interface { // Contents returns the list of contents in the collection - Contents() (map[name.Reference]OCI, error) + Contents() (map[string]OCI, error) } diff --git a/pkg/collection/chart/chart.go b/pkg/collection/chart/chart.go index f82ff41..66871ca 100644 --- a/pkg/collection/chart/chart.go +++ b/pkg/collection/chart/chart.go @@ -17,7 +17,7 @@ type tchart struct { config v1alpha1.ThickChart computed bool - contents map[gname.Reference]artifact.OCI + contents map[string]artifact.OCI } func NewChart(cfg v1alpha1.ThickChart) (artifact.Collection, error) { @@ -29,11 +29,11 @@ func NewChart(cfg v1alpha1.ThickChart) (artifact.Collection, error) { return &tchart{ chart: o, config: cfg, - contents: make(map[gname.Reference]artifact.OCI), + contents: make(map[string]artifact.OCI), }, nil } -func (c *tchart) Contents() (map[gname.Reference]artifact.OCI, error) { +func (c *tchart) Contents() (map[string]artifact.OCI, error) { if err := c.compute(); err != nil { return nil, err } @@ -70,12 +70,7 @@ func (c *tchart) chartContents() error { tag = gname.DefaultTag } - ref, err := gname.ParseReference(c.config.Name, gname.WithDefaultRegistry(""), gname.WithDefaultTag(tag)) - if err != nil { - return err - } - - c.contents[ref] = oci + c.contents[c.config.Name] = oci return nil } @@ -91,32 +86,22 @@ func (c *tchart) dependentImages() error { } for _, img := range imgs.Spec.Images { - ref, err := gname.ParseReference(img.Ref) - if err != nil { - return err - } - i, err := image.NewImage(img.Ref) if err != nil { return err } - c.contents[ref] = i + c.contents[img.Ref] = i } return nil } func (c *tchart) extraImages() error { for _, img := range c.config.ExtraImages { - ref, err := gname.ParseReference(img.Reference) - if err != nil { - return err - } - i, err := image.NewImage(img.Reference) if err != nil { return err } - c.contents[ref] = i + c.contents[img.Reference] = i } return nil } diff --git a/pkg/collection/k3s/k3s.go b/pkg/collection/k3s/k3s.go index 26e5df9..fbe648b 100644 --- a/pkg/collection/k3s/k3s.go +++ b/pkg/collection/k3s/k3s.go @@ -10,8 +10,6 @@ import ( "path" "strings" - "github.com/google/go-containerregistry/pkg/name" - "github.com/rancherfederal/hauler/internal/getter" "github.com/rancherfederal/hauler/pkg/artifact" "github.com/rancherfederal/hauler/pkg/content/file" @@ -38,7 +36,7 @@ type k3s struct { arch string computed bool - contents map[name.Reference]artifact.OCI + contents map[string]artifact.OCI channels map[string]string client *getter.Client } @@ -46,11 +44,11 @@ type k3s struct { func NewK3s(version string) (artifact.Collection, error) { return &k3s{ version: version, - contents: make(map[name.Reference]artifact.OCI), + contents: make(map[string]artifact.OCI), }, nil } -func (k *k3s) Contents() (map[name.Reference]artifact.OCI, error) { +func (k *k3s) Contents() (map[string]artifact.OCI, error) { if err := k.compute(); err != nil { return nil, err } @@ -98,11 +96,7 @@ func (k *k3s) executable() error { f := file.NewFile(fref) - ref, err := name.ParseReference("k3s", name.WithDefaultTag(k.dnsCompliantVersion()), name.WithDefaultRegistry("")) - if err != nil { - return err - } - + ref := fmt.Sprintf("k3s:%s", k.dnsCompliantVersion()) k.contents[ref] = f return nil } @@ -110,13 +104,7 @@ func (k *k3s) executable() error { func (k *k3s) bootstrap() error { namedBootstrapUrl := fmt.Sprintf("%s?filename=%s", bootstrapUrl, "k3s-init.sh") f := file.NewFile(namedBootstrapUrl) - - ref, err := name.ParseReference("k3s-init.sh", name.WithDefaultRegistry(""), name.WithDefaultTag("latest")) - if err != nil { - return err - } - - k.contents[ref] = f + k.contents["k3s-init.sh"] = f return nil } @@ -132,16 +120,12 @@ func (k *k3s) images() error { scanner := bufio.NewScanner(resp.Body) for scanner.Scan() { reference := scanner.Text() - ref, err := name.ParseReference(reference) - if err != nil { - return err - } o, err := image.NewImage(reference) if err != nil { return err } - k.contents[ref] = o + k.contents[reference] = o } return nil } diff --git a/pkg/content/chart/chart_test.go b/pkg/content/chart/chart_test.go index 5ab6b39..e55446f 100644 --- a/pkg/content/chart/chart_test.go +++ b/pkg/content/chart/chart_test.go @@ -1,73 +1 @@ package chart_test - -import ( - "context" - "os" - "path" - "testing" - - "github.com/google/go-containerregistry/pkg/name" - - "github.com/rancherfederal/hauler/pkg/apis/hauler.cattle.io/v1alpha1" - "github.com/rancherfederal/hauler/pkg/content/chart" - "github.com/rancherfederal/hauler/pkg/log" - "github.com/rancherfederal/hauler/pkg/store" -) - -func TestChart_Copy(t *testing.T) { - ctx := context.Background() - l := log.NewLogger(os.Stdout) - ctx = l.WithContext(ctx) - - tmpdir, err := os.MkdirTemp("", "hauler") - if err != nil { - t.Error(err) - } - defer os.Remove(tmpdir) - - s, err := store.NewStore(tmpdir) - if err != nil { - t.Error(err) - } - - type args struct { - ctx context.Context - registry string - } - tests := []struct { - name string - cfg v1alpha1.Chart - args args - wantErr bool - }{ - // TODO: This test isn't self-contained - { - name: "should work with unversioned chart", - cfg: v1alpha1.Chart{ - Name: "loki", - RepoURL: "https://grafana.github.io/helm-charts", - }, - args: args{ - ctx: ctx, - registry: "", - }, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c, err := chart.NewChart(tt.cfg.Name, tt.cfg.RepoURL, tt.cfg.Version) - if err != nil { - t.Fatal(err) - } - ref, err := name.ParseReference(path.Join("hauler", tt.cfg.Name)) - if err != nil { - t.Fatal(err) - } - - if _, err := s.AddArtifact(ctx, c, ref); (err != nil) != tt.wantErr { - t.Error(err) - } - }) - } -} diff --git a/pkg/content/image/image_test.go b/pkg/content/image/image_test.go index d7ca3d6..aa66c33 100644 --- a/pkg/content/image/image_test.go +++ b/pkg/content/image/image_test.go @@ -1,100 +1 @@ package image_test - -import ( - "context" - "os" - "path" - "path/filepath" - "testing" - - "github.com/google/go-containerregistry/pkg/name" - - "github.com/rancherfederal/hauler/pkg/apis/hauler.cattle.io/v1alpha1" - "github.com/rancherfederal/hauler/pkg/content/image" - "github.com/rancherfederal/hauler/pkg/log" - "github.com/rancherfederal/hauler/pkg/store" -) - -func TestImage_Copy(t *testing.T) { - ctx := context.Background() - l := log.NewLogger(os.Stdout) - ctx = l.WithContext(ctx) - - tmpdir, err := os.MkdirTemp("", "hauler") - if err != nil { - t.Error(err) - } - defer os.Remove(tmpdir) - - s, err := store.NewStore(tmpdir) - if err != nil { - t.Error(err) - } - - type args struct { - ctx context.Context - registry string - } - tests := []struct { - name string - cfg v1alpha1.Image - args args - wantErr bool - }{ - // TODO: These mostly test functionality we're not responsible for (go-containerregistry), refactor these to only stuff we care about - { - name: "should work with tagged image", - cfg: v1alpha1.Image{ - Ref: "busybox:1.34.1", - }, - args: args{ - ctx: ctx, - // registry: s.Registry(), - }, - wantErr: false, - }, - { - name: "should work with digest image", - cfg: v1alpha1.Image{ - Ref: "busybox@sha256:6066ca124f8c2686b7ae71aa1d6583b28c6dc3df3bdc386f2c89b92162c597d9", - }, - args: args{ - ctx: ctx, - // registry: s.Registry(), - }, - wantErr: false, - }, - { - name: "should work with tagged image", - cfg: v1alpha1.Image{ - Ref: "registry:2", - }, - args: args{ - ctx: ctx, - // registry: s.Registry(), - }, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - i, err := image.NewImage(tt.cfg.Ref) - if err != nil { - t.Error(err) - } - - ref, err := name.ParseReference(path.Join("hauler", filepath.Base(tt.cfg.Ref))) - if err != nil { - t.Fatal(err) - } - - if _, err := s.AddArtifact(ctx, i, ref); (err != nil) != tt.wantErr { - t.Error(err) - } - - // if err := s.Add(tt.args.ctx, i, ref); (err != nil) != tt.wantErr { - // t.Errorf("Copy() error = %v, wantErr %v", err, tt.wantErr) - // } - }) - } -} diff --git a/pkg/store/store.go b/pkg/store/store.go index 1a20d81..77d316f 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -9,6 +9,7 @@ import ( "github.com/google/go-containerregistry/pkg/name" ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/pkg/errors" "oras.land/oras-go/pkg/content" "oras.land/oras-go/pkg/oras" "oras.land/oras-go/pkg/target" @@ -24,6 +25,10 @@ type Store struct { cache cache.Cache } +var ( + ErrInvalidReference = errors.New("invalid reference") +) + func NewStore(rootdir string, opts ...Options) (*Store, error) { ociStore, err := content.NewOCI(rootdir) if err != nil { @@ -46,7 +51,7 @@ func NewStore(rootdir string, opts ...Options) (*Store, error) { // saved, the entirety of the layout is copied to the store (which is just a registry). This allows us to not only use // strict types to define generic content, but provides a processing pipeline suitable for extensibility. In the // future we'll allow users to define their own content that must adhere either by artifact.OCI or simply an OCI layout. -func (s *Store) AddArtifact(ctx context.Context, oci artifact.OCI, reference name.Reference) (ocispec.Descriptor, error) { +func (s *Store) AddArtifact(ctx context.Context, oci artifact.OCI, reference string) (ocispec.Descriptor, error) { stage, err := newLayout() if err != nil { return ocispec.Descriptor{}, err @@ -57,7 +62,13 @@ func (s *Store) AddArtifact(ctx context.Context, oci artifact.OCI, reference nam oci = cached } - if err := stage.add(ctx, oci, reference); err != nil { + // Ensure that index.docker.io isn't prepended + ref, err := name.ParseReference(reference, name.WithDefaultRegistry(""), name.WithDefaultTag("latest")) + if err != nil { + return ocispec.Descriptor{}, err + } + + if err := stage.add(ctx, oci, ref); err != nil { return ocispec.Descriptor{}, err } return stage.commit(ctx, s) diff --git a/pkg/store/store_test.go b/pkg/store/store_test.go index 53b4227..bf1d1a6 100644 --- a/pkg/store/store_test.go +++ b/pkg/store/store_test.go @@ -8,7 +8,6 @@ import ( "reflect" "testing" - "github.com/google/go-containerregistry/pkg/name" gv1 "github.com/google/go-containerregistry/pkg/v1" "github.com/google/go-containerregistry/pkg/v1/partial" "github.com/google/go-containerregistry/pkg/v1/random" @@ -41,24 +40,27 @@ func TestStore_AddArtifact(t *testing.T) { wantErr bool }{ { - name: "should add artifact with a valid reference", + name: "should add artifact with a valid tagged reference", args: args{ ctx: ctx, reference: "random:v1", }, wantErr: false, }, + { + name: "should fail with ErrInvalidReference when an invalid reference is provided", + args: args{ + ctx: ctx, + reference: "n0tV@l!d:v1", + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ref, err := name.ParseReference(tt.args.reference) - if err != nil { - t.Fatal(err) - } + oci, want := genArtifact(t, tt.args.reference) - oci, want := genArtifact(t, ref.Name()) - - got, err := s.AddArtifact(tt.args.ctx, oci, ref) + got, err := s.AddArtifact(tt.args.ctx, oci, tt.args.reference) if (err != nil) != tt.wantErr { t.Errorf("AddArtifact() error = %v, wantErr %v", err, tt.wantErr) return