Mqtt Broker configuration binding #3839

davidgraeff · 2017-07-15T05:05:58Z

Abstract

At the moment it is possible to configure brokers in a textual way. Without intensively watching the log file, there is no way to tell if the provided configuration is correct and if the connection to the broker have been established, neither via the GUI (PaperUI) nor machine readable (REST interface).

This binding makes the configuration of brokers more intuitive and let the user add broker connections as things. The connections are synced to the io.transport.mqtt.MqttService and are therefore available for all other bindings to use.

Features include:

2 different Bridges: SystemBrokerConnection, BrokerConnection
Rich status feedback for system broker connections. No longer skimming through log files if a connection doesn't work.
Auto discovery of brokers: Enumeration of MqttService shared broker connections, best effort subnet and localhost scan on standard Mqtt ports
Configurable reconnect time: This is not possible via textual configuration yet
Security Certificate and public key pinning for ssl connections to brokers
Extensive tests for all handlers, the discovery services and the SSLContextProvider.

Discussion

Points that need to be discussed might be:

No channels There are no channels on the broker Things. Everything is published to Thing properties right now. The connection status could be made available via a "status channel" additionally.
Security The public key and certificate hashes for key and certificate pinning are stored in the thing configuration. I'm not sure if this is the safest places to store them as the configuration can be changed via the rest interface. It is at least the only way to let the user configure them easily at the moment.

I'm happy to hear some feedback.

Cheers,
David

davidgraeff · 2017-07-30T09:08:01Z

@htreu The PR is ready for review

htreu

Hi @davidgraeff, I gave this a first round. Looks good from what I can say. Please find some comments inline.

htreu · 2017-08-07T09:22:03Z

.../src/test/java/org/eclipse/smarthome/binding/mqttbroker/MqttWillAndTestamentBuilderTest.java

+import org.junit.Test;
+
+/**
+ * Tests cases for {@link HexUtils}.


actually for the MqttWillAndTestamentBuilder

htreu · 2017-08-07T09:24:55Z

...inding.mqttbroker.test/src/test/java/org/eclipse/smarthome/binding/mqttbroker/UtilsTest.java

+    public void invalidHex() {
+        final String hexTest = "a2a1a0ll";
+        byte[] test = HexUtils.fromHex(hexTest);
+        assertThat(hexTest, is(HexUtils.toHexString(test)));


I guess this will fail in line 56, so the assertion is obsolete.

Nope, makes sense this way: First an ascii hex is decoded to bytes and then those bytes are encoded to ascii hex again and compared.

In line 53 (@Test(expected = IllegalArgumentException.class)) you expect the code to throw the IAE which is done by the code in line 56. So I guess line 57 is never reached.

The following test case will run happily:

@Test(expected = IllegalArgumentException.class) public void invalidHex() { final String hexTest = "a2a1a0ll"; HexUtils.fromHex(hexTest); assertThat(true, is(false)); // should fail here if the line is reached. }

True. fixed

htreu · 2017-08-07T09:32:04Z

.../src/test/java/org/eclipse/smarthome/binding/mqttbroker/discovery/NetworkDiscoveryTests.java

+ *
+ * @author David Graeff - Initial contribution
+ */
+public class NetworkDiscoveryTests {


Is this actually the MqttServiceDiscoveryServiceTest?

htreu · 2017-08-07T09:32:10Z

.../src/test/java/org/eclipse/smarthome/binding/mqttbroker/discovery/NetworkDiscoveryTests.java

+import org.mockito.MockitoAnnotations;
+
+/**
+ * Tests cases for {@link HexUtils}.


for MqttServiceDiscoveryService?

htreu · 2017-08-07T09:32:25Z

.../test/java/org/eclipse/smarthome/binding/mqttbroker/discovery/DiscoveryMqttServiceTests.java

+import org.mockito.stubbing.Answer;
+
+/**
+ * Tests cases for {@link HexUtils}.


for NetworkDiscoveryService?

htreu · 2017-08-07T13:25:00Z

...ker/src/main/java/org/eclipse/smarthome/binding/mqttbroker/internal/ssl/PinTrustManager.java

@@ -0,0 +1,131 @@
+package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;


license header missing

htreu · 2017-08-07T13:29:19Z

....mqttbroker/src/main/java/org/eclipse/smarthome/binding/mqttbroker/internal/ssl/PinType.java

@@ -0,0 +1,6 @@
+package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;


license header missing

htreu · 2017-08-07T13:29:25Z

...oker/src/main/java/org/eclipse/smarthome/binding/mqttbroker/internal/ssl/PinnedCallback.java

@@ -0,0 +1,9 @@
+package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;


license header missing

htreu · 2017-08-07T13:29:35Z

...oker/src/main/java/org/eclipse/smarthome/binding/mqttbroker/internal/ssl/PinnedCallback.java

+package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;
+
+public interface PinnedCallback {
+    void pinnedLearnedHash(Pin pin);


please add some javadoc (all methods).

htreu · 2017-08-07T13:29:41Z

...in/java/org/eclipse/smarthome/binding/mqttbroker/internal/ssl/PinningSSLContextProvider.java

@@ -0,0 +1,39 @@
+package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;


license header missing

htreu · 2017-08-24T08:34:42Z

extensions/binding/org.eclipse.smarthome.binding.mqttbroker/.gitignore

@@ -0,0 +1 @@
+OSGI-INF/*.xml


please create the folder OSGI-INF and place the .gitignore with content *.xml inside. right now the folder is marked as new for git after checkout and build.

davidgraeff · 2017-08-27T18:00:03Z

@htreu Changes incorporated

htreu · 2017-08-28T09:03:43Z

@davidgraeff looks good so far, please take another look at this comment #3839 (comment). Thanks.

maggu2810

It seems my comments has been incorporated.
So, okay from my side.

Thanks @davidgraeff for your whole MQTT work.

htreu · 2018-04-10T07:25:28Z

@davidgraeff do you want to wait for #5364 or adopt the timeout with another PR?

davidgraeff · 2018-04-10T07:54:20Z

It is not possible to test broker handlers without a timeout. I will never get a thing online/offline status update. Therefore I'll reckon waiting is the better option.

htreu · 2018-04-11T11:32:18Z

Travis complains, I guess we already know this one:

[ERROR] /home/travis/build/eclipse/smarthome/extensions/binding/org.eclipse.smarthome.binding.mqtt/src/main/java/org/eclipse/smarthome/binding/mqtt/internal/MqttBrokerHandlerFactory.java:[38] 
[ERROR] 	@Component(immediate = true, service = ThingHandlerFactory.class, configurationPid = { "MqttBrokerHandlerFactory" })
[ERROR] 	                                                                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[ERROR] Type mismatch: cannot convert from String[] to String

htreu · 2018-04-11T14:45:33Z

Some more Travis goodness:

[WARNING] /home/travis/build/eclipse/smarthome/extensions/binding/org.eclipse.smarthome.binding.mqtt/META-INF/MANIFEST.MF:[32]
The version of the package org.osgi.service.cm should not be specified
[ERROR] org.eclipse.smarthome.binding.mqtt.internal.ssl.PinnedCallback.java:[2]
Header line doesn't match pattern ^ \* Copyright \(c\) 2014,2018 Contributors to the Eclipse Foundation$

davidgraeff · 2018-04-12T12:01:08Z

@htreu Done

htreu

Tested locally (w/o actual broker), works nicely with the MQTT connections from the managed service.

Thanks for the awesome work @davidgraeff!

@kaikreuzer let me know if you want to have another look and change your review status.

kaikreuzer

Yes, I still do have a few comments, please see below.

kaikreuzer · 2018-04-16T08:59:00Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/ESH-INF/binding/binding.xml

+				 xmlns:binding="http://eclipse.org/smarthome/schemas/binding/v1.0.0"
+				 xsi:schemaLocation="http://eclipse.org/smarthome/schemas/binding/v1.0.0 http://eclipse.org/smarthome/schemas/binding-1.0.0.xsd">
+
+	<name>MQTT Broker connections</name>


This file describes the whole "mqtt" binding - not just the parts of this bundle.
So the name should imho be "MQTT Binding".

kaikreuzer · 2018-04-16T08:59:41Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/ESH-INF/binding/binding.xml

+				 xsi:schemaLocation="http://eclipse.org/smarthome/schemas/binding/v1.0.0 http://eclipse.org/smarthome/schemas/binding-1.0.0.xsd">
+
+	<name>MQTT Broker connections</name>
+	<description>Add/remove MQTT broker connections and manage system-wide configured connections</description>


Also update to be applicable for the binding in general.

kaikreuzer · 2018-04-16T08:59:55Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/ESH-INF/i18n/mqttbroker_de.properties

@@ -0,0 +1,7 @@
+binding.mqtt.name = MQTT Broker


kaikreuzer · 2018-04-16T09:00:01Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/ESH-INF/i18n/mqttbroker_de.properties

@@ -0,0 +1,7 @@
+binding.mqtt.name = MQTT Broker
+binding.mqtt.description = Konfiguriere MQTT Broker Verbindungen


kaikreuzer · 2018-04-16T09:00:24Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/ESH-INF/thing/thing-types.xml

+				<context>network-address</context>
+			</parameter>
+
+            <parameter name="secure" type="text" required="true">


please apply the formatter

kaikreuzer · 2018-04-16T09:37:53Z

...main/java/org/eclipse/smarthome/binding/mqtt/internal/discovery/NetworkDiscoveryService.java

+ *
+ * @author David Graeff - Initial contribution
+ */
+@Component(immediate = true, service = DiscoveryService.class, configurationPid = "org.eclipse.smarthome.NetworkDiscoveryService")


-> pid discovery.networkmqttbroker

kaikreuzer · 2018-04-16T09:39:01Z

...main/java/org/eclipse/smarthome/binding/mqtt/internal/discovery/NetworkDiscoveryService.java

+        lock.tryLock(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
+
+        if (testConnection.connectionState() == MqttConnectionState.CONNECTED) {
+            logger.trace("Found service device at {}",


change to fully parameterised logging: {}:{}.

kaikreuzer · 2018-04-16T09:39:52Z

...main/java/org/eclipse/smarthome/binding/mqtt/internal/discovery/NetworkDiscoveryService.java

+
+            ThingUID thingUID = MqttThingID.getThingUID(testConnection.getHost(), testConnection.getPort());
+            thingDiscovered(DiscoveryResultBuilder.create(thingUID).withTTL(120).withProperties(properties)
+                    .withLabel("Mqtt Broker (" + testConnection.getHost() + ")").build());


-> "MQTT Broker" and declare "host" as representation property

kaikreuzer · 2018-04-16T09:40:36Z

...main/java/org/eclipse/smarthome/binding/mqtt/internal/discovery/NetworkDiscoveryService.java

+                try {
+                    scanTarget(c);
+                } catch (ConfigurationException | MqttException | InterruptedException e) {
+                    logger.trace("Scan of {} failed", c.getHost() + ":" + String.valueOf(c.getPort()));


change to fully parameterised logging

kaikreuzer · 2018-04-16T09:41:31Z

...main/java/org/eclipse/smarthome/binding/mqtt/internal/discovery/NetworkDiscoveryService.java

+
+        for (MqttBrokerConnection c : o) {
+            scheduler.execute(() -> {
+                Thread.currentThread().setName("Discovery thread " + c.getHost() + ":" + String.valueOf(c.getPort()));


Do not change the thread names - they are from a re-used pool and they are already named by the framework.

davidgraeff · 2018-04-22T22:59:15Z

Comments are addressed. If the readme is meant to describe all features of the other MQTT bundles, it still needs some editing though.

kaikreuzer

Thanks for the update - looks good to me, just a few small issues left to fix; I'll quickly create a commit to fix them myself, so that @htreu can create the CQ for this PR.

f the readme is meant to describe all features of the other MQTT bundles, it still needs some editing though.

No, I think it is just fine as it is, thanks!

kaikreuzer · 2018-04-23T10:02:04Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/ESH-INF/thing/thing-types.xml

+            <parameter name="port" type="integer">
+                <label>Broker Port</label>
+                <description>The port is optional, if none is provided, the typical ports 1883 and 8883 (SSL) are used.</description>
+                <context>network-address</context>


the port must not have context network-address.

kaikreuzer · 2018-04-23T10:02:28Z

extensions/binding/org.eclipse.smarthome.binding.mqtt/META-INF/MANIFEST.MF

@@ -1,7 +1,7 @@
 Manifest-Version: 1.0
 Automatic-Module-Name: org.eclipse.smarthome.binding.mqtt
 Bundle-ManifestVersion: 2
-Bundle-Name: MQTT Broker Connections
+Bundle-Name: MQTT Binging


kaikreuzer · 2018-04-23T10:05:07Z

...main/java/org/eclipse/smarthome/binding/mqtt/internal/discovery/NetworkDiscoveryService.java


            Map<String, Object> properties = new HashMap<>();
            properties.put("host", testConnection.getHost());
            properties.put("port", testConnection.getPort());

            ThingUID thingUID = MqttThingID.getThingUID(testConnection.getHost(), testConnection.getPort());
            thingDiscovered(DiscoveryResultBuilder.create(thingUID).withTTL(120).withProperties(properties)
-                    .withLabel("Mqtt Broker (" + testConnection.getHost() + ")").build());
+                    .withRepresentationProperty(testConnection.getHost()).withLabel("MQTT Broker").build());


the representation property must be the key, not the value!

kaikreuzer · 2018-04-23T10:17:05Z

Done with 509ab06.

@htreu I think everything is ready for creating a CQ now.

htreu · 2018-04-23T14:23:28Z

Unfortunately we have to tranquillise Travis one more time:

unnecessary Mockito stubbings(org.eclipse.smarthome.binding.mqtt.internal.discovery.NetworkDiscoveryServiceTest)  Time elapsed: 0.072 sec  <<< ERROR!
org.mockito.exceptions.misusing.UnnecessaryStubbingException: 
Unnecessary stubbings detected in test class: NetworkDiscoveryServiceTest
Clean & maintainable test code requires zero unnecessary code.
Following stubbings are unnecessary (click to navigate to relevant line of code):
  1. -> at org.eclipse.smarthome.binding.mqtt.internal.discovery.NetworkDiscoveryServiceTest$NetworkDiscoveryServiceEx.createTestConnections(NetworkDiscoveryServiceTest.java:80)
  2. -> at org.eclipse.smarthome.binding.mqtt.internal.discovery.NetworkDiscoveryServiceTest$NetworkDiscoveryServiceEx.createTestConnections(NetworkDiscoveryServiceTest.java:81)
Please remove unnecessary stubbings or use 'silent' option. More info: javadoc for UnnecessaryStubbingException class.

This is the main mqtt binding. At the moment it is possible to configure brokers in a textual way. Without intensively watching the log file, there is no way to tell if the provided configuration is correct and if the connection to the broker have been established, neither via the GUI (PaperUI) nor machine readable (REST interface). This binding makes the configuration of brokers more intuitive and let the user add broker connections as things. The connections are synced to the io.transport.mqtt MqttService and are therefore available for all other bindings to use. Features include: * 2 different Bridges: SystemBrokerConnection, BrokerConnection * Rich status feedback for textual configured brokers. No longer skimming through log files if a connection doesn't work. * Auto discovery of brokers (Enumeration of MqttService shared broker connections, best effort subnet and localhost scan on standard Mqtt ports) * Configurable reconnect time (this is not possible via textual configuration yet) * Certificate and public key pinning for ssl connections to brokers * Extensive tests for all handlers, the discovery services and the SSLContextProvider. Signed-off-by: David Graeff <david.graeff@web.de>

htreu · 2018-04-24T12:12:01Z

FTR: CQ16168 created.

htreu · 2018-04-30T06:51:05Z

FTR: CQ16168 approved.

This is the main mqtt binding. At the moment it is possible to configure brokers in a textual way. Without intensively watching the log file, there is no way to tell if the provided configuration is correct and if the connection to the broker have been established, neither via the GUI (PaperUI) nor machine readable (REST interface). This binding makes the configuration of brokers more intuitive and let the user add broker connections as things. The connections are synced to the io.transport.mqtt MqttService and are therefore available for all other bindings to use. Features include: * 2 different Bridges: SystemBrokerConnection, BrokerConnection * Rich status feedback for textual configured brokers. No longer skimming through log files if a connection doesn't work. * Auto discovery of brokers (Enumeration of MqttService shared broker connections, best effort subnet and localhost scan on standard Mqtt ports) * Configurable reconnect time (this is not possible via textual configuration yet) * Certificate and public key pinning for ssl connections to brokers * Extensive tests for all handlers, the discovery services and the SSLContextProvider. Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff changed the title ~~[WIP] Mqtt binding~~ [WIP] Mqtt Broker configuration binding Jul 15, 2017

davidgraeff force-pushed the mqtt_binding branch 2 times, most recently from 66ca3ba to 44d52cd Compare July 21, 2017 12:19

davidgraeff changed the title ~~[WIP] Mqtt Broker configuration binding~~ [RFC] Mqtt Broker configuration binding Jul 21, 2017

davidgraeff force-pushed the mqtt_binding branch 8 times, most recently from c52073b to 9b841e5 Compare July 29, 2017 09:08

davidgraeff force-pushed the mqtt_binding branch 2 times, most recently from fca6078 to db4c2ca Compare July 30, 2017 08:22

davidgraeff changed the title ~~[RFC] Mqtt Broker configuration binding~~ Mqtt Broker configuration binding Jul 30, 2017

davidgraeff mentioned this pull request Jul 30, 2017

MqttEmbeddedBroker IO extension #3910

Merged

davidgraeff force-pushed the mqtt_binding branch from db4c2ca to 4705710 Compare July 30, 2017 09:13

htreu reviewed Aug 7, 2017

View reviewed changes

davidgraeff force-pushed the mqtt_binding branch 6 times, most recently from 50b18f5 to 4a12e3d Compare August 22, 2017 15:34

davidgraeff force-pushed the mqtt_binding branch from 4a12e3d to d74a140 Compare August 24, 2017 04:28

htreu reviewed Aug 24, 2017

View reviewed changes

davidgraeff force-pushed the mqtt_binding branch from d74a140 to 364bf2d Compare August 27, 2017 17:59

davidgraeff force-pushed the mqtt_binding branch 3 times, most recently from 2d83fa3 to 1bcef6c Compare April 9, 2018 13:44

davidgraeff mentioned this pull request Apr 9, 2018

MqttConnection: Add timeout #5364

Merged

davidgraeff force-pushed the mqtt_binding branch from 1bcef6c to 427d668 Compare April 9, 2018 13:59

maggu2810 approved these changes Apr 9, 2018

View reviewed changes

davidgraeff force-pushed the mqtt_binding branch from b481a71 to d984635 Compare April 11, 2018 09:26

htreu approved these changes Apr 13, 2018

View reviewed changes

kaikreuzer suggested changes Apr 16, 2018

View reviewed changes

kaikreuzer suggested changes Apr 23, 2018

View reviewed changes

kaikreuzer approved these changes Apr 23, 2018

View reviewed changes

davidgraeff force-pushed the mqtt_binding branch from 509ab06 to 0313e15 Compare April 23, 2018 16:12

davidgraeff force-pushed the mqtt_binding branch from 0313e15 to 3f904c1 Compare April 23, 2018 16:17

davidgraeff mentioned this pull request Apr 24, 2018

Mqtt generic things and Homie 3.x things #5450

Closed

htreu added the CQ label Apr 27, 2018

htreu removed the CQ label Apr 30, 2018

htreu merged commit 3250489 into eclipse-archived:master Apr 30, 2018

htreu added this to the 0.10.0 milestone Oct 30, 2018

		@@ -0,0 +1,131 @@
		package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;

		@@ -0,0 +1,6 @@
		package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;

		@@ -0,0 +1,9 @@
		package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;

		@@ -0,0 +1,39 @@
		package org.eclipse.smarthome.binding.mqttbroker.internal.ssl;

		@@ -0,0 +1,7 @@
		binding.mqtt.name = MQTT Broker
		binding.mqtt.description = Konfiguriere MQTT Broker Verbindungen

		@@ -0,0 +1 @@
		OSGI-INF/*.xml

Mqtt Broker configuration binding #3839

Mqtt Broker configuration binding #3839

Conversation

davidgraeff commented Jul 15, 2017 • edited Loading

Abstract

Discussion

davidgraeff commented Jul 30, 2017

htreu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Aug 27, 2017

htreu commented Aug 28, 2017

maggu2810 left a comment

Choose a reason for hiding this comment

htreu commented Apr 10, 2018

davidgraeff commented Apr 10, 2018

htreu commented Apr 11, 2018

htreu commented Apr 11, 2018

davidgraeff commented Apr 12, 2018

htreu left a comment

Choose a reason for hiding this comment

kaikreuzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgraeff commented Apr 22, 2018

kaikreuzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Apr 23, 2018

htreu commented Apr 23, 2018

htreu commented Apr 24, 2018

htreu commented Apr 30, 2018

davidgraeff commented Jul 15, 2017 •

edited

Loading

davidgraeff Aug 16, 2017 •

edited

Loading