IDD 5.0 review discussion - Service-discovery

AlexChiquito commented 7 months ago

In this Issue we will collect the comments about the service-discovery interface definition.

emanuelpalm commented 7 months ago

Link to reviewed document: eu.arrowhead.service-discovery-http-json.yml.

borditamas commented 7 months ago

AITIA review comments

https://github.com/eclipse-arrowhead/roadmap/blob/5a4f4b2d3149288a48bda69337f7e041a05d0161/5.0%20Draft/IDD/IDDs%20Service%20Registry/eu.arrowhead.service-discovery-http-json.yml#L7-L8

"The service allows for application systems..." and not for consumers.

Operation names should be mentioned by the appropriate endpoint descriptions.
Payload keys should not mix different formats. Use only camelCase or only kebab-case.
Too much type of 4xx response codes are used. 400, 401, 403 should be enough for the sake of simplicity.

https://github.com/eclipse-arrowhead/roadmap/blob/5a4f4b2d3149288a48bda69337f7e041a05d0161/5.0%20Draft/IDD/IDDs%20Service%20Registry/eu.arrowhead.service-discovery-http-json.yml#L64

"echo" operation should not be part of this service, but a different "monitor" service.

https://github.com/eclipse-arrowhead/roadmap/blob/5a4f4b2d3149288a48bda69337f7e041a05d0161/5.0%20Draft/IDD/IDDs%20Service%20Registry/eu.arrowhead.service-discovery-http-json.yml#L72

serviceInstanceIDs: In the management service the related field was named as serviceId. The same should be used everywhere.
We are missing the service metadata requirements from the request.

https://github.com/eclipse-arrowhead/roadmap/blob/5a4f4b2d3149288a48bda69337f7e041a05d0161/5.0%20Draft/IDD/IDDs%20Service%20Registry/eu.arrowhead.service-discovery-http-json.yml#L114

Returns 204 No Content why? We think 201 Created would be the most appropriate (maybe 200 OK still acceptable)
serviceID: Why the provider should specify the service instance id? How it knows what is unique? It should be generated based on the provider name, service definition and version by Service Registry and returned as a response.
interface here and in management is very different. Our opinion was described here

https://github.com/eclipse-arrowhead/roadmap/blob/5a4f4b2d3149288a48bda69337f7e041a05d0161/5.0%20Draft/IDD/IDDs%20Service%20Registry/eu.arrowhead.service-discovery-http-json.yml#L144-L144

This operation seems unnecessary as the same result could be achieved by the GET /service operation using the serviceInstanceIDs field.

DeregisterExpression and QueryExpression

What are these and where they are used?

NOTE:

According to the SysD, system-discovery and device-discovery should also have similar IDDs.

borditamas commented 5 months ago

@AlexChiquito @emanuelpalm @PerOlofsson-Sinetiq Could you please provide Sinetiq's feedback before the next RoadMap (05.02) in order to being able to discuss it there? As you know, last time the 14th of May (before AIMS 5.0 GA) was agreed to target the specification being finalized, so we don't have so much time.

DavidRutqvist commented 4 months ago

Hello, I will summarize my comments here, some of which overlaps with the above and other things are new. They can be divided into three areas, scope of a service registration record, relation between instances and types, and finally good API design.

Scope of a Service Registration Record

The goal of the service registration is to establish connectivity between a logical resource (service) and the consumer. In short, mapping a logical name to a network address.
Questions regarding what operations exists (the interface), how to authenticate, etc. is part of the service contract and should be maintained in a separate versioned document (i.e. the IDD).
The service type maps exactly onto one IDD, actually, it even maps onto one specific version of the IDD. Otherwise, interoperability cannot be guaranteed and also helps facilitate governance of deployed services.
Since services are the only object required when it comes to facilitating connectivity, we should remove the providerId property to not encourage misuse of the system concept. The mapping of services to systems from a governance perspective needs to happen in another service.

In summary, the record should then look something in the line with the following. Note that I did not make any changes to the property naming as suggested by AITIA above, I don't have anything against this. It was just to limit the changes for the sake of the discussion.

{
    "serviceID": "string",
    "serviceType": "eu.arrowhead.authorization-http-json:5.0.0",
    "addresses": {
      "additionalProp1": "tcp4:192.168.0.7:45326",
      "additionalProp2": "tcp4:192.168.0.7:45326",
      "additionalProp3": "tcp4:192.168.0.7:45326"
    },
    "metadata": {
      "basePath": "v2"
    },
    "timeToLive": "2d12h"
  }

Relation between instances and service types

As mentioned above, one service instance registers exactly one service type and one service type maps onto exactly one version of a contract (IDD).
After studying other solutions and implementing similar architectures in practice, we suggest defining service instance IDs to be unique only within a given service type. This would also align with, for example, DNS-SD that was used in Arrowhead before.
This has a few implications on the service discovery API namely:

The GET /services/{service-id} endpoint gets replaced by GET /services({service-type}/instances/{service-id}
The DELETE /services/{service-id} endpoint gets replaced by DELETE /services({service-type}/instances/{service-id}
For API completeness, an endpoint, GET /services({service-type}/instances, is added that lists all instances for a single service type.

Good API Design

All requests and responses should always have an envelop. E.g. we should not send or receive arrays/lists as the top-most object. This prevents adding new features (e.g. pagination) without breaking backwards-compatibility.
Relative times are less descriptive than absolute times (both from a readability and a technical perspective). Therefore, an expiresAt would be better than a timeToLive.

borditamas commented 4 months ago

Dear @DavidRutqvist

Regarding to the service-type:

According to our understanding service-type in your IDD is basically the serviceDefinition from v4.x, plus some interface information, (plus you suggests also the version), and you don't have a separate field for the service definition. What is the point of this merging? How to look up then for a service definition without any interface information?
We agree that a service definition, a version and an interface maps to an exact IDD, but don't see why these should be a single property.

Regarding to the serviceId:

What is the advantage of being unique only within a service-type? Why can't be unique within the Local Cloud?
Why the provider should specify the service instance id? How it knows what is unique? Even if the service id is unique within the service type, there could be collisions. We think that it should be generated based on the provider name, service definition and version by Service Registry during the registration request and returned as a response.

Regarding to your "Good API Design" points: we fully agree

Please also give feedback on the issue #87, because it belongs to the same domain and there are also open questions/suggestions. The question of interface representation would be the most important one.

jerkerdelsing commented 3 months ago

Sinteiq, can you comment on this.

jerkerdelsing commented 3 months ago

For now we should continue to use the latex documentation instead of languages specific to for example HTTP REST implementations.

jerkerdelsing commented 3 months ago

The service discovery record may need some modification due to the extendable service interface of sth SysMonitor and the SD_Monitoring service.

eclipse-arrowhead / roadmap