Skip to content
This repository has been archived by the owner. It is now read-only.

Implemented Option to choose default network interface #3930

Merged
merged 8 commits into from Aug 11, 2017

Conversation

@triller-telekom
Copy link
Contributor

@triller-telekom triller-telekom commented Aug 1, 2017

Fixes #3884

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

@triller-telekom
Copy link
Contributor Author

@triller-telekom triller-telekom commented Aug 2, 2017

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

Copy link
Contributor

@kaikreuzer kaikreuzer 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;
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 2, 2017

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[
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 2, 2017

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">
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 2, 2017

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.

Fixes eclipse-archived#3884

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom
Copy link
Contributor Author

@triller-telekom triller-telekom 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>
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

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,
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

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,
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

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("")) {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

better use .isEmpty()

*/
public interface NetworkAddressProvider {

public String getPrimaryIpv4HostAddress();
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

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`:
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

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
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

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) {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

change networkUtil to networkAddressProvider

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

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

@kaikreuzer kaikreuzer Aug 9, 2017

change networkAddressprovider to networkAddressProvider

this.networkAddressprovider = networkUtil;
}

protected void unsetNetworkAddressProvider(NetworkAddressProvider networkUtil) {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 9, 2017

dito

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
private List<ParameterOption> getIPv4Addresses() {
ArrayList<ParameterOption> interfaceOptions = new ArrayList<>();

HashSet<String> subnets = new HashSet<>();
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

Change first occurrence to Set

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

subnets.add(subNetString);
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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

primaryIP = addrString[0];
}

return primaryIP;
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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) {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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 {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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");
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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;
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

this line is superfluous.


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

## Obtaining the default IP address
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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)
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 10, 2017

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.

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
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) {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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]);
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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) {
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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


private String primaryAddress;

@SuppressWarnings("unchecked")
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

Why that?

private String primaryAddress;

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

@kaikreuzer kaikreuzer Aug 11, 2017

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();
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

please add JavaDoc

layout: documentation
---

# Provided Services
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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`:
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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.
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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

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");
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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);
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

.. 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.
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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()) + "/"
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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

Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@triller-telekom
Copy link
Contributor Author

@triller-telekom triller-telekom commented Aug 11, 2017

close/reopen because of unreachable maven repo

Copy link
Contributor

@kaikreuzer kaikreuzer 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);
;
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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)"));
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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)"));
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

"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)"));
Copy link
Contributor

@kaikreuzer kaikreuzer Aug 11, 2017

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


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

@kaikreuzer kaikreuzer Aug 11, 2017

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 added this to the 0.9.0 milestone Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants