✨ unpacker: switch from google/go-containerregistry to containers/image #399

joelanford · 2024-09-13T13:57:04Z

See:

I've structured this PR with a series of commits:

Copy containers_image.go directly from operator-controller
Modify it to use catalogd types
The majority of the actual changes necessary to integrate into catalogd and pass tests.

Signed-off-by: Joe Lanford [email protected]

Signed-off-by: Joe Lanford <[email protected]>

codecov · 2024-09-13T14:00:14Z

Codecov Report

Attention: Patch coverage is 58.22222% with 94 lines in your changes missing coverage. Please review.

Project coverage is 38.04%. Comparing base (dbdce9b) to head (7b41ed3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/source/containers_image.go	62.43%	46 Missing and 31 partials ⚠️
cmd/manager/main.go	0.00%	11 Missing ⚠️
...rnal/controllers/core/clustercatalog_controller.go	0.00%	3 Missing ⚠️
internal/storage/localdir.go	25.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   35.55%   38.04%   +2.48%     
==========================================
  Files          17       15       -2     
  Lines         734      786      +52     
==========================================
+ Hits          261      299      +38     
  Misses        443      443              
- Partials       30       44      +14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

joelanford · 2024-09-13T14:01:20Z

config/components/tls/patches/manager_deployment_certs.yaml

- op: add
-  path: /spec/template/spec/containers/1/args/-
-  value: "--ca-certs-dir=/var/certs"


Moved this to config/components/ca/patches/manager_deployment_cacerts.yaml (1) and changed the mount directory to /var/ca-certs (2). Why?

I figured the setup of the CA was a better place to put the volume mount for that CA

containers/image library complained about the original /var/certs containing the catalogd server crt/key, so this change puts just the CA in the /var/ca-certs directory separately.

joelanford · 2024-09-13T14:03:56Z

internal/errors/unrecoverable.go

+func NewUnrecoverable(err error) Unrecoverable {
+	return Unrecoverable{err}


Aligned this function with the similar change in operator-controller. There's no need for this to return a pointer. Returning a pointer here means that errors.As requires a double pointer, which is error prone.

internal/source/containers_image.go

joelanford · 2024-09-13T14:05:55Z

internal/source/containers_image.go

+		if cleanupErr := os.RemoveAll(unpackPath); cleanupErr != nil {
+			err = errors.Join(err, cleanupErr)
+		}
+		_, isDigestBased := imgRef.(reference.Canonical)
+		return nil, wrapUnrecoverable(fmt.Errorf("error unpacking image: %w", err), isDigestBased)


This is something we may want to transfer back over to operator-controller.

joelanford · 2024-09-13T14:08:06Z

internal/source/containers_image.go

+	return err
+}
+
+func applyLayerFilter(srcPath string) archive.Filter {


If/when we consolidate catalogd and operator-controller, it seems like we'll want this filter function to be configurable.

For now, this is updated from operator-controller's implementation to account for catalogd's ability to unpack just the relevant FBC root directory.

joelanford · 2024-09-13T14:09:25Z

internal/source/containers_image.go

+		cleanName := path.Clean(strings.TrimPrefix(h.Name, "/"))
+		relPath, err := filepath.Rel(cleanSrcPath, cleanName)
+		if err != nil {
+			return false, fmt.Errorf("error getting relative path: %w", err)
+		}
+		return relPath != ".." && !strings.HasPrefix(relPath, "../"), nil


I changed the implementation that catalogd had been using because I re-read it and couldn't convince myself if would work in unexpected, but still valid, scenarios.

I added a bunch of unit tests for this function to convince myself that this implementation is solid.

joelanford · 2024-09-13T14:10:47Z

internal/source/containers_image_test.go

-					assert.Contains(t, buf.String(), "found image in filesystem cache")
-					assert.Equal(t, tt.catalog.Status.ResolvedSource.Image.LastUnpacked, rs.ResolvedSource.Image.LastUnpacked)
+					assert.Contains(t, buf.String(), "image already unpacked")
+					assert.Equal(t, rs.ResolvedSource.Image.LastUnpacked.Time, unpackDirStat.ModTime())


Instead of reading status, we now expect the mod time of the directory to match.

joelanford · 2024-09-13T14:11:44Z

internal/source/containers_image_test.go

+					assert.NotEqual(t, rs.ResolvedSource.Image.LastUnpacked.Time, oldDigestModTime)
+					assert.NoDirExists(t, oldDigestDir)


Instead of reading status, we now expect the new unpack time to be different than the mod time of the old digest.

We also expect the old digest dir to no longer exist.

joelanford · 2024-09-13T14:12:01Z

internal/source/containers_image_test.go

 				assert.Error(t, err)
-				var unrecov *catalogderrors.Unrecoverable
-				isUnrecov := errors.As(err, &unrecov)
+				isUnrecov := errors.As(err, &catalogderrors.Unrecoverable{})


Changing this to remove the double-pointer thing.

joelanford · 2024-09-13T14:12:37Z

internal/source/unpacker.go

 	StateUnpacked State = "Unpacked"
 )

-type unpacker struct {


No need for all of this extra abstraction since we no longer have alternate source types.

joelanford · 2024-09-13T14:18:11Z

internal/controllers/core/clustercatalog_controller.go


-	var unrecov *catalogderrors.Unrecoverable
-	if errors.As(reconcileErr, &unrecov) {
+	if errors.As(reconcileErr, &catalogderrors.Unrecoverable{}) {


Changing this to remove the double-pointer thing.

internal/source/containers_image_test.go

joelanford · 2024-09-13T14:59:24Z

/hold

I want to update the e2e in a similar way that I did in operator-controller, where the image registry uses an untrusted cert and we mount an extra /etc/containers/registries.conf to allow an untrusted connection (and therefore verify that configuration is honored)

m1kola

Still need to review few more files. Will try to come back to this on Monday.

Going forward - can we please split large PRs into smaller ones as much as possible? For example build tag changes seem to be fairly independent here. It is easier to review small changes and smaller commits make code archaeology less challenging.

internal/source/containers_image.go

m1kola · 2024-09-13T15:26:22Z

internal/source/containers_image.go

+	//////////////////////////////////////////////////////
+	//
+	// Resolve a canonical reference for the image.
+	//
+	//////////////////////////////////////////////////////


Nit: I'm a fan of comments like this. Most of these sections look like they deserve own function.

I'll take a look to see if these can be function-ified. Only thing that comes to mind is if those functions would end up having to accept a bunch of parameters and return a bunch of values (and if so, maybe that wouldn't be much better?)

But good callout. I'll take a look.

Updated! I made functions for:

resolving the named and canonical refs

loading the signature policy

There isn't really a lot more to squeeze out of this IMO. At this point, it's basically a series of steps with pretty straightforward error handling and basically no nesting, conditionals, or loops.

@everettraven asked for comments and structure along these lines when I was putting operator-controller's PR together because containers/image is a bit less ergonomic than google/go-containerregistry (I agree). So these comments are included to help explain what is actually going on.

I'm happy to reformat if there are other suggestions on style, but I do think the grouping and walkthrough-style narrative is helpful for the uninitiated.

internal/source/containers_image.go

joelanford · 2024-09-13T19:36:32Z

Makefile

    -X "$(VERSION_PKG).gitTreeState=$(GIT_TREE_STATE)" \
    -X "$(VERSION_PKG).commitDate=$(GIT_COMMIT_DATE)"
 export GO_BUILD_GCFLAGS  := all=-trimpath=${PWD}
+export GO_BUILD_TAGS     := containers_image_openpgp


@m1kola To answer your question about build tags: containers/image needs this build tag in order to build without requiring a third-party library that requires CGO.

I'm not sure it would make sense to split that into a separate PR.

@joelanford I was thinking it was possible, for example, to introduce the plumbing for the build tags and add the actual tag in this PR. But that was just an example.

In general just asking (everyone) to be conscious about the PR sizes.

joelanford · 2024-09-13T19:40:50Z

internal/source/containers_image.go

+func setReadOnlyRecursive(root string) error {
+	if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+
+		fi, err := d.Info()
+		if err != nil {
+			return err
+		}
+
+		if err := func() error {
+			switch typ := fi.Mode().Type(); typ {
+			case os.ModeSymlink:
+				// do not follow symlinks
+				// 1. if they resolve to other locations in the root, we'll find them anyway
+				// 2. if they resolve to other locations outside the root, we don't want to change their permissions
+				return nil
+			case os.ModeDir:
+				return os.Chmod(path, 0500)
+			case 0: // regular file
+				return os.Chmod(path, 0400)
+			default:
+				return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
+			}
+		}(); err != nil {
+			return err
+		}
+		return nil
+	}); err != nil {
+		return fmt.Errorf("error making catalog cache read-only: %w", err)
+	}
+	return nil
+}
+
+func deleteRecursive(root string) error {
+	if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if !d.IsDir() {
+			return nil
+		}
+		if err := os.Chmod(path, 0700); err != nil {
+			return err
+		}
+		return nil
+	}); err != nil {
+		return fmt.Errorf("error making catalog cache writable for deletion: %w", err)
+	}
+	return os.RemoveAll(root)
+}


