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

TI API Rel 0.9.3: API Coherency with Edge Cloud terminology, CAMARA Guidelines and Consent Management #167

Closed
wants to merge 0 commits into from

Conversation

FabrizioMoggio
Copy link
Collaborator

@FabrizioMoggio FabrizioMoggio commented Jan 10, 2024

alignement of parameters with EdgeCloud_LCM: applicationId changed into appId and instanceId changed into appInstanceId.

  • Added openId authentication for Consent Management
  • Aligment with guidelines:
    • added xcorrelator
    • aligned returned Errors

@FabrizioMoggio FabrizioMoggio changed the title appId and appInstanceId TI API Rel 0.9.3: appId and appInstanceId update Jan 10, 2024
gainsley
gainsley previously approved these changes Jan 10, 2024
Comment on lines 357 to 358
AppId:
$ref: '#/components/schemas/AppId'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change in line 357 AppId for appId, avoiding starting with capital letter as in the rest of parameters.

Copy link
Collaborator

@sergiofranciscoortiz sergiofranciscoortiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good, just a couple of changes in yaml and md to keep using appId without a capital letter in TrafficInfluence object properties.

1) activate the optimal routing for any EAS instance: the TI API must be invoked with the applicationId. The Telco Operator Platform identifies all the EAS instances and activates the optimal routing on the Mobile Network.
2) activate the optimal routing in a specific Region or Zone: the TI API must be invoked with the applicationId and the Zones and Regions identifiers.
1) activate the optimal routing for any EAS instance: the TI API must be invoked with the "appId". The Telco Operator Platform identifies all the EAS instances and activates the optimal routing on the Mobile Network.
2) activate the optimal routing in a specific Region or Zone: the TI API must be invoked with the "appId" and the Zones and Regions identifiers.
3) activate the optimal routing for a user devices: the TI API can be invoked with a user Device identifier (“Device”). For each user Device, a TI API invocation is required.

## Version: 0.9.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Propose to change to version 0.9.3 as in yaml

@@ -218,8 +217,8 @@ Delete an existing TrafficInfluence resource
| ---- | ---- | ----------- | -------- |
| trafficInfluenceID | string | Identifier for the Traffic Influence resource. This parameter is returned by the API and must be used to update it (e.g., adding new users or deleting it). | No |
| apiConsumerId | string | Unique Identifier of the TI API Consumer. | Yes |
| applicationId | string | Unique ID representing the Edge Application<br>_Example:_ `"Virtual_Reality_Arena"` | Yes |
| instanceId | string | Unique identifier generated by the partner OP to identify an instance of the application on a specific zone. | No |
| AppId | string (uuid) | A globally unique identifier associated with the application. OP generates this identifier when the application is submitted over NBI.<br>_Example:_ `"6B29FC40-CA47-1067-B31D-00DD010662DA"` | No |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion to change AppId, for appId for coherence with the rest of the table avoiding capital letters at the beginning, change proposed also for yaml.

Copy link
Collaborator Author

@FabrizioMoggio FabrizioMoggio Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the parameters are defined in coherency with: https://github.com/camaraproject/EdgeCloud/blob/main/code/API_definitions/EdgeCloud_LcM.yaml

the parameters are appId and appInstanceId, while the schema names are AppId and AppInstanceId.

like:
appInstanceId:
$ref: '#/components/schemas/AppInstanceId'

from line 598 of EdgeCloud-LcM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems ok it most parts, schema is ok like that. It is just within TrafficInfluence Model, in the parameters included both in yaml ( line 357) and in Documentation file ( line 220), it seems more appropriate to keep consistency with the rest of parameters to start with lower case.

1) activate the optimal routing for any EAS instance: the TI API must be invoked with the applicationId. The Telco Operator Platform identifies all the EAS instances and activates the optimal routing on the Mobile Network.
2) activate the optimal routing in a specific Region or Zone: the TI API must be invoked with the applicationId and the Zones and Regions identifiers.
1) activate the optimal routing for any EAS instance: the TI API must be invoked with the "appId". The Telco Operator Platform identifies all the EAS instances and activates the optimal routing on the Mobile Network.
2) activate the optimal routing in a specific Region or Zone: the TI API must be invoked with the "appId" and the Zones and Regions identifiers.
3) activate the optimal routing for a user devices: the TI API can be invoked with a user Device identifier (“Device”). For each user Device, a TI API invocation is required.

## Version: 0.9.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Version: 0.9.2
## Version: 0.9.3

