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

Allow blank as an override for defaultResource #773

Merged
merged 4 commits into from
Oct 9, 2019

Conversation

jason-fox
Copy link
Contributor

@jason-fox jason-fox commented Apr 10, 2019

Potential fix/workaround for telefonicaid/iotagent-ul#320

When checking to see if IOTA_DEFAULT_RESOURCE is set, deliberately check for undefined rather than general truthiness, since a zero-length string is also false This allows docker-compose users to set the default resource to blank '' using an ENV as shown:

  iot-agent:
    image: iot-agent
    depends_on:
      - mongo-db
      - mosquitto
    networks:
      - default
    expose:
      - "4041"
    ports:
      - "4041:4041"
    environment:
...etc
      - IOTA_MQTT_HOST=mosquitto # The host name of the MQTT Broker
      - IOTA_MQTT_PORT=1883 # The port the MQTT Broker is listening on to receive topics
      - IOTA_DEFAULT_RESOURCE= # Default is blank. I'm using MQTT so I don't need a resource

This means that an MQTT device can be successfully provisioned without setting a dummy resource

curl -X POST \
  http://localhost:4041/iot/services \
  -H 'Content-Type: application/json' \
  -H 'fiware-service: openiot' \
  -H 'fiware-servicepath: /' \
  -d '{
 "services": [
   {
     "apikey":      "4jggokgpepnvsb2uv4s40d59ov",
     "cbroker":     "http://orion:1026",
     "entity_type": "Thing",
     "resource":    ""
   }
 ]
}'

curl -X POST \
  http://localhost:4041/iot/devices \
  -H 'Content-Type: application/json' \
  -H 'fiware-service: openiot' \
  -H 'fiware-servicepath: /' \
  -d '{
 "devices": [
   {
     "device_id":   "motion001",
     "entity_name": "urn:ngsd-ld:Motion:001",
     "entity_type": "Motion",
     "protocol":    "PDI-IoTA-UltraLight",
     "transport":   "MQTT",
     "timezone":    "Europe/Berlin",
     "attributes": [
       { "object_id": "c", "name":"count", "type":"Integer"}
      ],
      "static_attributes": [
         {"name":"refStore", "type": "Relationship","value": "urn:ngsi-ld:Store:001"}
      ]
   }
 ]
}
'

When a topic is sent to Mosquitto the default resource of '' is used.

This PR would mean that the docker-compose for the MQTT tutorial could be updated to work with the latest Ultralight IoT Agent and the issue FIWARE/tutorials.IoT-over-MQTT#8 can be closed thereafter

My opinion on the existing bug can be found here: telefonicaid/iotagent-ul#320 (comment) - in summary both the Agent and the Tutorial should be updated somehow. I think this is a better reasonable workaround for the existing situation, as forcing users to set a resource unnecessarily seems weird.

@mrutid
Copy link
Member

mrutid commented Apr 10, 2019

Hi! thanks for the contribs! We should take into account that resource field is common to every transport, including HTTP where specifies the path where devices for a given group will send measures. So probably only a "valid PATH" should be allowed here. Have you tried to test the behaviour of HTTP transport in this case? Makes sense for you?

@jason-fox
Copy link
Contributor Author

jason-fox commented Apr 10, 2019

  1. The default is the value in the config.js - obviously this file can be used directly, overwritten when creating my Docker image or a modified file injected as a docker volume.
  2. The code change allows a user to override/set a blank value '' - at the moment this can be any value except a blank, since the ENV truthiness calculation assumes false on blank as well as not set. In a dockerized environment it is currently possible to do this only by injecting a volume. Possible, but clunky.
  3. Within the Ultralight HTTPBindings.js a blank '' value will fall back to the constant HTTP_MEASURE_PATH which happens to be /iot/d

There is currently not a sanity check for an invalid path.

If I set defaultResource to a silly value like xxxxxxxxx rather than an absolute path the device will not be listening on the HTTP South Port - this is the case whether I am using the config.js or the Docker ENV.

The result is:

http://localhost:7896/iot/d?k=4jggokgpepnvsb2uv4s40d59ov&i=motion001

returns a 404 - Not Found, since the Agent isn't listening there, it is listening on http://localhost:7896 without any extra path.

So setting the value to blank '' is a fix which will only work in the case I am an IoT Agent over MQTT only - basically I'm saying I don't want to listen to HTTP traffic. If I wish to listen to both HTTP and MQTT then the MQTT provisioning will still need to add an unnecessary resource whenever the group is provisioned.

If there is a need for a sanity check on the path (or some useful debug) it will need to be within each individual IoT Agent code rather than the underlying library.

@@ -219,7 +219,7 @@ function processEnvironmentVariables() {
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an entry in CHANGES_NEXT_RELEASE file about the changes done by this PR should be added?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a444160

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

We need to test the change in our CI e2e environment to have additional guarantees regarding backward compatibility. Currently, the environment is busy, we will merge and test as soon as we have a free testing slot on it.

@fgalan
Copy link
Member

fgalan commented Oct 9, 2019

Free slot today! Merging.

@fgalan fgalan merged commit a0a90ae into telefonicaid:master Oct 9, 2019
@jason-fox jason-fox deleted the feature/defaultResource branch November 20, 2020 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants