Skip to content
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

REAP-10 Allow setting up per-cluster TLS connections #1524

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

rzvoncek
Copy link
Contributor

@rzvoncek rzvoncek commented Sep 20, 2024

This PR allows Reaper to use cluster-specific trust/key-stores when using HTTP instead of JMX (meaning Reaper is talking to mgmt-api).

The Reaper's yaml file can now have a new httpManagement.truststoresDir field:

httpManagement:
 enabled: ${REAPER_HTTP_MANAGEMENT_ENABLE}
 keystore: ${REAPER_HTTP_MANAGEMENT_KEYSTORE_PATH}
 truststore: ${REAPER_HTTP_MANAGEMENT_TRUSTSTORE_PATH}
 truststoresDir: ${REAPER_HTTP_MANAGEMENT_TRUSTSTORES_DIR}

This should point to a directory where per-cluster certificates will appear. The certs are expected to have format of clusterName-truststore|keystore.jks.

When adding a cluster, it is now necessary to specify the cluster name via the seed host, so the seed host becomes seedHost@clusterName. The clusterName should match the prefix of in the cert names in truststoresDir. If the cluster name is not provided, Reaper falls back to using the global keystores.

Hot reloading of the cluster-specific stores works the same way as for the global ones.

Copy link

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

@rzvoncek rzvoncek force-pushed the REAP-10-load-multiper-truststores branch 4 times, most recently from 1db0c5d to c01b9be Compare September 26, 2024 09:46
if (config.getHttpManagement().isEnabled()) {
if (config.getHttpManagement().getTruststoresDir() != null) {
if (!Files.exists(Paths.get(config.getHttpManagement().getTruststoresDir()))) {
throw new RuntimeException(String.format(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: There's odd logic here. If getTrustStoresDir() is null, then we won't have any RuntimeException.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do you rely on the truststore value to cause the RuntimeException somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I've added it here, in a checkConfiguration(), so that it fails early. Then the stacktrace is smaller and more obvious.

Otherwise, the RuntimeException would happen later, when it's trying to setup the watchers for changes, which in turn happens when actually adding a cluster. The stack traces at this point are incredibly long and nested.

@rzvoncek rzvoncek force-pushed the REAP-10-load-multiper-truststores branch 7 times, most recently from 6732873 to f735732 Compare September 30, 2024 14:38
@rzvoncek rzvoncek requested a review from burmanm October 2, 2024 09:31
@@ -507,6 +507,58 @@ jobs:

- uses: codecov/codecov-action@v1

snapshot-release:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussion/issue: I'm not a huge fan of releasing every PR's every commit as image. It really pollutes the Docker Hub. Merging on main is fine, but do we really need these for every PR commit? Especially since this will fail the PR build when a user uses a fork (which would be the default for most users).

If you just want the built image available as part of the PR, then you could use the upload-action so it would be available as download.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that poluting Dcker Hub is no good. I've got my image out and the related k8ssandra-operator PR is passing. I'm ok with dropping this entirely (pushed a commit doing so)

@rzvoncek rzvoncek requested a review from burmanm October 22, 2024 08:59
boolean watchTruststoreDir = tsd != null && !tsd.isEmpty() && Files.isDirectory(Paths.get(tsd));

try {
createSslWatcher(watchTruststore, watchKeystore, watchTruststoreDir);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: There's no if here to prevent creating ssl watcher. We would still always create the thread and use Filewatchers, just we wouldn't actually watch any events. If all of these are false, then we shouldn't call createSslWatcher() at all.

@rzvoncek rzvoncek requested a review from burmanm October 24, 2024 09:10
@rzvoncek rzvoncek force-pushed the REAP-10-load-multiper-truststores branch from 74783bd to ad7eff8 Compare October 24, 2024 12:40
@rzvoncek
Copy link
Contributor Author

Squashed locally, merging.

@rzvoncek rzvoncek merged commit 9d27d54 into master Oct 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants