-
Notifications
You must be signed in to change notification settings - Fork 31
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
✨ Ensure docker registry CA is trusted in e2e tests #377
Conversation
Codecov ReportAttention: Patch coverage is
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. |
cmd/manager/main.go
Outdated
@@ -269,3 +279,48 @@ func podNamespace() string { | |||
} | |||
return string(namespace) | |||
} | |||
|
|||
func newCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) { |
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.
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)
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.
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.
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.
seems reasonable. Would it be good to add a tracking issue in a comment, so we don't forget =D
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.
Some of this already seems to be copy/paste anyways?
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.
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
d8af8a7
to
dfcf05a
Compare
@@ -20,6 +20,7 @@ spec: | |||
matchLabels: | |||
control-plane: catalogd-controller-manager | |||
replicas: 1 | |||
minReadySeconds: 5 |
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.
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
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.
That does seem like a similar problem...
@@ -116,7 +116,7 @@ lint: $(GOLANGCI_LINT) ## Run golangci linter. | |||
.PHONY: test-upgrade-e2e | |||
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 |
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.
nit: reordering the targets just seems like churn?
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.
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
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.
ah, maybe we could use a comment about these target ordering needs then?
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.
added.
cmd/manager/main_test.go
Outdated
require.NoError(t, err) | ||
} | ||
|
||
func Test_newCertPool_empty(t *testing.T) { |
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.
Maybe could use a test for when there is an invalid certificate file?
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'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.
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.
ah got it, sounds good to do less here then
@@ -25,8 +17,8 @@ spec: | |||
algorithm: ECDSA | |||
size: 256 | |||
issuerRef: | |||
name: selfsigned-issuer | |||
kind: Issuer | |||
name: olmv1-ca |
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.
👍
f466b4b
to
3eb1883
Compare
3eb1883
to
33e3fc3
Compare
cmd/manager/main_test.go
Outdated
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.
nit: move newCertPool
to an internal package so we don't need a main_test.go
?
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.
moved into httputil
package, similar to /operator-controller
if i.CertPool != nil { | ||
tlsTransport.TLSClientConfig.RootCAs = i.CertPool | ||
} | ||
remoteOpts = append(remoteOpts, remote.WithTransport(tlsTransport)) |
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.
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?
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.
Updated to use if/else to add remote.WithTransport
.
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.
Nice one, Nico! Thank you ^^
cmd/manager/main.go
Outdated
@@ -269,3 +279,48 @@ func podNamespace() string { | |||
} | |||
return string(namespace) | |||
} | |||
|
|||
func newCertPool(caDir string, log logr.Logger) (*x509.CertPool, error) { |
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.
seems reasonable. Would it be good to add a tracking issue in a comment, so we don't forget =D
No description provided.