Skip to content

Conversation

@adjivas
Copy link

@adjivas adjivas commented Feb 27, 2025

Hello,

This PR adds the RegisterIP/BindingIP new fields for the IPv6 support and some refactors of the Sbi
For not break the previous configurations, the RegisterIPv4/BindingIPv4 are set into RegisterIP/BindingIP

The Sbi is now optional, because we can set it from the default udmSbi configuration
A lookup resolution is added to both RegisterIP/BindingIP

image

@andy89923
Copy link
Contributor

Hi @adjivas ,

As with the free5gc/ausf#34, we are going to release the latest version on the 'next' branch.
So, this PR should be based on 'next' or wait for the release be made,
Thanks!

@adjivas adjivas changed the base branch from main to next February 27, 2025 14:32
@adjivas adjivas force-pushed the main branch 3 times, most recently from 4ee9258 to 31b0c06 Compare February 27, 2025 16:27
@andy89923
Copy link
Contributor

Please rebase to the latest release, thanks!

@andy89923
Copy link
Contributor

andy89923 commented Mar 24, 2025

Hi @adjivas ,

The commit history should not be like this.
I would suggest that you create a new branch that only picks related commits and creates a new PR.

@adjivas adjivas changed the base branch from next to main March 24, 2025 10:54
@adjivas
Copy link
Author

adjivas commented Mar 24, 2025

Hello @andy89923
I fix that
Now the PR adds the IPv6 commit on the top of the history.
It's target the main branch.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds IPv6 support by introducing generic RegisterIP/BindingIP fields alongside the deprecated IPv4-specific ones, updates SB I defaulting and resolution logic, and refactors URI generation and context initialization to use netip and DNS lookups.

  • Add RegisterIP/BindingIP in Sbi and migrate deprecated IPv4 fields.
  • Update getSbiUri, SearchNFServiceUri, and server binding to use netip.AddrPort.
  • Replace string-based IPs in context with netip.Addr, implement resolveIP and GetIpEndPoint, and extend tests for IPv6.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
pkg/factory/config.go Renamed default IPv4 constants, added RegisterIP/BindingIP, refactored Sbi.validate defaults
internal/util/search_nf_service.go Enhanced IPv6 lookups, updated getSbiUri to parse addresses with netip
internal/util/init_context.go Removed legacy init logic in favor of new context-based initialization
internal/sbi/server.go Updated server binding address construction to use netip.AddrPort
internal/sbi/consumer/nrf_service.go Populating NF profile IPv4/IPv6 addresses based on new RegisterIP
internal/context/context_test.go Added tests covering IPv4, IPv6, deprecated fields, defaults, and ENV resolution
internal/context/context.go Switched to netip.Addr for IPs, implemented resolveIP, GetIpEndPoint, and replaced URI methods
Comments suppressed due to low confidence (1)

internal/context/context.go:475

  • Function GetIpEndPoint handles both IPv4 and IPv6 cases but lacks direct unit tests. Consider adding tests to verify correct endpoint output for each IP family.
func GetIpEndPoint(context *UDMContext) []models.IpEndPoint {

func getSbiUri(scheme models.UriScheme, ipAddress string, port int32) (uri string) {
addr, err := netip.ParseAddr(ipAddress)
if err != nil {
logger.InitLog.Errorf("Parse RegisterIP hostname %s failed: %+v", ipAddress, err)
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

After logging the parse error, the function continues using a zero-value Addr, which may produce an incorrect URI. Consider returning early or falling back to the raw ipAddress to avoid silently generating invalid addresses.

Suggested change
logger.InitLog.Errorf("Parse RegisterIP hostname %s failed: %+v", ipAddress, err)
logger.InitLog.Errorf("Parse RegisterIP hostname %s failed: %+v", ipAddress, err)
return "" // Return early if parsing fails

Copilot uses AI. Check for mistakes.
Comment on lines +462 to +465
}
resolvedIP := resolvedIPs[0].Unmap()
if resolvedIP := resolvedIP.String(); resolvedIP != ip {
logger.UtilLog.Infof("Lookup revolved %s into %s", ip, resolvedIP)
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Indexing into resolvedIPs without checking its length may panic if the lookup returns no addresses. Add a length check on resolvedIPs before accessing element [0], and handle empty results.

Suggested change
}
resolvedIP := resolvedIPs[0].Unmap()
if resolvedIP := resolvedIP.String(); resolvedIP != ip {
logger.UtilLog.Infof("Lookup revolved %s into %s", ip, resolvedIP)
return netip.Addr{} // Return zero-value address on error
}
if len(resolvedIPs) == 0 {
logger.InitLog.Errorf("No addresses resolved for %s", ip)
return netip.Addr{} // Return zero-value address if no IPs are resolved
}
resolvedIP := resolvedIPs[0].Unmap()
if resolvedIP := resolvedIP.String(); resolvedIP != ip {
logger.UtilLog.Infof("Lookup resolved %s into %s", ip, resolvedIP)

Copilot uses AI. Check for mistakes.
}
resolvedIP := resolvedIPs[0].Unmap()
if resolvedIP := resolvedIP.String(); resolvedIP != ip {
logger.UtilLog.Infof("Lookup revolved %s into %s", ip, resolvedIP)
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Typo in log message: "Lookup revolved" should be "Lookup resolved" to accurately describe the operation.

Suggested change
logger.UtilLog.Infof("Lookup revolved %s into %s", ip, resolvedIP)
logger.UtilLog.Infof("Lookup resolved %s into %s", ip, resolvedIP)

Copilot uses AI. Check for mistakes.
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.

2 participants