Fix oci image #1

gsanchezgavier · 2023-03-20T09:14:58Z

Reroller fails to get image digests when are generated by Buildkit (OCI). Some extra details

roobre

Left some suggestions. Overall I'm not super happy with moving things to src, but I abandoned this project first so I won't be pestering about style guides :P

roobre · 2023-03-20T12:26:19Z

src/registry/docker/docker.go

@@ -50,9 +48,18 @@ func DockerLikeImageInfo(baseUrl string) func(image, tag string) ([]string, erro
 			req.Header.Set("Authorization", "Bearer "+token)
 		}

+		digests := []string{}
+
+		retDigests, err := ociDigests(req)


Multiple functions mutating this request can be a bit error prone, I'd be weary of this.

yes is not the best way I agree, TBH I just extended the current code to contemplate this case, didn't want to do a bigger refactor for this knowing that we will replace it soon.

~~oh the request! you are right i didn't pay attention to that , I guess was a way to reuse auth, I will improve it~~ Set replace the previous values , so not ideal but no bug here I think

roobre · 2023-03-20T12:26:54Z

src/registry/docker/docker.go

+			return nil, fmt.Errorf("getting oci image digests from docker: %w", err)
+		}
+
+		digests = append(digests, retDigests...)


Do we need both OCI-formatted digests and v2 manifest digests?

I kept both to avoid breaking changes on the non oci images.

roobre · 2023-03-20T12:28:02Z

src/registry/docker/docker.go

+		// OCI images will not respond with the list of digest
+		return nil, nil


Rather than calling dockerContentDigest unconditionally and muting this error, would it be better to avoid calling dockerContentDigest at all if OCI digests are found?

I'm not completely sure about it (is a bit blurry to me how this API works) so I followed the existing mechanism that was adding digest to the array.

roobre · 2023-03-20T12:28:34Z

src/registry/docker/docker.go

+	if resp.Header.Get("Content-Type") != acceptHeader {
+		return nil, nil
+	}


Seems like this should be an error or at least a log, wouldn't it?

the api return different content-type event if the accept header is set to the image one. this was a quick way to filter this responses, but I didn't want to log error since this is expected and logging this will pollute the logs.

src/registry/docker/docker.go

roobre · 2023-03-20T12:30:03Z

src/registry/docker/docker_test.go

+	require.NoError(t, err)
+	require.Contains(t, digests, "sha256:7300aec653dbe8aaef96b92d2f8319a2cef8e03e99b4420ba846b46a1447ea6b")
+
+	// test buildkit image https://github.com/moby/buildkit/issues/2251


Could we split this to a second test? That way we will know which one is failing.

Co-authored-by: Roberto Santalla <roobre@users.noreply.github.com>

gsanchezgavier · 2023-03-20T13:54:19Z

Left some suggestions. Overall I'm not super happy with moving things to src, but I abandoned this project first so I won't be pestering about style guides :P

oh the reason for that was to have the possibility to do a go run to quickly test without having the image.

kang-makes

The hack Just Works ™️ but as there is a plan to decommission the canaries reroller I would prefer to just have it working that having a perfect solution.

I left a couple of comments that are worrying me a bit tho.

kang-makes · 2023-03-21T13:06:05Z

.github/workflows/release.yml

-      - uses: docker/login-action@v1
-        with:
-          username: roobre
-          password: ${{ secrets.DOCKERHUB_TOKEN }}


Suggested change

- uses: docker/login-action@v1

with:

username: roobre

password: ${{ secrets.DOCKERHUB_TOKEN }}

- uses: docker/login-action@v2

with:

registry: ghcr.io

username: ${{ github.repository_owner }}

password: ${{ secrets.GITHUB_TOKEN }}

kang-makes · 2023-03-21T13:07:27Z

.github/workflows/release.yml

-      - name: Build docker image
-        run: |
-          docker build . -t roobre/reroller:${DOCKER_IMAGE_TAG}
-          docker tag roobre/reroller:${DOCKER_IMAGE_TAG} roobre/reroller:latest


Suggested change

- name: Build docker image

run: |

docker build . -t roobre/reroller:${DOCKER_IMAGE_TAG}

docker tag roobre/reroller:${DOCKER_IMAGE_TAG} roobre/reroller:latest

- name: Build docker image

run: |

docker build . -t ghcr.io/${{ github.repository }}:${DOCKER_IMAGE_TAG}

docker tag ghcr.io/${{ github.repository }}:${DOCKER_IMAGE_TAG} ghcr.io/${{ github.repository }}:latest

kang-makes · 2023-03-21T13:08:17Z

.github/workflows/release.yml

-      - name: Push docker image
-        run: |
-          docker push roobre/reroller:${DOCKER_IMAGE_TAG}
-          docker push roobre/reroller:latest


Suggested change

- name: Push docker image

run: |

docker push roobre/reroller:${DOCKER_IMAGE_TAG}

docker push roobre/reroller:latest

- name: Push docker image

run: |

docker push ghcr.io/${{ github.repository }}:${DOCKER_IMAGE_TAG}

docker push ghcr.io/${{ github.repository }}:latest

kang-makes · 2023-03-21T13:09:38Z

.github/workflows/release.yml

GitHub has an OCI registry per repo. Instead of publishing to Roobre's registry, we can still publish to the GitHub one.

This way you will neither need to upload the image to your account nor we will not need a secret to upload to new relic's registry. We will be able to archive this repository easily too.

oh this is a very good one, I will do that.

kang-makes · 2023-03-21T15:22:08Z

go.mod


-go 1.16
+go 1.20


We are having issues with Go 1.20 and Alpine. We think that it is because we are compiling with CGO_ENABLED=1 but it might be more things failing. I would stick to 1.19 for now.

gsanchezgavier · 2023-03-22T08:22:22Z

I will open another PR with just the changes we finally need. removing the package arrangement that I did to the go run thing

gsanchezgavier added 4 commits March 20, 2023 09:52

support OCI images on docker

54d6d32

update deps

b85c8c3

remove CI pipeline

ea9d2ba

package reorder and rename

dbfc338

gsanchezgavier force-pushed the fix-oci-image branch from 69006e4 to dbfc338 Compare March 20, 2023 10:25

fix dockerfile

5869ab8

gsanchezgavier requested a review from roobre March 20, 2023 10:59

roobre reviewed Mar 20, 2023

View reviewed changes

Apply suggestions from code review

1b71c7b

Co-authored-by: Roberto Santalla <roobre@users.noreply.github.com>

fix from review

2c36dbb

gsanchezgavier requested a review from roobre March 21, 2023 07:55

kang-makes reviewed Mar 21, 2023

View reviewed changes

gsanchezgavier closed this Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix oci image #1

Fix oci image #1

gsanchezgavier commented Mar 20, 2023

roobre left a comment

roobre Mar 20, 2023

gsanchezgavier Mar 20, 2023

gsanchezgavier Mar 21, 2023 •

edited

Loading

roobre Mar 20, 2023

gsanchezgavier Mar 20, 2023

roobre Mar 20, 2023

gsanchezgavier Mar 20, 2023

roobre Mar 20, 2023

gsanchezgavier Mar 20, 2023

roobre Mar 20, 2023

gsanchezgavier commented Mar 20, 2023

kang-makes left a comment

kang-makes Mar 21, 2023

kang-makes Mar 21, 2023

kang-makes Mar 21, 2023

kang-makes Mar 21, 2023

gsanchezgavier Mar 22, 2023

kang-makes Mar 21, 2023

gsanchezgavier commented Mar 22, 2023

		// OCI images will not respond with the list of digest
		return nil, nil

-      - uses: docker/login-action@v1
-        with:
-          username: roobre
-          password: ${{ secrets.DOCKERHUB_TOKEN }}
+      - uses: docker/login-action@v2
+        with:
+          registry: ghcr.io
+          username: ${{ github.repository_owner }}
+          password: ${{ secrets.GITHUB_TOKEN }}

Fix oci image #1

Fix oci image #1

Conversation

gsanchezgavier commented Mar 20, 2023

roobre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsanchezgavier Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsanchezgavier commented Mar 20, 2023

kang-makes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsanchezgavier commented Mar 22, 2023

gsanchezgavier Mar 21, 2023 •

edited

Loading