search

operator-framework / catalogd

On-cluster FBC catalog content server
Apache License 2.0
14 stars 26 forks source link

⚠️ Serve catalog over HTTPS #263

Closed trgeiger closed 1 month ago

trgeiger commented 2 months ago

This adds the ability to use HTTPS on the catalog server.

If the TLS cert and key filenames are provided via the relevant flags, then the server will be set to use HTTPS. If neither or only one of those options are provided, the server will use HTTP.

This also changes the http-external-address flag to just external-address, with the user no longer prepending the address with http or https.

This PR also includes an overhaul of the organization of the manifests. Following the example of rukpak, cert-manager is used in the Makefile build targets so HTTPS is enabled in kind, but cert-manager is not a required dependency of catalogd. Instead, it's applied as an overlay which we target in the Makefile.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 48.84%. Comparing base (250e348) to head (e7133a6).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #263 +/- ## ======================================= Coverage 48.84% 48.84% ======================================= Files 8 8 Lines 434 434 ======================================= Hits 212 212 Misses 201 201 Partials 21 21 ```

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

trgeiger commented 1 month ago

Thanks, Bryce. Should we adjust anything in the readme's quickstart instructions or anywhere else in docs?

everettraven commented 1 month ago

Thanks, Bryce. Should we adjust anything in the readme's quickstart instructions or anywhere else in docs?

Ah, great question (and good catch). We should probably update the quickstart instructions for port-forwarding and the curl command for fetching catalog contents to reflect the new state with an HTTPS based server as opposed to HTTP. I think this information is also present in https://github.com/operator-framework/catalogd/blob/main/docs/fetching-catalog-contents.md

trgeiger commented 1 month ago

How should we handle changing the instructions, given that HTTP is still supported if no TLS keys are provided, and the cert-manager is now an overlay. Do we just add that information as caveats to the instructions? Should I detail pulling down the cert and providing it to curl? etc.

everettraven commented 1 month ago

How should we handle changing the instructions, given that HTTP is still supported if no TLS keys are provided, and the cert-manager is now an overlay. Do we just add that information as caveats to the instructions? Should I detail pulling down the cert and providing it to curl? etc.

I'm not too familiar with how the overlays work, but IIUC the default deployment (i.e what is released) is going to be including the cert-manager resources and enable TLS (please correct me if I am wrong). If that is the default I would document whatever the default is.

I would not document pulling down the cert and providing it to curl and instead document with curl -k to ignore the certs. We can include a disclaimer in the docs that this is only for demonstration purposes and that if they want to trust the certificates to follow some other documentation on how to do that (I'm sure there is some documentation on this that already exists)

trgeiger commented 1 month ago

I made a couple small edits to README.md and https://github.com/operator-framework/catalogd/blob/main/docs/fetching-catalog-contents.md, let me know what you think

trgeiger commented 1 month ago

Those were quick fixes, just made em so no need to hold off for another issue/pr for those.