search

omec-project / smf

8 stars 31 forks source link

chore: replace omec-project/pfcp with wmnsk/go-pfcp (work in progress) #279

Closed gruyaume closed 1 month ago

gruyaume commented 2 months ago

Description

:construction: This effort is still a work in progress.

Replace the omec PFCP package with go-pfcp allowing for the deprecation of the following projects:

Doing so, we also move all pfcp specific code to the pfcp/ directory. In other words, you won't see import of go-pfcp anywhere else than in /pfcp . Imports of omec-project/pfcp used to be present all over the code base, and this is why replacing the package requires so large of a code change.

Rationale

Ease of life for developers

We are currently using two different projects for PFCP communication between the SMF and the UPF making it hard to understand and contribute changes involving PFCP communication. The UPF uses wmnsk/go-pfcp and the SMF uses omec-project/pfcp, both with the same purpose. By depending on the same project for PFCP communication we make it simpler to execute code changes.

Security and reliability

go-pfcp is well maintained, tracks changes to 3GPP releases, has 53 dependents (making it more likely to receive bug fixes and security patches), and has only 1 dependency (go-cmp).

Maintainability

Archiving omec-project/pfcp repository would reduce by 2 the total number of repositories maintained for Aether, making the project easier to support with a limited amount of developers.

We did not refactor the upf-adapter sub-project

At Canonical, we do not use upf-adapter and the suite of tests there is too small to be confident that I did not break anything, therefore I did not have the confidence to touch this part of the project. I would encourage someone with familiarity with this component take care of the change as a follow-up change.

Better unit test coverage

The SMF lacked proper unit test coverage and through this change I added a bunch, moving the coverage from 12.5% to 17%.

To do prior to making this PR live

After this PR is merged

  1. Make the same change in the upf-adapter sub-project
  2. Deprecate the two following projects:

Follow up quality improvements

Working on this change, I identified changes that would improve the quality of the projects. The PR was large enough and I did not want to bundle those with the rest of the refactoring.

NodeID should be a string

Right now NodeID is a struct that gets manipulated and converted a lot for no good reason. Keeping this as a string that can represent an IPv4, IPv6 or FQDN address would simplify the code at a couple of places.

Reference:

thakurajayL commented 2 months ago

Thank you for this work. Please see if you can make sure changes are used in upf-adapter as well. That is one of the piece we want to make sure remains usable. Thanks.

gruyaume commented 2 months ago

Thank you for this work. Please see if you can make sure changes are used in upf-adapter as well. That is one of the piece we want to make sure remains usable. Thanks.

@thakurajayL As I mentioned in the PR description, we don't use this upf-adapter component, and the tests here are quite minimal, therefore I have no way of validating that my changes here are not breaking anything.

gab-arrobo commented 1 month ago

@gruyaume @thakurajayL, I was browsing the go-pfcp repo and it says that it's structs/code is based on release 16 (v16.7.0). What would be our plan when we want to move to a newer release (e.g., 17 or 18) for N4 interface?

gruyaume commented 1 month ago

@gruyaume @thakurajayL, I was browsing the go-pfcp repo and it says that it's structs/code is based on release 16 (v16.7.0). What would be our plan when we want to move to a newer release (e.g., 17 or 18) for N4 interface?

If and when there are changes to IE's that we need in our pfcp communication, we should propose changes in the upstream project. We would already need to do this as go-pfcp is used in the UPF.

Specifically for release 17 & 18, there is a draft PR in the go-pfcp repo. It seems somewhat out of date, but it should be pretty straightforward to re-work it and make it a live PR.