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

Application Endpoint Discovery First Contribution #245

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

crissancas
Copy link
Collaborator

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

The Application Discovery API extends beyond the capabilities of the
Simple Edge Discovery API by not only locating the nearest Edge Cloud Zone
but also directly linking to the application endpoints within those zones.
This PR covers intent 21.

Which issue(s) this PR fixes:

Fixes #212

Special notes for reviewers:

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

Copy link

github-actions bot commented May 20, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ ACTION actionlint 2 0 0.03s
✅ JSON eslint-plugin-jsonc 1 0 0 1.23s
✅ JSON jsonlint 1 0 0.19s
✅ JSON prettier 1 1 0 0.65s
✅ JSON v8r 1 0 1.6s
✅ OPENAPI spectral 3 0 6.14s
✅ REPOSITORY git_diff yes no 0.38s
✅ REPOSITORY secretlint yes no 4.45s
✅ YAML yamllint 3 0 0.74s

See detailed report in MegaLinter reports

MegaLinter is graciously provided by OX Security

type: string
enum:
- closest
- name: IPv4-Address
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to specify what all parameters combinations are valid as there are multiple header parameters proposed though it has been with other APIs too. IPv4-Address, IPv6-Address, Network-Access-Identifier and Phone-Number which is mandatory. Can all of them be present? If not then there should be some guidance for application developers what should be valid parameter combinations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, as you say it is the same for other APIs. I suggest we wait for Commonalities release 0.4 for guidance.

Choose a reason for hiding this comment

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

OK

description: A globally unique identifier associated with the application.
OP generates this identifier when the application is submitted over NBI.
AccessEndpoint:
type: object
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think for AccessEndpoint we need more discussion in terms of the data model. The reason is what should we expect in terms of the number of exposure endpoints of an application towards the application clients. May be an application is composed of multiple components and offers one or more endpoints to its clients for different purposes. In that case data model should consider those possibilities and accordingly consider them in defining the AccessEndpoint structure e.g. one or more ports, fqdns etc as an example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why AccessEndpoint and not ApplicationServerEndpoint?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or ApplicationInstanceEndpoint or AppInstanceEndpoint?

Choose a reason for hiding this comment

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

OK. Agree with AppInstanceEndpoint and as in the comment above in line 134, AppServerEndpoint is more specific and we are refering mostly the application in general

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.

Please see comments inline for review.

Authorization: Bearer {your_access_token}
Accept: application/json
Phone-Number: +441234567890
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think one more example should be provided here for the "initial requests may be made without explicit device identifiers"

Choose a reason for hiding this comment

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

OK. We are going to wait for commonalities 0.4.0 to come out for guidance.

enum:
- closest
- name: IPv4-Address
in: header
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should mention about what are valid combination from the give set of the user identifiers e.g. IPv4-Address, Network-Access-Identifier etc. If one of them or any of them or all should be present?

Choose a reason for hiding this comment

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

OK. Agree with you. We are going to wait for the new commonalities to come out for guidance.

format: uuid
description: A globally unique identifier associated with the application.
OP generates this identifier when the application is submitted over NBI.
AccessEndpoint:
Copy link
Collaborator

Choose a reason for hiding this comment

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

As commented earlier an application may have more than one endpoints and for that reason we may use AccessEndpoints to accommodate plurality. If so, then we would need to make the AccessEndpoints as an array of objects that is being defined below which contains the endpoint accessibility information?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, in my opinion if multiple endpoints are returned then there should be an additional field which allow to embed a purpose or qualifier for a given endpoint which can be used by the client applications to differentiate one endpoint with other e.g. low quality video stream or high quality video stream endpoint? But then these qualifier should come from the API invoker may be at application onboarding time. Any thoughts?

Copy link
Collaborator

@gunjald gunjald left a comment

Choose a reason for hiding this comment

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

Seems fine to me

This GUID will have been previously provided by the
Edge Cloud Zone provider.
AccessEndpoints:
type: object
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think one comment is lost for some reasons. From the last comment "May be an application is composed of multiple components and offers one or more endpoints to its clients for different purposes. In that case data model should consider those possibilities and accordingly consider them in defining the AccessEndpoint structure e.g. one or more ports, fqdns etc as an example."

In my opinion the AccessEndpoint should be an array of objects instead of a single object as it is defined now to have one or more endpoints. And each endpoint may have a qualifier defined by API invoker to help differentiate between the endpoints based on the purpose of the endpoint in client side code. May be such qualifiers can be provided during app onboarding time of the application along with the components to which the endpoint belongs to in the /app-endpoints response.

Choose a reason for hiding this comment

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

Agree with you and as it was concluded in the edge cloud meeting for this version we keep a single endpoint

@crissancas crissancas merged commit f048b00 into main Jun 13, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants