ISSUE:195 Provide SSL support to SchemaRegistryClient #221
Conversation
raju-saravanan
force-pushed
the
ssl
branch
2 times, most recently
from
8e38b58 to
a6c156a
Compare
There was a problem hiding this comment.
@raju-saravanan Overall LGTM, left few comments which you may want to address.
| .property(ClientProperties.FOLLOW_REDIRECTS, Boolean.TRUE) | ||
| .build(); | ||
| .property(ClientProperties.FOLLOW_REDIRECTS, Boolean.TRUE); | ||
| if(conf.containsKey("schema.registry.client.ssl")) { |
There was a problem hiding this comment.
schema.registry.client.ssl can be extracted as constant.
| if(conf.containsKey("schema.registry.client.ssl")) { | ||
| Map<String,String> sslConfigurations = (Map<String, String>) conf.get("schema.registry.client.ssl"); | ||
| clientBuilder.sslContext(createSSLContext(sslConfigurations)); | ||
| if(sslConfigurations.containsKey("hostnameVerifierClass")) { |
There was a problem hiding this comment.
hostnameVerifierClass can be extracted as constant.
| String defaultPropertyValue = null; | ||
| SslConfigurator sslConfigurator = SslConfigurator.newInstance(); | ||
| sslConfigurator.keyStoreType(sslConfigurations.getOrDefault("keyStoreType",defaultPropertyValue)) | ||
| .keyStoreFile(sslConfigurations.getOrDefault("keyStorePath",defaultPropertyValue)) |
There was a problem hiding this comment.
Why do we have defaultValue as null for all these properties?
There was a problem hiding this comment.
if any of these properties is set to null, then the SSLConfigurator would use a default value for the property.
There was a problem hiding this comment.
Invoking sslConfigurations.getOrDefault("keyStorePath",defaultPropertyValue) with defaultPropertyValue as null is equivalent to calling sslConfigurations.get("keyStorePath") as that returns null if the value for the given key does not exist as mentioned here.
-- updated with the right link
There was a problem hiding this comment.
Ok, I will replace getOrDefault() with get() here.
|
|
||
| protected final String rootUrl = String.format("http://localhost:%d/api/v1", DROPWIZARD_APP_RULE.getLocalPort()); | ||
| protected final Map<String, String> SCHEMA_REGISTRY_CLIENT_CONF = Collections.singletonMap(SchemaRegistryClient.Configuration.SCHEMA_REGISTRY_URL.name(), rootUrl); | ||
| protected static LocalSchemaRegistryServer localSchemaRegistryServer; |
There was a problem hiding this comment.
This class should not really be abstract class now as it has static methods which are shared with derived classes. It is better to use composition than extending this class. You can remove static fields and use this class instance wherever this class is extended and respective methods can be used with that instance.
There was a problem hiding this comment.
Should we convert this class to a Util class, as all the functions in the class are stateless?
There was a problem hiding this comment.
All static fields should be made instance fields and they maintain state for the given SchemaRegistryTestProfileType. So, it is better to have constructor which takes SchemaRegistryTestProfileType and you can also have respective close method which can have the same logic as current teardown method.
Other methods which are generating avro payloads for different avro schemas can be extracted into avro generator class for the given schemas.
| private final String v1APIPath = "api/v1"; | ||
|
|
||
| public SchemaRegistryTestConfiguration(SchemaRegistryTestProfileType testProfileType) { | ||
| switch (testProfileType) { |
There was a problem hiding this comment.
This logic should be out of constructor and it could be pulled into a factory method like below.
private SchemaRegistryTestConfiguration(String serverYAMLPath, String clientYAMLPath) {
this.serverYAMLPath = serverYAMLPath;
this.clientYAMLPath = clientYAMLPath;
}
public static SchemaRegistryTestConfiguration forProfileType(SchemaRegistryTestProfileType testProfileType) {
String serverYAMLPath;
String clientYAMLPath;
switch (testProfileType) {
case DEFAULT:
serverYAMLPath = "schema-registry-test.yaml";
clientYAMLPath = "schema-registry-client.yaml";
break;
case SSL:
serverYAMLPath = "ssl-schema-registry-test.yaml";
clientYAMLPath = "ssl-schema-registry-client.yaml";
break;
case DEFAULT_HA:
serverYAMLPath = "schema-registry-test-ha.yaml";
clientYAMLPath = null;
break;
case SSL_HA:
serverYAMLPath = "ssl-schema-registry-test-ha.yaml";
clientYAMLPath = "ssl-schema-registry-client.yaml";
break;
default:
throw new IllegalArgumentException("Unrecognized SchemaRegistryTestProfileType : " + testProfileType);
}
return new SchemaRegistryTestConfiguration(serverYAMLPath, clientYAMLPath);
}
| public class SchemaRegistryTestConfiguration { | ||
| private String serverYAMLPath; | ||
| private String clientYAMLPath; | ||
| private final String v1APIPath = "api/v1"; |
There was a problem hiding this comment.
This is a constant and it can be moved to SchemaRegistryClientUtil where this is used.
| @@ -0,0 +1,242 @@ | |||
| /** | |||
There was a problem hiding this comment.
I am not sure about licensing issues with EPL when we copy the code in ASL based sources like here. You may need to put their license(EPL) here and other classes where junit's code is copied.
There was a problem hiding this comment.
@satishd on the note of licenses i note the code base has a lot of glassfish dependencies which is GPL afaik, as this project is ASL is it not maybe better to look at apache projects for equiv replacements which also will be ASL.
There was a problem hiding this comment.
@michaelandrepearce We are not modifying any of the glassfish sources, but we are using those classes as libraries in classpath. AFAIK, it comes under "GPL Classpath Exception" and it is safe to use as library here.
There was a problem hiding this comment.
@raju-saravanan left few minor nits which you may want to address.
| public void setup() throws IOException { | ||
| schemaRegistryClient = new SchemaRegistryClient(SCHEMA_REGISTRY_CLIENT_CONF); | ||
| } | ||
| public class AvroSchemaRegistryClientUtil { |
There was a problem hiding this comment.
nit: it can be made final and have private constructor so that this can never be instantiated or extended.
| @@ -171,6 +174,9 @@ | |||
| private final SchemaMetadataCache schemaMetadataCache; | |||
| private final Cache<SchemaDigestEntry, SchemaIdVersion> schemaTextCache; | |||
|
|
|||
| private final String SSL_CONFIGURATION_KEY = "schema.registry.client.ssl"; | |||
There was a problem hiding this comment.
nit: better to keep it as constant, add static.
| @@ -171,6 +174,9 @@ | |||
| private final SchemaMetadataCache schemaMetadataCache; | |||
| private final Cache<SchemaDigestEntry, SchemaIdVersion> schemaTextCache; | |||
|
|
|||
| private final String SSL_CONFIGURATION_KEY = "schema.registry.client.ssl"; | |||
| private final String HOSTNAME_VERIFIER_CLASS_KEY = "hostnameVerifierClass"; | |||
There was a problem hiding this comment.
nit: better to keep it as constant, add static.
|
|
||
| private static final String INVALID_SCHEMA_PROVIDER_TYPE = "invalid-schema-provider-type"; | ||
| private static SchemaRegistryClient schemaRegistryClient; |
There was a problem hiding this comment.
minor nit: you may want to have capitals like other static fields.
|
|
||
| private static final String INVALID_SCHEMA_PROVIDER_TYPE = "invalid-schema-provider-type"; | ||
| private static SchemaRegistryClient schemaRegistryClient; | ||
| private static SchemaRegistryTestServerClientWrapper schemaRegistryTestServerClientWrapper; |
There was a problem hiding this comment.
minor nit: you may want to have capitals like other static fields.
| public class KafkaAvroSerDesTest extends AbstractAvroSchemaRegistryCientTest { | ||
| public class KafkaAvroSerDesTest { | ||
|
|
||
| private static SchemaRegistryTestServerClientWrapper schemaRegistryTestServerClientWrapper; |
There was a problem hiding this comment.
minor nit: you may want to have capitals like other static fields.
| private static final Logger LOG = LoggerFactory.getLogger(KafkaAvroSerDesWithKafkaServerTest.class); | ||
| private static EmbeddedKafkaCluster CLUSTER; | ||
| private static SchemaRegistryTestServerClientWrapper schemaRegistryTestServerClientWrapper; |
There was a problem hiding this comment.
minor nit: you may want to have capitals like other static fields.
| public class LocalRegistryServerHATest { | ||
| private static SchemaRegistryTestConfiguration schemaRegistryTestConfiguration; |
There was a problem hiding this comment.
minor nit: you may want to have capitals like other static fields.
| public class LocalRegistryServerTest { | ||
|
|
||
| private static SchemaRegistryTestConfiguration schemaRegistryTestConfiguration; |
There was a problem hiding this comment.
minor nit: you may want to have capitals like other static fields.
|
|
||
| private LocalSchemaRegistryServer localSchemaRegistryServer; | ||
| private SchemaRegistryTestConfiguration schemaRegistryTestConfiguration; | ||
| private Map<String, Object> CLIENT_CONF; |
There was a problem hiding this comment.
minor nit: you may want to have camelCase like other instance fields.
There was a problem hiding this comment.
+1 Thanks @raju-saravanan for addressing the review comments.
|
Closes #195 |
This PR contains the following changes :
As part of refactoring the test cases, I have used some of the features available in JUnit master branch (BeforeParam and AfterParam feature, taken from this pull request: @BeforeParam/@AfterParam for Parameterized runner junit-team/junit4#1435) but not available in JUnit 4.12.