mgmt: hawkbit: change hawkbit interface to support directly supplying server ip address

[nealjack]

The hawkbit subsystem assumes that getaddrinfo will supply a valid, usable IP address. This isn't the case when using a network layer that depends on NAT64 for IPv4 traffic, like OpenThread. A DNS address obtained while behind a NAT64 requires conversion to IPv6 and adding the NAT64 prefix. As of now, getaddrinfo is not aware of NAT64 and does not handle this. In this case it can be better for the application itself to handle DNS resolution and provide properly prefixed IP addresses to the hawkbit subsystem.

This pull request adds an additional interface field (server_hostname) for the server hostname and changes server_addr to optionally point to an IP address. This is a potentially less confusing naming scheme, as hawkbit does not currently work if an address is provided to server_addr instead of a hostname.

This commit also increases the default DNS name length slightly to accommodate larger hostnames longer than 20 bytes, i.e. (hawkbit.yourcompany.com). I also added some error handling in case a hostname is too long, so the hawkbit interface returns an error instead of quietly using a truncated and incorrect hostname.

This commit also changes how the controllerId is generated based on device id, and disentangles the two. The controllerId is what hawkbit uses to uniquely identify a device, and is not necessarily the same as the device id, and should be fully customizable by the user if needed. Previously, all custom device ids were being prepended with CONFIG_BOARD. When a user selects CONFIG_HAWKBIT_CUSTOM_DEVICE_ID, they should be able to specify the full controllerId used with hawkbit.

[carlescufi]

Thanks for the PR! Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request.

[github-actions]

Hello @nealjack, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[jukkar]

Please remove the subsys: prefix from commit subject of the both commits, it is not needed

[maass-hamburg]

The hawkbit subsystem assumes that getaddrinfo will supply a valid, usable IP address. This isn't the case when using a network layer that depends on NAT64 for IPv4 traffic, like OpenThread. A DNS address obtained while behind a NAT64 requires conversion to IPv6 and adding the NAT64 prefix. As of now, getaddrinfo is not aware of NAT64 and does not handle this. In this case it can be better for the application itself to handle DNS resolution and provide properly prefixed IP addresses to the hawkbit subsystem.

From what I know, when using a NAT64 and you want you want to resolve DNS addresses you should also have a DNS64 server, that will do the transition for you. You could even use a public DNS64 server like the one from Google (https://developers.google.com/speed/public-dns/docs/dns64) for that. The application and the end device should not care and therefore also not know, that there is even a NAT64 with a DNS64 in place. If that's not possible, I think it's better to implement some DNS64 resolver in zsock_getaddrinfo() that can be activated via Kconfig, your proposed way seems like a like a workaround, that is probably bringing more problems. For example changing the meaning of the setting "hawkbit/server_addr" and replacing it with "hawkbit/server_hostname" would lead to braking configurations of users that use that and then update to newest newest zephyr version. As the maintainer of the hawkbit subsystem, I'd like to not have such changes, if they are not necessary.

[maass-hamburg]

This commit also changes how the controllerId is generated based on device id, and disentangles the two. The controllerId is what hawkbit uses to uniquely identify a device, and is not necessarily the same as the device id, and should be fully customizable by the user if needed. Previously, all custom device ids were being prepended with CONFIG_BOARD. When a user selects CONFIG_HAWKBIT_CUSTOM_DEVICE_ID, they should be able to specify the full controllerId used with hawkbit.

Please note, that changes that are not directly related to each other, should at least go into their own commit.

This is even more unrelated and I recommend splitting it into it's own PR.

[rlubos]

As of now, getaddrinfo is not aware of NAT64 and does not handle this.
I think it's better to implement some DNS64 resolver in zsock_getaddrinfo()

Should it be though? https://man7.org/linux/man-pages/man3/getaddrinfo.3.html makes no mention of DNS64, there is AI_V4MAPPED flag, but it's a different kind of mapping. IMHO I agree that it should be the server responsibility to synthesize the DNS64 AAAA records. Otherwise, we'd be inventing some new, non-standardized API.

[rlubos]

For example changing the meaning of the setting "hawkbit/server_addr" and replacing it with "hawkbit/server_hostname" would lead to braking configurations of users that use that and then update to newest newest zephyr version. As the maintainer of the hawkbit subsystem, I'd like to not have such changes, if they are not necessary.

I agree we should avoid API changes, although I also see a need to be able to specify hostname when using numeric address representation for TLS purposes.

How about we simplify the change set, I. e. keep the current configuration as is (use server_addr for address resolving) to maintain backwards compatibility, but allow to specify server_hostname separately for TLS case, with a fallback to server_addr if not set?

[nealjack]

From what I know, when using a NAT64 and you want you want to resolve DNS addresses you should also have a DNS64 server, that will do the transition for you. You could even use a public DNS64 server like the one from Google (https://developers.google.com/speed/public-dns/docs/dns64) for that.

Unfortunately, the openthread border router implementation does not use a static, well-known NAT64 prefix (https://openthread.io/codelabs/openthread-border-router-nat64#1). It is dynamically generated and potentially different for every network. End devices must query for the prefix and synthesize an IPv6 address using otNat64SynthesizeIp6Address(). Using DNS64 would be arduous as the server would have to know beforehand what the dynamically generated prefix is. For Openthread, it is better to do the NAT64 translation locally.

I could see a world where zephyr's implementation of getaddrinfo does the translation if using Openthread as the L2 layer, but as @rlubos mentions, it would result in a non-standard getaddrinfo implementation.

How about we simplify the change set, I. e. keep the current configuration as is (use server_addr for address resolving) to maintain backwards compatibility, but allow to specify server_hostname separately for TLS case, with a fallback to server_addr if not set?

I agree with this proposal.

[nealjack]

This commit also changes how the controllerId is generated based on device id, and disentangles the two. The controllerId is what hawkbit uses to uniquely identify a device, and is not necessarily the same as the device id, and should be fully customizable by the user if needed. Previously, all custom device ids were being prepended with CONFIG_BOARD. When a user selects CONFIG_HAWKBIT_CUSTOM_DEVICE_ID, they should be able to specify the full controllerId used with hawkbit.

Please note, that changes that are not directly related to each other, should at least go into their own commit.

This is even more unrelated and I recommend splitting it into it's own PR.

I will work on splitting the PR to address this.

[maass-hamburg]

I agree we should avoid API changes, although I also see a need to be able to specify hostname when using numeric address representation for TLS purposes.

For that it's fine, but it should be put behind a Kconfig, which is disabled by default.

[maass-hamburg]

I could see a world where zephyr's implementation of getaddrinfo does the translation if using Openthread as the L2 layer, but as @rlubos mentions, it would result in a non-standard getaddrinfo implementation.

Put it behind a Kconfig, then it can be implemented and activated for only that use case.

[maass-hamburg]

@nealjack please remove the unrelated samples: sensor: lps22hh: fix title commit

[rlubos]

Please fix the documentation

[rlubos]

It should be domain_str - that's why documentation build fails.

[nealjack]

Thanks - should be fixed in the rebase

[maass-hamburg]

@nealjack rebase needed

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[maass-hamburg]

@nealjack pls rebase, due to some bug that failed ci and is now fixed in the current main

[rlubos]

@nealjack pls rebase, due to some bug that failed ci and is now fixed in the current main

Apparently rerun was enough, no need to rebase.