@@ -218,8 +217,8 @@ Delete an existing TrafficInfluence resource
| ---- | ---- | ----------- | -------- |
| trafficInfluenceID | string | Identifier for the Traffic Influence resource. This parameter is returned by the API and must be used to update it (e.g., adding new users or deleting it). | No |
| apiConsumerId | string | Unique Identifier of the TI API Consumer. | Yes |
| applicationId | string | Unique ID representing the Edge Application<br>_Example:_ `"Virtual_Reality_Arena"` | Yes |
| instanceId | string | Unique identifier generated by the partner OP to identify an instance of the application on a specific zone. | No |
| AppId | string (uuid) | A globally unique identifier associated with the application. OP generates this identifier when the application is submitted over NBI.<br>_Example:_ `"6B29FC40-CA47-1067-B31D-00DD010662DA"` | No |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
| AppId | string (uuid) | A globally unique identifier associated with the application. OP generates this identifier when the application is submitted over NBI.<br>_Example:_ `"6B29FC40-CA47-1067-B31D-00DD010662DA"` | No |
| appId | string (uuid) | A globally unique identifier associated with the application. OP generates this identifier when the application is submitted over NBI.<br>_Example:_ `"6B29FC40-CA47-1067-B31D-00DD010662DA"` | No |

description: Unique ID representing the Edge Application
instanceId:
$ref: '#/components/schemas/InstanceIdentifier'
AppId:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
AppId:
appId:

@@ -218,8 +217,8 @@ Delete an existing TrafficInfluence resource
| ---- | ---- | ----------- | -------- |
| trafficInfluenceID | string | Identifier for the Traffic Influence resource. This parameter is returned by the API and must be used to update it (e.g., adding new users or deleting it). | No |
| apiConsumerId | string | Unique Identifier of the TI API Consumer. | Yes |
| applicationId | string | Unique ID representing the Edge Application<br>_Example:_ `"Virtual_Reality_Arena"` | Yes |
| instanceId | string | Unique identifier generated by the partner OP to identify an instance of the application on a specific zone. | No |
| AppId | string (uuid) | A globally unique identifier associated with the application. OP generates this identifier when the application is submitted over NBI.<br>_Example:_ `"6B29FC40-CA47-1067-B31D-00DD010662DA"` | No |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems ok it most parts, schema is ok like that. It is just within TrafficInfluence Model, in the parameters included both in yaml ( line 357) and in Documentation file ( line 220), it seems more appropriate to keep consistency with the rest of parameters to start with lower case.

@FabrizioMoggio
Copy link
Collaborator Author

@sergiofranciscoortiz you are totally right, thank you.

@FabrizioMoggio
Copy link
Collaborator Author

In my understanding this PR is completed, can it be merged?

@@ -120,7 +119,7 @@ Creates a new TrafficInfluence resource

##### Description

Takes as input an object containing the intents from the API Consumer and creates a TrafficInfluence resource accordingly. The trafficInfluenceID parameter, that is part of the object, must not be valorized when creating a new resource. For this reason the trafficInfluenceID parameter must be avoided in the object, anyway it will be ignored by the API Producer. It is automatically generated by the system and returned in the response.
Gets in input an object containing the intents from the API Consumer and creates a TrafficInfluence resourse accordingly. The trafficInfluenceID parameter, that is part of the object, must not be valorized when creating a new resource. For this reason the trafficInfluenceID parameter must be avoided in the object, anyway it will be ignored by the API Producer. It is automatically generated by the system and returned in the response.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/resourse/resource

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure I'm going to fix it. Before I have checked the yaml and there it was correct. Now I see that the md file is wrong. Thank you vey much.

@@ -120,7 +119,7 @@ Creates a new TrafficInfluence resource

##### Description

Takes as input an object containing the intents from the API Consumer and creates a TrafficInfluence resource accordingly. The trafficInfluenceID parameter, that is part of the object, must not be valorized when creating a new resource. For this reason the trafficInfluenceID parameter must be avoided in the object, anyway it will be ignored by the API Producer. It is automatically generated by the system and returned in the response.
Gets in input an object containing the intents from the API Consumer and creates a TrafficInfluence resourse accordingly. The trafficInfluenceID parameter, that is part of the object, must not be valorized when creating a new resource. For this reason the trafficInfluenceID parameter must be avoided in the object, anyway it will be ignored by the API Producer. It is automatically generated by the system and returned in the response.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Takes as input" is correct (not "Gets in input")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I will fix it

Copy link
Collaborator Author

@FabrizioMoggio FabrizioMoggio Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong also in the YAML file, I'm going to fix it also there

Copy link
Collaborator

@Kevsy Kevsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of typos (see comments above ^) otherwise LGTM

@FabrizioMoggio
Copy link
Collaborator Author

I'm going to update the YAML according to #192

@sergiofranciscoortiz
Copy link
Collaborator

I'm going to update the YAML according to #192

I reckon that you meant to #191, to take into account erros/warnings from CAMARA linter in TI API

@FabrizioMoggio
Copy link
Collaborator Author

