Skip to content

Conversation

@Takuto88
Copy link
Collaborator

@Takuto88 Takuto88 commented Jan 21, 2026

This branch contains a couple of bugfixes that were authored during 39C3 for the GSUP implementation of the insert subscriber data / update location procedures. They originally stem from @lynxis's branch here but have been cleaned up for upstreaming: https://github.com/lynxis/pyhss/commits/39c3/

The only commit that I am not sure of is 5abb85d. There was an issue with the ISRs not being triggered correctly. I suspect that this was a first shot at fixing this and the "real" fixes were actually made in 6fcebf1. Maybe @lynxis can chime in and explain?

In any case the change in 5abb85d does not break anything and it did work correctly during the event, therefore I included it.

lynxis and others added 5 commits January 21, 2026 15:16
Note: Commit message added by committer. Code is from the commit author.

Triggering the ISD did not work reliably in practice when just looking
at the JSON data and I (the commiter) don't know why. But when using the
Database it does work.
The location information itself is not always unique. During 39C3, the
MSC and SGSN did not have different IDs, causing the ISR to not work
correctly. This patch also takes the peer role into account to make
identifying the correct peer more reliable.
Previously, we only looked at the peer id in order to find running
transactions. This is a problem when multiple transactions are running
concurrently for different IMSIs:

- IMSI A starts an ULR from Peer A
- IMSI B starts an ULR from Peer A
- IMSI B will find the Transaction from IMSI A that might not be in the
  expected state -> Failure

This fixes this by also taking the subscriber's IMSI into account when
handling these transactions.

Co-authored-by: Alexander Couzens <lynxis@fe80.eu>
The value error caused the connection to be reset, which is not at all
what should happen here.

Co-authored-by: Alexander Couzens <lynxis@fe80.eu>
These files were touched recently by both Alexander and me.
@Takuto88 Takuto88 changed the title WIP: GSUP fixes 39C3 GSUP fixes Jan 21, 2026
@Takuto88 Takuto88 marked this pull request as ready for review January 21, 2026 15:02
@Takuto88 Takuto88 requested a review from osmith42 January 21, 2026 15:02
@osmith42
Copy link
Collaborator

Commits look good, feel free to merge!

The only commit that I am not sure of is 5abb85d. There was an issue with the ISRs not being triggered correctly.

I suspect this is needed because json_data may not have all these attributes used in the sendDiameterRequest() call when doing the API call, but the database object has them:

                    diameterClient.sendDiameterRequest(
                        requestType='CLR',
                        hostname=json_data['serving_mme'],
                        imsi=json_data['imsi'], 
                        DestinationHost=json_data['serving_mme'], 
                        DestinationRealm=json_data['serving_mme_realm'], 
                        CancellationType=1
                    )

In any case, as you wrote, it doesn't hurt to have this commit and given that this is what was running at 39C3 I think it makes a lot of sense to merge it.

@lynxis
Copy link
Contributor

lynxis commented Jan 24, 2026

About 5abb85d:
I think we should not merge this specific commit yet, because it is fixing some parts, but breaks other a little bit.

@osmith42 is correct about his assumption.

So when looking over the code again. I think json_data isn't modified by UpdateObj() which is good.
But the code is still not correct.

We should check what was changed via the call:

a) if the enabled fields got changed,, lets send a CLR on 4G (as the code does) and also a CLR on 2G/3G
b) if the subscriber is still enabled, send a ISD on 2G/3G and a IDR on 4G.

@lynxis
Copy link
Contributor

lynxis commented Jan 24, 2026

@Takuto88 great that you take care to upstream the changes. Thanks a lot!

@Takuto88
Copy link
Collaborator Author

I think we should not merge this specific commit yet, because it is fixing some parts, but breaks other a little bit.

@lynxis: Does it? What exactly? Because this commit ran during the event and I don't recall any issues.

But the code is still not correct.
We should check what was changed via the call:

a) if the enabled fields got changed,, lets send a CLR on 4G (as the code does) and also a CLR on 2G/3G
b) if the subscriber is still enabled, send a ISD on 2G/3G and a IDR on 4G.

That's pretty much what the code does apart from the CLR for 2G/3G. That would be a new feature though and I would consider that out of scope for this specific PR. But I agree that this should happen, so I have created #304 to track this.

Personally, I would merge this as is unless there is something with commit 5abb85d that I am missing.

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.

3 participants