-
Notifications
You must be signed in to change notification settings - Fork 1
[WIP] - Add support for connection gating and address factory #1
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
Changes from 14 commits
5a08486
5e20c55
a724bd2
6c63858
bd8847a
eeb8273
f600d76
f9aae61
f130dc9
6817deb
2cbe26f
b23fb5b
37dd56f
4448a41
9d9d80c
9f4ebd6
40c6a6b
9f0e0ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| .history/ | ||
| .idea/ | ||
| p2p_poc | ||
| peer_cache.json |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,8 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "fmt" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "io" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "log" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "net/http" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "os" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "sync" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "time" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -21,6 +23,7 @@ import ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/libp2p/go-libp2p/core/peer" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/libp2p/go-libp2p/p2p/discovery/mdns" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| drouting "github.com/libp2p/go-libp2p/p2p/discovery/routing" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/libp2p/go-libp2p/p2p/net/conngater" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "github.com/multiformats/go-multiaddr" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -70,8 +73,8 @@ func NewClient(config Config) (P2PClient, error) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| hostOpts = append(hostOpts, libp2p.Identity(config.PrivateKey)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Configure announce addresses if provided (useful for K8s) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var announceAddrs []multiaddr.Multiaddr | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if len(config.AnnounceAddrs) > 0 { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var announceAddrs []multiaddr.Multiaddr | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for _, addrStr := range config.AnnounceAddrs { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| maddr, err := multiaddr.NewMultiaddr(addrStr) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -87,15 +90,88 @@ func NewClient(config Config) (P2PClient, error) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.Infof("Using custom announce addresses: %v", config.AnnounceAddrs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // define address factory to remove all private IPs from being broadcasted | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| addressFactory := func(addrs []multiaddr.Multiaddr) []multiaddr.Multiaddr { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| var publicAddrs []multiaddr.Multiaddr | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for _, addr := range addrs { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // if IP is not private, add it to the list | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) | |
| logger.Debugf("address is private? %v", isPrivateIP(config, addr)) |
Outdated
Copilot
AI
Oct 1, 2025
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 debug log statement should be at debug level rather than info level to avoid excessive logging in production.
| logger.Infof("address is private? %v", isPrivateIP(config, addr)) | |
| logger.Debugf("address is private? %v", isPrivateIP(config, addr)) |
Outdated
Copilot
AI
Oct 1, 2025
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.
Using context.Background() instead of the existing context (ctx) breaks the cancellation chain. This HTTP request won't be cancelled when the client shuts down, potentially causing resource leaks.
| ifconfig, err := GetPublicIP(context.Background()) | |
| ifconfig, err := GetPublicIP(ctx) |
Outdated
Copilot
AI
Oct 1, 2025
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.
The format specifier should be %d instead of %s for config.Port since it's an integer type.
| addr, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) | |
| addr, err := multiaddr.NewMultiaddr(fmt.Sprintf("/ip4/%s/tcp/%d", ifconfig, config.Port)) |
Outdated
Copilot
AI
Oct 1, 2025
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.
Incorrect CIDR notation for localhost. The loopback network should be '127.0.0.0/8', not '127.0.0.0/16'. The /16 mask would only cover 127.0.x.x instead of the full 127.x.x.x range.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network |
Outdated
Copilot
AI
Oct 1, 2025
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.
The CIDR mask for localhost should be /8 not /16. The standard localhost range is 127.0.0.0/8 which covers 127.0.0.0 to 127.255.255.255.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network |
Outdated
Copilot
AI
Oct 1, 2025
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.
The CIDR mask for localhost should be /8 not /16. The correct localhost range is 127.0.0.0/8.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network |
Outdated
Copilot
AI
Oct 1, 2025
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.
The CIDR 127.0.0.0/16 is incorrect for localhost addresses. It should be 127.0.0.0/8 to properly cover the entire 127.x.x.x range used for loopback addresses.
| "127.0.0.0/16", // Local network | |
| "127.0.0.0/8", // Local network (loopback) |
Copilot
AI
Oct 1, 2025
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.
Function comment is incomplete. It should document the return value, parameters, and provide examples of what constitutes a private IP address.
| // Function to check if an IP address is private | |
| // isPrivateIP checks if the given multiaddr represents a private IP address. | |
| // | |
| // Parameters: | |
| // addr: multiaddr.Multiaddr - The multiaddr to check. It should contain an IPv4 or IPv6 address. | |
| // | |
| // Returns: | |
| // bool - Returns true if the IP address is private (e.g., RFC1918 for IPv4, unique local or link-local for IPv6, or loopback addresses), false otherwise. | |
| // | |
| // Examples of private IP addresses: | |
| // IPv4: "10.0.0.1", "172.16.5.4", "192.168.1.100", "127.0.0.1" | |
| // IPv6: "::1" (loopback), "fc00::1" (unique local), "fe80::1" (link-local) |
Outdated
Copilot
AI
Oct 1, 2025
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.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
Outdated
Copilot
AI
Oct 1, 2025
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.
These debug log statements should be at debug level rather than info level, or removed entirely for production code as they will create excessive logging.
Copilot
AI
Oct 1, 2025
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.
The condition ip.To4() == nil will incorrectly return false for all IPv6 addresses, even though the function handles IPv6 private ranges. This should check if the IP is neither IPv4 nor IPv6, or handle IPv6 addresses properly.
| if ip == nil || ip.To4() == nil { | |
| if ip == nil { |
Copilot
AI
Oct 1, 2025
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 logic incorrectly rejects IPv6 addresses. The ip.To4() == nil check means IPv6 addresses will always return false, but IPv6 private ranges are defined below. Remove the ip.To4() == nil condition.
| if ip == nil || ip.To4() == nil { | |
| if ip == nil { |
Copilot
AI
Oct 1, 2025
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.
IPv6 addresses will be incorrectly classified as non-private. The condition ip.To4() == nil returns false for all IPv6 addresses, causing the function to return false even for private IPv6 ranges that are defined later in the function.
| if ip == nil || ip.To4() == nil { | |
| if ip == nil { |
Copilot
AI
Oct 1, 2025
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 function only handles IPv4 addresses due to the ip.To4() check, but the extractIPFromMultiaddr function only extracts IPv4 addresses. IPv6 private addresses (like link-local fe80::/10) will not be detected as private. Consider adding IPv6 support or documenting this limitation.
Outdated
Copilot
AI
Oct 1, 2025
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.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
Copilot
AI
Oct 1, 2025
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.
The private IP ranges are duplicated between the connection gater logic (lines 139-146) and the isPrivateIP function (lines 728-737). This duplication could lead to inconsistencies if one is updated without the other.
Outdated
Copilot
AI
Oct 1, 2025
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.
Multiple debug log statements in address filtering logic that will be called frequently during network operations. Consider using Debug level logging or removing verbose logging to avoid performance impact in production.
Copilot
AI
Oct 1, 2025
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.
Function comment is incomplete. It should document the return values, error conditions, and provide examples of expected input/output formats.
| // Function to extract IP information from a Multiaddr (supports IPv4 and IPv6) | |
| // extractIPFromMultiaddr extracts the IP address (IPv4 or IPv6) from a multiaddr.Multiaddr. | |
| // | |
| // Returns: | |
| // - string: The extracted IP address as a string if found. | |
| // - error: An error if the IP address cannot be extracted or the multiaddr does not contain an IP4/IP6 protocol. | |
| // | |
| // Error conditions: | |
| // - If the multiaddr does not contain an IP4 or IP6 protocol, an error is returned. | |
| // - If the protocol value cannot be extracted, an error is returned. | |
| // | |
| // Example: | |
| // addr, _ := multiaddr.NewMultiaddr("/ip4/127.0.0.1/tcp/4001") | |
| // ip, err := extractIPFromMultiaddr(addr) | |
| // // ip == "127.0.0.1", err == nil | |
| // | |
| // addr, _ := multiaddr.NewMultiaddr("/ip6/::1/tcp/4001") | |
| // ip, err := extractIPFromMultiaddr(addr) | |
| // // ip == "::1", err == nil | |
| // | |
| // addr, _ := multiaddr.NewMultiaddr("/tcp/4001") | |
| // ip, err := extractIPFromMultiaddr(addr) | |
| // // ip == "", err != nil |
Outdated
Copilot
AI
Oct 1, 2025
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.
The response body should be closed in a defer statement after checking for errors, not in the return statement. The current approach may not close the body if there's an error reading it.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
Outdated
Copilot
AI
Oct 1, 2025
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.
The response body should be closed before returning, not as part of the return statement. If resp.Body.Close() returns an error, it will be ignored. The body should be closed in a defer statement or before the return.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
Outdated
Copilot
AI
Oct 1, 2025
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.
The response body should be closed before returning, not as part of the return statement. If resp.Body.Close() returns an error, it will be ignored. The body should be closed in a defer statement or the close error should be handled properly.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer func() { | |
| if cerr := resp.Body.Close(); cerr != nil { | |
| log.Printf("error closing response body: %v", cerr) | |
| } | |
| }() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
Outdated
Copilot
AI
Oct 1, 2025
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.
The return statement combines the response body with the Close() error, which will always return an error type as the second value instead of a string. The body should be closed in a defer statement and only the string should be returned.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), nil |
Outdated
Copilot
AI
Oct 1, 2025
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.
Returning the result of resp.Body.Close() as the error. This should be handled separately - close the body and return nil as the error, or store the close error in a variable if needed.
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), resp.Body.Close() | |
| defer resp.Body.Close() | |
| body, err := io.ReadAll(resp.Body) | |
| if err != nil { | |
| return "", err | |
| } | |
| return string(body), 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.
The
announceAddrsvariable is declared but the existing address factory logic usingconfig.AnnounceAddrsis removed without preserving its functionality. This will break the original announce address feature when both custom announce addresses and the new address factory are used together.