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

IDD 5.0 review discussion - Service-discovery #86

Open
AlexChiquito opened this issue Feb 19, 2024 · 8 comments
Open

IDD 5.0 review discussion - Service-discovery #86

AlexChiquito opened this issue Feb 19, 2024 · 8 comments
Labels
5.0 Core Specification The issue concerns fundamental Arrowhead specifications or documentation Core System: Service Registry The issue concerns the Core Service Registry system

Comments

@AlexChiquito
Copy link
Contributor

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

@emanuelpalm emanuelpalm added 5.0 Core System: Service Registry The issue concerns the Core Service Registry system Core Specification The issue concerns fundamental Arrowhead specifications or documentation labels Feb 19, 2024
@emanuelpalm
Copy link
Contributor

emanuelpalm commented Feb 19, 2024

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

@borditamas
Copy link
Member

AITIA review comments

top of either of the HTTP or HTTPS protocols with payloads encoded in JSON. The service allows
for consumers to register, deregister and search for _service records_, provided by themselves

"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.

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


  • 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.

  • 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

  • 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
Copy link
Member

@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
Copy link

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:
  1. The GET /services/{service-id} endpoint gets replaced by GET /services({service-type}/instances/{service-id}
  2. The DELETE /services/{service-id} endpoint gets replaced by DELETE /services({service-type}/instances/{service-id}
  3. 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
Copy link
Member

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
Copy link
Member

Sinteiq, can you comment on this.

@jerkerdelsing
Copy link
Member

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

@jerkerdelsing
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 Core Specification The issue concerns fundamental Arrowhead specifications or documentation Core System: Service Registry The issue concerns the Core Service Registry system
Projects
None yet
Development

No branches or pull requests

5 participants