✨ Certificate support for image registry #960

tmshort · 2024-06-20T17:23:27Z

Fixes: #921

Remove the InsecureSkipTLSVerify annotations.

Create a ClusterIssuer CA (via cert-manager) that is used by OLMv1 e2e
Update the operator controller to specify a cert directory, rather than a single file.
Use this directory for catalogd and image-registries
Update the deployment to reference CAs appropriately

Description

Reviewer Checklist

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

netlify · 2024-06-20T17:23:43Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`3ca71ba`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/6679b799478a640008286750
😎 Deploy Preview	https://deploy-preview-960--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.

Remove the InsecureSkipTLSVerify annotations. * Create a ClusterIssuer CA (via openssl) that is used by OLMv1 e2e * Update the operator controller to specify a cert directory, rather than a single file. * Use this directory for catalogd and image-registries * Update the deployment to reference CAs appropriately Signed-off-by: Todd Short <tshort@redhat.com>

codecov · 2024-06-20T19:30:33Z

Codecov Report

Attention: Patch coverage is 70.00000% with 12 lines in your changes missing coverage. Please review.

Project coverage is 79.28%. Comparing base (58b4363) to head (3ca71ba).
Report is 4 commits behind head on main.

Files	Patch %	Lines
internal/httputil/httputil.go	66.66%	5 Missing and 5 partials ⚠️
...nternal/controllers/clusterextension_controller.go	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #960      +/-   ##
==========================================
- Coverage   80.14%   79.28%   -0.86%     
==========================================
  Files          16       16              
  Lines        1103     1120      +17     
==========================================
+ Hits          884      888       +4     
- Misses        152      159       +7     
- Partials       67       73       +6

Flag	Coverage Δ
e2e	`57.14% <65.00%> (-1.07%)`	⬇️
unit	`53.21% <20.00%> (-0.64%)`	⬇️

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.

tmshort · 2024-06-21T00:39:54Z

To consolidate catalogd's certificate would require modifying catalogd; it would also allow catalogd to be placed back into its own namespace.

config/overlays/e2e/kustomization.yaml

internal/controllers/clusterextension_controller.go

config/overlays/certs/kustomization.yaml

cmd/manager/main.go

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort · 2024-06-21T23:08:05Z

I can't get kustomize to use a different directory, so I'm applying the yaml for the certificates directly from a file, which also means I can apply them for Tilt from the same source.

joelanford · 2024-06-22T00:33:56Z

internal/httputil/httputil.go

