Core/Network: Allow IPv6 for default network interface. #4218

davidgraeff · 2017-09-08T08:43:13Z

Clean up NetUtils. Fix NetworkConfigOptionProvider.

Fixes #4205

Signed-off-by: David Gräff david.graeff@web.de

triller-telekom

Thank you for adding IPv6 support.

Please see my comment in line.

triller-telekom · 2017-09-12T11:36:28Z

bundles/core/org.eclipse.smarthome.core/ESH-INF/config/networkinterface.xml

@@ -7,7 +7,7 @@
 	<config-description uri="system:network">
 		<parameter name="primaryAddress" type="text">
 			<label>Primary Address</label>
-			<description>A subnet (e.g. 192.168.1.0/24) <!-- or an IP address (e.g. 192.168.1.5) --></description>
+			<description>An IPv4/IPv6 CIDR notation (e.g. 192.168.1.0/24 or 192.168.1.12 or fe80:0:0:0:0:0:c0a8:11%eth0/120)</description>


The idea was to have the NETWORK address here and not the IP address directly. The reason behind it is that if the ESH installation obtains its IP address via DHCP it might get a different one from the same network and we do not want to require the user to change the configuration if this happens. That's why the original implementation in #3930 calculated the NETWORK address and stored it as a configuration value.

I think we should restore this behavior and for IPv6 we would store the subnet, not the specific address as well. Because in IPv6 a host could use auto configuration and choose a different IP from the same subnet on every restart of the system (privacy extension).

you're right that there is an IP in the comment, that's my mistake

davidgraeff · 2017-09-12T11:56:44Z

You need to read the changes carefully :) the user selects the current IP with network prefix length. So it works as before but is less confusing for the user because we show what is currently assigned and it works for ipv4 and ipv6. There is no such think as network address actually. It's always IP + prefix.

triller-telekom

You are right, I overlooked your inRange method.

One minor comment: Do we want to offer the user that he can select if he prefers ipv4 or ipv6?

Also I am curious how you tested the modified method of the NetUtil class, because without #4253 you cannot save a configuration through paperui. That was broken when we introduced the configuration-pid.

triller-telekom · 2017-09-12T15:17:51Z

...es/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/NetUtil.java

-            return hostAddress;
-        } catch (SocketException ex) {
-            LOGGER.error("Could not retrieve network interface: {}", ex.getMessage(), ex);
+    private CidrAddress getAnyAssignedIPAddress(boolean preferIPv4) {


You are only calling this method with true which is hardcoded in the sources. Should we offer this parameter as an option for the user to choose?

The API can be used for both yes. And in the future we will probably switch the default to IPv6. But this is the default address that we pick here, that is used if the user didn't configure anything. Why should this be configurable?

Because the user might prefer to use IPv6 now? And if you only want to provide this option in the future, then please remove the parameter for now.

If the user wants ipv6 then he just chooses from the list that we present in paperui. Personally I would just pick the first IP we get from java to bind to. I just tried to be backwards compatible here by selecting an ipv4. Please be aware that no matter if we pick ipv4 or ipv6 as default, both protocols will work anyway.

If that is the case then I do not see the need for this parameter. I it's only about backward compatibility that you prefer IPv4 here then you can remove this parameter and prefer IPv4 nevertheless. In my opinion a method with a boolean parameter that is only called with true is superfluous.

The idea is to remove the parameter and to not prefer any IP version. I do not see why ipv4 should be preferred here, I know some pretty good ipv6 only setup. And the IP version doesn't actually matter, it is the interface that we bind to. At least on Linux such a socket works for ipv4+ipv6.

Fine for me. But please have a look at my netmask comment below.

triller-telekom · 2017-09-12T15:27:17Z

...ore/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/CidrAddress.java

+        if (prefix % 2 != 0) {
+            throw new IllegalArgumentException("Not a valid prefix. This must be valid: prefix mod 2 == 0");
+        }
+        if (prefix < 0 || prefix >= address.getAddress().length * 8) {


One error that I also have done in my previous PR: RFC1878 allows a prefix of 32 for a single host route, so you should test prefix > address.getAddress().length * 8

@davidgraeff Did you see this comment regarding the 32 bit netmask?

davidgraeff · 2017-09-13T02:52:56Z

Regarding testing: I didn't test with paperui, that needs to be done. I maybe should add a unit test for modified.

triller-telekom · 2017-09-13T07:38:20Z

PR #4253 has been merged, please rebase your branch.

It would be nice to have a test for the modified method. How about giving the user an option to set the preference for ipv4 vs ipv6? And please have a look at my comment regarding RFC1878.

kaikreuzer · 2017-09-25T07:05:07Z

What's the current status here? If I get it correctly, we are waiting for updates from @davidgraeff, right?

triller-telekom · 2017-09-25T09:51:42Z

Ye sit is about my comment regarding the 32 bit netmask here.

davidgraeff · 2017-09-25T10:32:50Z

done, no time for the modified test, though

triller-telekom · 2017-09-25T10:52:21Z

Travis failed: @davidgraeff Can you please have a look?

outOfBoundsPrefix(org.eclipse.smarthome.core.net.CidrTest)  Time elapsed: 0.002 sec  <<< FAILURE!
java.lang.AssertionError: Expected exception: java.lang.IllegalArgumentException
	at org.junit.internal.runners.statements.ExpectException.evaluate(ExpectException.java:32)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:264)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:124)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray2(ReflectionUtils.java:208)
	at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:156)
	at org.apache.maven.surefire.booter.ProviderFactory.invokeProvider(ProviderFactory.java:82)
	at org.eclipse.tycho.surefire.osgibooter.OsgiSurefireBooter.run(OsgiSurefireBooter.java:95)
	at org.eclipse.tycho.surefire.osgibooter.HeadlessTestApplication.run(HeadlessTestApplication.java:21)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.equinox.internal.app.EclipseAppContainer.callMethodWithException(EclipseAppContainer.java:587)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:198)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:388)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:243)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:653)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:590)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1499)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1472)

sjsf · 2017-09-26T15:50:40Z

...ore/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/CidrAddress.java

+    private static final Pattern IPV6PATTERN = Pattern
+            .compile("^(([0-9a-fA-F]{1,4}:){7}([0-9a-fA-F]){1,4}(%[\\S]*)?)$");
+
+    public CidrAddress(@NonNull InetAddress address, int networkPrefixLength) {


public constructors also should have some javadoc

sjsf · 2017-09-26T15:50:46Z

...ore/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/CidrAddress.java

+        return m2.matches();
+    }
+
+    public CidrAddress(String cidrNotation) throws IllegalArgumentException {


sjsf · 2017-09-26T15:58:27Z

...es/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/NetUtil.java

-            primaryAddress = primaryAddressConf;
+        try {
+            primaryAddress = new CidrAddress(primaryAddressConf);
+        } catch (IllegalArgumentException e) {


Wouldn't it make sense to at least log a warning? I mean, in the end it's the user who accidentally made a mistake/typo, the service silently "accepts" this configuration but it somehow does not really work as expected. Or did I miss anything?

sjsf · 2017-09-26T16:03:56Z

...es/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/net/NetUtil.java

-            return null;
-        }
+    public static @Nullable String getLocalIpv4HostAddress() {
+        return new NetUtil().getPrimaryIpv4HostAddress();


In this case, the user's configured primaryAddress will never be taken into account. This might not meet expectations...

This is why this method is deprecated. It is a static method, we have no bundle configuration, so the user selected address is not available.

sjsf · 2017-09-26T16:05:49Z

...e/org.eclipse.smarthome.core.test/src/test/java/org/eclipse/smarthome/core/net/CidrTest.java

+
+    @Test(expected = IllegalArgumentException.class)
+    public void outOfBoundsPrefix() {
+        new CidrAddress("192.168.5.8/33");


Does this work for IPv6 addresses? (i.e. is "2001:db8:0:0:8:800:200c:417a/33" valid)?

I have added test cases for IPv6

sjsf · 2017-10-02T09:44:16Z

Please have a look at the build failure

davidgraeff · 2017-10-02T10:06:46Z

[INFO] Eclipse SmartHome LIRC Binding ..................... SUCCESS [ 2.265 s]
[INFO] Eclipse SmartHome NTP Binding ...................... SUCCESS [ 2.119 s]
[INFO] Eclipse SmartHome NTP Binding Tests ................ SUCCESS [ 6.387 s]
[INFO] Eclipse SmartHome Sonos Binding .................... FAILURE [ 0.319 s]
[INFO] Tradfri Binding .................................... SKIPPED
[INFO] Tradfri Binding Tests .............................. SKIPPED

sjsf · 2017-10-02T10:53:22Z

A few lines below you can find the details:

[ERROR] Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:1.0.0:compile (default-compile) on project org.eclipse.smarthome.binding.sonos: Compilation failure: Compilation failure: 
[ERROR] /home/travis/build/eclipse/smarthome/extensions/binding/org.eclipse.smarthome.binding.sonos/src/main/java/org/eclipse/smarthome/binding/sonos/internal/SonosHandlerFactory.java:[125] 
[ERROR] 	final int port = HttpServiceUtil.getHttpServicePort(bundleContext);
[ERROR] 	                                                    ^^^^^^^^^^^^^
[ERROR] Null type mismatch (type annotations): required '@NonNull BundleContext' but this expression has type '@Nullable BundleContext'

Therefore it is related to your change of adding the null annotations to HttpServiceUtil.

davidgraeff · 2017-10-02T18:39:10Z

I agree. Should the null annotation be removed again? Or the failing bindings be fixed in this PR?

sjsf · 2017-10-05T07:09:55Z

Either way is fine - if it's just this one case then maybe it's forth fixing the binding right away. However, if you end up needing to fix pretty much everything everywhere, then I'd rather suggest going back. I'd leave the decision to you.

triller-telekom · 2017-11-03T10:25:15Z

@davidgraeff I think for now it would be best to remove this annotation and afterwards we can merge this PR. Blocking a PR for so long just because of this annotation is not really worth it in my opinion.

davidgraeff · 2017-11-03T18:05:26Z

I agree. I'm just not at home at the moment, I might have time on the weekend for all my open PRs.

openhab-bot · 2017-11-13T14:38:26Z

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/neeo-remote-binding-transport/33408/59

…tils. Fix NetworkConfigOptionProvider Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff · 2017-11-15T14:28:50Z

@triller-telekom @SJKA Problematic annotation removed

kaikreuzer

Thanks, @davidgraeff!

… up NetUtils. Fix NetworkConfigOptionProvider (#4218)" This reverts commit 753a771.

…e. Clean up NetUtils. Fix NetworkConfigOptionProvider (#4218)"" This reverts commit 0be827d.

davidgraeff · 2017-11-27T19:46:41Z

I consider this closed with #4616

davidgraeff mentioned this pull request Sep 8, 2017

NetUtil: Netmask '32' is out of bounds #4205

Closed

davidgraeff force-pushed the netutils3 branch 2 times, most recently from c4de760 to bd64e01 Compare September 8, 2017 09:48

triller-telekom suggested changes Sep 12, 2017

View reviewed changes

triller-telekom reviewed Sep 12, 2017

View reviewed changes

davidgraeff force-pushed the netutils3 branch from bd64e01 to c3d4dc8 Compare September 25, 2017 10:26

davidgraeff force-pushed the netutils3 branch from c3d4dc8 to 59da90a Compare September 25, 2017 18:46

triller-telekom mentioned this pull request Sep 26, 2017

prevent NPE if no network interface is present #4348

Closed

sjsf reviewed Sep 26, 2017

View reviewed changes

davidgraeff force-pushed the netutils3 branch from 59da90a to db71b25 Compare September 27, 2017 06:37

davidgraeff force-pushed the netutils3 branch from db71b25 to 5cf3179 Compare November 14, 2017 13:03

Core/Network: Allow IPv6 for default network interface. Clean up NetU…

1d19444

…tils. Fix NetworkConfigOptionProvider Signed-off-by: David Graeff <david.graeff@web.de>

davidgraeff force-pushed the netutils3 branch from 5cf3179 to 1d19444 Compare November 14, 2017 13:03

kaikreuzer approved these changes Nov 17, 2017

View reviewed changes

kaikreuzer merged commit 753a771 into eclipse-archived:master Nov 17, 2017

kaikreuzer added a commit that referenced this pull request Nov 18, 2017

Revert "Core/Network: Allow IPv6 for default network interface. Clean…

58215e8

… up NetUtils. Fix NetworkConfigOptionProvider (#4218)" This reverts commit 753a771.

kaikreuzer added a commit that referenced this pull request Nov 18, 2017

Revert "Core/Network: Allow IPv6 for default network interface. Clean…

0be827d

… up NetUtils. Fix NetworkConfigOptionProvider (#4218)" This reverts commit 753a771.

kaikreuzer added a commit that referenced this pull request Nov 20, 2017

Revert "Revert "Core/Network: Allow IPv6 for default network interfac…

08b9300

…e. Clean up NetUtils. Fix NetworkConfigOptionProvider (#4218)"" This reverts commit 0be827d.

triller-telekom mentioned this pull request Nov 23, 2017

Compare real netmasks not configured ip against netmask #4616

Merged

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

davidgraeff deleted the netutils3 branch July 9, 2018 10:38

Core/Network: Allow IPv6 for default network interface. #4218

Core/Network: Allow IPv6 for default network interface. #4218

Conversation

davidgraeff commented Sep 8, 2017 • edited

triller-telekom 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 commented Sep 12, 2017

triller-telekom 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

davidgraeff commented Sep 13, 2017

triller-telekom commented Sep 13, 2017

kaikreuzer commented Sep 25, 2017

triller-telekom commented Sep 25, 2017

davidgraeff commented Sep 25, 2017

triller-telekom commented Sep 25, 2017

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

sjsf Sep 26, 2017 • edited

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

sjsf commented Oct 2, 2017

davidgraeff commented Oct 2, 2017

sjsf commented Oct 2, 2017

davidgraeff commented Oct 2, 2017

sjsf commented Oct 5, 2017

triller-telekom commented Nov 3, 2017

davidgraeff commented Nov 3, 2017

openhab-bot commented Nov 13, 2017

davidgraeff commented Nov 15, 2017

kaikreuzer left a comment

Choose a reason for hiding this comment

davidgraeff commented Nov 27, 2017

davidgraeff commented Sep 8, 2017 •

edited

sjsf Sep 26, 2017 •

edited