eclipse-californium / californium.tools

Californium project
Other
59 stars 57 forks source link

update ep parameters even with RD registeration interface. #32

Closed vyshakg30 closed 6 years ago

vyshakg30 commented 7 years ago

According RFCs an end point must be able to update its parameters and lifetime value with registration interface as well.

since the 'handlepost' is called to update paramenters when the interface returned by RD is used by the ep, in the solution I have called the same function to process the request and update.

reference : https://tools.ietf.org/html/draft-ietf-core-resource-directory-04#section-5.3

boaks commented 7 years ago

FMPOV, the RDResource implements

https://tools.ietf.org/html/draft-ietf-core-resource-directory-04#section-5.2

This defines

The RD then creates a new resource or updates an existing resource in the RD and returns its location.

With your PR, I'm not sure, if the location is still returned. I would guess, though return is added in line 99, it would not return that location.

Example of 5.2:

Req: POST coap://rd.example.com/rd?ep=node1
...
Res: 2.01 Created
Location: /rd/4521

The 5.3 is implemented in the RDNodeResource, but it has an other contract.

Example of 5.3:

Req: POST /rd/4521

Res: 2.04 Changed

So the difference for me seems to be, that the one targets to resource "/rd", the other to "/rd/path"; one returns the "location", the other not.

Therefore it seems for me to be not a solution to call resource.handlePOST(exchange);.

When analysing the difference between RDResource.handlePOST and RDNodeResource.handlePOST it seems just to be the trigger of the ltExpiryFuture. maybe we find a other way to trigger the update of the ltExpiryFuture within RDResource.handlePOST .

boaks commented 7 years ago

The exact processing of the lt and con when "updates an existing resource" according "5.2 Registration" seems to be not completely specified.

If a registration for an endpoint already exists and is updated by a registration without lt or con, should than the old values or the default/empty values be used?

So I guess, to provide the right fix, the draft has first clarifying the intended details.

vyshakg30 commented 7 years ago

I perfectly understand what you are saying,and infact also thought about which life time parameter must be used for refreshing the timer. but quite frankly this confusion also exists in update section as well.

https://tools.ietf.org/html/draft-ietf-core-resource-directory-04#section-5.3 Update interface : although RFC says to use previous values if no value is returned it further mentions this. lt := Lifetime (optional). Lifetime of the registration in seconds. Range of 60-4294967295. If no lifetime is included, a default value of 86400 (24 hours) SHOULD be assumed.

despite the doubt in update interface for refreshing lt provided in RFC,an approach has been chosen to refresh lifetime of the resource, hence i feel we should also choose an approach to update lifetime in registration interface as well.

The reason i am insisting on this is because registration interface responses by merely sending a response code 2.04 i.e, 'changed resource' without actually making any change to the node in RD. This in a way would make RD appear to be INCONSISTENT.

I strongly feel there is a need for some change in code here.

boaks commented 7 years ago

I agree, that the current implementation doesn't proper handle the "updates an existing resource". But, I also explained the difference between 5.2 and 5.3 and therefore you PR will fix refreshing the expire timer but also indroduce an error in the response.

So to fix the 5.2 implementation, in my opinion, an other or modified PR would be required. And I would appreciate, if that fix would be based on a clarification from the ietf core mailing list about the wanted detail behaviour when parameter are not provided.

boaks commented 7 years ago

Just as update, the current draft is:

https://tools.ietf.org/html/draft-ietf-core-resource-directory-11#page-14

But FMPOV it continues to be unclear about the "update via registration" (now section 5.3 in the new draft).

A new registration may be created at any time to supersede an existing registration, replacing the registration parameters and links.

lt := Lifetime (optional). Lifetime of the registration in seconds. Range of 60-4294967295. If no lifetime is included in the initial registration, a default value of 86400 (24 hours) SHOULD be assumed. If the lt parameter is not included in a registration refresh or update operation, the most recently supplied value SHALL be re-used.

So it is either replacing or recently supplied shall be re-used.

Also the response for "update via registration" seems to be still unclear:

Success: 2.01 "Created" or 201 "Created". The Location header option MUST be included in the response when a new registration resource is created.

seems not to cover that case explicitly. I'm still in doubt, if the "update via registration" case is specified good enough to fix the implementation.

vyshakg30 commented 7 years ago

true. it would be better if some one from ietf could comment on this. i will drop them a note and get back with what they have to say.

boaks commented 7 years ago

It would be great, if you can spend the time to try to ask this in the core mailing list.

If so, try also to clarify the use-cases for call register instead of the update.

FMPOV, assuming that register should use previous state is somehow corrupting the important use case of a crashed device, ,which would use registers (again) without knowing about the past.

vyshakg30 commented 7 years ago

-I have dropped them a mail with the queries. -I feel the aspect of which Life time value to be used when 'LT' parameter is absent is made clear in the latest draft. i.e., : 1)If 'LT' parameter absent for First registeration Life time(LT) value - 24 hrs : refreshes the Node with LT value 2)If 'LT' parameter absent for subsequent registerations Life time(LT) value - previous 'LT' value. : refreshes the Node with LT value

Generic : case. 3)If 'LT' parameter present for registerations Life time(LT) value - LT parameter in the URI : refreshes the Node with LT value

Or am I missing some thing ?

boaks commented 7 years ago

Or am I missing some thing ?

In my very personal opinion, NO specification should state something general as:

A new registration may be created at any time to supersede an existing registration, replacing the registration parameters and links.

and then withdraw that statement a some lines below. Your assumption, that it is "defined", depends on assuming, that the order of the statements is relevant. But having two statements in conflict may also indicate, that it was forgotten to clean it up. So in my opinion, if its the intended to keep the old LT, the general statement should include at least an hint to that intention (e.g append "except the LT").

vyshakg30 commented 7 years ago

alright got it...unless its made explicitly clear the manner in which LT value should be processed we shouldnt freeze the implementation. I will add LT value query to the mail as well.

boaks commented 7 years ago

I'm not sure, but I guess, the discussion about the rd draft is moved to

https://github.com/core-wg/resource-directory/issues

boaks commented 7 years ago

https://github.com/core-wg/resource-directory/issues/33

boaks commented 6 years ago

Hi @vyshakg30,

the issue with specification is fixed, see https://github.com/core-wg/resource-directory/pull/42. There is no longer a special handling of the lifetime. So, are you still interested in working on a PR for fixing your original issue according the spec?

vyshakg30 commented 6 years ago

hey @boaks sorry been away just got myself upto speed. just to make things clear

The mechanism is useful in updates, but at the registration interface they do not make sense.

does that mean we wont use registeration interface to update the lifetime of the node or should we use first value 'lt' ?

boaks commented 6 years ago

does that mean we wont use registeration interface to update the lifetime of the node or should we use first value 'lt' ?

It says, the registration interface MUST not use the previous lifetime and instead use the default lifetime, if the lifetime is not provided in the registration.

So don't call "resource.handlePOST(exchange);" to trigger an update, instead we can replace the previous registered resource by the new one of the new registration.

vyshakg30 commented 6 years ago

Got it..

instead we can replace the previous registered resource by the new one of the new registration.

I trust u dont mean delete the existing node and add a new node? because according the draft https://tools.ietf.org/html/draft-ietf-core-resource-directory-11#section-5.4.1

A new registration may be created at any time to supersedean existing registration, replacing the registration parameters and links.

so that this could alllow retaining old links that were sent in previous registrations, and the new links could be added to the list along with 'LT' reset. If we could just conclude on this I would be happy to get the implementation going.

vyshakg30 commented 6 years ago

and also currently(existing working of RD) if there are new links included in duplicate registration requests those links get added to the node.

boaks commented 6 years ago

@vyshakg30

it's not that we conclude to understand the RFC draft. It's that the RFC author conclude to your understanding :-).

The spec. cited:

replacing the registration parameters and links.

you wrote:

so that this could alllow retaining old links that were sent in previous registrations

why is replacing (from the spec.) allowing retaining? I don't understand that.

vyshakg30 commented 6 years ago

got it :)

So don't call "resource.handlePOST(exchange);" to trigger an update, instead we can replace the previous registered resource by the new one of the new registration.

are you by any chance implying we completely remove the node from RD(making it lose all the prior links of that node) and add a new node corresponding to duplicate registration request.?

if the answer is NO , then my query is irrelevant. if its a Yes then we could have an issue. because in our current implementation of RD all the Core links from duplicate registration requests get added to the Node(Resource) unlike mentioned in the spec

A new registration may be created at any time to supersede an existing registration, replacing the registration parameters and links.

and I was under the impression that by mentioning 'superscede' the draft folks meant just building on top of existing Node(Resource) by adding any new links, as this is exactly how the current RD implementation operates.

boaks commented 6 years ago

If that is your impression about the supersede, just create an issue in the rd github and ask for clarification.

From my understanding, which may be wrong, I would, indeed just remove the node of the previous registration and replace it by the new one. But though I'm not using the rd, it's not a prio task for me, to get clarifications on the issues related to that spec.

vyshakg30 commented 6 years ago

got it.. added the issue. https://github.com/core-wg/resource-directory/issues/55

vyshakg30 commented 6 years ago

'supersede' meaning was clarified. Delete and Recreate approach was mentioned . Have made changes accordingly.

boaks commented 6 years ago

Thanks for your patience!

Sometimes changing the code is not the real task ;-)

vyshakg30 commented 6 years ago

Welcome. very true :)