Skip to content

Feature: Add IPv6 support #34

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.

Sign up for GitHub

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

Jump to bottom
Open
adjivas wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Feature: Add IPv6 support #34

adjivas wants to merge 8 commits into from

Conversation

@adjivas
Copy link

@adjivas adjivas commented Feb 14, 2025

Hello,

This PR adds the RegisterIP/BindingIP fields, we can set them with IPv4 or IPv6 values
For not introduce any breaking changes, the RegisterIPv4/BindingIPv4 are the fall back of the RegisterIP/BindingIP fields.
It's adds a validation rule to avoid missing or duplicate field.

I test this PR with this fix of the Free5gs's Nrf to have this valid results:
image
image

@adjivas adjivas changed the title IPv6 support Feature: Add IPv6 support Feb 19, 2025
@andy89923 andy89923 self-requested a review February 20, 2025 07:02
andy89923
andy89923 reviewed Feb 20, 2025
View reviewed changes
Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

Sorry, I am not digging into the implementation yet.
Here are some quick suggestions for this PR!

Welcome to discuss, thanks!

andy89923
andy89923 reviewed Feb 20, 2025
View reviewed changes
@adjivas adjivas marked this pull request as draft February 27, 2025 13:24
@adjivas
Copy link
Author

adjivas commented Feb 27, 2025

Some refactors will be added before I unset the Draft state of this pull request

@andy89923
Copy link
Contributor

andy89923 commented Feb 27, 2025

Hi @adjivas ,

Thanks for the contribution.
We are going to release R17 very soon which is base on 'next' branch.

I suggest to change the base branch to 'next' to see the compatible.

@adjivas adjivas changed the base branch from main to next February 27, 2025 13:56
@adjivas
Copy link
Author

adjivas commented Feb 27, 2025 •
edited
Loading

Hi @andy89923 ,

Thank's for this new suggestion, so I will work with the 'next' branch

I open this PR for the IPv6 support free5gc/udm#48
It's include some helpful refactors, some refactors idea I will add to the next commits of this PR

@andy89923 andy89923 mentioned this pull request Feb 27, 2025
Open
@adjivas adjivas marked this pull request as ready for review March 3, 2025 16:01
@andy89923
Copy link
Contributor

andy89923 commented Mar 3, 2025

Please rebase to the latest release, thanks!

@adjivas adjivas changed the base branch from next to main March 3, 2025 18:01
@adjivas
Copy link
Author

adjivas commented Mar 3, 2025

Hello @andy89923
I rebased this branch into main

andy89923
andy89923 reviewed Mar 4, 2025
View reviewed changes
Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the PRs!

We will test all the PRs related together when they are ready.

@adjivas adjivas mentioned this pull request Mar 6, 2025
Closed
@adjivas
Copy link
Author

adjivas commented Mar 24, 2025

Hello @andy89923
This PR is mature for review

@andy89923
Copy link
Contributor

andy89923 commented Mar 24, 2025

Hi @adjivas ,

Thanks for the PR!
However, can you rebase this PR?

Your 'main' is 24 commits behind main branch in free5gc.

@adjivas
Copy link
Author

adjivas commented Mar 24, 2025

@andy89923 sure it's done!

andy89923
andy89923 reviewed Mar 24, 2025
View reviewed changes
Copy link
Contributor

@andy89923 andy89923 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM!

@Alonza0314 Please help test these PRs with IPv6 support.

pkg/factory/config.go
Comment on lines +107 to +110
RegisterIPv4 string `yaml:"registerIPv4,omitempty" valid:"host,optional"` // IP that is registered at NRF.
RegisterIP string `yaml:"registerIP,omitempty" valid:"host,optional"` // IP that is registered at NRF.
BindingIPv4 string `yaml:"bindingIPv4,omitempty" valid:"host,optional"` // IP used to run the server in the node.
BindingIP string `yaml:"bindingIP,omitempty" valid:"host,optional"` // IP used to run the server in the node.
Copy link
Contributor

@andy89923 andy89923 Mar 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ianchen0119 ,

Do you think we need RegisterIPv4 and RegisterIP simultaneously?

Keep RegisterIPv4 to maintain various places for old configuration.

Copy link
Collaborator

@tim-ywliu tim-ywliu Sep 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove RegisterIPv4 and keep RegisterIP only

Copy link
Author

@adjivas adjivas Sep 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tim-ywliu Hello,
Some config files like free5gc/ausfcfg.yaml use the field RegisterIPv4
I kept it in the SBI for compatibility, but I removed it from the
context

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andy89923 andy89923 andy89923 left review comments

@tim-ywliu tim-ywliu tim-ywliu left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@adjivas @andy89923 @tim-ywliu