Skip to content

NM-295: publish TCP uplink flags and peer tcp_proxy_endpoint - #4106

Open
abhishek9686 wants to merge 449 commits into
developfrom
NM-295
Open

NM-295: publish TCP uplink flags and peer tcp_proxy_endpoint#4106
abhishek9686 wants to merge 449 commits into
developfrom
NM-295

Conversation

@abhishek9686

Copy link
Copy Markdown
Member

Add gateway tcp_proxy_enabled/listen port and client use_tcp_uplink, and populate PeerIDs on host-level pulls so clients receive dial targets.

Describe your changes

Provide Issue ticket number if applicable/not in title

Provide testing steps

Checklist before requesting a review

  • My changes affect only 10 files or less.
  • I have performed a self-review of my code and tested it.
  • If it is a new feature, I have added thorough tests, my code is <= 1450 lines.
  • If it is a bugfix, my code is <= 200 lines.
  • My functions are <= 80 lines.
  • I have had my code reviewed by a peer.
  • My unit tests pass locally.
  • Netmaker is awesome.

…n from ValidateUser (already validated in UpdateUser);
@tenki-reviewer

tenki-reviewer Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Review complete. 2 potential issues to review.

Files Reviewed: 10
Findings: 2

By Severity:

  • 🟡 Medium: 2

This PR introduces TCP proxy support for gateways but contains a data-loss bug where Node.Fill overwrites TCP proxy fields, plus silent parameter ignoring, stale state propagation issues, and an API model gap that omits TCP proxy config from list endpoints.

Files Reviewed (10 files)
controllers/gateway.go
logic/nodes.go
logic/peers.go
models/api_node.go
models/gateway.go
models/metrics.go
models/node.go
orchestrator/node.go
orchestrator/options.go
schema/nodes.go

@tenki-reviewer tenki-reviewer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review complete. 2 potential issues to review.


Summary

This PR adds TCP proxy (tcp_proxy) support to the gateway subsystem, allowing gateways to relay TCP traffic. The changes span 10 files including the gateway controller, node logic, peer configuration, API models, and schema.

Critical Issue

  • models/node.go:191Node.Fill overwrites all fields from a server-side nodeGet call, including the newly added TcpProxyEnabled and TcpProxyListenPort. Since the server may not return these fields, this causes silent data loss on the client side every time Fill is called.

Medium Issues

  • controllers/gateway.go:472 — The use_tcp_uplink query parameter is silently ignored in the auto-assign gateway path; the code sets UseTcpUplink but never reads it back when selecting a gateway.
  • controllers/gateway.go:401 — Disabling TCP proxy on a gateway does not clear UseTcpUplink on its relayed clients, leaving them in a stale state attempting to connect via TCP to a gateway that no longer proxies.
  • models/node.go:281 — The NodeWithHost API model (used by list endpoints) lacks the new TCP proxy fields (TcpProxyEnabled, TcpProxyListenPort, UseTcpUplink), so API consumers cannot see TCP proxy configuration.
  • logic/peers.go:838tcpProxyEndpointForPeer always prefers IPv4 over IPv6 when the client has a dual-stack setup, potentially forcing IPv4 even when the client is configured for IPv6.

Comment thread controllers/gateway.go
Comment on lines +401 to +414
node.TcpProxyEnabled = req.Enabled
if req.Enabled {
node.TcpProxyListenPort = req.ListenPort
if node.TcpProxyListenPort <= 0 {
node.TcpProxyListenPort = schema.DefaultTcpProxyListenPort
}
} else {
node.TcpProxyListenPort = 0
}

