-
Notifications
You must be signed in to change notification settings - Fork 26
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
⚠️ Make catalog server serve catalog contents over HTTPS #243
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,9 +77,11 @@ func main() { | |
catalogdVersion bool | ||
systemNamespace string | ||
catalogServerAddr string | ||
httpExternalAddr string | ||
httpsExternalAddr string | ||
cacheDir string | ||
gcInterval time.Duration | ||
certfile string | ||
keyfile string | ||
) | ||
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.") | ||
|
@@ -89,10 +91,12 @@ func main() { | |
"Enabling this will ensure there is only one active controller manager.") | ||
flag.StringVar(&systemNamespace, "system-namespace", "", "The namespace catalogd uses for internal state, configuration, and workloads") | ||
flag.StringVar(&catalogServerAddr, "catalogs-server-addr", ":8083", "The address where the unpacked catalogs' content will be accessible") | ||
flag.StringVar(&httpExternalAddr, "http-external-address", "http://catalogd-catalogserver.catalogd-system.svc", "The external address at which the http server is reachable.") | ||
flag.StringVar(&httpsExternalAddr, "https-external-address", "https://catalogd-catalogserver.catalogd-system.svc", "The external address at which the http server is reachable.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are effectively removing a flag from this executable. Is this considered a breaking change? Should the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That being said, and in addition to @grokspawn's comment. Would it be possible for this to be set to an |
||
flag.StringVar(&cacheDir, "cache-dir", "/var/cache/", "The directory in the filesystem that catalogd will use for file based caching") | ||
flag.BoolVar(&catalogdVersion, "version", false, "print the catalogd version and exit") | ||
flag.DurationVar(&gcInterval, "gc-interval", 12*time.Hour, "interval in which garbage collection should be run against the catalog content cache") | ||
flag.StringVar(&certfile, "tls-cert", "/var/certs/tls.crt", "The certificate file used for serving catalog contents over HTTPS") | ||
flag.StringVar(&keyfile, "tls-key", "/var/certs/tls.key", "The key file used for serving catalog contents over HTTPS") | ||
Comment on lines
+98
to
+99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These defaults could be annoying for running the binary locally outside of a container. WDYT about:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, do we need to think about what happens during a key rotation? We might need a file watcher to detect when the keys change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I can't think of a good reason for this flag to carry a full URL with a scheme. Let's just make it |
||
opts := zap.Options{ | ||
Development: true, | ||
} | ||
|
@@ -149,7 +153,7 @@ func main() { | |
os.Exit(1) | ||
} | ||
|
||
baseStorageURL, err := url.Parse(fmt.Sprintf("%s/catalogs/", httpExternalAddr)) | ||
baseStorageURL, err := url.Parse(fmt.Sprintf("%s/catalogs/", httpsExternalAddr)) | ||
if err != nil { | ||
setupLog.Error(err, "unable to create base storage URL") | ||
os.Exit(1) | ||
|
@@ -168,6 +172,9 @@ func main() { | |
WriteTimeout: 5 * time.Minute, | ||
}, | ||
ShutdownTimeout: &shutdownTimeout, | ||
ServeTLS: true, | ||
CertFile: certfile, | ||
KeyFile: keyfile, | ||
} | ||
|
||
if err := mgr.Add(&catalogServer); err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
apiVersion: cert-manager.io/v1 | ||
kind: Certificate | ||
metadata: | ||
name: catalogserver-cert | ||
namespace: system | ||
spec: | ||
secretName: catalogd-catalogserver-cert | ||
duration: 2160h # 90d | ||
renewBefore: 360h # 15d | ||
subject: | ||
organizations: | ||
- operator-framework | ||
isCA: false | ||
dnsNames: | ||
- catalogd-catalogserver.catalogd-system.svc | ||
issuerRef: | ||
name: catalogd-catalogserver-selfsigned-issuer | ||
kind: Issuer | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
apiVersion: cert-manager.io/v1 | ||
kind: Issuer | ||
metadata: | ||
name: catalogserver-selfsigned-issuer | ||
namespace: system | ||
spec: | ||
selfSigned: {} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
resources: | ||
- issuer.yaml | ||
- certificate.yaml |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,5 +14,5 @@ kind: Kustomization | |
resources: | ||
- ../crd | ||
- ../rbac | ||
- ../certmanager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will break downstream for Red Hat because they use a different certificate manager on OCP clusters. I'd suggest setting things up similar to what we did in rukpak: https://github.com/operator-framework/rukpak/tree/main/manifests |
||
- ../manager | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,18 @@ type Server struct { | |
// ShutdownTimeout is an optional duration that indicates how long to wait for the server to shutdown gracefully. If not set, | ||
// the server will wait indefinitely for all connections to close. | ||
ShutdownTimeout *time.Duration | ||
|
||
// ServeTLS is an optional bool that indicates that the server should | ||
// serve over HTTPS | ||
ServeTLS bool | ||
|
||
// CertFile is the certificate file to use when serving over HTTPS. | ||
// Only used and required when ServeTLS is "true". | ||
CertFile string | ||
|
||
// KeyFile is the key file to use when serving over HTTPS. | ||
// Only used and required when ServeTLS is "true". | ||
KeyFile string | ||
} | ||
|
||
// Start starts the server. It will block until the server is stopped or an error occurs. | ||
|
@@ -116,7 +128,15 @@ func (s *Server) addr() string { | |
|
||
func (s *Server) serve() error { | ||
if s.Listener != nil { | ||
if s.ServeTLS { | ||
return s.Server.ServeTLS(s.Listener, s.CertFile, s.KeyFile) | ||
} | ||
Comment on lines
+131
to
+133
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I finally got the upstream PR to make the controller-runtime HTTP server runnable exported, so I think we should plan to drop this package when there is a release containing it With that in mind, I would suggest setting up TLS via an explicit There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean always creating a listener in main.go and passing that into the Server creation, conditionally enabling TLS if the cert/key exist? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @trgeiger that is my understanding. I would also revert the changes that I made to the server.Server. I read the wrong section of the net/http package and realize now that the changes I made were unnecessary and passing in the listener is the "right" way to go here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, that clears that up for me |
||
return s.Server.Serve(s.Listener) | ||
} | ||
|
||
if s.ServeTLS { | ||
return s.Server.ListenAndServeTLS(s.CertFile, s.KeyFile) | ||
} | ||
|
||
return s.Server.ListenAndServe() | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -93,7 +93,7 @@ var _ = Describe("Catalog Unpacking", func() { | |||||
// the ProxyGet() call below needs an explicit port value, so if | ||||||
// value from url.Port() is empty, we assume port 80. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if port == "" { | ||||||
port = "80" | ||||||
port = "443" | ||||||
} | ||||||
resp := kubeClient.CoreV1().Services(ns).ProxyGet(url.Scheme, name, port, url.Path, map[string]string{}) | ||||||
rc, err := resp.Stream(ctx) | ||||||
|
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.
possible I'm getting wrapped around the axle of nomenclature, but the description says that this defaults to https, and given the name it seems that the primary focus is https and I'm not clear on how they would disable it. All the relevant flags have actionable default values.
I'm not certain that we need to have an http escape hatch here, but if there isn't one can you update the PR description to say that we're moving exclusively to secured connections?
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.
Description updated