-			RootCAs:    caCertPool,
-			MinVersion: tls.VersionTLS12,
+		if info.IsDir() {
+			return nil


Is the intention to recurse through all subdirectories? I'm tempted to suggest that we stick to finding files in a single named directory without recursing, but I'm curious if you have a specific reason.

To avoid recursing, we can return filepath.SkipDir here.

Suggested change

return nil

return filepath.SkipDir

No, it's intended to skip directories. I didn't see this in any examples.

This appears to skip "." as a directory, meaning, it skips the current directory... so the e2es fail.
(Or something equally nefarious.)

OK, found one example, and the example has a comparison with a directory name, so we'd need to do that to properly use filepathSkipDir; not sure it's worth it?

Maybe switch over to os.ReadDir(), which makes the intent more clear anyways?

joelanford · 2024-06-22T00:39:25Z

internal/httputil/httputil.go

-
-	if caCert != "" {
-		// tlsFileWatcher, err := certwatcher.New(caCert, "")
+func LoadCerts(caDir string) (string, error) {


Seems like it would be a nicer abstraction if this was:

Suggested change

func LoadCerts(caDir string) (string, error) {

func AddCertificatesFromDirectory(certPool *x509.CertPool, caDir string) error { ... }

In which case, we could add each file's content, collect errors reading/adding each file, and then tell callers which specific files failed for which reasons.

This is not compatible with the current rukpak API, which expects a string of PEM. Maybe when rukpak is fully integrated into operator-controller? I agree that this would be better for a larger directory. There does not appear to be a way to extract certificates/PEM from a CertPool, so it can't be done with this signature, yet.

Oh I was just looking at LoadCerts being called in BuildHTTPClient, which turns around and consumes the certs string by immediately adding to the caCertPool.

And that made me think we could pass the caCertPool into this function and call AppendCertsFromPEM for each file's contents. Then, when my suggested function signature returns, you end up with caCertPool having everything in caDir added to it.

Seems like I'm missing something though?

It's also used when creating a BundleDeployment for a rukpak API.

operator-controller/internal/controllers/clusterextension_controller.go

Line 252 in ed4e40e

bd := r.generateBundleDeploymentForUnpack(bundle.Image, ext)

So, we need to change the rukpak API to accept a caPool first, then we can do as you suggest.

joelanford · 2024-06-22T00:40:14Z

scripts/install.tpl.sh

+kubectl apply -f testdata/certs/issuers.yaml
+


This won't work when we publish this install script in our release.

I tried numerous ways to get the secret for the ClusterIssuser into the cert-manager namespace, but despite trying various transformations:, I couldn't get kustomize to do it (it wanted to put stuff into the olmv1-system directory. Using a separate file at least avoids duplication among the two e2e tests. But I can always do it via a HEREDOC.

i.e. if you can figure out how to make kustomize work the way we want, I'd much prefer to do it that way!

Signed-off-by: Todd Short <tshort@redhat.com>

m1kola · 2024-06-24T12:19:27Z

config/overlays/tls/kustomization.yaml

manager_deployment_cert.yaml and manager_cert_patch.yaml seem to patch the same deployment and same fields but in different ways. I think it would be good to standardise.

m1kola · 2024-06-24T12:26:51Z

config/overlays/tls/resources/manager_cert.yaml

+  issuerRef:
+    name: olmv1-ca
+    kind: ClusterIssuer
+    group: cert-manager.io


This introduces a dependency on ClusterIssuer which is only being created In install.tpl.sh/in Tilt.

This means it is no longer possible to do this to install:

kubectl apply -f https://github.com/operator-framework/catalogd/releases/download/$CATALOGD_VERSION/catalogd.yaml kubectl apply -f https://github.com/operator-framework/operator-controller/releases/download/$VERSION/operator-controller.yaml

You have to checkout the repo run the install script.

Are we ok with that?

Also about related changes in scripts/install.tpl.sh where we create CA, etc.

As far as I understand - we need to create CA and patch deployment of the manger only for E2E setups. If it is the case - can we put this in config/overlays/e2e/?

I would like to avoid having to create all this on every cluster just because our E2E setup requires it.

We need cert-manager regardless, AFAICT, as catalogd now creates certificates. So, the method listed above no longer works (or at least it shouldn't).
I tried to put this into the e2e overlay, but could not get the namespace to work as expected. Please see #960 (comment); I have been unable to figure out how to apply resources into the cert-manager namespace. i.e. kustomize seems to be locking me into a single namespace...

cmd/manager/main.go

tmshort · 2024-06-24T13:33:20Z

TL;DR: The challenge I've run into w/ adding the certificates is being able to put them into the cert-manager namespace for the ClusterIssuer. I tried various kustomize options and transformations and none of them seemed to work. I would love to get rid of the HEREDOC and yaml used to create the certificates. If anyone has any ideas, I'm all ears.

m1kola · 2024-06-24T15:03:54Z

A number of my comments in this PR were based on the understanding that this is only about making our E2E setup work with TLS. After chatting with @joelanford and @tmshort I got more context.

The primary objective of the PR to get rid of InsecureSkipTLSVerify and use TLS in E2E, but it also prepares the ground for using single ClusterIssuer for all OLMv1 components. E.g. we can use it for catalogd too.

Explanation from @joelanford:

create a single ClusterIssuer in the standard install

any OLMv1 component that needs a certificate gets it from there, no matter which namespace that component is in.

any OLMv1 component that needs a CA can create a certificate (if it doesn't already have one), and that cert secret will contain the CA.

That setup would mean that we could move catalogd back to its own namespace if we wanted to, and it would mean that the e2e registry would be automatically trusted by the standard setup because operator-controller would already have the CA.

tmshort · 2024-06-24T15:37:11Z

Three follow-ons to this PR:

Update the config/kustomization of operator-controller to create the ClusterIssuer and required resources in the cert-manager namespace (rather than via HEREDOC and separate YAML), for use by operator-controller, and eventually catalogd. The challenge here is the multiple namespaces. This may require an RFC or other documentation.
Unifying catalogd and operator-controller's use of a common ClusterIssuer for certificates.
2a. Possibly move catalogd back into it's own namespace, especially if the olmv1-system namespace security parameters are overwritten
Update rukpak to accept a CertPool in the API, with corresponding change to operator-controller.

everettraven · 2024-06-24T16:02:14Z

Follow ups should be captured by this epic: #967

everettraven

The changes themselves look fine to me, but I noticed the e2es are failing. Any insights as to why?

tmshort · 2024-06-24T18:19:36Z

The changes themselves look fine to me, but I noticed the e2es are failing. Any insights as to why?

I made a change that passed due to caching issues... I had to fix it up and it should be ok now.

joelanford · 2024-06-24T18:38:12Z

testdata/certs/issuers.yaml

I don't love that this is duplicated here and in the HEREDOC, but I also recognize that this setup is temporary. Can we just make sure that our follow-ups include a callout to remove this duplication?

See #967 (comment)

tmshort requested a review from a team as a code owner June 20, 2024 17:23

tmshort mentioned this pull request Jun 20, 2024

✨ Certificate support for image registry #956

Closed

4 tasks

tmshort force-pushed the cert-update2 branch from bfa68a5 to b128ce3 Compare June 20, 2024 19:24

m1kola reviewed Jun 21, 2024

View reviewed changes

tmshort added 2 commits June 21, 2024 13:15

fixup! Certificate support for image registry

0c20937

Signed-off-by: Todd Short <tshort@redhat.com>

fixup! Certificate support for image registry

6155df5

Signed-off-by: Todd Short <tshort@redhat.com>

joelanford reviewed Jun 22, 2024

View reviewed changes

fixup! Certificate support for image registry

0693c6a

Signed-off-by: Todd Short <tshort@redhat.com>

tmshort force-pushed the cert-update2 branch from 875a278 to 0693c6a Compare June 23, 2024 17:22

fixup! Certificate support for image registry

cf5b281

m1kola reviewed Jun 24, 2024

View reviewed changes

fixup! Certificate support for image registry

421d281

everettraven reviewed Jun 24, 2024

View reviewed changes

fixup! Certificate support for image registry

3ca71ba

everettraven approved these changes Jun 24, 2024

View reviewed changes

joelanford reviewed Jun 24, 2024

View reviewed changes

tmshort added this pull request to the merge queue Jun 24, 2024

Merged via the queue into operator-framework:main with commit d510ad1 Jun 24, 2024
15 of 17 checks passed

tmshort deleted the cert-update2 branch June 24, 2024 19:15

skattoju mentioned this pull request Jul 2, 2024

✨ Adding a token getter to get service account tokens #1006

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Certificate support for image registry #960

✨ Certificate support for image registry #960

tmshort commented Jun 20, 2024 •

edited

Loading

netlify bot commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

tmshort commented Jun 21, 2024

tmshort commented Jun 21, 2024

joelanford Jun 22, 2024

tmshort Jun 22, 2024 •

edited

Loading

tmshort Jun 23, 2024 •

edited

Loading

tmshort Jun 23, 2024

joelanford Jun 24, 2024

tmshort Jun 24, 2024

joelanford Jun 22, 2024

tmshort Jun 22, 2024

joelanford Jun 22, 2024

tmshort Jun 23, 2024

joelanford Jun 22, 2024

tmshort Jun 22, 2024 •

edited

Loading

tmshort Jun 22, 2024 •

edited

Loading

m1kola Jun 24, 2024

m1kola Jun 24, 2024

m1kola Jun 24, 2024

tmshort Jun 24, 2024

tmshort commented Jun 24, 2024

m1kola commented Jun 24, 2024

tmshort commented Jun 24, 2024 •

edited

Loading

everettraven commented Jun 24, 2024

everettraven left a comment

tmshort commented Jun 24, 2024

joelanford Jun 24, 2024

tmshort Jun 24, 2024

	func LoadCerts(caDir string) (string, error) {
	func AddCertificatesFromDirectory(certPool *x509.CertPool, caDir string) error { ... }

✨ Certificate support for image registry #960

✨ Certificate support for image registry #960

Conversation

tmshort commented Jun 20, 2024 • edited Loading

Description

Reviewer Checklist

netlify bot commented Jun 20, 2024 • edited Loading

✅ Deploy Preview for olmv1 ready!

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

tmshort commented Jun 21, 2024

tmshort commented Jun 21, 2024

Choose a reason for hiding this comment

tmshort Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

tmshort Jun 23, 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 Jun 22, 2024 • edited Loading

Choose a reason for hiding this comment

tmshort Jun 22, 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

tmshort commented Jun 24, 2024

m1kola commented Jun 24, 2024

tmshort commented Jun 24, 2024 • edited Loading

everettraven commented Jun 24, 2024

everettraven left a comment

Choose a reason for hiding this comment

tmshort commented Jun 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmshort commented Jun 20, 2024 •

edited

Loading

netlify bot commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

tmshort Jun 22, 2024 •

edited

Loading

tmshort Jun 23, 2024 •

edited

Loading

tmshort Jun 22, 2024 •

edited

Loading

tmshort Jun 22, 2024 •

edited

Loading

tmshort commented Jun 24, 2024 •

edited

Loading