search

operator-framework / operator-controller

Apache License 2.0
28 stars 47 forks source link

:sparkles: Certificate support for image registry #960

Closed tmshort closed 1 week ago

tmshort commented 1 week ago

Fixes: #921

Remove the InsecureSkipTLSVerify annotations.

Description

Reviewer Checklist

netlify[bot] commented 1 week ago

Deploy Preview for olmv1 ready!

Name Link
Latest commit 3ca71ba867f727fd33e6f2ade490af902ca95664
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...

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[bot] commented 1 week ago

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 :warning:
...nternal/controllers/clusterextension_controller.go 71.42% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/operator-framework/operator-controller/pull/960/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | Coverage Δ | | |---|---|---| | [e2e](https://app.codecov.io/gh/operator-framework/operator-controller/pull/960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `57.14% <65.00%> (-1.07%)` | :arrow_down: | | [unit](https://app.codecov.io/gh/operator-framework/operator-controller/pull/960/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework) | `53.21% <20.00%> (-0.64%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=operator-framework#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tmshort commented 1 week ago

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

tmshort commented 1 week ago

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.

tmshort commented 1 week ago

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 commented 1 week ago

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:

  1. create a single ClusterIssuer in the standard install
  2. any OLMv1 component that needs a certificate gets it from there, no matter which namespace that component is in.
  3. 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 commented 1 week ago

Three follow-ons to this PR:

  1. 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.
  2. 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
  3. Update rukpak to accept a CertPool in the API, with corresponding change to operator-controller.
everettraven commented 1 week ago

Follow ups should be captured by this epic: https://github.com/operator-framework/operator-controller/issues/967

tmshort commented 1 week ago

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.