feat: OCI source working with public repositories #190
Conversation
|
I will look into the tests failing, I forgot to check if the tests were still working haha. |
|
Thank you so much for your contribution! Having said that, most of my comments are purely cosmetics. |
pkg/client/repo/oci/oci.go
Outdated
| parseOpts := []name.Option{} | ||
| if r.insecure { | ||
| client = utils.InsecureClient | ||
| parseOpts = append(parseOpts, name.Insecure) | ||
| } |
There was a problem hiding this comment.
I believe you forgot to actually use these options, right? They should be defined before name.ParseReference and be used there.
To prevent this kind of (human) error, we can define an internal method:
func (r *Repo) parseReference(ref string) (*name.Reference, error)However, I believe you missed it because it is not necessary by the time you are parsing the reference, but when you are actually fetching the artifact. Is that right?
|
Hi @wilmardo!! Would you have some time to update this MR? |
Signed-off-by: wilmardo <info@wilmardenouden.nl>
…ng compare Signed-off-by: wilmardo <info@wilmardenouden.nl>
|
@tompizmor @jotadrilo I think I have addressed all the comments, updated the tests and rebased to master. Also 0457468 will superseed #186 Maybe @makkes could test this MR to be sure. I tested it locally like this: ---
source:
repo:
kind: OCI
disableChartsIndex: true
url: https://registry.hub.docker.com/bitnamicharts
target:
repo:
kind: OCI
url: https://localhost:8080 # local test target repo
auth:
username: username
password: password
charts:
- rabbitmq-cluster-operatorRegistry was started like this: Without With Also tested the authentication this way which works, and gives a Unauthorized when removed from the config. |
Signed-off-by: wilmardo <info@wilmardenouden.nl>
Signed-off-by: wilmardo <info@wilmardenouden.nl>
Signed-off-by: wilmardo <info@wilmardenouden.nl>
|
Thank you so much @wilmardo!! I will give it a try as soon as I can! |
|
Thanks @wilmardo. I took the PR for a spin with the following results: The config from #190 (comment) works fine. Great job! This config works fine: ---
source:
repo:
kind: OCI
disableChartsIndex: true
url: https://registry.hub.docker.com/bitnamicharts
target:
repo:
kind: OCI
# docker run -d -p 8080:5000 --restart=always --name registry registry:2
url: http://localhost:5002
charts:
- rabbitmq-cluster-operatorThis config fails: ---
source:
repo:
kind: OCI
disableChartsIndex: true
url: http://localhost:5002
target:
repo:
kind: OCI
url: http://localhost:5003
charts:
- rabbitmq-cluster-operatorThe error message is: |
|
@makkes per the error it looks like a |
|
@wilmardo just one minor thing and it will be ready! |
Signed-off-by: wilmardo <info@wilmardenouden.nl>
|
@makkes Thanks for testing! I overlooked the classic missing/double slash when URL path concatenating addressed in @tompizmor Will address the minor thing and I will retest the insecure just once more. The missing |
Signed-off-by: wilmardo <info@wilmardenouden.nl>
|
As assumed, that isn't needed (at least for our usecase). ready for review again :) |
Signed-off-by: wilmardo <info@wilmardenouden.nl>
Signed-off-by: wilmardo <info@wilmardenouden.nl>
|
Found another little edge case using plus-signs in the chartname edfc60a (also unified the
For example this wasn't working properly: |
| u.Path = path.Join("v2", u.Path, name, "manifests", version) | ||
| req, err := http.NewRequestWithContext(ctx, "GET", u.String(), nil) | ||
| req.Header.Set("Accept", ImageManifestMediaType) | ||
| u.Path = path.Join(u.Path, "/", chartName) |
There was a problem hiding this comment.
This doesn't seem to make sense as path.Join uses forward slashes as separators: "Join joins any number of path elements into a single path, separating them with slashes".
There was a problem hiding this comment.
Yes, this is to ensure a / is prepended before the path to do an easy concat later u.Host + u.Path
Example to illustrate the problem:
url = "https://localhost:8080"
u.Path = path.Join("", "/", "testchartName")
u.Path == "/testchartName"
Without this:
url = "https://localhost:8080"
u.Path = path.Join("", "testchartName")
u.Path == "testchartName"
And that will break the concat, since the slash is missing. For when the slash is already present when the url is set like this https://localhost:8080/``path.Join will normalize the slashes.
The root of this problem is the u.Host + u.Path. I need to get the host + path without the schema. I personally found this more readable then this:
fmt.Sprintf("%s:%s", path.Join(strings.TrimPrefix(r.url.String(), fmt.Sprintf("%s://", r.url.Scheme)), chartName), version)There was a problem hiding this comment.
Thanks by the way for peer reviewing this PR! Very helpful!
Closes #182
Rewrites the OCI
Fetchfunction to use thego-containerregistrypackage instead simple a simple GET that was inFetchAndCache.Since the code is OCI specific I moved it into the
oci.goand left theFetchAndCacheuntouched.This makes repositories like GHCR.io and Docker Hub work since they require an oauth flow which is now done by the package.
Has the the fix of #186 implemented as well (for the
FetchandListChartVersionsfunction, that PR is still relevant for theHas). Most of thego-containerregistrylogic was taken from other functions already using the package.This isn't yet ready for production I think, I need some feedback/input on:
cache.invalidatestill correct? Compared to theFetchAndCacheit replaces (link)General feedback is of course welcome! I am not as fluent in Go as I would like ;)
The
FetchAndCachefuntion is now only used by helm classic and could be moved into there to instead of the shared utils.Tests
Test file:
Current 0.20.1 version:
With this PR: