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

joelanford · 2024-08-30T03:08:19Z

Description

See https://docs.google.com/document/d/1s-nfpJqecWGuriFIVXr2REbZLUB8N8QxLp1MQKLPhlE/edit#heading=h.x3tfh25grvnv

This tilt-support PR is required to make the tilt test pass: Tiltfile: accept optional build tags tilt-support#9

Reviewer Checklist

API Go Documentation
Tests: Unit Tests (and E2E Tests, if appropriate)
Comprehensive Commit Messages
Links to related GitHub Issue(s)

netlify · 2024-08-30T03:08:34Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`41f0aaf`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/66e0e49a75d7440008172f95
😎 Deploy Preview	https://deploy-preview-1194--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-08-30T03:16:33Z

Codecov Report

Attention: Patch coverage is 74.64789% with 36 lines in your changes missing coverage. Please review.

Project coverage is 76.82%. Comparing base (d09f325) to head (41f0aaf).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/rukpak/source/containers_image.go	72.30%	19 Missing and 17 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1194      +/-   ##
==========================================
+ Coverage   76.49%   76.82%   +0.32%     
==========================================
  Files          40       40              
  Lines        2336     2369      +33     
==========================================
+ Hits         1787     1820      +33     
+ Misses        392      383       -9     
- Partials      157      166       +9

