Kms module #47
Kms module #47
Conversation
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
There was a problem hiding this comment.
Thanks @chris-giblin, this is going to be a nice!
A couple of questions in addition to a few line comments:
- Why are the value and keyprotect modules not in the root pom.xml as modules?
- And they're also missing the service loader manifests in META-INF, right?
Thanks!
| * | ||
| * @return | ||
| */ | ||
| String getName(); |
There was a problem hiding this comment.
With this being an instance method an implementation might not return a constant value, which would foil the dplicate detection in the manager. So perhaps we should use an annotation on the implementation class to provide that name. That way the name would have to be a compile-time constant.
We should probably specify the legal syntax of these names a little more tightly too. We want to avoid collisions between names, so perhaps establishing a convention where a DNS name is used as a prefix, and requiring the rest of the name to be a DNS label. Thus the KMS implementations we provided would be strimzi.io/vault, strimzi.io/key-protect and strimzi.io/test, which would allow someone else to implement example.com/vault if they wanted to have their own Vault KMS impl.
I also wonder about whether we should call this name, given that the json seems to refer to it as type (maybe simplest to change the json type ⇒ factoryName)
There was a problem hiding this comment.
For the name syntax, I have created issue #48
I agree type and name should be consistent. I propose going with factoryName.
| if (INSTANCE.dups.size() > 1) { | ||
| throw new KmsException( | ||
| "Invalid KMS provider configuration, duplicate short names: " | ||
| + INSTANCE.dups.toString()); |
There was a problem hiding this comment.
Can the message give a hint about how to fix the problem? ("Remove one of the KMS provider jars from the classpath, or rename one of them and rebuild the jar).
Also, no need for the toString (the compiler will add it for you).
There was a problem hiding this comment.
Committed a fix. Please have a look
| * | ||
| * @return | ||
| */ | ||
| private List<String> getDuplicateNames() { |
There was a problem hiding this comment.
Perhaps this method should just return a String, being an error message for whatever errors are encountered during initialization. That way we can also validate the factory names here.
There was a problem hiding this comment.
I like returning a List (actually it should be a Set). The caller can use easily convert to String if needed. I might be missing your point though.
There was a problem hiding this comment.
My point was that:
- The raising of an exception happens at the call to
getInstance, not at class initialization time. - If we're going to validate the name of each factory then that's a 2nd kind of error, which would require another field to convey from the initialisation-time to the
getInstancetime. Nor can we discount the possibility of other kinds of error in the future. - So it just seems more frugal to me to use a String error message, and convert the set to a message here, since from the users PoV there is no difference in behaviour: They still see an exception with an error message.
| Iterator<KmsFactory> it = loader.iterator(); | ||
| while (it.hasNext()) { | ||
| KmsFactory factory = it.next(); | ||
| String name = factory.getName().toLowerCase(); |
There was a problem hiding this comment.
Need a Locale to avoid locale-dependent behaviour.
| Iterator<KmsFactory> it = loader.iterator(); | ||
| while (it.hasNext()) { | ||
| KmsFactory factory = it.next(); | ||
| if (factory.getName().equalsIgnoreCase(type)) { |
There was a problem hiding this comment.
I think this will be locale-dependent, which we want to avoid.
(IIRC checkstyle and/or findbugs catches this sort of thing. We're using it in the strimzi-kafka-operator repo if you wanted to copy the config, but that's for another PR).
There was a problem hiding this comment.
Created issue #49
| while (it.hasNext()) { | ||
| KmsFactory factory = it.next(); |
There was a problem hiding this comment.
ServiceLoader is Iterable, so we could write this as for (KmsFactory factory : loader) {
There was a problem hiding this comment.
@chris-giblin do you have some unpushed commits, because while you say "done" this code doesn't seem to have changed.
There was a problem hiding this comment.
@tombentley indeed, there was an unpushed commit. Apologies. The commit has now been added to this PR. Please review when you get the chance. Thanks.
| Integer num = nameMap.get(name); | ||
| if (num == null) { | ||
| num = Integer.valueOf(0); | ||
| } |
There was a problem hiding this comment.
Integer num = getOrDefault(name, Integer.valueOf(0))
| package io.strimzi.kafka.topicenc.kms; | ||
|
|
||
| /** | ||
| * Interface to a KMS factory. |
There was a problem hiding this comment.
We should mention that implementations are discovered via KmsFactoryManager using the service loader mechanism.
The idea of this PR is to introduce |
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
Signed-off-by: Chris Giblin <[email protected]>
| Map<String, KmsDefinition> validKmsDefs, | ||
| Map<String, KeyMgtSystem> kmsPool) { | ||
|
|
||
| String kmsName = createKey(policy.getKmsName(), Locale.getDefault()); |
There was a problem hiding this comment.
The default locale will depend on the runtime machine. You need to use a constant locale, like Locale.ROOT or Locale.ENGLISH to have consistent behaviour.
There was a problem hiding this comment.
I have created issue #50 for how Local is consistently managed and specified.
In this PR:
kmskms-testIssues addressed in this PR: