search

edgexfoundry / device-coap-c

EdgeX device service for CoAP-based REST protocol
Apache License 2.0
3 stars 18 forks source link

Review for CoAP device service #3

Closed iain-anderson closed 3 years ago

iain-anderson commented 3 years ago

This service is proposed for adoption into the main edgexfoundry project. The process for doing so is documented here: https://wiki.edgexfoundry.org/display/FA/Process+for+new+Device+Services

iain-anderson commented 3 years ago

Note that for testing purposes the Cotel tool may be useful https://github.com/kb2ma/cotel

kb2ma commented 3 years ago

Note that for testing purposes the Cotel tool may be useful https://github.com/kb2ma/cotel

Thanks, Iain. Cotel does provide a GUI but is Linux-only at present. Windows and Mac users might want to try Copper for Chrome, which is a Chrome extension.

iain-anderson commented 3 years ago

@kb2ma I've done an initial review on the code. (I see that because I created the PR I can't request changes or approve, only comment. This is something we'll need to fix in our procedures)

kb2ma commented 3 years ago

I am developing and testing against device-sdk-c v1.3.1. Please let me know if there is a better choice.

kb2ma commented 3 years ago

I am developing and testing against device-sdk-c v1.3.1. Please let me know if there is a better choice.

That was a mistake. I am using device-sdk-c v1.2.2 to implement updates for this PR, which is the version I used during development. I am testing against Hanoi release containers as well as Geneva though.

iain-anderson commented 3 years ago

I am developing and testing against device-sdk-c v1.3.1. Please let me know if there is a better choice.

That was a mistake. I am using device-sdk-c v1.2.2 to implement updates for this PR, which is the version I used during development. I am testing against Hanoi release containers as well as Geneva though.

OK, we should probably move onto 1.3.1 at some point. There's an API change relating to configuration - let me know if you want some help with that.

kb2ma commented 3 years ago

OK, we should probably move onto 1.3.1 at some point. There's an API change relating to configuration - let me know if you want some help with that.

Got it. I backed off on the API upgrade because it was not specifically called out in the review. Let me get through the other comments, and then we can revisit.

kb2ma commented 3 years ago

@kb2ma I've done an initial review on the code. (I see that because I created the PR I can't request changes or approve, only comment. This is something we'll need to fix in our procedures)

I am adding updates to my promote_review branch. I'm also referencing individual commits in the comments to make it easy to review.

kb2ma commented 3 years ago

@iain-anderson, I'm following up on the comments above to update the Alpine version to v3.12 for the container build. The build now generates a number of warnings from CMake like below. The item below is from device-sdk-c (v1.2.2), but I also see them in the libcoap and device-coap-c builds, too. Has the project established a way to resolve these warnings?

CMake Warning (dev) at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:272 (message):
  The package name passed to `find_package_handle_standard_args`
  (LIBMICROHTTP) does not match the name of the calling package
  (LibMicroHTTP).  This can lead to problems in calling code that expects
  `find_package` result variables (e.g., `_FOUND`) to follow a certain
  pattern.
Call Stack (most recent call first):
  cmake/FindLibMicroHTTP.cmake:4 (find_package_handle_standard_args)
  CMakeLists.txt:20 (find_package)
This warning is for project developers.  Use -Wno-dev to suppress it.

[Edit: I have committed the change to use Alpine 3.12.]

kb2ma commented 3 years ago

Thanks for all of the comments! I have worked through them now. Below are links to the items that need some review from the original commenter, as well as a couple of items I would like to defer for future work.

Needs Review

Future Work

iain-anderson commented 3 years ago

CMake Warning (dev) at /usr/share/cmake/Modules/FindPackageHandleStandardArgs.cmake:272 (message): The package name passed to find_package_handle_standard_args (LIBMICROHTTP) does not match the name of the calling package (LibMicroHTTP). This can lead to problems in calling code that expects find_package result variables (e.g., _FOUND) to follow a certain pattern. Call Stack (most recent call first): cmake/FindLibMicroHTTP.cmake:4 (find_package_handle_standard_args) CMakeLists.txt:20 (find_package)find_package (Libcoap REQUIRED) This warning is for project developers. Use -Wno-dev to suppress it.

This can be fixed by renaming FindLibcoap.cmake to FindLIBCOAP.cmake and changing the find_package call to find_package (LIBCOAP REQUIRED) and similarly for the other dependencies.

I'll create an issue against the SDK and will fix those warnings for 2.0

kb2ma commented 3 years ago

I have a couple of security questions/comments.

Presently the example configuration uses the unspecified address (0.0.0.0/::) to listen for incoming CoAP messages. It would be more secure to leave the CoapBindAddr parameter empty to force the user to supply an address. At the very least, I plan to add some text to the config file to recommend use of the address for the specific interface over which messages are expected. This text then is reproduced in the README.

Is there any requirement to store the CoAP/DTLS pre-shared key (PskKey in the configuration) in Vault? I was just reviewing the Secret Distribution ADR. If I understand correctly, there is not a C SDK to retrieve secrets from Vault. In this case the ADR recommends dynamic injection of the values into the process environment. Am I reading this correctly? Is there some template/example already for this approach?

iain-anderson commented 3 years ago

Hi Ken, we can go with PskKey in the configuration as-is. For the 2.0 release the C SDK should support storing secrets in vault and we can update accordingly when that becomes available.

kb2ma commented 3 years ago

At the very least, I plan to add some text to the config file

Actually, I updated the text in the Docker Compose section of the README. It seems easier to specify a particular address when port mapping at the container level. Addressed in 2e48e2f.

kb2ma commented 3 years ago

This can be fixed by renaming FindLibcoap.cmake to FindLIBCOAP.cmake and changing the find_package call to find_package (LIBCOAP REQUIRED) and similarly for the other dependencies.

Addressed in 2c675bd

kb2ma commented 3 years ago

@iain-anderson, I have addressed your latest comments: the CMake warnings and support for non-IP addresses in resolveAddress(). I also added a note to the README about narrowing the IP address in the Compose port mapping attribute.

Let me know if there is anything else.

iain-anderson commented 3 years ago

Hi @kb2ma We agreed in today's working group meeting that we're ok to progress this now. I've pulled your updates in, next step is to move to SDK 1.3.1 and then we can seek formal approval for adopting the service into EdgeX proper.

kb2ma commented 3 years ago

This is fantastic, Iain! Thanks for the review and merge.

I'll try compiling against v1.3.1 toward the goal of creating a PR. I'll create an issue on this repository if I have any questions.

iain-anderson commented 3 years ago

WG approval recorded 15th March, will recommend to the TSC that the service is adopted.