[GitHub] [skywalking-java] wu-sheng commented on a diff in pull request #540: [Feature] Support to configure custom decryption methods for kafka configurations

["wu-sheng (via GitHub)" <gi...@apache.org>]

wu-sheng commented on code in PR #540:
URL: https://github.com/apache/skywalking-java/pull/540#discussion_r1207742755


##########
apm-sniffer/config/agent.config:
##########
@@ -263,6 +263,10 @@ plugin.kafka.topic_management=${SW_PLUGIN_KAFKA_TOPIC_MANAGEMENT:skywalking-mana
 plugin.kafka.topic_logging=${SW_PLUGIN_KAFKA_TOPIC_LOGGING:skywalking-logs}
 #  isolate multi OAP server when using same Kafka cluster (final topic name will append namespace before Kafka topics with `-` ).
 plugin.kafka.namespace=${SW_KAFKA_NAMESPACE:}
+# Specify which class to decrypt encrypted configuration of kafka.You can set encrypted information in `plugin.kafka.producer_config_json` or `plugin.kafka.producer_config` if you need.
+plugin.kafka.decrypt_class=${SW_KAFKA_DECRYPT_CLASS:}
+# Specify which mathed to decrypt encrypted configuration of kafka.

Review Comment:
   ```suggestion
   # Specify which method to decrypt encrypted configuration of kafka.
   ```
   



##########
apm-sniffer/optional-reporter-plugins/kafka-reporter-plugin/src/main/java/org/apache/skywalking/apm/agent/core/kafka/KafkaProducerManager.java:
##########
@@ -153,6 +156,18 @@ private void notifyListeners(KafkaConnectionStatus status) {
         }
     }
 
+    private Map<String, String> decrypt(Map<String, String> config) {
+        try {
+            Class<?> decryptClazz = Class.forName(Kafka.DECRYPT_CLASS);
+            Method decryptMethod = decryptClazz.getMethod(Kafka.DECRYPT_METHOD, Map.class);
+            return (Map<String, String>) decryptMethod.invoke(decryptClazz.newInstance(), config);
+        } catch (ClassNotFoundException | NoSuchMethodException | InstantiationException | IllegalAccessException | InvocationTargetException e) {
+            // ignore
+            LOGGER.warn("The decrypt class {} is not exist, exception:{}.", Kafka.DECRYPT_CLASS, e);

Review Comment:
   And, you are reflecting the class without checking whether this config existence, which mean , this warning will be there as always. AFAIK.
   
   Although I have suggested removing most of the reflection, still, you will still need reflections to create interface implementation. You should check the class existence before invoking.



##########
apm-sniffer/optional-reporter-plugins/kafka-reporter-plugin/src/main/java/org/apache/skywalking/apm/agent/core/kafka/KafkaProducerManager.java:
##########
@@ -153,6 +156,18 @@ private void notifyListeners(KafkaConnectionStatus status) {
         }
     }
 
+    private Map<String, String> decrypt(Map<String, String> config) {
+        try {
+            Class<?> decryptClazz = Class.forName(Kafka.DECRYPT_CLASS);
+            Method decryptMethod = decryptClazz.getMethod(Kafka.DECRYPT_METHOD, Map.class);
+            return (Map<String, String>) decryptMethod.invoke(decryptClazz.newInstance(), config);

Review Comment:
   I don't think reflection for the whole process is a good idea. You should add an interface in the reporter project,
   
   ```java
   public interface KafkaConfigExtension {
       Map<String, String> decode(Map<String, String> config);
   }
   ```
   
   Then this interface would be released, and user could simply depend on and package. Reflection should be something that has to be that way.
   
   Notice, your saying `decrypt` actually is just `decode`. Because decrypting usually means you have in-built algorithms.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org