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

AMQP/MQTT: Gateways may subscribe for commands to disabled devices #1764

Closed
calohmn opened this issue Feb 12, 2020 · 6 comments
Closed

AMQP/MQTT: Gateways may subscribe for commands to disabled devices #1764

calohmn opened this issue Feb 12, 2020 · 6 comments
Labels
C&C Command and Control help wanted Issues that we would welcome any help from (new) contributors with

Comments

@calohmn
Copy link
Contributor

calohmn commented Feb 12, 2020

Currently, when a gateway subscribes for commands of a specific device, there is no check whether that device is actually enabled.

See this example trace:
AMQPAdapter_CommandSubscription

The command subscription is successful - only sending the notification event fails.

(When a northbound application actually tries to send a command to that device, that request will be rejected however - the device registration data is checked when trying to find the mapped gateway.)

@calohmn calohmn added the C&C Command and Control label Feb 12, 2020
@sophokles73
Copy link
Contributor

While I tend to agree that the adapters should reject a subscription request if the device is disabled at the point in time of the subscription request, I wonder how we want to deal with a situation where the device is being disabled after the subscription has been created?

@sophokles73
Copy link
Contributor

@calohmn any additional thoughts?

@calohmn
Copy link
Contributor Author

calohmn commented Mar 27, 2020

I wonder how we want to deal with a situation where the device is being disabled after the subscription has been created?

If we implement a periodic command subscription renewal, done by the protocol adapters (to prevent stale commandHandlingAdapterInstance entries in the Device Connection service - see #1858), we could also check the device registration at these points in time.
When finding out about a disabled device, we could invoke the handler, suggested in #1859, to close the subscription (meaning closing the MQTT connection, for example).
The question is, if closing the subscription and thereby explicitly informing the device that it won't receive commands, is all that important. It's the same question also raised in #1859. I tend to think it's probably not that important.

@sophokles73 sophokles73 added the help wanted Issues that we would welcome any help from (new) contributors with label Jun 16, 2020
@sophokles73
Copy link
Contributor

@calohmn IMHO the least we should do is perform the check during subscription establishment.

@calohmn
Copy link
Contributor Author

calohmn commented Jun 15, 2022

[...] I wonder how we want to deal with a situation where the device is being disabled after the subscription has been created?

The fix for #293 includes closing a MQTT device connection if the device got disabled. In case an MQTT gateway is connected and a device connected to that gateway gets disabled, any existing command subscriptions for that disabled device are left unchanged at the moment.
But I think that behaviour should be kept as is, since the MQTT adapter can not inform the connected gateway that certain command subscriptions have now been removed. If we would just remove the command subscription handler for such a device in the MQTT adapter, the MQTT gateway wouldn't know that it has to SUBSCRIBE again when the device gets enabled again at some later point.

IMHO the least we should do is perform the check during subscription establishment.

@sophokles73 yes, that check should be added.

@calohmn calohmn added good first issue Issues that do not require much in-depth knowledge of Hono and removed good first issue Issues that do not require much in-depth knowledge of Hono labels Jun 15, 2022
@calohmn
Copy link
Contributor Author

calohmn commented Jun 15, 2022

IMHO the least we should do is perform the check during subscription establishment.

Actually, that check has already been added in #2377.

@calohmn calohmn closed this as completed Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C&C Command and Control help wanted Issues that we would welcome any help from (new) contributors with
Projects
None yet
Development

No branches or pull requests

2 participants