[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1587: MINIFICPP-2135 Add SSL support for Prometheus reporter

["szaszm (via GitHub)" <gi...@apache.org>]

szaszm commented on code in PR #1587:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1587#discussion_r1228023558


##########
docker/test/integration/cluster/checkers/PrometheusChecker.py:
##########
@@ -18,7 +18,16 @@
 
 class PrometheusChecker:
     def __init__(self):
-        self.prometheus_client = PrometheusConnect(url="http://localhost:9090", disable_ssl=True)
+        self.use_ssl = False
+
+    def enable_ssl(self):
+        self.use_ssl = True
+
+    def _getClient(self):
+        if self.use_ssl:
+            return PrometheusConnect(url="https://localhost:9090", disable_ssl=True)

Review Comment:
   `disable_ssl=True`?



##########
docker/test/integration/cluster/containers/MinifiContainer.py:
##########
@@ -109,12 +110,16 @@ def _create_properties(self):
             if not self.options.enable_provenance:
                 f.write("nifi.provenance.repository.class.name=NoOpRepository\n")
 
-            if self.options.enable_prometheus:
+            if self.options.enable_prometheus or self.options.enable_prometheus_with_ssl:
                 f.write("nifi.metrics.publisher.agent.identifier=Agent1\n")
                 f.write("nifi.metrics.publisher.class=PrometheusMetricsPublisher\n")
                 f.write("nifi.metrics.publisher.PrometheusMetricsPublisher.port=9936\n")
                 f.write("nifi.metrics.publisher.metrics=RepositoryMetrics,QueueMetrics,PutFileMetrics,processorMetrics/Get.*,FlowInformation,DeviceInfoNode,AgentStatus\n")
 
+            if self.options.enable_prometheus_with_ssl:
+                f.write("nifi.metrics.publisher.PrometheusMetricsPublisher.certificate=/tmp/resources/prometheus-ssl/minifi-cpp-flow.crt\n")
+                f.write("nifi.metrics.publisher.PrometheusMetricsPublisher.ca.certificate=/tmp/resources/prometheus-ssl/root-ca.pem\n")

Review Comment:
   Why do we have both `minifi-cpp-flow.crt` and `prometheus.crt`? One TLS-enabled web server needs 1 key+cert pair.



##########
extensions/prometheus/PrometheusMetricsPublisher.cpp:
##########
@@ -33,18 +33,28 @@ PrometheusMetricsPublisher::PrometheusMetricsPublisher(const std::string &name,
 void PrometheusMetricsPublisher::initialize(const std::shared_ptr<Configure>& configuration, const std::shared_ptr<state::response::ResponseNodeLoader>& response_node_loader) {
   state::MetricsPublisher::initialize(configuration, response_node_loader);
   if (!exposer_) {
-    exposer_ = std::make_unique<PrometheusExposerWrapper>(readPort());
+    exposer_ = std::make_unique<PrometheusExposerWrapper>(readExposerConfig());
   }
   loadAgentIdentifier();
 }
 
-uint32_t PrometheusMetricsPublisher::readPort() {
+PrometheusExposerConfig PrometheusMetricsPublisher::readExposerConfig() const {
   gsl_Expects(configuration_);
+  PrometheusExposerConfig config;
   if (auto port = configuration_->get(Configuration::nifi_metrics_publisher_prometheus_metrics_publisher_port)) {
-    return std::stoul(*port);
+    config.port = std::stoul(*port);

Review Comment:
   `stoul` may throw, we should handle that as well



-- 
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: issues-unsubscribe@nifi.apache.org

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