oci: Use default registry for Inspektor Gadget #2307

mauriciovasquezbernal · 2023-12-18T17:01:43Z

Testing

Before

It's not possible to run a gadget by just specifying their name:

$ sudo -E ig image list
INFO[0000] Experimental features enabled                
REPOSITORY                                  TAG                                         DIGEST 

$ sudo -E ig run trace_exec
INFO[0000] Experimental features enabled                
Error: getting gadget info: getting gadget image: pulling image "trace_exec": creating remote repository: creating remote repository: invalid reference: missing repository

After

Now it's possible

$ sudo -E ig image list
INFO[0000] Experimental features enabled                
REPOSITORY                                  TAG                                         DIGEST   

$ sudo -E ig run trace_exec
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME     PID            PPID           UID           GID           R… COMM

Fixes #2190

mqasimsarfraz

I tested it and works fine. It will simplify running official gadgets.

I have only one question regarding UX with list command. I am wondering if it makes sense to not show the default domain / default repo prefix when list the images. This should be similar to what docker does:

→ docker images
REPOSITORY   TAG       IMAGE ID   CREATED   SIZE
→ docker pull docker.io/library/alpine
Using default tag: latest
latest: Pulling from library/alpine
661ff4d9561e: Pull complete 
Digest: sha256:51b67269f354137895d43f3b3d810bfacd3945438e94dc5ac55fdac340352f48
Status: Downloaded newer image for alpine:latest
docker.io/library/alpine:latest
→ docker images
REPOSITORY   TAG       IMAGE ID       CREATED       SIZE
alpine       latest    f8c20f8bbcb6   5 weeks ago   7.38MB

Also, it will encourage to use names like trace_open for official gadget (after I just listed them using list). What do you think?

pkg/oci/oci.go

mqasimsarfraz · 2024-01-12T12:18:15Z

pkg/oci/oci.go

