✨ Ensure docker registry CA is trusted in e2e tests #377

thetechnick · 2024-09-06T11:53:45Z

No description provided.

codecov · 2024-09-06T11:55:19Z

Codecov Report

Attention: Patch coverage is 44.44444% with 25 lines in your changes missing coverage. Please review.

Project coverage is 34.68%. Comparing base (52246e1) to head (c2dd89f).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/httputil/certutil.go	62.96%	5 Missing and 5 partials ⚠️
cmd/manager/main.go	0.00%	7 Missing ⚠️
internal/source/image_registry_client.go	33.33%	4 Missing and 2 partials ⚠️
internal/source/unpacker.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #377      +/-   ##
==========================================
+ Coverage   33.95%   34.68%   +0.72%     
==========================================
  Files          16       17       +1     
  Lines         698      741      +43     
==========================================
+ Hits          237      257      +20     
- Misses        435      451      +16     
- Partials       26       33       +7

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

joelanford · 2024-09-06T12:20:03Z

cmd/manager/main.go

 	return string(namespace)
 }
+
+func newCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) {


Not sure how much we care, since I assume this will be temporary waiting for the containers/image implementation to come in, but in operator-controller, @tmshort ended up implementing a cert pool watcher and a helper that uses it to build an http client (on the assumption that our CA might be rotated out from under us)

https://github.com/operator-framework/operator-controller/blob/main/internal/httputil/certpoolwatcher.go

https://github.com/operator-framework/operator-controller/blob/c4470cc02d4871975d25747fee6fdd9467a3af6d/internal/httputil/httputil.go#L9

Yes, I've seen that.
But I feel bad to copy paste that much code from operator-controller.

I would prefer to put the containers/image and cert pool stuff into some form of shared lib, before adding it to catalogd. That's why this implementation is super minimal right now.

seems reasonable. Would it be good to add a tracking issue in a comment, so we don't forget =D

Some of this already seems to be copy/paste anyways?

It's the minimum amount of c/p I could get away with.
Already opened a ticket to refactor (Ideally after combining repos):
https://issues.redhat.com/browse/OPRUN-3535

thetechnick · 2024-09-09T10:45:58Z

config/base/manager/manager.yaml

    matchLabels:
      control-plane: catalogd-controller-manager
  replicas: 1
+  minReadySeconds: 5


Added this, because of frequent timing issues with webhooks coming online:

Error from server (InternalError): error when creating "./config/base/default/clustercatalogs/default-catalogs.yaml": Internal error occurred: failed calling webhook "inject-metadata-name.olm.operatorframework.io": failed to call webhook: Post "https://catalogd-webhook-service.olmv1-system.svc:443/mutate-olm-operatorframework-io-v1alpha1-clustercatalog?timeout=10s": context deadline exceeded

https://github.com/operator-framework/catalogd/actions/runs/10769410253/job/29860558691?pr=377

That does seem like a similar problem...

bentito · 2024-09-09T13:20:16Z

Makefile

 test-upgrade-e2e: export TEST_CLUSTER_CATALOG_NAME := test-catalog
 test-upgrade-e2e: export TEST_CLUSTER_CATALOG_IMAGE := docker-registry.catalogd-e2e.svc:5000/test-catalog:e2e
-test-upgrade-e2e: kind-cluster cert-manager build-container kind-load image-registry run-latest-release pre-upgrade-setup only-deploy-manifest wait post-upgrade-checks kind-cluster-cleanup ## Run upgrade e2e tests on a local kind cluster
+test-upgrade-e2e: kind-cluster cert-manager build-container kind-load run-latest-release image-registry pre-upgrade-setup only-deploy-manifest wait post-upgrade-checks kind-cluster-cleanup ## Run upgrade e2e tests on a local kind cluster


nit: reordering the targets just seems like churn?

The image-registry now depends on the OLM-ca Issuer being created before.
Otherwise the image-registry cert can't be created, which means the secret is missing, which means the image-registry Deployment never gets ready and the installation timeouts while waiting:

see:

error: timed out waiting for the condition on deployments/docker-registry
make: *** [Makefile:102: image-registry] Error 1
https://github.com/operator-framework/catalogd/actions/runs/10768994795/job/29859251956

ah, maybe we could use a comment about these target ordering needs then?

bentito · 2024-09-09T14:22:16Z

cmd/manager/main_test.go

+	require.NoError(t, err)
+}
+
+func Test_newCertPool_empty(t *testing.T) {


Maybe could use a test for when there is an invalid certificate file?

I've opened a ticket to switch to use the operator-controller implementation of this functionality ASAP:
https://issues.redhat.com/browse/OPRUN-3535
In light of this I would like to keep the effort on this PR low, but I can add additional test cases if you think it's necessary.

ah got it, sounds good to do less here then

bentito · 2024-09-09T14:23:55Z

test/tools/imageregistry/imgreg.yaml

  issuerRef:
-    name: selfsigned-issuer
-    kind: Issuer
+    name: olmv1-ca


joelanford · 2024-09-11T18:16:32Z

cmd/manager/main_test.go

nit: move newCertPool to an internal package so we don't need a main_test.go?

moved into httputil package, similar to /operator-controller

joelanford · 2024-09-11T18:19:00Z

internal/source/image_registry_client.go

+	if i.CertPool != nil {
+		tlsTransport.TLSClientConfig.RootCAs = i.CertPool
+	}
+	remoteOpts = append(remoteOpts, remote.WithTransport(tlsTransport))


Further down in line 91 we again call remote.WithTransport, but with a different transport. Maybe some slight refactoring is in order to build up a transport and then call the method just once at the end?

Updated to use if/else to add remote.WithTransport.

perdasilva

Nice one, Nico! Thank you ^^

perdasilva · 2024-09-09T12:22:29Z

cmd/manager/main.go

 	return string(namespace)
 }
+
+func newCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) {


seems reasonable. Would it be good to add a tracking issue in a comment, so we don't forget =D

thetechnick requested a review from a team as a code owner September 6, 2024 11:53

thetechnick mentioned this pull request Sep 6, 2024

⚠ Remove TLSVerify/PullSecret fields #369

Merged

joelanford reviewed Sep 6, 2024

View reviewed changes

thetechnick force-pushed the e2e-tls branch 3 times, most recently from d8af8a7 to dfcf05a Compare September 9, 2024 08:24

thetechnick commented Sep 9, 2024

View reviewed changes

bentito reviewed Sep 9, 2024

View reviewed changes

test/tools/imageregistry/imgreg.yaml

issuerRef:

name: selfsigned-issuer

kind: Issuer

name: olmv1-ca

Copy link

Contributor

bentito Sep 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

thetechnick force-pushed the e2e-tls branch from f466b4b to 3eb1883 Compare September 10, 2024 07:41

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

Ensure docker registry CA is trusted in e2e tests

33e3fc3

thetechnick force-pushed the e2e-tls branch from 3eb1883 to 33e3fc3 Compare September 10, 2024 07:49

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

joelanford reviewed Sep 11, 2024

View reviewed changes

Restructure based on feedback

c2dd89f

perdasilva approved these changes Sep 12, 2024

View reviewed changes

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

Merged via the queue into operator-framework:main with commit f1b46a3 Sep 12, 2024

thetechnick deleted the e2e-tls branch September 12, 2024 12:21

✨ Ensure docker registry CA is trusted in e2e tests #377

✨ Ensure docker registry CA is trusted in e2e tests #377

Uh oh!

Conversation

thetechnick commented Sep 6, 2024

Uh oh!

codecov bot commented Sep 6, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

perdasilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Sep 6, 2024 •

edited

Loading