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.

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

enable http registries as ocm repositories #676

Merged
merged 36 commits into from
Apr 23, 2024

Conversation

fabianburth
Copy link
Contributor

@fabianburth fabianburth commented Feb 29, 2024

Description

So far, it was not possible for the ocm tooling to communicate with OCI registries with http, but only with https. The reason therefore was primarily that the scheme was not considered in the parsing and it defaulted to https.
This PR extends the code, especially the parsing rules, to be able to also handle http.

Furthermore, it was not possible to specify localhost in OCI commands (e.g. ocm get artifact).
This PR also extends the code to be able to handle localhost.

Consequently, the following specifications are now possible:
For OCI commands (such as ocm get artifact):
http://example.com/repo:version
http://localhost:8080/repo:version
localhost:8080/repo:version(The port, thus :8080 is required, here. So, localhost/repo:version is not possible, since e.g. localhost/example:1.0.0 is also a valid dockerhub artifact reference and would therefore be ambiguous.)
localhost//repo:version (The scheme, thus http:// can be omitted, if the host is separated from the repo with an unambiguous double slash (//).)
http://localhost:8080//repo:version (A combination of the previous two formats is also possible, although kind of unintuitive.)

For OCM commands (such as ocm transfer):
OCIRegistry::http://localhost:80/test//github.com/example (The type, thus OCIRegistry:: is required)
OCIRegistry::localhost:80/test//github.com/example (Works as well without the scheme, but will default to https)
and the variations with an actual domain
OCIRegistry::http://example.com:80/test//github.com/example
http://example.com:80/test//github.com/example
example.com:80/test//github.com/example (will default to https)
example.com/test//github.com/example (will default to https)
...

Furthermore, since using http might potentially be insecure and should only be used in test scenarios, the user should get a warning. To enable this, a corresponding log message was introduced on log level warn and the general default log level for the cli tool was changed from error to warn.

Short types for repositories that were expected to work (e.g. oci) are now registered with corresponding repository and repository spec handlers.

make generate also checks whether the examples are running. This failed on mac, since the component of the 01-getting-started did not contain an arm64 darwin executable due to a mistake in its build. This is now fixed, so after the next release make generate will be callable on mac again. Until then, one can call e.g. go generate ./cmds/ocm/... manually.

What type of PR is this? (check all applicable)

  • 🍕 Feature
  • 🐛 Bug Fix
  • 📝 Documentation Update
  • 🎨 Style
  • 🧑‍💻 Code Refactor
  • 🔥 Performance Improvements
  • ✅ Test
  • 🤖 Build
  • 🔁 CI
  • 📦 Chore (Release)
  • ⏩ Revert

Added tests?

  • 👍 yes
  • 🙅 no, because they aren't needed
  • 🙋 no, because I need help
  • Separate ticket for tests # (issue/pr)

Added to documentation?

Adjusted the existing ocm references and oci references cli documentations and added additional examples for the
most common use cases.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my 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

@fabianburth fabianburth force-pushed the http-registries branch 2 times, most recently from 95fd337 to 841487b Compare March 1, 2024 09:18
@fabianburth fabianburth self-assigned this Mar 4, 2024
@fabianburth fabianburth added the area/ipcei IPCEI-CIS label Mar 7, 2024
@fabianburth fabianburth marked this pull request as ready for review March 11, 2024 08:45
@fabianburth fabianburth force-pushed the http-registries branch 9 times, most recently from a334ec4 to 71dd942 Compare March 11, 2024 12:52
hilmarf
hilmarf previously approved these changes Mar 18, 2024
Copy link
Member

@hilmarf hilmarf left a comment

Choose a reason for hiding this comment

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

LGTM