+// needs to be already validated before.
+func splitIGDomain(name string) (domain, remainder string) {
+	i := strings.IndexRune(name, '/')
+	if i == -1 || (!strings.ContainsAny(name[:i], ".:") && name[:i] != localhost && strings.ToLower(name[:i]) == name[:i]) {


Just for my understanding, for strings.ToLower(name[:i]) == name[:i]) we assume it it a domain name since it contains capital letters?

TBH I took this from https://github.com/distribution/reference/blob/v0.5.0/normalize.go#L126 without studying it in depth. I think your analysis is correct, it assumes it's a domain if there are capital letters, but domains are supported to be lower case, aren't they?

It was introduced in distribution/reference@ca2a562, the tests cases added parse the domain with upper case, not sure what this is for and the commit doesn't contain anymore information.

domains are supported to be lower case, aren't they?

IIUC, domains are case insensitive, https://serverfault.com/a/261344

It was introduced in distribution/reference@ ca2a562, the tests cases added parse the domain with upper case, not sure what this is for and the commit doesn't contain anymore
information.

I found more information here, distribution/distribution#2979 and it gives more context why they did it.

burak-ok

Apart from the naming this looks good. I also added a unit test for the split function since it seems important :D
90b0934

pkg/oci/oci.go

mauriciovasquezbernal

What do you think?

I agree. Updated the PR to trim the prefix when listing images.

pkg/oci/oci.go

mauriciovasquezbernal · 2024-01-12T14:44:52Z

pkg/oci/oci.go

+// needs to be already validated before.
+func splitIGDomain(name string) (domain, remainder string) {
+	i := strings.IndexRune(name, '/')
+	if i == -1 || (!strings.ContainsAny(name[:i], ".:") && name[:i] != localhost && strings.ToLower(name[:i]) == name[:i]) {


TBH I took this from https://github.com/distribution/reference/blob/v0.5.0/normalize.go#L126 without studying it in depth. I think your analysis is correct, it assumes it's a domain if there are capital letters, but domains are supported to be lower case, aren't they?

It was introduced in distribution/reference@ca2a562, the tests cases added parse the domain with upper case, not sure what this is for and the commit doesn't contain anymore information.

mqasimsarfraz · 2024-01-12T19:16:52Z

pkg/oci/oci.go

+			if named, ok := parsed.(reference.Named); ok {
+				repository = named.Name()
+			}
+			repository = strings.TrimPrefix(repository, defaultDomain+"/"+officialRepoPrefix)


nit: Perhaps we can also trim the defaultDomain for cases like ghcr.io/foo/bar to be foo/bar

I prefer to trim it only when the full thing matches. I'm don't think we should be treating any other think than "ghcr.io/inspektor-gadget/gadgets" in a special way.

mqasimsarfraz

LGTM! I tested it and it works as described in the PR description.

- Don't export it as there is not any reason to export this function as users interact through other functions that don't require it. - Make it receive a "image reference.Named" to avoid calling getRepositoryFromImage that is somehow duplicated. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

Move logic of getTagFromImage() and getRepositoryFromImage() directly to ListGadgetImages. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

Signed-off-by: Burak Ok <burakok@microsoft.com>

mauriciovasquezbernal requested a review from alban as a code owner December 18, 2023 17:01

mauriciovasquezbernal force-pushed the mauricio/default-registry branch 3 times, most recently from 87f8289 to 8b0a502 Compare December 18, 2023 18:48

mauriciovasquezbernal force-pushed the mauricio/default-registry branch from 8b0a502 to 584de27 Compare January 11, 2024 16:41

mqasimsarfraz reviewed Jan 12, 2024

View reviewed changes

burak-ok reviewed Jan 12, 2024

View reviewed changes

pkg/oci/oci.go Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/default-registry branch from 584de27 to b46259a Compare January 12, 2024 15:02

mauriciovasquezbernal commented Jan 12, 2024

View reviewed changes

mqasimsarfraz reviewed Jan 12, 2024

View reviewed changes

mqasimsarfraz approved these changes Jan 12, 2024

View reviewed changes

mauriciovasquezbernal and others added 3 commits January 15, 2024 13:28

oci: Remove uneeded functions

6974598

Move logic of getTagFromImage() and getRepositoryFromImage() directly to ListGadgetImages. Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>

pkg/oci: Add test for splitIGDomain

5301680

Signed-off-by: Burak Ok <burakok@microsoft.com>

mauriciovasquezbernal force-pushed the mauricio/default-registry branch from b46259a to 5301680 Compare January 15, 2024 18:28

mauriciovasquezbernal merged commit 36ffbe9 into main Jan 15, 2024
53 checks passed

mauriciovasquezbernal deleted the mauricio/default-registry branch January 15, 2024 18:54

mqasimsarfraz mentioned this pull request Jan 23, 2024

pkg/oci: Do not trim repository prefix if default. #2401

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oci: Use default registry for Inspektor Gadget #2307

oci: Use default registry for Inspektor Gadget #2307

mauriciovasquezbernal commented Dec 18, 2023

mqasimsarfraz left a comment

mqasimsarfraz Jan 12, 2024

mauriciovasquezbernal Jan 12, 2024

mqasimsarfraz Jan 12, 2024

burak-ok left a comment

mauriciovasquezbernal left a comment

mauriciovasquezbernal Jan 12, 2024

mqasimsarfraz Jan 12, 2024 •

edited

mauriciovasquezbernal Jan 15, 2024

mqasimsarfraz left a comment

oci: Use default registry for Inspektor Gadget #2307

oci: Use default registry for Inspektor Gadget #2307

Conversation

mauriciovasquezbernal commented Dec 18, 2023

Testing

Before

After

mqasimsarfraz left a comment

Choose a reason for hiding this comment

mqasimsarfraz Jan 12, 2024

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 12, 2024

Choose a reason for hiding this comment

mqasimsarfraz Jan 12, 2024

Choose a reason for hiding this comment

burak-ok left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 12, 2024

Choose a reason for hiding this comment

mqasimsarfraz Jan 12, 2024 • edited

Choose a reason for hiding this comment

mauriciovasquezbernal Jan 15, 2024

Choose a reason for hiding this comment

mqasimsarfraz left a comment

Choose a reason for hiding this comment

mqasimsarfraz Jan 12, 2024 •

edited