if err := node.SetTcpProxy(r.Context()); err != nil {
logic.ReturnErrorResponse(w, r, logic.FormatError(err, logic.Internal))
return
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Disabling TCP proxy on a gateway leaves stale UseTcpUplink=true on its relayed clients (bug)

When TCP proxy is disabled on a gateway via PUT /api/nodes/{network}/{nodeid}/gateway/tcp_proxy with {"enabled":false}, the handler (controllers/gateway.go:374-425) only updates the gateway node's own TcpProxyEnabled and TcpProxyListenPort via node.SetTcpProxy (schema/nodes.go:534-542). It does not reset UseTcpUplink to false on any client nodes that were previously assigned to this gateway with use_tcp_uplink=true. This leaves the database in an inconsistent state where clients indicate they should use TCP uplink but the gateway does not accept TCP connections. While the wire protocol is protected (tcpProxyEndpointForPeer checks the gateway's TcpProxyEnabled independently), the stale UseTcpUplink=true setting persists in the database and API responses until the client is explicitly unassigned.

💡 Suggestion: In the updateGatewayTcpProxy handler, when req.Enabled is false, also reset use_tcp_uplink to false on all nodes whose relayed_by_node_id matches this gateway's ID.

📋 Prompt for AI Agents

In controllers/gateway.go, function updateGatewayTcpProxy (around line 401-414), inside the else branch (when req.Enabled is false, line 407-409), just before or after setting node.TcpProxyListenPort = 0, add a DB update to clear use_tcp_uplink on relayed clients:

if err := db.FromContext(r.Context()).Model(&schema.Node{}).Where("relayed_by_node_id = ? AND use_tcp_uplink = ?", node.ID, true).Update("use_tcp_uplink", false).Error; err != nil {
    slog.Error("failed to clear use_tcp_uplink on relayed clients after disabling TCP proxy", "gateway", node.ID, "error", err)
}

This ensures that when TCP proxy support is removed from a gateway, all clients that opted into TCP uplink are marked accordingly, keeping the database consistent.

Comment thread logic/peers.go Outdated
Comment on lines +838 to +846
if peerHost.EndpointIP != nil && !peerHost.EndpointIP.IsUnspecified() {
ip = peerHost.EndpointIP
} else if peerHost.EndpointIPv6 != nil && !peerHost.EndpointIPv6.IsUnspecified() {
ip = peerHost.EndpointIPv6
}
if ip == nil {
return ""
}
return net.JoinHostPort(ip.String(), strconv.Itoa(port))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 tcpProxyEndpointForPeer ignores client IP stack, always prefers IPv4 over IPv6 (bug)

tcpProxyEndpointForPeer() (logic/peers.go:829-847) unconditionally prefers peerHost.EndpointIP (IPv4) over peerHost.EndpointIPv6 (IPv6) at lines 838-842. This differs from the regular WireGuard endpoint selection (lines 473-488) which matches the requesting host's IP stack. Result: IPv6-only clients receive an unreachable IPv4 TCP proxy endpoint when the gateway is dual-stack, breaking TCP uplink for those clients.

💡 Suggestion: Accept the requesting host as a parameter and use the same stack-matching logic as the regular peer endpoint selection (lines 474-478): if host has IPv4, prefer peer IPv4; if host has IPv6 with no IPv4, prefer peer IPv6. Alternatively, provide both IPv4 and IPv6 TCP endpoints and let the client select the reachable one.

📋 Prompt for AI Agents

In logic/peers.go, modify tcpProxyEndpointForPeer to accept a *schema.Host parameter (the requesting client host) and match the IP stack selection logic used for regular WireGuard endpoints at lines 473-488:

func tcpProxyEndpointForPeer(peer *models.Node, peerHost *schema.Host, clientHost *schema.Host) string {
    if peer == nil || peerHost == nil || !peer.IsGw || !peer.TcpProxyEnabled {
        return ""
    }
    port := peer.TcpProxyListenPort
    if port <= 0 {
        port = schema.DefaultTcpProxyListenPort
    }
    var ip net.IP
    if clientHost.EndpointIP != nil && peerHost.EndpointIP != nil {
        ip = peerHost.EndpointIP
    } else if clientHost.EndpointIPv6 != nil && peerHost.EndpointIPv6 != nil {
        ip = peerHost.EndpointIPv6
    }
    if ip == nil {
        return ""
    }
    return net.JoinHostPort(ip.String(), strconv.Itoa(port))
}

Then update the call site at line 549 to pass host as the third argument.

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