@hilmarf hilmarf added this to the 2024-Q1 milestone Mar 18, 2024
@hilmarf hilmarf added the size/l Size of pull request is large (see gardener-robot robot/bots/size.py) label Mar 19, 2024
cmds/ocm/topics/oci/refs/topic.go Outdated Show resolved Hide resolved
cmds/ocm/topics/oci/refs/topic.go Outdated Show resolved Hide resolved
cmds/ocm/topics/ocm/refs/topic.go Show resolved Hide resolved
cmds/ocm/topics/ocm/refs/topic.go Show resolved Hide resolved
pkg/contexts/oci/ref.go Outdated Show resolved Hide resolved
pkg/contexts/ocm/repositories/comparch/type.go Outdated Show resolved Hide resolved
pkg/contexts/ocm/repositories/comparch/uniform.go Outdated Show resolved Hide resolved
pkg/contexts/ocm/repositories/ctf/type.go Outdated Show resolved Hide resolved
pkg/contexts/ocm/repositories/genericocireg/type.go Outdated Show resolved Hide resolved
pkg/contexts/ocm/repositories/ocireg/type.go Outdated Show resolved Hide resolved
@Skarlso
Copy link
Contributor

Skarlso commented Mar 19, 2024

I'm honestly a bit against this. There is a reason for not supporting insecure repositories. ocm-controller is using self-signed certificates to set up testing registries that use HTTPS. It's super easy to create a secure registry for testing.

In the wild you should never EVER use an insecure HTTP registry. So I don't see the point of this change.

@fabianburth
Copy link
Contributor Author

I'm open for discussion. Currently, you will at least get a warning if you use http and if you do not specify a scheme, it will default to https.
But on one hand, there are several people that just want to start a registry locally and fool around a bit to get to know ocm's functionality. On the other hand, I'm not sure whether it really is that easy for all application using ocm to get your application trust the self-signed certificate.

@Skarlso
Copy link
Contributor

Skarlso commented Mar 21, 2024

@fabianburth you're right, the lower the entry for getting started the better the adoption. :)

@Skarlso
Copy link
Contributor

Skarlso commented Mar 21, 2024

On the other hand, I'm not sure whether it really is that easy for all application using ocm to get your application trust the self-signed certificate.

You can use mkcert to generate a self-signed, trusted cert, than when you are launching the registry, just pass the cert in as an environment property and everything should work. :)

@fabianburth
Copy link
Contributor Author

So, I'm not strictly talking about command line usage here. Let's consider integration tests and say you start your application and also your registry in cluster. Now, it might already be kind of a hassle to get your application to trust the certificate, mightn't it?

@Skarlso
Copy link
Contributor

Skarlso commented Mar 21, 2024

Now, it might already be kind of a hassle to get your application to trust the certificate, mightn't it?

Not really, no. In fact, we use an HTTPS registry in our e2e tests here:

https://github.com/open-component-model/ocm-controller/tree/main/e2e

You'll see that we deploy cert-manager, generate a certificate, download it, install in the local store using mkcert, and done. Everything is running with HTTPS.

This is all we needed to do to get things running https://github.com/open-component-model/ocm-controller/blob/main/hack/prime_test_cluster.sh.

We only test using HTTPS.

The same goes for github actions which is our CI.

The oci upload handler performed an path.Join() on the reference, which removes double slashes - also converting e.g. http:// to http:/ and causing the upload to fail.
… ocm repo notations

Before, there was no particular regex for correctly parsing these notations. Therefore, ocm assumed they are specifying ctfs and attempted to interpret them as filepaths.
For ocm commands such as ocm transfer cv <source> <target>, some users confused the notation of the target ocm repository with the notation of components and attempted to use double slashes. If the transfer command was attempted with e.g. OCI::ghcr.io/fabianburth//github.com/mycomponent, the double slash was cleaned up to a single slash and the component was uploaded under ghcr.io/fabianburth/github.com/mycomponent/github.com/mycomponent.
@mandelsoft mandelsoft merged commit bcf0741 into open-component-model:main Apr 23, 2024
12 checks passed
@fabianburth fabianburth deleted the http-registries branch April 24, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ipcei IPCEI-CIS component/ocm-core size/l Size of pull request is large (see gardener-robot robot/bots/size.py)
Projects
Status: 🔒Closed
Development

Successfully merging this pull request may close these issues.

Transfer component archive fails with local oci registry
5 participants