-
Notifications
You must be signed in to change notification settings - Fork 55
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
Send Deregistration Notify before UDM modifies the UE context #25
Send Deregistration Notify before UDM modifies the UE context #25
Conversation
* If sending Deregistration Notify after UDM modifies the UE context, old AMF can't deregister for the UE.
|
||
return nil, nil, nil | ||
} else { | ||
if oldAmf3GppAccessRegContext == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this condition is unnecessary because the UDM sent the dereg notification to the old AMF.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the UE is initially registered with the UDM through the AMF, it triggers the execution of this function block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if old context was existed, it should reply StatusOK with new request in body
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #25 (comment)
@@ -217,6 +217,22 @@ func RegistrationAmf3gppAccessProcedure(registerRequest models.Amf3GppAccessRegi | |||
ue, _ := udm_context.Getself().UdmUeFindBySupi(ueID) | |||
oldAmf3GppAccessRegContext = ue.Amf3GppAccessRegistration | |||
} | |||
// TS 23.502 4.2.2.2.2 14d: UDM initiate a Nudm_UECM_DeregistrationNotification to the old AMF | |||
// corresponding to the same (e.g. 3GPP) access, if one exists | |||
if oldAmf3GppAccessRegContext != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why move dereg procedure here? oldAmf3GppAccessRegContext
was already retrieved and will not be overwritten by new context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can create the goroutine for sending deregistration notifications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see #25 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In HandleRegistrationAmf3gppAccessRequest():
// step 3: handle the message
header, response, problemDetails := RegistrationAmf3gppAccessProcedure(registerRequest, ueID)
// step 4: process the return value from step 3
if response != nil {
if header != nil {
// status code is based on SPEC, and option headers
return httpwrapper.NewResponse(http.StatusCreated, header, response)
}
return httpwrapper.NewResponse(http.StatusOK, nil, response)
} else if problemDetails != nil {
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YouShengLiu you missed this patch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will supplement it.
// TS 23.502 4.2.2.2.2 14d: UDM initiate a Nudm_UECM_DeregistrationNotification to the old AMF | ||
// corresponding to the same (e.g. 3GPP) access, if one exists | ||
if oldAmf3GppAccessRegContext != nil { | ||
deregistData := models.DeregistrationData{ | ||
DeregReason: models.DeregistrationReason_SUBSCRIPTION_WITHDRAWN, | ||
AccessType: models.AccessType__3_GPP_ACCESS, | ||
} | ||
// Deregistration Notify Triggered | ||
pd := callback.SendOnDeregistrationNotification(ueID, | ||
oldAmf3GppAccessRegContext.DeregCallbackUri, | ||
deregistData, | ||
) | ||
if pd != nil { | ||
return nil, nil, pd | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TS 23.502 4.2.2.2.2 14d: UDM initiate a Nudm_UECM_DeregistrationNotification to the old AMF
// corresponding to the same (e.g. 3GPP) access, if one exists
if oldAmf3GppAccessRegContext != nil {
if !ue.SameAsStoredGUAMINon3gpp(*oldAmfNon3GppRegCtx.Guami) {
deregistData := models.DeregistrationData{
DeregReason: models.DeregistrationReason_SUBSCRIPTION_WITHDRAWN,
AccessType: models.AccessType__3_GPP_ACCESS,
}
// Deregistration Notify Triggered
pd := callback.SendOnDeregistrationNotification(ueID,
oldAmf3GppAccessRegContext.DeregCallbackUri,
deregistData,
)
if pd != nil {
return nil, nil, pd
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrite like this:
// TS 23.502 4.2.2.2.2 14d: UDM initiate a Nudm_UECM_DeregistrationNotification to the old AMF
// corresponding to the same (e.g. 3GPP) access, if one exists
if oldAmf3GppAccessRegContext != nil {
if !ue.SameAsStoredGUAMI3gpp(*oldAmfNon3GppRegCtx.Guami) {
deregistData := models.DeregistrationData{
DeregReason: models.DeregistrationReason_SUBSCRIPTION_WITHDRAWN,
AccessType: models.AccessType__3_GPP_ACCESS,
}
logger.UecmLog.Infof("Send DeregNotif to old AMF GUAMI=%v", oldAmf3GppAccessRegContext.Guami)
go func() {
pd := callback.SendOnDeregistrationNotification(ueID,
oldAmf3GppAccessRegContext.DeregCallbackUri,
deregistData) // Deregistration Notify Triggered
if pd != nil {
logger.UecmLog.Errorf("RegistrationAmf3gppAccess: send DeregNotif fail %v", pd)
}
}()
}
return nil, ®isterRequest, nil
} else {
header = make(http.Header)
udmUe, _ := udm_context.Getself().UdmUeFindBySupi(ueID)
header.Set("Location", udmUe.GetLocationURI(udm_context.LocationUriAmf3GppAccessRegistration))
return header, ®isterRequest, nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This handler deals with the 3GPP access case. Why do we need to check the non-3GPP access context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to R15 TS 29.503 - 5.3.2.2.2 AMF registration for 3GPP access,
2a. On success, and if another AMF is registered for 3GPP access, the UDM updates the Amf3GppAccessRegistration resource by replacing it with the received resource information, and responds with "204 No Content".
UDM shall invoke the Deregistration Notification service operation towards the old AMF using the callback URI provided by the old AMF.
If the old Amf3GppAccessRegistration context exists, it should return "204 No Content" with an empty response body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @YouShengLiu
Please see TS 23.502 4.2.2.2.1 Step 14d:
When the UDM stores the associated Access Type (e.g. 3GPP) together with the serving AMF as indicated in
step 14a, it will cause the UDM to initiate a Nudm_UECM_DeregistrationNotification (see clause 5.2.3.2.2) to the old AMF corresponding to the same (e.g. 3GPP) access
So, we need to check whether the access type is 3GPP or not if there is the UECM context for the same UE.
If and only if yes, UDM sends the Nudm_UECM_DeregistrationNotification to the old AMF.
* Compare the old UECM context with the new one to determine whether to send a deregistration notification. * Use goroutine to send a deregistration notification to old AMF.
logger.UecmLog.Errorf("RegistrationAmf3gppAccess: send DeregNotify fail %v", pd) | ||
} | ||
}() | ||
} | ||
return nil, nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
return nil, ®isterRequest, nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I will modify it.
…nNotification * Supplement the missed patch. * Add query parameters to differentiate whether the type is implicit or explicit.
@YouShengLiu |
- Remove the query parameter - Replace the DeregReason with UE_INITIAL_REGISTRATION, since it's triggered during the registration procedure
callback.SendOnDeregistrationNotification(ueID, oldAmf3GppAccessRegContext.DeregCallbackUri, | ||
deregistData) // Deregistration Notify Triggered | ||
if !ue.SameAsStoredGUAMI3gpp(*oldAmf3GppAccessRegContext.Guami) { | ||
deregistData := models.DeregistrationData{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modify as below:
// Based on TS 23.502 4.2.2.2.2, If the serving NF removal reason indicated by the UDM is Initial Registration,
// the old AMF invokes the Nsmf_PDUSession_ReleaseSMContext (SM Context ID). Thus we give different
// dereg cause based on registration parameter from serving AMF
deregReason := models.DeregistrationReason_UE_REGISTRATION_AREA_CHANGE
if request.InitialRegistrationInd {
deregReason = models.DeregistrationReason_UE_INITIAL_REGISTRATION
}
deregistData := models.DeregistrationData{
DeregReason: deregReason,
AccessType: models.AccessType__3_GPP_ACCESS,
}
|
||
return nil, nil, nil | ||
logger.UecmLog.Infof("Send DeregNotify to old AMF GUAMI=%v", oldAmf3GppAccessRegContext.Guami) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this logger into go routine.
… InitialRegistrationInd is true
free5gc/amf#107