Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
MC-1296 Deploy Reaper capable of interaction with encrypted mgmt-api #1421
base: main
Are you sure you want to change the base?
MC-1296 Deploy Reaper capable of interaction with encrypted mgmt-api #1421
Changes from all commits
4b5d2b9
ea8f3c0
8415450
f0c6a02
7b3e21b
cd78ce8
c7c0baa
55d9605
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Issue: I'm not sure this is the right logic - in particular clusterName is not always populated. Is there a function you can re-use in cass operator to figure out the resource names?
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.
The
actualDc.Spec.ClusterName
is a field ofCassandraDatacenterSepc
in cass-operator. It's declared like this:This looks like the field must be a string of at least two characters. Otherwise, I think, the cass-operator's webhook will prevent creating the DC. In other words, if a
CassandraDatacenter
object shows up, this field must be at least 2 chars long. Please correct me if I'm missing something.Anyhow, we could also take the K8ssandraCluster object name, or
kc.Spec.Cassandra.ClusterName
. I'd say all these should be set to the same thing, but I don't really know.Could someone else please chime in?
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.
Re-reading this... You're only using it for setting names for the truststore/keystore files that get materialised in the secret right? So this will just affect the keys in the aggregated secret that contains all of the encryption materials for the different Cassandra clusters?
It may not matter too much if that's right, although I'd still suggest you use something like the namespace-name of the k8ssandra cluster since otherwise multiple clusters with the same name in different namespaces can overwrite each other...
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.
This is correct. I'll make a fix.
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.
After some investigation, and a chat with Alex, we concluded that this is, in fact, a blocking issue.
We need a proper way of making Reaper (and its truststore) capable to tell two equally named clusters from different namespaces apart. It's not just for encryption, but for monitoring also.
We'll be putting this ticket on hold until we fix this in Reaper.
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.
Issue: Do you need to ensure that the content is correct? What if the secrets have rotated?
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.
This is a good catch. The rotation is not currently handled. I've made MC-1367 to add that.
What other correctnes checks could you think of ?
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.
Fingerprints are probably sufficient until you need to think about rotation. We can think more about rotating the certs and CAs down the track (although CA related work is on hold at present, so I'm not sure when that will happen).
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.
So what you mean is to detect a mismatch between what's already in the secret and what's coming in we just compare the fingerprints (until we deal with rotation)?
Also, is it ok to interpret "fingerprint" as a "hash" of the secret?