Skip to content
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

Jump to bottom

feat: add oci pull capabilities #5075

Merged
merged 8 commits into from
Oct 9, 2024
Merged

feat: add oci pull capabilities #5075

merged 8 commits into from
Oct 9, 2024

Conversation

ricardomaraschini
Copy link
Contributor

@ricardomaraschini ricardomaraschini commented Oct 3, 2024 •
edited
Loading

Description

implement logic to download artifacts from a registry using oras. these are not yet in use but will soon be. this is part of the implementation for this adr.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@ricardomaraschini ricardomaraschini force-pushed the adds-download-from-oras-function branch 2 times, most recently from dabdb5a to b6c3686 Compare October 3, 2024 18:46
@ricardomaraschini ricardomaraschini marked this pull request as ready for review October 3, 2024 18:55
@ricardomaraschini ricardomaraschini requested a review from a team as a code owner October 3, 2024 18:55
@ricardomaraschini
feat: add oras pull capabilities
b5e2955 
implement logic to download artifacts from a registry using oras. these
are not yet in use but will soon be.

Signed-off-by: Ricardo Maraschini <[email protected]>
laverya
laverya reviewed Oct 4, 2024
View reviewed changes
internal/oras/oras.go Outdated Show resolved Hide resolved
@ricardomaraschini @laverya
Update internal/oras/oras.go
b0fb0e8 
Co-authored-by: Andrew Lavery <[email protected]>
Signed-off-by: Ricardo Maraschini <[email protected]>
twz123
twz123 reviewed Oct 8, 2024
View reviewed changes
internal/oras/oras.go Outdated Show resolved Hide resolved
internal/oras/oras.go Outdated Show resolved Hide resolved
internal/oras/oras.go Outdated Show resolved Hide resolved
internal/oras/oras.go Outdated Show resolved Hide resolved
twz123
twz123 reviewed Oct 8, 2024
View reviewed changes
Copy link
Member

@twz123 twz123 left a comment

Choose a reason for hiding this comment

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

Some nits in the unit test.

internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
internal/oras/oras_test.go Outdated Show resolved Hide resolved
@ricardomaraschini
chore: rename package from oras to oci
efe0794 
rename package from oras to oci.

Signed-off-by: Ricardo Maraschini <[email protected]>
@ricardomaraschini
chore: rename option structs
2b0b34a 
using DownloadOption and downloadOptions instead.

Signed-off-by: Ricardo Maraschini <[email protected]>
@ricardomaraschini
chore: address some pr comments
5ecc030 
makes the test code better.

Signed-off-by: Ricardo Maraschini <[email protected]>
@ricardomaraschini
chore: remove dangling references to oras
f87788d 
remove some references to oras from some code comments.

Signed-off-by: Ricardo Maraschini <[email protected]>
@ricardomaraschini ricardomaraschini changed the title feat: add oras pull capabilities feat: add oci pull capabilities Oct 8, 2024
@twz123
Copy link
Member

twz123 commented Oct 9, 2024

Not directly related to this PR, but something that came to my mind while reviewing this: Do we want to support plain HTTP OCI registries? We didn't specify this in the ADR, but it seems like a reasonable thing to do. Especially given that OCI artifacts are content-addressed, specifying artifacts by their hash is probably secure enough, even when not using TLS. Moreover, we're not disallowing plain HTTP downloads either.

The main question is: How to switch? Do we want to have our own custom scheme in the URL, à la oci+http or sth. like that?

twz123
twz123 previously approved these changes Oct 9, 2024
View reviewed changes
internal/oci/oci_test.go Outdated Show resolved Hide resolved
@twz123
Copy link
Member

twz123 commented Oct 9, 2024

Are multi-arch artifacts a thing? If yes, we should probably add some test cases for those.

@ricardomaraschini
feat: stop downloading artifact into a temporary location
efc3f72 
this commit changes the oci downloader in some aspects:

- stops downloading the artifact into a temporary location.
- does not fail if multiple artifacts are present.
- allow users to specify the artifact they desire to download.
- if multiple artifacts are present and no artifact name is provided
  downloads the first one.

this commit also adds a test to verify the new behavior.

Signed-off-by: Ricardo Maraschini <[email protected]>
@ricardomaraschini
Copy link
Contributor Author

ricardomaraschini commented Oct 9, 2024 •
edited
Loading

Are multi-arch artifacts a thing? If yes, we should probably add some test cases for those.

Apparently yes, do we want to support them immediately or this is something we can implement as a follow up PR ? It involves ManifestList / ImageIndex IIUC and implementing THAT is going go get nasty real quick.

@ricardomaraschini
feat: add support for oci download using plain http
99991a5 
allows pulling oci artifacts from plain http endpoints.

Signed-off-by: Ricardo Maraschini <[email protected]>
@ricardomaraschini
Copy link
Contributor Author

Not directly related to this PR, but something that came to my mind while reviewing this: Do we want to support plain HTTP OCI registries? We didn't specify this in the ADR, but it seems like a reasonable thing to do. Especially given that OCI artifacts are content-addressed, specifying artifacts by their hash is probably secure enough, even when not using TLS. Moreover, we're not disallowing plain HTTP downloads either.

The main question is: How to switch? Do we want to have our own custom scheme in the URL, à la oci+http or sth. like that?

Implemented here, please take a look.

@twz123
Copy link
Member

twz123 commented Oct 9, 2024

or this is something we can implement as a follow up PR ?

Sure, I was already okay with the previous version of this PR. Letz move on.

@ricardomaraschini ricardomaraschini merged commit e01d584 into main Oct 9, 2024
89 checks passed
@ricardomaraschini ricardomaraschini deleted the adds-download-from-oras-function branch October 9, 2024 17:52
@twz123 twz123 mentioned this pull request Oct 10, 2024
Merged
16 tasks
twz123
twz123 reviewed Oct 11, 2024
View reviewed changes
internal/oci/oci.go
Comment on lines +123 to +138
func findArtifactDescriptor(all []ocispec.Descriptor, opts downloadOptions) (ocispec.Descriptor, error) {
for _, desc := range all {
if desc.MediaType == ocispec.MediaTypeEmptyJSON {
continue
}
// if no artifact name is specified, we use the first one.
fname := opts.artifactName
if fname == "" || fname == desc.Annotations[ocispec.AnnotationTitle] {
return desc, nil
}
}
if opts.artifactName == "" {
return ocispec.Descriptor{}, fmt.Errorf("no artifacts found")
}
return ocispec.Descriptor{}, fmt.Errorf("artifact %q not found", opts.artifactName)
}
Copy link
Member

@twz123 twz123 Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When having a look at our implementation, I realize that the ADR just says: "We'll download via OCI", but not at all what's meant by this. As far as I understand, there's no common understanding of what it means to download an artifact via OCI, i.e. it is very much implementation dependent.

I think we should augment the ADR with a spec what k0s means/expects when it tries to download files from OCI registries. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, this is what we mention in the ADR:

Implement support in Autopilot for pulling artifacts, such as k0s binaries and image bundles

So we at least know what we expect to find in the registry. We should at least write down what we expect when it comes to file names and formats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twz123 twz123 twz123 approved these changes

@laverya laverya laverya left review comments

@ncopa ncopa Awaiting requested review from ncopa ncopa is a code owner automatically assigned from k0sproject/k0s-maintainers

@makhov makhov Awaiting requested review from makhov makhov is a code owner automatically assigned from k0sproject/k0s-maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ricardomaraschini @twz123 @laverya