⚠ add TLS overlay for Catalogd v0.13.0 web server TLS #888
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
trgeiger
force-pushed
the
https-catalogd
branch
3 times, most recently
from
a30b4d6
to
8da32c6
Compare
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-merge-robot
added
the
needs-rebase
trgeiger
changed the title
openshift-merge-robot
removed
the
needs-rebase
trgeiger
force-pushed
the
https-catalogd
branch
3 times, most recently
from
e7c719c
to
d330542
Compare
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
cmd/manager/main.go
Outdated
| cert, err := os.ReadFile(tlsCert) | ||
| if err != nil { | ||
| log.Fatalf("Failed to read certificate file: %v", err) | ||
| } |
There was a problem hiding this comment.
We probably need to anticipate rotation of this cert as well.
There was a problem hiding this comment.
I've been trying to figure out a solution for this, but I'm hitting a wall. It looks like this is an issue others have encountered before and there's not a native mechanism for reloading rootCAs in go: golang/go#35887 (comment)
There was a problem hiding this comment.
Oh that looks like a minefield. I suppose for now we can punt on handling CA rotation, and revisit if it becomes a practical issue. Maybe just make a separate issue for this?
There was a problem hiding this comment.
That sounds good to me, might be a good contribution to make to Go at some point
openshift-merge-robot
removed
the
needs-rebase
openshift-merge-robot
added
the
needs-rebase
openshift-merge-robot
removed
the
needs-rebase
|
Barring any other feedback, the only remaining issue is deciding how we want to handle installing Catalogd to the operator-controller namespace for the purposes of Tilt. We could add some custom logic to one of the tiltfiles, but we could also change Catalogd's default namespace to just be operator-controller-system if we don't have any compelling reasons to keep it in its own namespace. @everettraven would appreciate your thoughts when you're back in the office. |
| ) | ||
| flag.StringVar(&metricsAddr, "metrics-bind-address", ":8080", "The address the metric endpoint binds to.") | ||
| flag.StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") | ||
| flag.StringVar(&caCert, "ca-cert", "", "The TLS certificate to use for verifying HTTPS connections to the Catalogd web server.") |
There was a problem hiding this comment.
Wondering if we should make it more generic: not only about catalogd. E.g. what if we want to pull bundles from a registry with a self-signed certs?
See #905 (comment) for example.
There was a problem hiding this comment.
I think that is reasonable, but I'd probably recommend doing it in a follow up. Looking at the comment thread you shared, it seems like that is more tailored specifically to the direct image registry client image unpacker hitting a cert issue. It would definitely be nice to generalize the ca certificate loading and behavior but I think it makes the most sense to keep this PR scoped to the catalogd use case and then in a follow up issue look into how we can expand it to support other use cases.
scripts/install.tpl.sh
Outdated
| @@ -35,8 +35,8 @@ function kubectl_wait() { | |||
| kubectl apply -f "https://github.com/cert-manager/cert-manager/releases/download/${cert_mgr_version}/cert-manager.yaml" | |||
| kubectl_wait "cert-manager" "deployment/cert-manager-webhook" "60s" | |||
|
|
|||
| kubectl apply -f "https://github.com/operator-framework/catalogd/releases/download/${catalogd_version}/catalogd.yaml" | |||
| kubectl_wait "catalogd-system" "deployment/catalogd-controller-manager" "60s" | |||
| curl -L https://github.com/operator-framework/catalogd/releases/download/${catalogd_version}/catalogd.yaml | sed s/catalogd-system/operator-controller-system/g | kubectl apply -f - | |||
There was a problem hiding this comment.
I think this is fine temporarily, but if we are going to use a common namespace I'd like to see if there is a way we could use something like kustomize to have a more declarative and consistent override than using sed.
I wonder if we could make use of the remote targets functionality in Kustomize to help us achieve this and it be a bit more robust (I think we could make sure it only changes the namespace): https://github.com/kubernetes-sigs/kustomize/blob/master/examples/remoteBuild.md
There was a problem hiding this comment.
Good point. However, if we do decide to make all OLMv1 components just install into the same namespace by default, then we wouldn't need to do this anymore.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #888 +/- ##
==========================================
+ Coverage 79.15% 79.34% +0.19%
==========================================
Files 15 16 +1
Lines 1084 1104 +20
==========================================
+ Hits 858 876 +18
- Misses 157 158 +1
- Partials 69 70 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
trgeiger
force-pushed
the
https-catalogd
branch
2 times, most recently
from
7ebe52c
to
ae60a03
Compare
Fix references to Catalog and CatalogSpec
Signed-off-by: Tayler Geiger <tayler@redhat.com>
Use v0.14.0 of Catalogd which also uses olmv1-system namespace
Description
Updates the Catalogd dependency to v0.14.0 which uses TLS for the web server and olmv1-system for the namespace
Reviewer Checklist