sure :-) #191

@FabrizioMoggio FabrizioMoggio changed the title TI API Rel 0.9.3: appId and appInstanceId update TI API Rel 0.9.3: API Coherency with Edge Cloud terminology, CAMARA Guidelines and Consent Management Apr 11, 2024
Copy link
Collaborator

@javierlozallu javierlozallu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adapt the YAML to the maximum line length of 80 characters to pass MegaLinter control.

@FabrizioMoggio
Copy link
Collaborator Author

I'm going to adapt the YAML according to the MegaLinter results. Anyway please note that the new version has more features to be carefully checked (@Kevsy , @gunjald). The important one are:

Consent Management: Client Credential is now substituted by openId to support 3Legs
xcorrelator: it was added as an input parameter and as an output header

@javierlozallu
Copy link
Collaborator

@FabrizioMoggio regarding the 3 legs auth support, could you give a brief overview of the implementation and whether we should adopt it in the other APIs? As 3 legs is for Consent Management and based on your implementation, you can use it for specific methods and specify the action allowed, so I think we should implement it in LCM where needed.

Also I see that xcorrelator is being implemented in others APIs in CAMARA, is this mandatory for all the APIs?

@FabrizioMoggio
Copy link
Collaborator Author

@javierlozallu 3Legs is based on OpenId and let you to specify the level of access right (read or write). I based the TI API implementation according to what done in CallForwardingSignal: https://github.com/camaraproject/CallForwardingSignal/blob/main/code/API_definitions/Call_Forwarding_Signal.yaml

Because that API is a very simple one it is maybe easier to understand than the TI API if you need an example. We can have a discussion about 3Legs in an Edge Cloud call.

The TI API needs Consent Management not because it uses MSISDN that is indeed owned by the API Consumer and not provided back by the API Producer as a new information. The TI API needs Consent Management because it informs the Operator about the Device position. This must be allowed by the End User. Maybe this also applies to LCM.

x-correlator, should be included as defined in the Guidelines: https://github.com/camaraproject/Commonalities/blob/main/documentation/API-design-guidelines.md#9-architecture-headers

@javierlozallu
Copy link
Collaborator

Thank you very much! @FabrizioMoggio

It is now clearer to me and I can imagine some use cases where Consent would be needed in LCM and perhaps for Simple Edge API, so I will add this item to the agenda (I will share it on the meeting list later this afternoon) so that we can discuss it at tomorrow's meeting.

@Kevsy
Copy link
Collaborator

Kevsy commented Apr 16, 2024

Hi @javierlozallu

I can imagine some use cases where Consent would be needed in LCM and perhaps for Simple Edge API,

Correct, Simple Edge Discovery 0.9.4-rc.1 already includes the Identity and Consent management guidelines for consent (per closed issue camaraproject/SimpleEdgeDiscovery#21 )

@javierlozallu
Copy link
Collaborator

Yes @Kevsy, I noticed that SED already has it and I have put it on the table that I am going to present at the meeting.

@FabrizioMoggio
Copy link
Collaborator Author

FabrizioMoggio commented Apr 30, 2024

this version of the TI API YAML passes MegaLinter. Just one error was intentionally left. The error is on Carriage Returns that must be changed from windows to Linux formatting. I avoided doing it so far otherwise it changes every line and the other modification to the code are no more visible.

@FabrizioMoggio
Copy link
Collaborator Author

Considering you can view the modified code so far in the commit history, I'm going to upload a new version with the Carriage Return fixed.

FabrizioMoggio added a commit to FabrizioMoggio/EdgeCloud that referenced this pull request May 3, 2024
@FabrizioMoggio
Copy link
Collaborator Author

The last Commit has not produced any MegaLinter warning or error.

@FabrizioMoggio
Copy link
Collaborator Author

@Kevsy I have implemented the changes you requested a long time ago. :-) I don't know why the system is not acknowledging that. Currently the system is still requesting for those comments to be addressed. Maybe you can do some action.

@FabrizioMoggio
Copy link
Collaborator Author

FabrizioMoggio commented May 14, 2024

Hi, how can we move forward with this PR? Do I need to update the folder structure to the new one?

@javierlozallu
Copy link
Collaborator

Hi, how can we move forward with this PR? Do I need to update the folder structure to the new one?

@Kevsy I have implemented the changes you requested a long time ago. :-) I don't know why the system is not acknowledging that. Currently the system is still requesting for those comments to be addressed. Maybe you can do some action.

Hi @Kevsy, could you help us with this?

@javierlozallu
Copy link
Collaborator

Hi, how can we move forward with this PR? Do I need to update the folder structure to the new one?

Yes, I think that the conflict is related to that. Only the folder for the yaml has been changed, the .md file doesn't have to be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants