Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented Option to choose default network interface #3930

Merged
merged 8 commits into from Aug 11, 2017

Conversation

Projects
None yet
3 participants
@triller-telekom
Copy link
Contributor

commented Aug 1, 2017

Fixes #3884

Signed-off-by: Stefan Triller stefan.triller@telekom.de

@triller-telekom triller-telekom force-pushed the triller-telekom:networkChooser branch from 617471b to c355177 Aug 2, 2017

@triller-telekom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2017

Close/reopen due to eclipse repository not available...

@kaikreuzer
Copy link
Member

left a comment

I think there needs to be some cleanup wrt ip address vs network interfaces.
You might also want to check, what other methods might be worthwhile for NetworkInterfaceService - e.g. we could support retrieving the IPv6 address for the primary interface as well as broadcast addresses, etc.

* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*/
package org.eclipse.smarthome.config.core.net;

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 2, 2017

Member

Is there any need to have this in an exported package?

<config-description uri="system:network">
<parameter name="defaultInterface" type="text">
<label>Default Interface</label>
<description><![CDATA[

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 2, 2017

Member

No need to put this in a CDATA section.

http://eclipse.org/smarthome/schemas/config-description-1.0.0.xsd">

<config-description uri="system:network">
<parameter name="defaultInterface" type="text">

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 2, 2017

Member

Does this parameter really hold a network interface identifier or rather an IPv4 address?
Specifying the interface probably makes sense as it does not have to be updated, if the runtime is assigned a new ip address (on the same interface and subnet) - but this means that your NetworkInterfaceService should also be adapted.

@kaikreuzer kaikreuzer added the Core label Aug 2, 2017

@triller-telekom triller-telekom force-pushed the triller-telekom:networkChooser branch from d213661 to 8054c64 Aug 3, 2017

Implemented Option to choose default network interface
Fixes #3884

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>

@triller-telekom triller-telekom force-pushed the triller-telekom:networkChooser branch from 8054c64 to 5875895 Aug 9, 2017

@triller-telekom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2017

rebased against current master to resolve conflicts

<parameter name="primaryAddress" type="text">
<label>Primary Address</label>
<description><![CDATA[<p>The primary network to be used</p>
<p>Alternative: Enter an IP address (XXX.XXX.XXX.XXX) manually</p>]]></description>

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

You do not mention the alternative to the alternative :-)

Better say:

A subnet (e.g. 192.168.1.0/24) or an IP address (e.g. 192.168.1.5)
Import-Package: com.google.common.base,
com.google.common.collect,
com.google.gson,
org.apache.commons.lang.reflect,
org.eclipse.jdt.annotation;resolution:=optional,
org.apache.commons.net.util,

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

We imho must not introduce this dependency to the ESH core bundles.

@@ -39,6 +39,7 @@ Import-Package: com.google.common.base,
org.apache.commons.io,
org.apache.commons.lang,
org.eclipse.jdt.annotation;resolution:=optional,
org.apache.commons.net.util,

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

We imho must not introduce this dependency to the ESH core bundles.

@Modified
public synchronized void modified(Map<String, Object> config) {
String defaultInterfaceConfig = (String) config.get(PRIMARY_ADDRESS);
if (defaultInterfaceConfig == null || defaultInterfaceConfig.equals("")) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

better use .isEmpty()

*/
public interface NetworkAddressProvider {

public String getPrimaryIpv4HostAddress();

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

How about adding a NonNull annotation? :-)
On an interface, you can remove the "public" keyword.

@@ -180,6 +180,27 @@ Furthermore bindings can specify a localized description of the thing status by
rate_limit=Device is blocked by remote service for {0} minutes. Maximum limit of {1} configuration changes per {2} has been exceeded. For further info please refer to device vendor.
```

## Offering a callback URL

Some things might require a `callback` URL which should be bound to a certain network interface. A user can configure his default network address via Paper UI under `Configuration -> System -> Network Settings`. To obtain this configured address the `ThingHandlerFactory` needs a `service reference` to the `NetworkInterfaceService` in its `OSGI-INF/MyHandlerFactory.xml`:

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

Please add a new line for every sentence - this makes diffing and reviews easier.

The correct name seems to be NetworkAddressProvider.
The example is not good, because we by now ask people to use DS annotations, not XML files.

@@ -180,6 +180,27 @@ Furthermore bindings can specify a localized description of the thing status by
rate_limit=Device is blocked by remote service for {0} minutes. Maximum limit of {1} configuration changes per {2} has been exceeded. For further info please refer to device vendor.
```

## Offering a callback URL

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

I am not sure whether this is the correct place for the documentation of this feature as the feature is completely independent of bindings. It is often used for the implementation of AudioSinks and potentially other extensions. We should rather have a place for such general system services.

@@ -171,4 +172,12 @@ protected void unsetAudioHTTPServer(AudioHTTPServer audioHTTPServer) {
this.audioHTTPServer = null;
}

protected void setNetworkAddressProvider(NetworkAddressProvider networkUtil) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

change networkUtil to networkAddressProvider

@@ -171,4 +172,12 @@ protected void unsetAudioHTTPServer(AudioHTTPServer audioHTTPServer) {
this.audioHTTPServer = null;
}

protected void setNetworkAddressProvider(NetworkAddressProvider networkUtil) {
this.networkAddressprovider = networkUtil;

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 9, 2017

Member

change networkAddressprovider to networkAddressProvider

this.networkAddressprovider = networkUtil;
}

protected void unsetNetworkAddressProvider(NetworkAddressProvider networkUtil) {

This comment has been minimized.

Copy link
@kaikreuzer

triller-telekom added some commits Aug 10, 2017

Removed dependency to apache.commons.net + use DS annotations
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Store subnets and temporarily limit to options
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Link documentation in menu
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
private List<ParameterOption> getIPv4Addresses() {
ArrayList<ParameterOption> interfaceOptions = new ArrayList<>();

HashSet<String> subnets = new HashSet<>();

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Change first occurrence to Set

String subNetString = NetUtil.getIpv4NetAddress(ipv4Address, ifAddr.getNetworkPrefixLength()) + "/"
+ String.valueOf(ifAddr.getNetworkPrefixLength());

subnets.add(subNetString);

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Although you didn't add any annotation to NetUtil.getIpv4NetAddress, I assume that subNetString can be null here.

primaryIP = addrString[0];
}

return primaryIP;

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

How did you convince your IDE that primaryIP is definitely not null here?

}

/**
* Get the first candidate for a local IPv4 host address (non loopback, non localhost).
*/
@Deprecated

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Please add a message about what people should now use instead.

* @param netMask netmask in bits (i.e. 24)
* @return network a device is in (i.e. 192.168.5.0)
*/
public static String getIpv4NetAddress(String ipAddressString, short netMask) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

where are the null annotation?

public class NetUtil {
@Component(name = "org.eclipse.smarthome.network", property = { "service.config.description.uri=system:network",
"service.config.label=Network Settings", "service.config.category=system" })
public class NetUtil implements NetworkAddressProvider {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Sorry, just realised your name choice now: "Provider" has a very special meaning in many contexts in ESH. NetworkAddressService would probably a more neutral choice.

String ip = getIPv4inSubnet(primaryAddress);
if (ip == null) {
// an error has occurred, using first interface like nothing has been configured
LOGGER.warn("Error in IP configuration, will continue to use first interface");

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Change to:

`"Invalid address '{}', will use first interface instead.", primaryAddress);`
}
} catch (SocketException ex) {
LOGGER.error("Could not retrieve network interface: {}", ex.getMessage(), ex);
return null;

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

this line is superfluous.


In this chapter useful services of the Eclipse SmartHome project are described.

## Obtaining the default IP address

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Use the service name for the header: "Network Address Service" or something like that.

@@ -29,6 +30,7 @@
*
* @author Karel Goderis - Initial contribution
*/
@Component(name = "org.eclipse.smarthome.binding.sonos.discovery.zoneplayer", immediate = true)

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 10, 2017

Member

Afaik, we do not need a specific service name for that one, so I'd suggest to go for the default and remove that name property here.

triller-telekom added some commits Aug 11, 2017

renamed NetworkAddressProvider to NetworkAddressService
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Use List instead of ArrayList for variable definition
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
* @param netMask netmask in bits (i.e. 24)
* @return network a device is in (i.e. 192.168.5.0)
*/
public static @NonNull String getIpv4NetAddress(@NonNull String ipAddressString, short netMask) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

If you want to assure to always return valid non-null results, you should at least also throw IllegalArgumentExceptions. Or what would you return if I pass you "ABC" as my ipAddressString?

String[] ipv4AddressOctets = ipAddressString.split("\\.");
String netAddress = "";
for (int i = 0; i < 4; i++) {
netAddress += Integer.parseInt(ipv4AddressOctets[i]) & Integer.parseInt(netMaskOctets[i]);

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

You'll need to catch NumberFormatException - this should not bubble up in the call stack.

* @param netMask netmask in bits (i.e. 24)
* @return network a device is in (i.e. 192.168.5.0)
*/
public static @NonNull String getIpv4NetAddress(@NonNull String ipAddressString, short netMask) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

This method is a wonderful candidate for a couple of unit tests!


private String primaryAddress;

@SuppressWarnings("unchecked")

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

Why that?

private String primaryAddress;

@SuppressWarnings("unchecked")
protected void activate(ComponentContext componentContext) {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

Instead of pulling the properties out of the context, why don't you simply implement activate(Map<String, Object> props)?

public interface NetworkAddressService {

@Nullable
String getPrimaryIpv4HostAddress();

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

please add JavaDoc

layout: documentation
---

# Provided Services

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

Shouldn't it now say "Framework Utilities"?

## Network Address Service

A user can configure his default network address via Paper UI under `Configuration -> System -> Network Settings`.
To obtain this configured address the `ThingHandlerFactory` needs a `service reference` to the `NetworkAddressService` in its `MyHandlerFactory.java`:

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

This is still binding specific doc

To obtain this configured address the `ThingHandlerFactory` needs a `service reference` to the `NetworkAddressService` in its `MyHandlerFactory.java`:

```java
@Reference

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

Do you really think we need to explain how OSGi DS works for every single service we offer?
Wouldn't it be more important to state that the NetworkAddressService IS an OSGi service that can be used?

```

Now the `MyHandlerFactory` can obtain the configured IP address via `networkAddressService.getPrimaryIpv4HostAddress()`.
This IP address can for example be used for things (i.e. AudioSinks) that require a callback URL which they can offer to a device.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

No, examples are ThingHandlerFactory and AudioSink implementations. (note: thing!=AudioSink)

Added jUnit test and Javadocs
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
String primaryAddressConf = (String) config.get(PRIMARY_ADDRESS);
if (primaryAddressConf == null || primaryAddressConf.isEmpty() || !isValidIPConfig(primaryAddressConf)) {
// if none is specified we return the default one for backward compatibility
LOGGER.warn("Non valid IP configuration found, will continue to use first interface");

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

As >90% of users will have exactly one interface, there is no need to set this configuration parameter.
Why should you bother them with a warning?
You should only show a warning if the parameter is invalid.

+ "' out of bounds (1-31)";

if (!isValidIPConfig(ipAddressString) || netMask < 1 || netMask > 31) {
throw new IllegalArgumentException(errorString);

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

You should mention this exception in the JavaDoc.

/**
* Returns the user configured primary IPv4 address of the system
*
* @return IPv4 address as a String in format xxx.xxx.xxx.xxx

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

.. or null if bla bla bla...?

## Network Address Service

A user can configure his default network address via Paper UI under `Configuration -> System -> Network Settings`.
The `NetworkAddressService` is an OSGi service that can be used like any other OSGi service by adding a service reference to it.

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

This should be the first sentence of the paragraph. Maybe also mention that it is a configurable service, mentioning its service id. The Paper UI example above is then just one way - in general anything that uses ConfigurationAdmin can be used to configure the service.

}

String ipv4Address = addr.getHostAddress();
String subNetString = NetUtil.getIpv4NetAddress(ipv4Address, ifAddr.getNetworkPrefixLength()) + "/"

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

What do you do if this now throws the newly introduced IllegalArgumentException?

More tests
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2017

close/reopen because of unreachable maven repo

@kaikreuzer
Copy link
Member

left a comment

Thanks - just some last comments, but nothing that prevents merging it now.


try {
network = NetUtil.getIpv4NetAddress("192.168.5.8", (short) 32);
;

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

Is this line required?

;
} catch (IllegalArgumentException iae) {
assertThat(iae.getMessage(),
is("IP '192.168.5.8' not a valid IPv4 address or netmask '32' out of bounds (1-31)"));

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

I would actually except a clear message about the error and not "this or that is wrong".

"Netmask '32' is out of bounds (1-31)"

Would be the expected result, do you agree?

network = NetUtil.getIpv4NetAddress("192.168.58", (short) 24);
} catch (IllegalArgumentException iae) {
assertThat(iae.getMessage(),
is("IP '192.168.58' not a valid IPv4 address or netmask '24' out of bounds (1-31)"));

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

"IP '192.168.58' is not a valid IPv4 address."

network = NetUtil.getIpv4NetAddress("SOME_TEXT", (short) 24);
} catch (IllegalArgumentException iae) {
assertThat(iae.getMessage(),
is("IP 'SOME_TEXT' not a valid IPv4 address or netmask '24' out of bounds (1-31)"));

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

"IP 'SOME_TEXT' is not a valid IPv4 address"


String ipv4Address = addr.getHostAddress();
try {
// InetAddress.getHostAddress() is missing a @NonNull annotation

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Aug 11, 2017

Member

You can make it clear by code, which conversion you are suppressing - no need for this comment. Just do above the following:

        @SuppressWarnings("null")
        @NonNull
        String ipv4Address = addr.getHostAddress();

I think that is the better style we should follow.

@kaikreuzer kaikreuzer merged commit 0c4353e into eclipse:master Aug 11, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ip-validation
Details

@triller-telekom triller-telekom deleted the triller-telekom:networkChooser branch Aug 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.