-
Notifications
You must be signed in to change notification settings - Fork 67
✨unpacker: switch from google/go-containerregistry to containers/image #1194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,7 +91,7 @@ help-extended: #HELP Display extended help. | |
|
||
.PHONY: lint | ||
lint: $(GOLANGCI_LINT) #HELP Run golangci linter. | ||
$(GOLANGCI_LINT) run $(GOLANGCI_LINT_ARGS) | ||
$(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS) | ||
|
||
.PHONY: tidy | ||
tidy: #HELP Update dependencies. | ||
|
@@ -111,15 +111,15 @@ verify: tidy fmt vet generate manifests crd-ref-docs #HELP Verify all generated | |
|
||
.PHONY: fix-lint | ||
fix-lint: $(GOLANGCI_LINT) #EXHELP Fix lint issues | ||
$(GOLANGCI_LINT) run --fix $(GOLANGCI_LINT_ARGS) | ||
$(GOLANGCI_LINT) run --fix --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS) | ||
|
||
.PHONY: fmt | ||
fmt: #EXHELP Formats code | ||
go fmt ./... | ||
|
||
.PHONY: vet | ||
vet: #EXHELP Run go vet against code. | ||
go vet ./... | ||
go vet -tags '$(GO_BUILD_TAGS)' ./... | ||
|
||
.PHONY: bingo-upgrade | ||
bingo-upgrade: $(BINGO) #EXHELP Upgrade tools | ||
|
@@ -155,10 +155,17 @@ UNIT_TEST_DIRS := $(shell go list ./... | grep -v /test/) | |
COVERAGE_UNIT_DIR := $(ROOT_DIR)/coverage/unit | ||
test-unit: $(SETUP_ENVTEST) #HELP Run the unit tests | ||
rm -rf $(COVERAGE_UNIT_DIR) && mkdir -p $(COVERAGE_UNIT_DIR) | ||
eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && CGO_ENABLED=1 go test -count=1 -race -short $(UNIT_TEST_DIRS) -cover -coverprofile ${ROOT_DIR}/coverage/unit.out -test.gocoverdir=$(ROOT_DIR)/coverage/unit | ||
|
||
eval $$($(SETUP_ENVTEST) use -p env $(ENVTEST_VERSION) $(SETUP_ENVTEST_BIN_DIR_OVERRIDE)) && \ | ||
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 | ||
|
||
E2E_REGISTRY_CERT_REF := ClusterIssuer/olmv1-ca # By default, we'll use a trusted CA for the registry. | ||
image-registry: ## Setup in-cluster image registry | ||
./hack/test/image-registry.sh $(E2E_REGISTRY_NAMESPACE) $(E2E_REGISTRY_NAME) | ||
./hack/test/image-registry.sh $(E2E_REGISTRY_NAMESPACE) $(E2E_REGISTRY_NAME) $(E2E_REGISTRY_CERT_REF) | ||
|
||
build-push-e2e-catalog: ## Build the testdata catalog used for e2e tests and push it to the image registry | ||
./hack/test/build-push-e2e-catalog.sh $(E2E_REGISTRY_NAMESPACE) $(LOCAL_REGISTRY_HOST)/$(E2E_TEST_CATALOG_V1) | ||
|
@@ -173,6 +180,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The intention here is to setup our standard e2e test with:
This setup proves that our image puller respect what shows up in The extension developer e2e and the upgrade e2e are left using the standard olmv1-ca to:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the intent is to explicitly have a separate (untrusted) trust chain. The other places the CA were used are:
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) |
||
test-e2e: run image-registry build-push-e2e-catalog registry-load-bundles e2e e2e-coverage kind-clean #HELP Run e2e test suite on local kind cluster | ||
|
||
.PHONY: extension-developer-e2e | ||
|
@@ -243,6 +251,7 @@ export CGO_ENABLED | |
|
||
export GIT_REPO := $(shell go list -m) | ||
export VERSION_PATH := ${GIT_REPO}/internal/version | ||
export GO_BUILD_TAGS := containers_image_openpgp | ||
export GO_BUILD_ASMFLAGS := all=-trimpath=$(PWD) | ||
export GO_BUILD_GCFLAGS := all=-trimpath=$(PWD) | ||
export GO_BUILD_FLAGS := | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ import ( | |
"path/filepath" | ||
"time" | ||
|
||
"github.com/containers/image/v5/types" | ||
"github.com/spf13/pflag" | ||
"go.uber.org/zap/zapcore" | ||
apiextensionsv1client "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/typed/apiextensions/v1" | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loss of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not seeing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, in BuildHTTPClient There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
unpacker := &source.ContainersImageRegistry{ | ||
BaseCachePath: filepath.Join(cachePath, "unpack"), | ||
SourceContext: &types.SystemContext{ | ||
DockerCertPath: caCertDir, | ||
OCICertPath: caCertDir, | ||
}, | ||
} | ||
|
||
clusterExtensionFinalizers := crfinalizer.NewFinalizers() | ||
if err := clusterExtensionFinalizers.Register(controllers.ClusterExtensionCleanupUnpackCacheFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { | ||
return crfinalizer.Result{}, os.RemoveAll(filepath.Join(unpacker.BaseCachePath, obj.GetName())) | ||
return crfinalizer.Result{}, unpacker.Cleanup(ctx, &source.BundleSource{Name: obj.GetName()}) | ||
})); err != nil { | ||
setupLog.Error(err, "unable to register finalizer", "finalizerKey", controllers.ClusterExtensionCleanupUnpackCacheFinalizer) | ||
os.Exit(1) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
apiVersion: kustomize.config.k8s.io/v1alpha1 | ||
kind: Component | ||
namespace: olmv1-system | ||
resources: | ||
- registries_conf_configmap.yaml | ||
patches: | ||
- path: manager_e2e_registries_conf_patch.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
apiVersion: apps/v1 | ||
kind: Deployment | ||
metadata: | ||
name: controller-manager | ||
namespace: system | ||
spec: | ||
template: | ||
spec: | ||
containers: | ||
- name: kube-rbac-proxy | ||
- name: manager | ||
volumeMounts: | ||
- name: e2e-registries-conf | ||
mountPath: /etc/containers | ||
volumes: | ||
- name: e2e-registries-conf | ||
configMap: | ||
name: e2e-registries-conf |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
apiVersion: v1 | ||
kind: ConfigMap | ||
metadata: | ||
name: e2e-registries-conf | ||
namespace: system | ||
data: | ||
registries.conf: | | ||
[[registry]] | ||
prefix = "docker-registry.operator-controller-e2e.svc.cluster.local:5000" | ||
insecure = true | ||
location = "docker-registry.operator-controller-e2e.svc.cluster.local:5000" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if you want to go all out and put each flag on its own line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤷♂️
I was mainly focused on "like flags" and "can I see it without side-to-side scrolling?"
Notes
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)test.govoerdir
after the UNIT_TEST_DIRS because those flags are passed to the test binary.