Add Copy packages command for registry to registry image and chart copy #4420

TerryHowe · 2022-12-15T03:09:03Z

New command that takes advantage of oras copy command to copy images from one registry to another. The primary use case for this would be to copy images from ECR to Harbor or other private registry.

supports multiple architecture image copy duplicating arm and amd
preserves shasum of images from ECR to harbor so we can use the shasum for installation

codecov · 2022-12-15T03:35:33Z

Codecov Report

Merging #4420 (9cadc20) into main (e2f3ecd) will increase coverage by 0.09%.
The diff coverage is 91.60%.

@@            Coverage Diff             @@
##             main    #4420      +/-   ##
==========================================
+ Coverage   70.95%   71.05%   +0.09%     
==========================================
  Files         405      412       +7     
  Lines       32926    33069     +143     
==========================================
+ Hits        23364    23498     +134     
- Misses       8081     8089       +8     
- Partials     1481     1482       +1

Impacted Files	Coverage Δ
pkg/registry/client.go	`82.00% <82.00%> (ø)`
pkg/registry/credentials.go	`88.88% <88.88%> (ø)`
pkg/registry/artifact.go	`100.00% <100.00%> (ø)`
pkg/registry/cache.go	`100.00% <100.00%> (ø)`
pkg/registry/certificates.go	`100.00% <100.00%> (ø)`
pkg/registry/copy.go	`100.00% <100.00%> (ø)`
pkg/registry/storage.go	`100.00% <100.00%> (ø)`
pkg/providers/vsphere/vsphere.go	`62.92% <0.00%> (+0.42%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

TerryHowe · 2022-12-15T14:02:31Z

/test eks-anywhere-release-tooling-test-presubmit

TerryHowe · 2022-12-20T18:43:56Z

/test eks-anywhere-release-tooling-test-presubmit

TerryHowe · 2022-12-22T12:21:35Z

Closes: #4109

go.mod

pkg/registry/cache.go

g-gaston · 2022-12-29T14:04:43Z

pkg/registry/cache_test.go

+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"


Could we use gomega? That's the preferred test library in this repo

I thought we had a discussion that everyone hated gomega

pkg/registry/credentials.go

pkg/registry/ociregistry.go

pkg/registry/ociregistry_test.go

g-gaston · 2022-12-29T14:51:06Z

pkg/registry/orasinterface.go

+)
+
+// OrasInterface thin layer for oras.
+type OrasInterface interface {


Suggested change

type OrasInterface interface {

type Oras interface {

since the type already says it's an interface and this what we expose to the outside caller

g-gaston · 2022-12-29T14:52:38Z

pkg/registry/orasinterface.go

+	return result, err
+}
+
+// Resolve call oras Resolve.


could we explain a bit what this does?

g-gaston · 2022-12-29T14:56:04Z

pkg/registry/orasinterface.go

+}
+
+// OrasImplementation thin wrapper on oras.
+type OrasImplementation struct{}


curious why you decided to create this layer on top of oras.
I was thinking instead we could just define the interface with same same signature as the oras original methods
And then compose those original oras objects into one to satisfy that interface

like

type OrasInterface interface { Repository(ctx context.Context, name string) (orasregistry.Repository, error) Resolve(ctx context.Context, reference string) (ocispec.Descriptor, error) CopyGraph(ctx context.Context, src content.ReadOnlyStorage, dst content.Storage, root ocispec.Descriptor, opts oras.CopyGraphOptions) error }

Although it's still coupled to the oras module, since it uses its structs as input and output, it still can be implemented in different ways since it's not dependent on the "service" objects (remote.Registry, rasregistry.Repository, etc.).

I only did this to get enough test coverage.

Ah got you. Then if it doesn't require too much work, I would to change this to something similar to what I suggested. I believe it should allow us to reach the same amount of coverage.

Yeh, compose isn't going to work, but I was going to rearrange the code a bit to get the coverage back up AND get rid of this OrasImplementation

cmd/eksctl-anywhere/cmd/copypackages.go

ewollesen · 2023-01-06T00:00:58Z

cmd/eksctl-anywhere/cmd/copypackages.go

+	if err := copyPackagesCmd.MarkFlagRequired("bundle"); err != nil {
+		log.Fatalf("Cannot mark 'bundle' flag as required: %s", err)
+	}
+	copyPackagesCmd.Flags().StringVarP(&copyPackagesCommand.dstCert, "dst-cert", "", "", "Package bundles file to read artifact dependencies from")


Looks like some sort of cut and paste errors? Both here on line 39 and on line 40?

Thanks, fixed.

ewollesen · 2023-01-06T00:07:38Z

pkg/registry/cache.go

+}
+
+// NewCache creates an OCI registry client.
+func NewCache() *Cache {


How useful is this cache, given that this is used via the CLI and won't be long-lived?

I imagine we don't care, but this too isn't thread-safe... But unless we decide to parallelize the for loop that's reading from this cache, I suppose it's not important.

There is no intention of parallelizing this for now, this is for convenience so we we can mindless loop over the images to copy and reuse a client. The normal case, there are two sources and one destination.

oras paralielizes blob (aka image layer) copy under the covers

pkg/registry/cache_test.go

ewollesen · 2023-01-06T00:12:13Z

pkg/registry/client.go

+	project     string
+	certFile    string
+	insecure    bool
+	dryRun      bool


Maybe an idea for the future, but if there were a separate implementation, DryRunRegistryClient or the like, then the code doesn't need to branch on the dry run flag (i.e. it's simpler), and is therefore easier to read, and less prone to someone adding a method implementation and forgetting to check the value of the dry run flag and abort before making changes.

That was my original intention, but it ended up having a lot of repeated code. It was a complete copy of the client minus the actual call to CopyGraph

ewollesen · 2023-01-06T00:15:32Z

pkg/registry/client.go

+
+// Init registry configuration.
+func (or *OCIRegistryClient) Init() error {
+	if or.initialized {


I guess there's not much to worry about, but this is a race condition... Consider a sync.Mutex or a sync.Once if it's really important that this code only be run once per instantiation. I don't see anything here that isn't safe to run more than once?

ewollesen · 2023-01-06T00:19:15Z

pkg/registry/client.go

+		return nil
+	}
+
+	credentialStore := NewCredentialStore()


For the future, it's typically a better pattern to build these in your main, and pass them to a constructor. That makes your code more testable (you can easily pass in a test implementation), and it corrals the possible Init() errors to the main method where they can more easily be dealt with.

pkg/registry/client.go

g-gaston · 2023-01-09T13:38:51Z

pkg/registry/client.go

+)
+
+// Context describes aspects of a registry.
+type Context struct {


I personally find a bit confusing to name this context given how common context.Context is in Go and that this one doesn't implement context.Context.

What do you think about about RegistryConfig?

sounds good to me.

lint doesn't like RegistryContext, so I'll go with something else like maybe StorageContext

g-gaston · 2023-01-09T13:44:56Z

pkg/registry/orasinterface.go

+}
+
+// OrasImplementation thin wrapper on oras.
+type OrasImplementation struct{}


Ah got you. Then if it doesn't require too much work, I would to change this to something similar to what I suggested. I believe it should allow us to reach the same amount of coverage.

TerryHowe · 2023-01-09T20:20:49Z

/approve

eks-distro-bot · 2023-01-09T20:20:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: TerryHowe

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~~ [TerryHowe]

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

g-gaston

/lgtm
/unapprove
Not sure if you were gonna change the tests to use gomega, so un-approving jic. Feel free to approve it again if not.

TerryHowe · 2023-01-10T15:47:22Z

/test eks-anywhere-release-tooling-test-presubmit

TerryHowe · 2023-01-10T16:03:27Z

Not sure if you were gonna change the tests to use gomega, so un-approving jic. Feel free to approve it again if not.

I am copy/pasta some of these files to packages and it doesn't use gomega 🤷 which is part of the reasoning.

jonahjon · 2023-01-10T16:20:34Z

/lgtm

Good luck with those tests

TerryHowe · 2023-01-10T17:25:36Z

/test eks-anywhere-presubmit
/test eks-anywhere-release-tooling-test-presubmit

TerryHowe · 2023-01-10T17:51:13Z

/test eks-anywhere-release-tooling-test-presubmit

TerryHowe · 2023-01-10T17:53:11Z

/override eks-anywhere-release-tooling-test-presubmit

this is failing because of the golden file being out of date again. I just merged a new one this morning and already it is out of date.

eks-distro-bot · 2023-01-10T17:53:16Z

@TerryHowe: Overrode contexts on behalf of TerryHowe: eks-anywhere-release-tooling-test-presubmit

In response to this:

/override eks-anywhere-release-tooling-test-presubmit

this is failing because of the golden file being out of date again. I just merged a new one this morning and already it is out of date.

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.

TerryHowe · 2023-01-10T18:13:37Z

/override eks-anywhere-release-tooling-test-presubmit

eks-distro-bot · 2023-01-10T18:13:42Z

@TerryHowe: Overrode contexts on behalf of TerryHowe: eks-anywhere-release-tooling-test-presubmit

In response to this:

/override eks-anywhere-release-tooling-test-presubmit

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.

TerryHowe · 2023-01-11T01:29:13Z

/cherry-pick release-0.14

eks-distro-pr-bot · 2023-01-11T01:29:17Z

@TerryHowe: #4420 failed to apply on top of branch "release-0.14":

Applying: Add copy packages command
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	release/api/v1alpha1/artifact_types.go
Falling back to patching base and 3-way merge...
Auto-merging release/api/v1alpha1/artifact_types.go
CONFLICT (content): Merge conflict in release/api/v1alpha1/artifact_types.go
CONFLICT (add/add): Merge conflict in pkg/registry/storage.go
Auto-merging pkg/registry/storage.go
CONFLICT (add/add): Merge conflict in pkg/registry/credentials.go
Auto-merging pkg/registry/credentials.go
CONFLICT (add/add): Merge conflict in pkg/registry/cache.go
Auto-merging pkg/registry/cache.go
Auto-merging go.sum
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
CONFLICT (add/add): Merge conflict in cmd/eksctl-anywhere/cmd/copypackages.go
Auto-merging cmd/eksctl-anywhere/cmd/copypackages.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add copy packages command
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.14

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.

eks-distro-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 15, 2022

TerryHowe changed the title ~~Copy packages~~ Add Copy packages command for registry to registry image and chart copy Dec 15, 2022

eks-distro-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 19, 2022

TerryHowe force-pushed the copy-packages branch from 0985076 to 319de31 Compare December 20, 2022 18:21

TerryHowe force-pushed the copy-packages branch from 319de31 to a8a9a13 Compare December 21, 2022 20:02

g-gaston reviewed Dec 29, 2022

View reviewed changes

TerryHowe force-pushed the copy-packages branch 3 times, most recently from 99751f9 to 728b6e1 Compare January 5, 2023 20:27

ewollesen reviewed Jan 5, 2023

View reviewed changes

cmd/eksctl-anywhere/cmd/copypackages.go Outdated Show resolved Hide resolved

ewollesen reviewed Jan 6, 2023

View reviewed changes

TerryHowe force-pushed the copy-packages branch 2 times, most recently from c114e62 to 3ca097d Compare January 8, 2023 12:58

g-gaston reviewed Jan 9, 2023

View reviewed changes

eks-distro-bot added the approved label Jan 9, 2023

TerryHowe force-pushed the copy-packages branch from 30d33d6 to e8451c9 Compare January 9, 2023 20:26

g-gaston approved these changes Jan 10, 2023

View reviewed changes

eks-distro-bot assigned g-gaston Jan 10, 2023

eks-distro-bot added the lgtm label Jan 10, 2023

g-gaston removed the approved label Jan 10, 2023

Terry Howe added 2 commits January 10, 2023 08:57

Add copy packages command

b61c72a

Add tests for package copy

d7921d0

eks-distro-pr-bot added 12 commits January 10, 2023 08:57

Fix tests and coverage

a944b97

Create certificates object

fff5f71

Fix tests and lint

a5385e2

Bring back client test

2ab1463

Clean file path

b630472

Rename Context to RegistryContext

6cb85e4

Create new registry copy command

654d3c4

Rename context struct

cc94167

Update test coverage

162647f

Remove unused file

f5a2b2b

Remove unnecessary getters

fd0d31c

Fix the sync.Once call

9cadc20

TerryHowe force-pushed the copy-packages branch from e8451c9 to 9cadc20 Compare January 10, 2023 15:57

eks-distro-bot added approved and removed lgtm labels Jan 10, 2023

eks-distro-bot assigned jonahjon Jan 10, 2023

eks-distro-bot added the lgtm label Jan 10, 2023

eks-distro-bot merged commit 849bea5 into aws:main Jan 10, 2023

TerryHowe deleted the copy-packages branch January 24, 2023 16:21

TerryHowe mentioned this pull request May 22, 2023

Add download packages command #5745

Closed

Add Copy packages command for registry to registry image and chart copy #4420

Add Copy packages command for registry to registry image and chart copy #4420

Conversation

TerryHowe commented Dec 15, 2022 • edited Loading

codecov bot commented Dec 15, 2022 • edited Loading

Codecov Report

TerryHowe commented Dec 15, 2022

TerryHowe commented Dec 20, 2022

TerryHowe commented Dec 22, 2022

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

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

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

TerryHowe commented Jan 9, 2023

eks-distro-bot commented Jan 9, 2023

g-gaston left a comment

Choose a reason for hiding this comment

TerryHowe commented Jan 10, 2023

TerryHowe commented Jan 10, 2023

jonahjon commented Jan 10, 2023

TerryHowe commented Jan 10, 2023

TerryHowe commented Jan 10, 2023

TerryHowe commented Jan 10, 2023

eks-distro-bot commented Jan 10, 2023

TerryHowe commented Jan 10, 2023

eks-distro-bot commented Jan 10, 2023

TerryHowe commented Jan 11, 2023

eks-distro-pr-bot commented Jan 11, 2023

TerryHowe commented Dec 15, 2022 •

edited

Loading

codecov bot commented Dec 15, 2022 •

edited

Loading