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
HBASE-27347 Port FileWatcher from ZK to autodetect keystore/truststore changes in TLS connections #4869
Conversation
🎊 +1 overall
This message was automatically generated. |
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.
LGTM, overall, just have some minor remarks.
} | ||
// Note: we don't care about delete events | ||
if (shouldResetContext) { | ||
if (LOG.isDebugEnabled()) { |
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.
nit: Maybe worth info logging? My thought here is that this wouldn't be a frequent event, yet a important one to get logged at higher level than debugging.
resetContext.run(); | ||
} else { | ||
if (LOG.isDebugEnabled()) { | ||
LOG.debug("Ignoring watch event and keeping previous default SSL context. Event kind: " |
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.
nit: parameterized logging?
* revision</a> | ||
*/ | ||
@InterfaceAudience.Private | ||
public final class FileChangeWatcher { |
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.
Is this a straight copy from Zookeeper? Couldn't we just reuse the ZK impl directly, as we already have ZK as a dependency?
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.
Urm... I don't have a strong opinion. We were following this pattern in the entire implementation of TLS. It's probably better to stay on the safe side and avoid sideeffects of ZK non-backward compatible changes. They're quite unlikely though.
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.
Thanks for clarifying. Not a big deal for me. And on the flip side, it would be actually a bit weird to depend on ZK for a functionality that isn't really ZK specific, so I'm ok with this copy approach.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks @wchevreuil for merging the patch. Please cherry pick it to branch-2 as well. |
…e changes in TLS connections (apache#4869) Signed-off-by: wchevreuil@apache.org
This patch is the port of ZooKeeper's FileWatcher's functionality which we can take advantage to detect changes in truststore / keystore files for TLS. Cert / key renewal processes don't need HBase services to be restarted with this patch.
cc @bbeaudreault @Apache9