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

[SMF] relocation of user-location-info on top level for Gx,Gy #2331

Merged
merged 2 commits into from
May 25, 2023

Conversation

EugeneBogush
Copy link
Contributor

Now "3GPP-User-Location-Info" located in the wrong place.

image

at according 3GPP TS 32.299 "3GPP-User-Location-Info" is 3GPP specific AVP, which used in CCR,
and must locate on top level, as below:

image

@acetcom acetcom merged commit b391cc5 into open5gs:main May 25, 2023
3 checks passed
@acetcom
Copy link
Member

acetcom commented May 25, 2023

@EugeneBogush

Thank you so much!
Sukchan

@EugeneBogush EugeneBogush deleted the gxy_user_location_info branch May 25, 2023 13:53
@pespin
Copy link
Contributor

pespin commented Jun 7, 2023

3GPP TS 32.299 version 16.2.0 Release 16 (https://www.etsi.org/deliver/etsi_ts/132200_132299/132299/16.02.00_60/ts_132299v160200p.pdf).

All references I find about "3GPP-User-Location-Info" are inside following AVPs:

Location-Info :: = < AVP Header: 3460>
ProSe-Direct-Communication-Reception-Data-Container :: = < AVP Header: 3461>
PS-Information :: = < AVP Header: 874>
Related- Change-Condition- Information :: = < AVP Header: 3925>
Service-Data-Container :: = < AVP Header: 2040>
Traffic-Data-Volumes :: = < AVP Header: 2046>

Specifically, the one where it used to be before this change, section "7.2.158 PS-Information AVP" states:

PS-Information :: = < AVP Header: 874>
...
  [ 3GPP-User-Location-Info ]

Furthermore, in 6.4.2 Credit-Control-Request message, this AVP doesn't show as listed as top level for the message.

So, can somebody explain why this patch is changing the previous state of things? Am I missing something? I don't see this patch providing specific references to back it, even though I requested them when I was pinged some weeks ago in the initial PR which introduced this AVP long time ago.

@acetcom @EugeneBogush

@acetcom
Copy link
Member

acetcom commented Jun 10, 2023

I currently do not have time to review this part.

To @EugeneBogush
Could you please respond to @pespin's comment?

Thanks a lot!
Sukchan

@pespin
Copy link
Contributor

pespin commented Jun 12, 2023

@acetcom I'll submit a PR reverting the change in the next days if there's no answer from @EugeneBogush .

@EugeneBogush
Copy link
Contributor Author

EugeneBogush commented Jun 12, 2023 via email

@pespin
Copy link
Contributor

pespin commented Jun 12, 2023

Ok I understand the issue now. TS 129 212 you shared seems to specify the Gx interface, while TS 32.299 specifies the Gy interface. The position of the AVP is different in each interface.

@EugeneBogush do you mind submitting a patch reverting the Gy part of your commit?

@EugeneBogush
Copy link
Contributor Author

EugeneBogush commented Jun 12, 2023

@EugeneBogush do you mind submitting a patch reverting the Gy part of your commit?

Yes, but you need to understand where to insert 3gpp-User-location-info for Gy
and this applies not only to 3GPP-User-Location-Info

@pespin
Copy link
Contributor

pespin commented Jun 12, 2023

@EugeneBogush not sure what you mean. The 3gpp-User-location-info was in the good place before your patch, so all you need to do is to revert your changes in src/smf/gy-path.c.

@EugeneBogush
Copy link
Contributor Author

so all you need to do is to revert your changes in src/smf/gy-path.c.

but it will not correctly.

earlier you mentioned the PS-information field, I think it's more correct to place it here

PS-Information :: = < AVP Header: 874>
...
[ 3GPP-User-Location-Info ]

@pespin
Copy link
Contributor

pespin commented Jun 12, 2023

@EugeneBogush it was already in PS-Information before your change:

    /* PS-Information, TS 32.299 sec 7.2.158 */
    ret = fd_msg_avp_new(ogs_diam_gy_ps_information, 0, &avpch1);
...
    /* 3GPP-User-Location-Info, 3GPP TS 29.061 16.4.7.2 22 */
    smf_fd_msg_avp_add_3gpp_uli(sess, avpch1);

It's also validated in our TTCN3 tests:
https://gitea.osmocom.org/ttcn3/osmo-ttcn3-hacks/src/branch/master/ggsn_tests/GGSN_Tests.ttcn#L552
https://jenkins.osmocom.org/jenkins/view/TTCN3/job/ttcn3-ggsn-test-ogs/465/artifact/logs/ggsn-tester/GGSN_Tests.TC_gy_charging_cc_time.pcap.gz

@EugeneBogush
Copy link
Contributor Author

it was already in PS-Information before your change:

yes, sorry.

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.

None yet

3 participants