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

Unable to handle notification after device ip change in M11 #1421

Closed
JaroslawLegierski opened this issue Mar 15, 2023 · 22 comments
Closed

Unable to handle notification after device ip change in M11 #1421

JaroslawLegierski opened this issue Mar 15, 2023 · 22 comments
Labels
bug Dysfunctionnal behavior server Impact LWM2M server

Comments

@JaroslawLegierski
Copy link
Contributor

Version(s)

leshan-2.0.0-M11

Which components

leshan-server-demo

Tested With

leshan-clint-demo

What happened

We have device with observed resource (e.g. /6/0/0) cid is on. After device ip address change (without reg update) when Leshan server is receiving Notification we are getting following exception:

16:17:42.521 [CoapServer(main)#1] WARN org.eclipse.leshan.server.demo.servlet.EventServlet - Unable to handle notification of [AWaY4UQ8lx:/6/0/0]
java.lang.NullPointerException: null
	at org.eclipse.leshan.server.model.VersionedModelProvider$DynamicModel.getObjectModel(VersionedModelProvider.java:70)
	at org.eclipse.leshan.server.model.VersionedModelProvider$DynamicModel.getResourceModel(VersionedModelProvider.java:61)
	at org.eclipse.leshan.core.node.codec.senml.LwM2mNodeSenMLDecoder.getResourceType(LwM2mNodeSenMLDecoder.java:555)
	at org.eclipse.leshan.core.node.codec.senml.LwM2mNodeSenMLDecoder.extractLwM2mResources(LwM2mNodeSenMLDecoder.java:468)
	at org.eclipse.leshan.core.node.codec.senml.LwM2mNodeSenMLDecoder.parseRecords(LwM2mNodeSenMLDecoder.java:271)
	at org.eclipse.leshan.core.node.codec.senml.LwM2mNodeSenMLDecoder.decodeTimestampedData(LwM2mNodeSenMLDecoder.java:182)
	at org.eclipse.leshan.core.node.codec.DefaultLwM2mDecoder.decodeTimestampedData(DefaultLwM2mDecoder.java:201)
	at org.eclipse.leshan.server.californium.endpoint.ServerCoapMessageTranslator.createObservation(ServerCoapMessageTranslator.java:108)
	at org.eclipse.leshan.server.californium.endpoint.CaliforniumServerEndpointsProvider$2.onNotification(CaliforniumServerEndpointsProvider.java:168)
	at org.eclipse.californium.core.network.CoapEndpoint$NotificationDispatcher.onNotification(CoapEndpoint.java:810)
	at org.eclipse.californium.core.network.BaseMatcher$2.onResponse(BaseMatcher.java:250)
	at org.eclipse.californium.core.coap.Request.setResponse(Request.java:916)
	at org.eclipse.californium.core.server.ServerMessageDeliverer.deliverResponse(ServerMessageDeliverer.java:280)
	at org.eclipse.californium.core.network.stack.BaseCoapStack$StackTopAdapter.receiveResponse(BaseCoapStack.java:214)
	at org.eclipse.californium.core.network.stack.AbstractLayer.receiveResponse(AbstractLayer.java:89)
	at org.eclipse.californium.core.network.stack.ExchangeCleanupLayer.receiveResponse(ExchangeCleanupLayer.java:105)
	at org.eclipse.californium.core.network.stack.ObserveLayer.receiveResponse(ObserveLayer.java:154)
	at org.eclipse.californium.core.network.stack.BlockwiseLayer.receiveResponse(BlockwiseLayer.java:875)
	at org.eclipse.californium.core.network.stack.ReliabilityLayer.receiveResponse(ReliabilityLayer.java:308)
	at org.eclipse.californium.core.network.stack.AbstractLayer.receiveResponse(AbstractLayer.java:89)
	at org.eclipse.californium.core.network.stack.BaseCoapStack.receiveResponse(BaseCoapStack.java:132)
	at org.eclipse.californium.core.network.CoapEndpoint$1.receiveResponse(CoapEndpoint.java:323)
	at org.eclipse.californium.core.network.UdpMatcher$4.run(UdpMatcher.java:457)
	at org.eclipse.californium.elements.util.SerialExecutor$1.run(SerialExecutor.java:293)
	at org.eclipse.californium.core.network.CoapEndpoint$6.run(CoapEndpoint.java:1366)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)

In M10 this was working fine

How to reproduce

The main problem with Notification is that in M10 registration was returned based on registrationid:

Registration registration = server.getRegistrationStore().getRegistration(observation.getRegistrationId());

but in M11 is used identity instead of registrationid

Identity identity = identityHandler.getIdentity(coapResponse);
ClientProfile profile = toolbox.getProfileProvider().getProfile(identity);
Registration registration= profile.getRegistration();

Unfortunately, the identity changes after changing the ip as well.

Therefore is not possible now use leshanServerBuilder.setUpdateRegistrationOnNotification(true);

I have prepared the following commit which can correct this problem.

Relevant Output

No response

@JaroslawLegierski JaroslawLegierski added the bug Dysfunctionnal behavior label Mar 15, 2023
@sbernard31
Copy link
Contributor

It seems that you have indeed found a problem. 👍

I should not have added this out of specification setUpdateRegistrationOnNotification feature ... 😬
I guess you already know but I strongly not encourage you to use it. (See : OpenMobileAlliance/OMA_LwM2M_for_Developers#555)

Anyway, some question to better understand the issue :

  1. Is it something you face in production or just in your experimentation tests ?
  2. Is it something you face with CoAPs only or also with CoAP ?

@JaroslawLegierski
Copy link
Contributor Author

JaroslawLegierski commented Mar 16, 2023

We need this feature on production for our customers only for COAPS

In general, the problem is more complex:

We have customers with devices that often changes ip addresses, but after that they:

  1. processing send operation
  2. sending notification message (as result of observation)

unfortunately, without updating registration before!

I know it's out of the scope the OMA LwM2M specification, but Leshan's competitors supports it .....

Do you see the possibility to implement such business logic in Leshan as a non-standard function which can be activated on demand (e.g. leshanServerBuilder.acceptNotifySendwithoutRegistrationUpdate(true) similar to leshanServerBuilder.setUpdateRegistrationOnNotification(true)) ?

@sbernard31
Copy link
Contributor

sbernard31 commented Mar 17, 2023

We need this feature on production for our customers only for COAPS

So Let's try to think about this problem.

I know it's out of the scope the OMA LwM2M specification, but Leshan's competitors supports it .....

That's not a so strong argument to me.
More important point to me are :

  1. is it in the scope of the project ? This is very debatable as this seems to be out of specification. (maybe we need clarification?)
  2. is it really useful to do that ? Maybe, I'm not totally sure.
  3. does it affect negatively Leshan design ? I don't know for now.

I understand that you try to push this in Leshan but what happens here is exactly what I explain at https://github.com/eclipse/leshan/wiki/How-Leshan-should-behave-with-Non-Compliant-Implementations-%3F#first-idea--using-robustness-principle

Supporting non-compliant behavior could make it widespread and so could force other implementation to support it too.

This would be better :

Do you see the possibility to implement such business logic in Leshan as a non-standard function which can be activated on demand (e.g. leshanServerBuilder.acceptNotifySendwithoutRegistrationUpdate(true) similar to leshanServerBuilder.setUpdateRegistrationOnNotification(true)) ?

First let's try to think how code should be modified.

I think a bit about that and there is maybe 1 issue with : ClientProfileProvider.getProfile(Identity) and more generally when we search by identity.

I try to explain :
For CoAP, Identity contains only "socket address" (I think that makes sense, because we have nothing more to use)
For CoAPs, Identity contains "TLS Identifier" + "socket address" and maybe this is not so good, "socket peer address" should not be used to identify the peer.

If that was the case when we search by identity for CoAPs even if socket address changed we will be able to find the profile.

You could try to simulate this by creating your own RegistrationStore and implementing a custom getRegistrationByIdentity(Identity) which don't use socketaddress in it's key when Identity is "secure".

Let me know if this is not clear enough.

@sbernard31 sbernard31 added the server Impact LWM2M server label Mar 28, 2023
@JaroslawLegierski
Copy link
Contributor Author

Please find here my PoC using CustomInMemoryRegistrationStore. I replaced "Identifier" + "socket address" by:

  • PskIdentity for PSK
  • RawPublicKey in RPK
  • 509CommonName for X509 certificate
  • OscoreIdentity in OSCORE

For unsecure connections I left the existing business logic unchanged. I also sucessfully tested this solution for PSK, RPK or X509.

After implementation of these changes in case device IP address change it is possible to handle Send and Notify by the server without registration update.

@sbernard31
Copy link
Contributor

sbernard31 commented Apr 5, 2023

Thx for testing this.

So I guess we have an issue with our Identity concept... 🤔
We need to find a better design about this but this will probably lead to some Redis Serialization break again 😬
(See #1417)

I will try to take time to think about this. (Do not hesitate to share ideas too)

@sbernard31

This comment was marked as off-topic.

@sbernard31

This comment was marked as off-topic.

@sbernard31

This comment was marked as off-topic.

@sbernard31
Copy link
Contributor

I created an issue to talk about new design for Identity : #1436

@JaroslawLegierski
Copy link
Contributor Author

I created PoC similar to previous one but implemented using Redis https://github.com/JaroslawLegierski/leshan/tree/opl/send_not_without_update_v4
maybe it will be useful in developing the concept of "new Identity" ?

I would also like briefly present our point of view by return to our last discussion:

1 is it in the scope of the project ? This is very debatable as this seems to be out of specification. (maybe we need clarification?)

It would be great if we could get some clarification from OMA. In my opinion, the blur area in the specification is due to the fact that it was written BEFORE the CID was specified and widely used as a solution for IP/Port volatility.

2 is it really useful to do that ? Maybe, I'm not totally sure.

Yes, Battery-powered devices need to minimize the power consumption (minimize handshakes + Minimize exchanges). The CID feature makes it possible to reach the server if the IP has changed, without having to make a new handshakes, and without having to perform an UPDATE before each SEND or NOTIFY. The UPDATE becomes then necessary only to signal to the server that device is “present” and ready to receive operations.

3 does it affect negatively Leshan design ? I don't know for now.

Indeed, hard to say. But at least, as explained here https://github.com/eclipse/leshan/wiki/How-Leshan-should-behave-with-Non-Compliant-Implementations-%3F#first-idea--using-robustness-principle, Leshan can be strict in regard of the specification, but should allow any solution maker to “customize” some specific behavior, such as this one: Accepting Notify and Send even when a device is “sleeping”, whatever its IP has changed or not: Leshan can rely on the DTLS layer’s responsibility to ensure the connection has been “securely accepted”.

@sbernard31
Copy link
Contributor

I created PoC similar to previous one but implemented using Redis https://github.com/JaroslawLegierski/leshan/tree/opl/send_not_without_update_v4
maybe it will be useful in developing the concept of "new Identity" ?

You mainly change the identity serializer correct ? Too bad it seems we missed to add it to RedisRegistrationStore.Builder 🤦 ...
But at least we find a short term solution for this problem ! 👍

I would also like briefly present our point of view by return to our last discussion:

Yep discussion are welcome :)

It would be great if we could get some clarification from OMA. In my opinion, the blur area in the specification is due to the fact that it was written BEFORE the CID was specified and widely used as a solution for IP/Port volatility.

There are already several issues about that :

Maybe we could try to make more simple question and see if we succeed to get answer from OMA but at least for Update Registration it seems pretty clear, right ?
Do not hesitate to participate to OMA discussion if you think there is something to add.

(If you want to have more impact that just opening issue and hoping that someone at OMA will answer, unfortunately you need to be an OMA member)

Battery-powered devices need to minimize the power consumption (minimize handshakes + Minimize exchanges). The CID feature makes it possible to reach the server if the IP has changed, without having to make a new handshakes, and without having to perform an UPDATE before each SEND or NOTIFY.

I understand and it makes sense.
Do you have real measure about impact of sending an UPDATE before SEND or NOTIFY ? or is it just theoretical ?

The UPDATE becomes then necessary only to signal to the server that device is “present” and ready to receive operations.

This makes me think there are 2 questions :

  1. accepting notify and/or send from device with changing IP.
  2. updating registration on notification or send

Do you need only 1) or both ?

