-
Notifications
You must be signed in to change notification settings - Fork 274
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.
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
feat: upgrade to oras-go v2 and support OCI registries requiring authentication. Closes #2819 #3620
Conversation
@@ -32,7 +31,8 @@ require ( | |||
github.com/mattn/go-isatty v0.0.19 | |||
github.com/mitchellh/colorstring v0.0.0-20190213212951-d06e56a500db | |||
github.com/olekukonko/tablewriter v0.0.5 | |||
github.com/opencontainers/image-spec v1.0.2 | |||
github.com/opencontainers/image-spec v1.1.0-rc.3 | |||
github.com/oras-project/oras-credentials-go v0.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is going to be merged into oras-go oras-project/oras-credentials-go#80
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah!
Is there a timeline for the merge? It wasn't obvious to me from the discussion thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, they are talking about waiting for v1.0.0-rc.1, currently at v0.2.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked in their Slack about the timeline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not take too long:
We need to wait for couple of weeks or a month to ensure that all APIs are indeed stable. The stability is currently being validated by oras CLI and notation CLI
Thank you for the PR @pdecat . We have been meaning to get to this, (open issue), but were always held back with the conflicts in go dependency resolution. |
Did not see that issue, updated PR description to link to it.
I don't see any conflict nowadays, isn't it? |
db001d0
to
4be5a04
Compare
pkg/ociinstaller/ocidownloader.go
Outdated
log.Println("[TRACE] ociDownloader.Pull:", "pulling...") | ||
|
||
copyOpt := oras.DefaultCopyOptions | ||
// TODO: use WithTargetPlatform to limit downloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is desired or not as there are checks later to ensure binaries for several platforms exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this limits the layers downloaded, then we should probably include it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It apparently require more changes than I expected.
As a test, just adding this:
diff --git a/pkg/ociinstaller/ocidownloader.go b/pkg/ociinstaller/ocidownloader.go
index bb6f8815..cfb3318a 100644
--- a/pkg/ociinstaller/ocidownloader.go
+++ b/pkg/ociinstaller/ocidownloader.go
@@ -79,8 +79,10 @@ func (o *ociDownloader) Pull(ctx context.Context, ref string, mediaTypes []strin
log.Println("[TRACE] ociDownloader.Pull:", "pulling...")
copyOpt := oras.DefaultCopyOptions
- // TODO: use WithTargetPlatform to limit downloads
- // copyOpt.WithTargetPlatform()
+ copyOpt.WithTargetPlatform(&ocispec.Platform{
+ Architecture: "amd64",
+ OS: "linux",
+ })
manifestDescriptor, err := oras.Copy(ctx, repo, tag, fileStore, tag, copyOpt)
if err != nil {
log.Println("[ERROR] ociDownloader.Pull:", "failed to pull", ref, err)
Results in:
steampipe plugin install csv:latest
2023-06-26 15:09:57.723 UTC [TRACE] steampipe: parse complete after 1 decode passes
2023-06-26 15:09:57.723 UTC [TRACE] steampipe: ensureInstallDir /home/patrick/.steampipe
2023-06-26 15:09:57.727 UTC [TRACE] steampipe: nothing to migrate in /home/patrick/.steampipe/internal/update-check.json
2023-06-26 15:09:57.728 UTC [TRACE] steampipe: No memory limit set
2023-06-26 15:09:57.728 UTC [TRACE] steampipe: ociDownloader.Download: downloading us-docker.pkg.dev/steampipe/plugins/turbot/csv:latest
2023-06-26 15:09:57.728 UTC [TRACE] steampipe: ociDownloader.Pull: preparing to pull ref us-docker.pkg.dev/steampipe/plugins/turbot/csv:latest tag latest destDir /home/patrick/.steampipe/plugins/tmp-0e41-4be8-9d66
2023-06-26 15:09:57.728 UTC [TRACE] steampipe: ociDownloader.Pull: pulling...
csv:latest [=========>----------------------------------------------------------] Downloading
2023-06-26 15:09:58.426 UTC [ERROR] steampipe: ociDownloader.Pull: failed to pull us-docker.pkg.dev/steampipe/plugins/turbot/csv:latest fail to recognize platform from unknown config applicati
csv:latest [====================================================================] Done
Skipped the following plugin:
Plugin: csv@latest
Reason: fail to recognize platform from unknown config application/vnd.turbot.steampipe.config.v1+json: expect application/vnd.oci.image.config.v1+json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdecat can you please remove this comment if it's not working/required?
4be5a04
to
9f0b13f
Compare
} | ||
|
||
// Fetch the config from the file store | ||
configData, err := content.FetchAll(ctx, fileStore, manifest.Config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified code a bit with feedback from the oras-go maintainers: https://cloud-native.slack.com/archives/CJ1KHJM5Z/p1687852628854409?thread_ts=1687832097.450759&cid=CJ1KHJM5Z
5f43524
to
62caef9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one small tweak and I think we're good to merge it
cmd/plugin.go
Outdated
@@ -409,7 +409,13 @@ func runPluginUpdateCmd(cmd *cobra.Command, args []string) { | |||
ref := ociinstaller.NewSteampipeImageRef(p) | |||
isExists, _ := plugin.Exists(p) | |||
if isExists { | |||
runUpdatesFor = append(runUpdatesFor, versionData.Plugins[ref.DisplayImageRef()]) | |||
if strings.HasPrefix(ref.DisplayImageRef(), "hub.steampipe.io/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put a const in for hub.steampipe.io/
in the consts package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
pkg/ociinstaller/imageref.go
Outdated
pluginNameAndStream := strings.Split(split[len(split)-1], "@") | ||
|
||
return org, pluginNameAndStream[0], pluginNameAndStream[1] | ||
if strings.HasPrefix(r.DisplayImageRef(), "hub.steampipe.io/") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put a const in for hub.steampipe.io
in the consts package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@pdecat We have a couple of tests which verify that OCI installations are working as expected (correct binaries in the expected locations) Can you rebase your branch with |
62caef9
to
2eeffaa
Compare
Added the constant, and rebased on main. |
Looks like an acceptance test failed because of Github rate limiting:
https://github.com/turbot/steampipe/actions/runs/5532253026/jobs/10094278301?pr=3620#step:2:57 Also, a check is failing on MacOS:
https://github.com/turbot/steampipe/actions/runs/5532253026/jobs/10094274375?pr=3620#step:15:16 |
Tests related to this PR seem to pass on Linux:
https://github.com/turbot/steampipe/actions/runs/5532253026/jobs/10094278681?pr=3620 But not on MacOS:
https://github.com/turbot/steampipe/actions/runs/5532253026/jobs/10094274027?pr=3620 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdecat requested a couple of (very) small changes. Thank you!
pkg/ociinstaller/ocidownloader.go
Outdated
log.Println("[TRACE] ociDownloader.Pull:", "pulling...") | ||
|
||
copyOpt := oras.DefaultCopyOptions | ||
// TODO: use WithTargetPlatform to limit downloads |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdecat can you please remove this comment if it's not working/required?
pkg/ociinstaller/ocidownloader.go
Outdated
if err != nil { | ||
return &desc, nil, nil, layers, err | ||
log.Println("[ERROR] ociDownloader.Pull:", "failed to pull", ref, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a TRACE
level log. Raw log messages should not end up in the console if we can avoid it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
pkg/ociinstaller/ocidownloader.go
Outdated
// Fetch the config from the file store | ||
configData, err := content.FetchAll(ctx, fileStore, manifest.Config) | ||
if err != nil { | ||
log.Println("[ERROR] ociDownloader.Pull:", "failed to fetch config", manifest.Config.MediaType, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TRACE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
This PR upgrades the oras client library to v2 and adds support for OCI registries requiring authentication (tested with Gitlab).
Resolves #2819