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

issue_194_PR #217

Closed
wants to merge 6 commits into from
Closed

Conversation

SyeddR
Copy link
Collaborator

@SyeddR SyeddR commented Sep 8, 2023

What type of PR is this?

What this PR does / why we need it:

  • Identifies the application consumer that initiates a qos session. The ID is systematically relayed to downstream systems, including but not limited to, the Network Exposure Function (NEF) and the Policy Control Function (PCF) within the 5G framework. The primary utility of this transmission is to facilitate the application of relevant policies, ensuring that the application consumer is appropriately tagged within usage records.

Which issue(s) this PR fixes:

Fixes #194

Special notes for reviewers:

For lack of the better term i am proposing "applicationConsumerId" as string type field. Open for suggestions

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

@eric-murray
Copy link
Collaborator

Hi @SyeddR

I had understood that the applicationConsumerId must be associated with one, and only one application. Is that the case? If so, my preference would be for this to be a sub-property of the applicationServer property, the reason being that I'm not a fan of "flat" input parameter structures, and much prefer that related parameters be grouped, particularly when some (such as this one) are optional.

Of course, you could argue that this should also apply to applicationServerPorts, and I'd agree with you :-)

I also agree that adding this property to the ApplicationServer schema requires some thought, as we need to ensure that the API consumer provides at least one IP address, but there are ways in OAS that that can be mandated whilst still allowing applicationConsumerId to be optionally specified.

@SyeddR
Copy link
Collaborator Author

SyeddR commented Sep 21, 2023

@eric-murray yes applicationConsumerId must be associated with one and only one application. I dont have a strong opinion on keeping it under applicationServer property or outside of it. Either way works for me.

@SyeddR SyeddR closed this Sep 21, 2023
@SyeddR SyeddR reopened this Sep 21, 2023
@jlurien
Copy link
Collaborator

jlurien commented Sep 22, 2023

I suggested to open an issue on Identity and Consent WG about this topic, as identification of Application is being discussed there in the context of authentication, and access token should contain all required information.

@SyeddR
Copy link
Collaborator Author

SyeddR commented Oct 16, 2023

As per the action items from the discussion in the last meeting.

  1. Changed the naming from applicationConsumerId to applicationId to avoid confusion with the consumer of the application.
  2. Added more description for the purpose of applicationId and how it is different than a clientId.
  3. Moved the parameter under application server object.

@@ -433,6 +433,7 @@ components:
description: Attributes required to create a session
type: object
properties:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the blank line here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed it.

SyeddR and others added 2 commits October 25, 2023 13:00
Clarified that applicationId is not sufficient to identify the application server
@eric-murray
Copy link
Collaborator

@SyeddR
I'm fine with the proposal, but updated the ApplicationServer schema description to clarify that applicationId itself is not sufficient to identify the application server, and that an IP address is still required.

I'm a bit concerned that the schema definition will not catch API consumers who only provide the applicatonId (because that will satisfy the minProperties: 1 directive), so am wondering if we need to re-organise the schema so that a single ipAddress property is now required. One for discussion.

There appear to be some conflicts now with the main branch (which maybe I introduced - apologies if that is the case), Can you synchronise your fork with main to see if that fixes things?

@SyeddR
Copy link
Collaborator Author

SyeddR commented Oct 30, 2023

There appear to be some conflicts now with the main branch (which maybe I introduced - apologies if that is the case), Can you synchronise your fork with main to see if that fixes things?

@eric-murray My fork shows all synced with latest updates. Can you try to resolve the conflict at your end.

@eric-murray
Copy link
Collaborator

@SyeddR
OK, fixed and now approved from my side. It seems my original edits were not properly merged into your own fork, but subsequent edits were, so all good now.

Copy link
Collaborator

@RandyLevensalor RandyLevensalor left a comment

Choose a reason for hiding this comment

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

Could we apply some constraints to this?

description: |
A public identifier that uniquely identifies the application initiating a QoS session. The applicationId is more granular than the clientId, which identifies the unique customer. A client or customer could have one or more applications that need to be tracked. The Id is systematically relayed to downstream systems, including but not limited to, the Network Exposure Function (NEF) and the Policy Control Function (PCF) within the 5G framework.
The primary utility of this transmission is to facilitate the application of relevant policies, ensuring that the application is appropriately tagged within usage records.
type: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we some constraints on the string size and values?
minLength: 3
maxLength: 256 (default value)
pattern: (should we exclude strings like "??? or ***"

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SyeddR any response to this?

@hdamker hdamker marked this pull request as draft November 6, 2023 23:44
@hdamker
Copy link
Collaborator

hdamker commented Nov 6, 2023

Converted to draft status as agreed within our call on Nov 3rd.

@hdamker
Copy link
Collaborator

hdamker commented Jul 11, 2024

This PR is stale and can't be merged into the current (splitted) API structure anymore.

@hdamker hdamker closed this Jul 11, 2024
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.

Add Application Function Id (afId) or Sponsor Id
5 participants