Accepting Notify and Send even when a device is “sleeping”, whatever its IP has changed or not: Leshan can rely on the DTLS layer’s responsibility to ensure the connection has been “securely accepted”.

I think there is something not so good with current Identity design, so it should be refactored. (#1436)
Your tests seems to confirm a new design for Identity will allow to support that without too much impact. So we are going in that direction.

For "updating registraiton on notification or send", we can also try to make this doable with current code but I don't know how this should looks like for now.

@JaroslawLegierski
Copy link
Contributor Author

I understand and it makes sense.
Do you have real measure about impact of sending an UPDATE before SEND or NOTIFY ? or is it just theoretical ?

This is theoretical, but we have some use cases where constrained devices will do a NOTFY each hour and only 1 UPDATE each day. If we force them to do an UPDATE (+ ACK) each hour before each NOTIFY, it will drastically increase the traffic and their CPU consumption.

This makes me think there are 2 questions :

accepting notify and/or send from device with changing IP.
updating registration on notification or send
Do you need only 1) or both ?

At least we need 1) We do not want the device to be considered as “present” because he sent a NOTIFY or a SEND. It is the role of UPDATE

however ...

For "updating registraiton on notification or send", we can also try to make this doable with current code but I don't know how this should looks like for now.

... during my last tests I prepared this solution ... maybe it will be interesting to someone ?

@sbernard31
Copy link
Contributor

This is theoretical

OK if one day you get real numbers about it, please share it with us.

At least we need 1) We do not want the device to be considered as “present” because he sent a NOTIFY or a SEND. It is the role of UPDATE

Ok thx for clarification. Let's focus on 1) for now. (Let me know if you have any opinion/idea about #1436)

For "updating registration on notification or send", we can also try to make this doable with current code but I don't know how this should looks like for now.

Just to clarify, I mean I don't know if we should :

  • add an option in builder and so officially support it . (like you did and like it's done for notify)
  • OR just have an API which is permissive enough to let someone doing it by its own.

during my last tests I prepared this solution ... maybe it will be interesting to someone ?

Thx for sharing this. Just a little remark, ideally this should not be done in SendResource because this is specific to Californium implementation. To have the code available for all transport it would be better to update registration at higher level (E.g. SendHandler)

@sbernard31
Copy link
Contributor

@JaroslawLegierski at #1373 (comment) you said :

currently most important for us is #1421 topic

Just to be sure, there is no urgency because we find a workaround using a custom RegistrationStore, right ?

I don't know if I was clear enough, but I agree to try to have a real solution in next release but this means working on :

Do you want I start to move forward on #1436 now (I didn't start to code about that because I was waiting for feedback) ?
Or maybe you prefer to continue your work on it and then I help by reviewing code when needed ?

@JaroslawLegierski
Copy link
Contributor Author

Just to be sure, there is no urgency because we find a workaround using a custom RegistrationStore, right ?

Exactly btw my colleagues will want to test this solution as soon as it will be available in M12 Release Candidate (master)

I don't know if I was clear enough, but I agree to try to have a real solution in next release but this means working on :

Redesign peer's Identity class. #1436
and maybe

#1417

Do you want I start to move forward on #1436 now (I didn't start to code about that > because I was waiting for feedback) ?
Or maybe you prefer to continue your work on it and then I help by reviewing code when needed ?

I think we can wait a while for feedback. I will try to continue my work in the meantime (Your help will be very welcome) I see only one potential problem - in next week starting from Tuesday I will be on bussiness trip. But I'll see what I can finish before I leave.

@sbernard31
Copy link
Contributor

Your help will be very welcome

Just let me know

in next week starting from Tuesday I will be on bussiness trip

If wanted, Just share your work and I could try to continue your work.
(Note that in May I will not be so much available, especially for the end of the month)

@JaroslawLegierski
Copy link
Contributor Author

If wanted, Just share your work and I could try to continue your work.
(Note that in May I will not be so much available, especially for the end of the month)

Please find here first working version IpPeer implementation. More information is described in #1436

@Warmek

This comment was marked as off-topic.

@sbernard31
Copy link
Contributor

sbernard31 commented Jul 3, 2023

So an update about this :

  1. ✔️ the peer's identity refactoring is almost finished (See Identity Refactoring #1445 (comment)) but not yet integrated.
  2. leshanServerBuilder.setUpdateRegistrationOnNotification(true) update the registration but not the client transport data.
  3. ❌ For now there is no feature implemented to add a registration update on send.

@sbernard31
Copy link
Contributor

@sbernard31
Copy link
Contributor

I think we can close this bug.

The only remaining part is the out of specification "update registration on Send" so let's continue discussion about this at : #1464

@sbernard31
Copy link
Contributor

sbernard31 commented Nov 28, 2023

I improved documentation about "update registration on Send" : #1544 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Dysfunctionnal behavior server Impact LWM2M server
Projects
None yet
Development

No branches or pull requests

3 participants