Skip to content

[client] Revert migrate deprecated grpc client code #4805

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

Merged
lixmal merged 1 commit into from
Nov 18, 2025
+7 −35
Merged

[client] Revert migrate deprecated grpc client code #4805

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 7 additions & 35 deletions client/grpc/dialer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,20 @@ import (
"context"
"crypto/tls"
"crypto/x509"
"errors"
"fmt"
"runtime"
"time"

"github.com/cenkalti/backoff/v4"
log "github.com/sirupsen/logrus"
"google.golang.org/grpc"
"google.golang.org/grpc/connectivity"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/keepalive"

"github.com/netbirdio/netbird/util/embeddedroots"
)

// ErrConnectionShutdown indicates that the connection entered shutdown state before becoming ready
var ErrConnectionShutdown = errors.New("connection shutdown before ready")

// Backoff returns a backoff configuration for gRPC calls
func Backoff(ctx context.Context) backoff.BackOff {
b := backoff.NewExponentialBackOff()
Expand All @@ -31,26 +26,6 @@ func Backoff(ctx context.Context) backoff.BackOff {
return backoff.WithContext(b, ctx)
}

// waitForConnectionReady blocks until the connection becomes ready or fails.
// Returns an error if the connection times out, is cancelled, or enters shutdown state.
func waitForConnectionReady(ctx context.Context, conn *grpc.ClientConn) error {
conn.Connect()

state := conn.GetState()
for state != connectivity.Ready && state != connectivity.Shutdown {
if !conn.WaitForStateChange(ctx, state) {
return fmt.Errorf("wait state change from %s: %w", state, ctx.Err())
}
state = conn.GetState()
}

if state == connectivity.Shutdown {
return ErrConnectionShutdown
}

return nil
}

// CreateConnection creates a gRPC client connection with the appropriate transport options.
// The component parameter specifies the WebSocket proxy component path (e.g., "/management", "/signal").
func CreateConnection(ctx context.Context, addr string, tlsEnabled bool, component string) (*grpc.ClientConn, error) {
Expand All @@ -68,25 +43,22 @@ func CreateConnection(ctx context.Context, addr string, tlsEnabled bool, compone
}))
}

conn, err := grpc.NewClient(
connCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()

conn, err := grpc.DialContext(
connCtx,
addr,
transportOption,
WithCustomDialer(tlsEnabled, component),
grpc.WithBlock(),
grpc.WithKeepaliveParams(keepalive.ClientParameters{
Time: 30 * time.Second,
Timeout: 10 * time.Second,
}),
)
if err != nil {
return nil, fmt.Errorf("new client: %w", err)
}

ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()

if err := waitForConnectionReady(ctx, conn); err != nil {
_ = conn.Close()
return nil, err
return nil, fmt.Errorf("dial context: %w", err)
}
Comment on lines +46 to 62
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Nov 18, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify: This reverts to deprecated gRPC APIs that should not be used with gRPC v1.73.0.

The code now uses grpc.DialContext and grpc.WithBlock(), both of which were deprecated in gRPC v1.70.0. Since the project uses gRPC v1.73.0, this revert introduces deprecated API usage. The modern approach is grpc.NewClient with explicit connection state management if needed.

Please verify the deprecation status and recommended migration path for these gRPC APIs in version 1.73.0:

Additionally, check if there was a specific bug or issue with the original grpc.NewClient implementation that necessitated this revert:

🏁 Script executed:

#!/bin/bash
# Search for related issues, bug reports, or comments about grpc.NewClient problems
rg -n -C5 'grpc\.NewClient|waitForConnectionReady|ErrConnectionShutdown' --type go

# Check git history for the original migration commit and this revert
git log --oneline --grep="grpc" --all | head -20

# Look for any TODO/FIXME/BUG comments related to gRPC connection handling
rg -n 'TODO|FIXME|BUG|HACK' --type go -g '*grpc*' -C3

Length of output: 3387

🌐 Web query:

grpc.DialContext grpc.WithBlock deprecated gRPC 1.73.0 alternative

💡 Result:

Short answer: stop using grpc.DialContext(..., grpc.WithBlock()) — use grpc.NewClient (introduced v1.63) and, if you must block until the channel is ready, call the ClientConn's Connect + WaitForStateChange / check connectivity state yourself. grpc.NewClient does no I/O by default; Dial/DialContext and the WithBlock/WithTimeout DialOptions are deprecated. [1][2][3]

Example sketch:

  • cc := grpc.NewClient(target, opts...) // no I/O
  • if err := cc.Connect(ctx); err != nil { ... } // triggers connection attempt
  • // optionally wait for Ready via cc.WaitForStateChange / cc.GetState loop

References:

  1. grpc-go package docs (NewClient, DialContext deprecation). [1]
  2. grpc-go anti-patterns / guidance recommending NewClient. [2]
  3. clientconn implementation notes describing DialContext behavior (WithBlock behaviour). [3]

This revert to deprecated gRPC APIs contradicts gRPC v1.73.0 best practices and creates codebase inconsistency.

The grpc.DialContext and grpc.WithBlock() APIs are deprecated in gRPC v1.70.0 and later in favor of grpc.NewClient. Since the project uses gRPC v1.73.0, this change reintroduces deprecated APIs that should not be used.

Additionally, other files in the codebase (flow/client/client.go:68, client/ui/event/event.go:167) continue using the modern grpc.NewClient approach, creating inconsistency.

Required changes:

  • Revert to grpc.NewClient (modern API)
  • If blocking connection state is required, use ClientConn.Connect(ctx) instead of grpc.WithBlock()


return conn, nil
Expand Down
Loading