fix: implement allowed NF types for service and add unit tests#214
fix: implement allowed NF types for service and add unit tests#214DBGR18 wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR tightens SMF NF profile service advertisement by setting AllowedNfTypes for nsmf-pdusession so that only AMF and SMF can consume it, and adds a unit test around the new mapping logic.
Changes:
- Add a per-service
AllowedNfTypesmapping fornsmf-pdusession(AMF, SMF) when buildingNFServices. - Refactor
SetupNFProfileNF service construction to allow conditional population ofAllowedNfTypes. - Add a unit test for the service-to-allowed-NF-types mapping helper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/context/nf_profile.go | Conditionally sets AllowedNfTypes on the nsmf-pdusession NF service via a new helper. |
| internal/context/nf_profile_test.go | Adds a unit test for allowedNfTypesForService. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package context | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| "github.com/free5gc/openapi/models" | ||
| ) | ||
|
|
||
| func TestAllowedNfTypesForService(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| serviceName models.ServiceName | ||
| want []models.NrfNfManagementNfType | ||
| }{ | ||
| { | ||
| name: "nsmf-pdusession allows AMF and SMF", | ||
| serviceName: models.ServiceName_NSMF_PDUSESSION, | ||
| want: []models.NrfNfManagementNfType{ | ||
| models.NrfNfManagementNfType_AMF, | ||
| models.NrfNfManagementNfType_SMF, | ||
| }, | ||
| }, | ||
| { | ||
| name: "other services remain unrestricted", | ||
| serviceName: models.ServiceName_NSMF_EVENT_EXPOSURE, | ||
| want: nil, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := allowedNfTypesForService(tt.serviceName) | ||
| if !reflect.DeepEqual(got, tt.want) { | ||
| t.Fatalf("allowedNfTypesForService(%q) = %v, want %v", tt.serviceName, got, tt.want) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
This test file uses package context, while the other tests under internal/context use the external test package pattern (package context_test). Consider switching to package context_test for consistency and to avoid tests depending on unexported internals; you can validate behavior by calling SetupNFProfile and asserting the resulting NFServices[*].AllowedNfTypes for nsmf-pdusession.
| package context | |
| import ( | |
| "reflect" | |
| "testing" | |
| "github.com/free5gc/openapi/models" | |
| ) | |
| func TestAllowedNfTypesForService(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| serviceName models.ServiceName | |
| want []models.NrfNfManagementNfType | |
| }{ | |
| { | |
| name: "nsmf-pdusession allows AMF and SMF", | |
| serviceName: models.ServiceName_NSMF_PDUSESSION, | |
| want: []models.NrfNfManagementNfType{ | |
| models.NrfNfManagementNfType_AMF, | |
| models.NrfNfManagementNfType_SMF, | |
| }, | |
| }, | |
| { | |
| name: "other services remain unrestricted", | |
| serviceName: models.ServiceName_NSMF_EVENT_EXPOSURE, | |
| want: nil, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got := allowedNfTypesForService(tt.serviceName) | |
| if !reflect.DeepEqual(got, tt.want) { | |
| t.Fatalf("allowedNfTypesForService(%q) = %v, want %v", tt.serviceName, got, tt.want) | |
| } | |
| }) | |
| } | |
| package context_test | |
| import ( | |
| "reflect" | |
| "testing" | |
| context "github.com/free5gc/smf/internal/context" | |
| "github.com/free5gc/openapi/models" | |
| ) | |
| func TestAllowedNfTypesForService(t *testing.T) { | |
| smfContext := context.GetSelf() | |
| smfContext.NfProfile = models.NrfNfManagementNfProfile{} | |
| smfContext.SetupNFProfile(models.UriScheme_HTTPS) | |
| var pdusessionAllowed []models.NrfNfManagementNfType | |
| var eventExposureAllowed []models.NrfNfManagementNfType | |
| var foundPduSession bool | |
| var foundEventExposure bool | |
| for _, svc := range smfContext.NfProfile.NfServices { | |
| switch svc.ServiceName { | |
| case models.ServiceName_NSMF_PDUSESSION: | |
| pdusessionAllowed = svc.AllowedNfTypes | |
| foundPduSession = true | |
| case models.ServiceName_NSMF_EVENT_EXPOSURE: | |
| eventExposureAllowed = svc.AllowedNfTypes | |
| foundEventExposure = true | |
| } | |
| } | |
| if !foundPduSession { | |
| t.Fatalf("NF profile did not include service %q", models.ServiceName_NSMF_PDUSESSION) | |
| } | |
| if !reflect.DeepEqual(pdusessionAllowed, []models.NrfNfManagementNfType{ | |
| models.NrfNfManagementNfType_AMF, | |
| models.NrfNfManagementNfType_SMF, | |
| }) { | |
| t.Fatalf("AllowedNfTypes for %q = %v, want %v", models.ServiceName_NSMF_PDUSESSION, pdusessionAllowed, []models.NrfNfManagementNfType{ | |
| models.NrfNfManagementNfType_AMF, | |
| models.NrfNfManagementNfType_SMF, | |
| }) | |
| } | |
| if !foundEventExposure { | |
| t.Fatalf("NF profile did not include service %q", models.ServiceName_NSMF_EVENT_EXPOSURE) | |
| } | |
| if !reflect.DeepEqual(eventExposureAllowed, nil) { | |
| t.Fatalf("AllowedNfTypes for %q = %v, want nil", models.ServiceName_NSMF_EVENT_EXPOSURE, eventExposureAllowed) | |
| } |
| import ( | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| "github.com/free5gc/openapi/models" | ||
| ) |
There was a problem hiding this comment.
Tests in this repository commonly use testify/require or testify/assert rather than reflect.DeepEqual + manual t.Fatalf. Aligning with the existing assertion style will make failures more readable and keep the test suite consistent.
| if allowedNfTypes := allowedNfTypesForService(models.ServiceName(serviceName)); len(allowedNfTypes) > 0 { | ||
| nfService.AllowedNfTypes = allowedNfTypes | ||
| } | ||
|
|
||
| *c.NfProfile.NFServices = append(*c.NfProfile.NFServices, nfService) |
There was a problem hiding this comment.
The new AllowedNfTypes wiring is implemented here, but the added unit test only exercises allowedNfTypesForService directly. Add a test that covers SetupNFProfile end-to-end (i.e., that NFServices includes nsmf-pdusession with AllowedNfTypes set to AMF+SMF) so a future refactor doesn’t accidentally drop this assignment.
Co-authored-by: Copilot <copilot@github.com>
fix issue #852
This pull request enhances the logic for setting allowed NF types for each service in the NF profile and adds targeted unit tests to ensure correct behavior. The main improvement is that the
AllowedNfTypesfield is now restricting thensmf-pdusessionservice to AMF and SMF consumers.According to 3GPP TS 29.502 Section 5.2.1, the valid consumers for the Nsmf_PDUSession service are strictly limited to AMF and SMF. This commit registers the AllowedNfTypes for the nsmf-pdusession NF profile to NRF, preventing unauthorized NFs (e.g., NEF) from obtaining OAuth2 access tokens and reaching the SMF business logic.