Probably makes sense to port these back over to operator-controller and use them there as well.

m1kola

Just noticed one place where we duplicate if. Other than that - looks good.

I spent a lot of time staring at internal/source/containers_image.go. I think I understand what is going on there and didn't manage to spot any issues. But it might be good to have another pair of eyes on it.

m1kola · 2024-09-16T09:40:42Z

Makefile

    -X "$(VERSION_PKG).gitTreeState=$(GIT_TREE_STATE)" \
    -X "$(VERSION_PKG).commitDate=$(GIT_COMMIT_DATE)"
 export GO_BUILD_GCFLAGS  := all=-trimpath=${PWD}
+export GO_BUILD_TAGS     := containers_image_openpgp


@joelanford I was thinking it was possible, for example, to introduce the plumbing for the build tags and add the actual tag in this PR. But that was just an example.

In general just asking (everyone) to be conscious about the PR sizes.

internal/storage/localdir.go

Signed-off-by: Joe Lanford <[email protected]>

m1kola

Re-approving after unused error check removed.

joelanford added 2 commits September 13, 2024 08:43

copy containers_image.go from operator-controller

ea391e3

Signed-off-by: Joe Lanford <[email protected]>

initial modifications for naming and types

65794f3

Signed-off-by: Joe Lanford <[email protected]>

joelanford requested a review from a team as a code owner September 13, 2024 13:57

joelanford commented Sep 13, 2024

View reviewed changes

internal/source/containers_image.go Show resolved Hide resolved

joelanford commented Sep 13, 2024

View reviewed changes

joelanford force-pushed the containers-image branch from e7c87ae to aef0190 Compare September 13, 2024 14:17

joelanford commented Sep 13, 2024

View reviewed changes

joelanford force-pushed the containers-image branch from aef0190 to 3dd5765 Compare September 13, 2024 14:35

m1kola reviewed Sep 13, 2024

View reviewed changes

internal/source/containers_image_test.go Show resolved Hide resolved

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2024

m1kola reviewed Sep 13, 2024

View reviewed changes

joelanford force-pushed the containers-image branch from 3dd5765 to 58c39a8 Compare September 13, 2024 19:13

joelanford commented Sep 13, 2024

View reviewed changes

joelanford force-pushed the containers-image branch from 58c39a8 to 7af3c76 Compare September 13, 2024 19:39

joelanford commented Sep 13, 2024

View reviewed changes

joelanford force-pushed the containers-image branch 4 times, most recently from 23a6eff to 9878970 Compare September 14, 2024 18:10

joelanford removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 14, 2024

m1kola previously approved these changes Sep 16, 2024

View reviewed changes

unpacker: switch to containers/image-based implementation

7b41ed3

Signed-off-by: Joe Lanford <[email protected]>

joelanford dismissed m1kola’s stale review via 7b41ed3 September 16, 2024 12:15

joelanford force-pushed the containers-image branch from 9878970 to 7b41ed3 Compare September 16, 2024 12:15

joelanford enabled auto-merge September 16, 2024 13:01

m1kola approved these changes Sep 16, 2024

View reviewed changes

joelanford added this pull request to the merge queue Sep 16, 2024

Merged via the queue into operator-framework:main with commit ffdb9c3 Sep 16, 2024

joelanford deleted the containers-image branch September 16, 2024 14:25

m1kola mentioned this pull request Sep 16, 2024

🌱 Bump catalogd to v0.27.0 operator-framework/operator-controller#1267

Closed

4 tasks

		func NewUnrecoverable(err error) Unrecoverable {
		return Unrecoverable{err}

		assert.NotEqual(t, rs.ResolvedSource.Image.LastUnpacked.Time, oldDigestModTime)
		assert.NoDirExists(t, oldDigestDir)

✨ unpacker: switch from google/go-containerregistry to containers/image #399

✨ unpacker: switch from google/go-containerregistry to containers/image #399

Uh oh!

Conversation

joelanford commented Sep 13, 2024

Uh oh!

codecov bot commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joelanford commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

m1kola left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joelanford Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

m1kola left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Sep 13, 2024 •

edited

Loading

joelanford commented Sep 13, 2024 •

edited

Loading

m1kola left a comment •

edited

Loading

joelanford Sep 13, 2024 •

edited

Loading