Flag	Coverage Δ
e2e	`58.21% <57.74%> (+0.63%)`	⬆️
unit	`52.93% <67.60%> (+0.53%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

internal/controllers/clusterextension_controller.go

internal/rukpak/source/containers_image.go

internal/rukpak/source/containers_image_test.go

tmshort · 2024-09-05T19:47:40Z

cmd/manager/main.go

@@ -194,14 +195,18 @@ func main() {
 		setupLog.Error(err, "unable to create CA certificate pool")
 		os.Exit(1)
 	}
-	unpacker := &source.ImageRegistry{
-		BaseCachePath:   filepath.Join(cachePath, "unpack"),
-		CertPoolWatcher: certPoolWatcher,


Loss of CertPoolWatcher: certPoolWatcher here...

Not seeing ContainersImageRegistry use certificate, so is the certPoolWatcher even necessary?
And if that's the case, this could lead to more code being deleted... but presumably it's used somewhere?

Yes, in BuildHTTPClient

Yeah, the containers/image source implementation lets us set the CA files on a context struct, which I think are reloaded on every pull.

And yeah, the certPoolWatcher should still be used to connect to catalogd.

internal/rukpak/source/containers_image.go

joelanford · 2024-09-06T19:11:50Z

cmd/manager/main.go

+	defaultAuthConfigPath := filepath.Join("/etc", "containers", "auth.json")
+	if _, err := os.Stat(defaultAuthConfigPath); err == nil {
+		unpacker.SourceContext.AuthFilePath = defaultAuthConfigPath


This doesn't seem to be a default for containers/image, interestingly. Researching this a bit more.

Primary read/write file for auth seems to be ${XDG_RUNTIME_DIR}/containers/auth.json or $HOME/.config/containers/auth.json: https://github.com/containers/image/blob/main/docs/containers-auth.json.5.md#description

Looking at this, we should probably revisit the RFC and highlight the default location for all the configs we may care about

So it turns out that file location is just not a thing (yet at least): containers/image#1746

We could follow the pattern that podman and skopeo do. If REGISTRY_AUTH_FILE is set in the environment, we set its value on this field. I'll switch the PR to do that for now.

I took all of the auth stuff back out of the PR for now. We should discuss and handle separately.

This PR is now focused on containers/image and use of /etc/containers system configuration (as an unsupported/subject-to-change internal API)

tmshort

A few comments. Mostly surrounding the need for a new Issuer?

tmshort · 2024-09-10T13:49:25Z

Makefile

+
+

Nit: extra blank line.

tmshort · 2024-09-10T13:52:08Z

Makefile

+            CGO_ENABLED=1 go test \
+                -tags '$(GO_BUILD_TAGS)' \
+                -cover -coverprofile ${ROOT_DIR}/coverage/unit.out \
+                -count=1 -race -short \
+                $(UNIT_TEST_DIRS) \
+                -test.gocoverdir=$(ROOT_DIR)/coverage/unit


Wondering if you want to go all out and put each flag on its own line?

🤷‍♂️

I was mainly focused on "like flags" and "can I see it without side-to-side scrolling?"

Notes

I wanted UNIT_TEST_DIRS as close to the end as possible because I sometimes find myself copying/pasting the command line from a full run so that I can run unit tests in a specific package (and yes, I know we could make UNIT_TEST_DIRS overrideable, but in other projects that do that I always forget what the var name is, so I end up copy/pasting anyway)

I was forced to put test.govoerdir after the UNIT_TEST_DIRS because those flags are passed to the test binary.

tmshort · 2024-09-10T13:56:30Z

Makefile

@@ -173,6 +182,7 @@ build-push-e2e-catalog: ## Build the testdata catalog used for e2e tests and pus
 test-e2e: KIND_CLUSTER_NAME := operator-controller-e2e
 test-e2e: KUSTOMIZE_BUILD_DIR := config/overlays/e2e
 test-e2e: GO_BUILD_FLAGS := -cover
+test-e2e: E2E_REGISTRY_CERT_REF := Issuer/selfsigned-issuer


Should this be ClusterIssuer/olmv1-ca? Did you really want to create another CA/Issuer? Note that we already have a self-signed-issuer in the cert-manager namespace.

The intention here is to setup our standard e2e test with:

an image registry the serves with an untrusted certificate

a mounted /etc/containers/registries.conf file that instructs operator-controller to allow insecure connections to the image registry.

This setup proves that our image puller respect what shows up in /etc/containers/registries.conf (if the registries.conf volume mount is removed, the tests fail)

The extension developer e2e and the upgrade e2e are left using the standard olmv1-ca to:

Verify that op-con works without a mounted /etc/containers/registry.conf

Avoid issues with pre-upgrade setup which depends on using the unmodified manifest of the previous release (meaning we can't mount /etc/containers to allow op-con to trust an insecure registry)

I honestly don't love the current setup. It seems overly complex and brittle, and I do acknowledge that this PR makes it ever so slightly more complex and brittle.

I'd be in favor of looking at our e2e test code and fixtures holistically in order to do a bit of cleanup and refactoring, but IMO, in a separate PR.

As long as the intent was to have a separate trust chain for the registries, then ok. The current CAs are used for communication catalogd <-> operator-controller, and api-server -> catalogd (for webhooks)

Yes, the intent is to explicitly have a separate (untrusted) trust chain. The other places the CA were used are:

catalogd -> e2e image registry

operator-controller -> e2e image registry

Prior to this PR, the same exact image registry setup was used for every e2e (e2e, upgrade-e2e, and developer-extension-e2e).

With this PR, the vanilla e2e is explicitly changed to use the untrusted self-signed cert (while the other e2e variants remain unchanged)

tmshort · 2024-09-10T13:57:32Z

hack/test/image-registry.sh

+---
+ apiVersion: cert-manager.io/v1
+ kind: Issuer
+ metadata:
+   name: selfsigned-issuer
+   namespace: ${namespace}
+ spec:
+   selfSigned: {}


Not sure why you want another issuer (a few comments on this)

See #1194 (comment)

tmshort · 2024-09-10T14:04:15Z

internal/rukpak/source/unpacker.go

+type Unrecoverable struct {
+	error
+}


Is there an explicit check for this type of error?

Hmm. It appears that we do not check for this error type in our reconciler. I brought this over from the now-deleted image_registry.go where it was used.

I do check for it in unit tests.

WDYT about addressing this in a follow-up?

Depends on how many other changes will occur if you update... but at least it's isolated.

config/components/registries-conf/registries_conf_configmap.yaml

internal/rukpak/source/containers_image_test.go

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

tmshort

/lgtm

LalatenduMohanty · 2024-09-12T16:41:29Z

/hold Adding a hold as @everettraven and me can use few hours to review and test the PR . Will remove the hold before EOD today.

LalatenduMohanty

/lgtm

LalatenduMohanty · 2024-09-12T19:04:00Z

/hold cancel

joelanford requested a review from a team as a code owner August 30, 2024 03:08

joelanford force-pushed the containers-image branch 3 times, most recently from 8222c11 to 6558ab3 Compare August 30, 2024 14:11

joelanford commented Aug 30, 2024

View reviewed changes

internal/controllers/clusterextension_controller.go Outdated Show resolved Hide resolved

joelanford force-pushed the containers-image branch 7 times, most recently from 295179a to 7c8a6e0 Compare September 3, 2024 14:07

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2024

joelanford force-pushed the containers-image branch from 7c8a6e0 to 56d6266 Compare September 3, 2024 14:11

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2024

joelanford force-pushed the containers-image branch 3 times, most recently from 0145e41 to df384cd Compare September 4, 2024 12:13

joelanford mentioned this pull request Sep 4, 2024

⚠ Remove TLSVerify/PullSecret fields operator-framework/catalogd#369

Merged

everettraven reviewed Sep 4, 2024

View reviewed changes

internal/rukpak/source/containers_image.go Outdated Show resolved Hide resolved

LalatenduMohanty self-requested a review September 4, 2024 14:16

everettraven reviewed Sep 4, 2024

View reviewed changes

internal/rukpak/source/containers_image_test.go Show resolved Hide resolved

everettraven reviewed Sep 4, 2024

View reviewed changes

internal/rukpak/source/containers_image_test.go Show resolved Hide resolved

joelanford force-pushed the containers-image branch from df384cd to f21e3e5 Compare September 4, 2024 17:56

everettraven previously approved these changes Sep 4, 2024

View reviewed changes

tmshort reviewed Sep 5, 2024

View reviewed changes

joelanford force-pushed the containers-image branch from f21e3e5 to 19b26b5 Compare September 5, 2024 20:43

thetechnick reviewed Sep 6, 2024

View reviewed changes

internal/rukpak/source/containers_image.go Show resolved Hide resolved

joelanford dismissed everettraven’s stale review via d3d9b39 September 6, 2024 19:08

joelanford commented Sep 6, 2024

View reviewed changes

joelanford force-pushed the containers-image branch 7 times, most recently from ee7fe25 to 1506cea Compare September 10, 2024 11:59

tmshort reviewed Sep 10, 2024

View reviewed changes

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

33af250

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford force-pushed the containers-image branch from 1506cea to 3d14a53 Compare September 10, 2024 19:42

use insecure registry to exercise /etc/containers in e2e

41f0aaf

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>

joelanford force-pushed the containers-image branch from 3d14a53 to 41f0aaf Compare September 11, 2024 00:30

tmshort approved these changes Sep 12, 2024

View reviewed changes

openshift-ci bot assigned tmshort Sep 12, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2024

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 12, 2024

everettraven approved these changes Sep 12, 2024

View reviewed changes

LalatenduMohanty approved these changes Sep 12, 2024

View reviewed changes

openshift-ci bot assigned LalatenduMohanty Sep 12, 2024

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

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

Merged via the queue into operator-framework:main with commit 097d897 Sep 12, 2024
17 of 18 checks passed

joelanford deleted the containers-image branch September 12, 2024 19:22

joelanford mentioned this pull request Sep 13, 2024

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

Merged

skattoju mentioned this pull request Sep 25, 2024

✨ Remove unpacked status #1309

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joelanford commented Aug 30, 2024 •

edited

Loading

netlify bot commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

tmshort Sep 5, 2024

tmshort Sep 5, 2024 •

edited

Loading

tmshort Sep 5, 2024

joelanford Sep 5, 2024

joelanford Sep 6, 2024

everettraven Sep 6, 2024

everettraven Sep 6, 2024

joelanford Sep 7, 2024

joelanford Sep 7, 2024

joelanford Sep 9, 2024

tmshort left a comment

tmshort Sep 10, 2024

tmshort Sep 10, 2024

joelanford Sep 10, 2024

tmshort Sep 10, 2024 •

edited

Loading

joelanford Sep 10, 2024

joelanford Sep 10, 2024

tmshort Sep 10, 2024

joelanford Sep 10, 2024

tmshort Sep 10, 2024

joelanford Sep 10, 2024

tmshort Sep 10, 2024

joelanford Sep 10, 2024

tmshort Sep 10, 2024

tmshort left a comment

LalatenduMohanty commented Sep 12, 2024 •

edited

Loading

LalatenduMohanty left a comment

LalatenduMohanty commented Sep 12, 2024

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

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

Conversation

joelanford commented Aug 30, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Aug 30, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Aug 30, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tmshort Sep 5, 2024 • 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

Choose a reason for hiding this comment

tmshort 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

tmshort Sep 10, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort left a comment

Choose a reason for hiding this comment

LalatenduMohanty commented Sep 12, 2024 • edited Loading

LalatenduMohanty left a comment

Choose a reason for hiding this comment

LalatenduMohanty commented Sep 12, 2024

joelanford commented Aug 30, 2024 •

edited

Loading

netlify bot commented Aug 30, 2024 •

edited

Loading

codecov bot commented Aug 30, 2024 •

edited

Loading

tmshort Sep 5, 2024 •

edited

Loading

tmshort Sep 10, 2024 •

edited

Loading

LalatenduMohanty commented Sep 12, 2024 •

edited

Loading