thin-edge / thin-edge.io

The open edge framework for lightweight IoT devices
https://thin-edge.io
Apache License 2.0
222 stars 55 forks source link

empty service.type value leads to invalid c8y smart rest message being sent #2383

Closed reubenmiller closed 1 year ago

reubenmiller commented 1 year ago

Describe the bug

tedge-mapper-c8y is sending invalid service registration messages to Cumulocity IoT when the service.type setting is set to an empty string.

Below shows the Cumulocity smart rest message to register a service (message id 102), and the ,, shows that the service type is missing. Cumulocity also publishes an error message on the local c8y/s/e topic indicating that the service name and type can not be empty values.

2023-10-28T11:17:01.996203Z    c8y/s/us   102,TST_transform_perpendicular_burrito:device:main:service:tedge-agent,,tedge-agent,up
2023-10-28T11:17:02.138625Z    c8y/s/e    41,102,Service uniqueID, type and name are required

To Reproduce

The test case is already covered by the following system test, however the test case seems to be flakey.

Test if all c8y services using default service type when service type configured as empty

But the following manual procedure can be executed to observer the error:

  1. Connect thin-edge.io to Cumulocity IoT

  2. Configure the service.type

    tedge config set service.type ""
  3. Restart the mapper

    sudo systemctl restart tedge-mapper-c8y
  4. Open a different console and subscribe to the local cumulocity topic

    tedge mqtt sub 'c8y/#'
  5. In the previous console, restart the tedge-agent

    sudo systemctl restart tedge-agent
  6. Observe the output from the tedge mqtt sub command in the previous steps

    The bug is visible if there is an error message received on the c8y/s/e topic indicating that the service name/type should not be empty.

Expected behavior

A non-empty value should always be used in the service type field of the 102 smart rest message, and there should not be any corresponding c8y/s/e message received.

An empty value used by service.type should be treated the same as an unset value (e.g. using service as the default value).

Screenshots

Environment (please complete the following information):

Additional context

Bravo555 commented 1 year ago

Some things that contributed to this bug:

  1. There are integration tests which check what fragment values are present on Cumulocity when empty service.type is used, but there is no tests that check if the converter is emitting correct messages in the first place.
  2. ,, being present inside a smartrest message is obviously invalid smartrest, and if proper abstraction was used that checked for this type of errors, it would be much easier to detect the bug.
  3. "" (empty string) is invalid value for a service.type. Instead of the caller code having to handle "" as invalid value, it would be better if tedge_config returned default value (service) if the user set service.type to empty string, or it should even disallow setting service.type to invalid value in the first place. A user setting service.type to "" might be confused by service default being used, and it would better to tell them earlier that "" is an invalid value for service.type.

These will be worked on in order.

reubenmiller commented 1 year ago

Closing as there was already a system test covering this scenario, and the linked PR just re-enabled the test case.