Skip to content

Commit

Permalink
Remove usage of deprecated grpc-go methods (#5725)
Browse files Browse the repository at this point in the history 
* Remove usage of deprecated grpc-go methods

Replace usage of deprecated `grpc.Dial()`/`grpc.DialContext()` methods
with `grpc.NewClient()`. Also remove usage of `grpc.WithBlock()`,
`grpc.FailOnNonTempDialError()`, and `grpc.WithReturnConnectionError()`
options.

The combination of these changes results in a couple behavioral changes
when setting up gRPC clients:

1. gRPC will no longer dial when creating the client. Instead, it will
wait until the client is used for the first time with an RPC invocation.

2. gRPC uses the DNS resolver by default when building the
`*grpc.ClientConn` using `grpc.NewClient()`, whereas previously it used
to resolve addresses the `passthrough` resolver by default. The result
of this change in behavior is that for any invocations of
`grpc.Dial()`/`grpc.DialContext()` that did not specify a URI scheme,
gRPC now implicitly tries to resolve the address passed to
`grpc.NewClient()` using DNS. This breaks some assumptions in the code.
The workaround to preserve the previous address resolution behavior is
to prepend addresses with no scheme defined with the resolver URI scheme
`passthrough:`.

Also refactored some test-related code in `cmd/spire-server/cli/common`
into a new `test/clitest` package, since it is not intended
for use in production code.

Fixes #5152.

Signed-off-by: Ryan Turner <[email protected]>
  • Loading branch information
@rturner3
rturner3 authored Jan 20, 2025
1 parent 05c641a commit 5521b42
Show file tree
Hide file tree
Showing 72 changed files with 545 additions and 474 deletions.
13 changes: 7 additions & 6 deletions cmd/spire-agent/cli/api/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ import (
"github.com/mitchellh/cli"
"github.com/spiffe/go-spiffe/v2/proto/spiffe/workload"
"github.com/spiffe/go-spiffe/v2/spiffeid"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
commoncli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/pkg/common/x509util"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/fakes/fakeworkloadapi"
"github.com/spiffe/spire/test/spiretest"
"github.com/spiffe/spire/test/testca"
Expand Down Expand Up @@ -416,9 +416,10 @@ func TestValidateJWTCommand(t *testing.T) {
Claims: &structpb.Struct{
Fields: map[string]*structpb.Value{
"aud": {
Kind: &structpb.Value_ListValue{ListValue: &structpb.ListValue{
Values: []*structpb.Value{{Kind: &structpb.Value_StringValue{StringValue: "foo"}}},
},
Kind: &structpb.Value_ListValue{
ListValue: &structpb.ListValue{
Values: []*structpb.Value{{Kind: &structpb.Value_StringValue{StringValue: "foo"}}},
},
},
},
},
Expand Down Expand Up @@ -504,7 +505,7 @@ func setupTest(t *testing.T, newCmd func(env *commoncli.Env, clientMaker workloa
}, newWorkloadClient)

test := &apiTest{
addr: common.GetAddr(addr),
addr: clitest.GetAddr(addr),
stdin: stdin,
stdout: stdout,
stderr: stderr,
Expand Down Expand Up @@ -538,7 +539,7 @@ func (s *apiTest) afterTest(t *testing.T) {
}

func (s *apiTest) args(extra ...string) []string {
return append([]string{common.AddrArg, s.addr}, extra...)
return append([]string{clitest.AddrArg, s.addr}, extra...)
}

func assertOutputBasedOnFormat(t *testing.T, format, stdoutString, expectedStdoutJSON string, expectedStdoutPretty ...string) {
Expand Down
2 changes: 1 addition & 1 deletion cmd/spire-agent/cli/api/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func newWorkloadClient(ctx context.Context, addr net.Addr, timeout time.Duration
if err != nil {
return nil, err
}
conn, err := util.GRPCDialContext(ctx, target)
conn, err := util.NewGRPCClient(target)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/spire-agent/cli/healthcheck/healthcheck.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (c *healthCheckCommand) run() error {
if err != nil {
return err
}
conn, err := util.GRPCDialContext(context.Background(), target)
conn, err := util.NewGRPCClient(target)
if err != nil {
return err
}
Expand Down
39 changes: 24 additions & 15 deletions cmd/spire-server/cli/agent/agent_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ import (
agentv1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/agent/v1"
"github.com/spiffe/spire-api-sdk/proto/spire/api/types"
"github.com/spiffe/spire/cmd/spire-server/cli/agent"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
commoncli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/spiretest"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
Expand Down Expand Up @@ -92,10 +92,13 @@ func TestBan(t *testing.T) {
expectStderr: "Error: a SPIFFE ID is required\n",
},
{
name: "wrong UDS path",
args: []string{common.AddrArg, common.AddrValue},
name: "wrong UDS path",
args: []string{
clitest.AddrArg, clitest.AddrValue,
"-spiffeID", "spiffe://example.org/spire/agent/agent1",
},
expectReturnCode: 1,
expectStderr: common.AddrError,
expectStderr: "Error: " + clitest.AddrError,
},
{
name: "server error",
Expand Down Expand Up @@ -152,10 +155,13 @@ func TestEvict(t *testing.T) {
expectedStderr: "Error: a SPIFFE ID is required\n",
},
{
name: "wrong UDS path",
args: []string{common.AddrArg, common.AddrValue},
name: "wrong UDS path",
args: []string{
clitest.AddrArg, clitest.AddrValue,
"-spiffeID", "spiffe://example.org/spire/agent/agent1",
},
expectedReturnCode: 1,
expectedStderr: common.AddrError,
expectedStderr: "Error: " + clitest.AddrError,
},
{
name: "server error",
Expand Down Expand Up @@ -221,9 +227,9 @@ func TestCount(t *testing.T) {
},
{
name: "wrong UDS path",
args: []string{common.AddrArg, common.AddrValue},
args: []string{clitest.AddrArg, clitest.AddrValue},
expectedReturnCode: 1,
expectedStderr: common.AddrError,
expectedStderr: "Error: " + clitest.AddrError,
},
{
name: "Count by expiresBefore: month out of range",
Expand Down Expand Up @@ -448,9 +454,9 @@ func TestList(t *testing.T) {
},
{
name: "wrong UDS path",
args: []string{common.AddrArg, common.AddrValue},
args: []string{clitest.AddrArg, clitest.AddrValue},
expectedReturnCode: 1,
expectedStderr: common.AddrError,
expectedStderr: "Error: " + clitest.AddrError,
},
{
name: "List by expiresBefore: month out of range",
Expand Down Expand Up @@ -740,10 +746,13 @@ func TestShow(t *testing.T) {
expectedStderr: "Error: rpc error: code = Internal desc = internal server error\n",
},
{
name: "wrong UDS path",
args: []string{common.AddrArg, common.AddrValue},
name: "wrong UDS path",
args: []string{
clitest.AddrArg, clitest.AddrValue,
"-spiffeID", "spiffe://example.org/spire/agent/agent1",
},
expectedReturnCode: 1,
expectedStderr: common.AddrError,
expectedStderr: "Error: " + clitest.AddrError,
},
{
name: "show selectors",
Expand Down Expand Up @@ -801,7 +810,7 @@ func setupTest(t *testing.T, newClient func(*commoncli.Env) cli.Command) *agentT
stdin: stdin,
stdout: stdout,
stderr: stderr,
args: []string{common.AddrArg, common.GetAddr(addr)},
args: []string{clitest.AddrArg, clitest.GetAddr(addr)},
server: server,
client: client,
}
Expand Down
8 changes: 3 additions & 5 deletions cmd/spire-server/cli/authoritycommon/test/authoritycommontest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,14 @@ import (

"github.com/mitchellh/cli"
localauthorityv1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/localauthority/v1"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
commoncli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/spiretest"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
)

var (
AvailableFormats = []string{"pretty", "json"}
)
var AvailableFormats = []string{"pretty", "json"}

type localAuthorityTest struct {
Stdin *bytes.Buffer
Expand Down Expand Up @@ -55,7 +53,7 @@ func SetupTest(t *testing.T, newClient func(*commoncli.Env) cli.Command) *localA
Stdin: stdin,
Stdout: stdout,
Stderr: stderr,
Args: []string{common.AddrArg, common.GetAddr(addr)},
Args: []string{clitest.AddrArg, clitest.GetAddr(addr)},
Server: server,
Client: client,
}
Expand Down
6 changes: 3 additions & 3 deletions cmd/spire-server/cli/bundle/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ import (
"github.com/mitchellh/cli"
bundlev1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/bundle/v1"
"github.com/spiffe/spire-api-sdk/proto/spire/api/types"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
common_cli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/pkg/common/pemutil"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/spiretest"
"github.com/stretchr/testify/require"
"google.golang.org/grpc"
Expand Down Expand Up @@ -203,7 +203,7 @@ func setupTest(t *testing.T, newClient func(*common_cli.Env) cli.Command) *bundl
cert1: cert1,
cert2: cert2,
key1Pkix: key1Pkix,
addr: common.GetAddr(addr),
addr: clitest.GetAddr(addr),
stdin: stdin,
stdout: stdout,
stderr: stderr,
Expand Down Expand Up @@ -241,7 +241,7 @@ func (s *bundleTest) afterTest(t *testing.T) {
}

func (s *bundleTest) args(extra ...string) []string {
return append([]string{common.AddrArg, s.addr}, extra...)
return append([]string{clitest.AddrArg, s.addr}, extra...)
}

type fakeBundleServer struct {
Expand Down
25 changes: 0 additions & 25 deletions cmd/spire-server/cli/common/common_windows.go
View file

This file was deleted.

6 changes: 3 additions & 3 deletions cmd/spire-server/cli/entry/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import (
"github.com/mitchellh/cli"
entryv1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/entry/v1"
"github.com/spiffe/spire-api-sdk/proto/spire/api/types"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
common_cli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/spiretest"
"github.com/spiffe/spire/test/util"
"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -155,7 +155,7 @@ func (e *entryTest) afterTest(t *testing.T) {
}

func (e *entryTest) args(extra ...string) []string {
return append([]string{common.AddrArg, e.addr}, extra...)
return append([]string{clitest.AddrArg, e.addr}, extra...)
}

type fakeEntryServer struct {
Expand Down Expand Up @@ -242,7 +242,7 @@ func setupTest(t *testing.T, newClient func(*common_cli.Env) cli.Command) *entry
})

test := &entryTest{
addr: common.GetAddr(addr),
addr: clitest.GetAddr(addr),
stdin: stdin,
stdout: stdout,
stderr: stderr,
Expand Down
12 changes: 6 additions & 6 deletions cmd/spire-server/cli/federation/common_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ import (
"github.com/spiffe/go-spiffe/v2/spiffeid"
trustdomainv1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/trustdomain/v1"
"github.com/spiffe/spire-api-sdk/proto/spire/api/types"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
common_cli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/pkg/common/pemutil"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/fakes/fakeserverca"
"github.com/spiffe/spire/test/spiretest"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -124,7 +124,7 @@ func (c *cmdTest) afterTest(t *testing.T) {
}

func (c *cmdTest) args(extra ...string) []string {
return append([]string{common.AddrArg, c.addr}, extra...)
return append([]string{clitest.AddrArg, c.addr}, extra...)
}

type fakeServer struct {
Expand Down Expand Up @@ -222,7 +222,7 @@ func setupTest(t *testing.T, newClient func(*common_cli.Env) cli.Command) *cmdTe
})

test := &cmdTest{
addr: common.GetAddr(addr),
addr: clitest.GetAddr(addr),
stdin: stdin,
stdout: stdout,
stderr: stderr,
Expand All @@ -241,7 +241,7 @@ func createBundle(t *testing.T, trustDomain string) (*types.Bundle, string) {
td := spiffeid.RequireTrustDomainFromString(trustDomain)
bundlePath := path.Join(t.TempDir(), "bundle.pem")
ca := fakeserverca.New(t, td, &fakeserverca.Options{})
require.NoError(t, os.WriteFile(bundlePath, pemutil.EncodeCertificates(ca.Bundle()), 0600))
require.NoError(t, os.WriteFile(bundlePath, pemutil.EncodeCertificates(ca.Bundle()), 0o600))

return &types.Bundle{
TrustDomain: td.Name(),
Expand All @@ -253,13 +253,13 @@ func createBundle(t *testing.T, trustDomain string) (*types.Bundle, string) {

func createCorruptedBundle(t *testing.T) string {
bundlePath := path.Join(t.TempDir(), "bundle.pem")
require.NoError(t, os.WriteFile(bundlePath, []byte("corrupted-bundle"), 0600))
require.NoError(t, os.WriteFile(bundlePath, []byte("corrupted-bundle"), 0o600))
return bundlePath
}

func createJSONDataFile(t *testing.T, data string) string {
jsonDataFilePath := path.Join(t.TempDir(), "bundle.pem")
require.NoError(t, os.WriteFile(jsonDataFilePath, []byte(data), 0600))
require.NoError(t, os.WriteFile(jsonDataFilePath, []byte(data), 0o600))
return jsonDataFilePath
}

Expand Down
18 changes: 9 additions & 9 deletions cmd/spire-server/cli/healthcheck/healthcheck_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import (
"testing"

"github.com/mitchellh/cli"
"github.com/spiffe/spire/cmd/spire-server/cli/common"
common_cli "github.com/spiffe/spire/pkg/common/cli"
"github.com/spiffe/spire/test/clitest"
"github.com/spiffe/spire/test/spiretest"
"github.com/stretchr/testify/suite"
"google.golang.org/grpc"
Expand Down Expand Up @@ -58,24 +58,24 @@ func (s *HealthCheckSuite) TestBadFlags() {
}

func (s *HealthCheckSuite) TestFailsIfEndpointDoesNotExist() {
code := s.cmd.Run([]string{common.AddrArg, common.AddrValue})
code := s.cmd.Run([]string{clitest.AddrArg, clitest.AddrValue})
s.NotEqual(0, code, "exit code")
s.Equal("", s.stdout.String(), "stdout")
spiretest.AssertHasPrefix(s.T(), s.stderr.String(), common.AddrError)
spiretest.AssertHasPrefix(s.T(), s.stderr.String(), "Error: server is unhealthy: unable to determine health\n")
}

func (s *HealthCheckSuite) TestFailsIfEndpointDoesNotExistVerbose() {
code := s.cmd.Run([]string{common.AddrArg, common.AddrValue, "-verbose"})
code := s.cmd.Run([]string{clitest.AddrArg, clitest.AddrValue, "-verbose"})
s.NotEqual(0, code, "exit code")
s.Equal("", s.stdout.String(), "stdout")
spiretest.AssertHasPrefix(s.T(), s.stderr.String(), common.AddrError)
s.Equal("Checking server health...\n", s.stdout.String(), "stdout")
spiretest.AssertHasPrefix(s.T(), s.stderr.String(), "Failed to check health: "+clitest.AddrError)
}

func (s *HealthCheckSuite) TestSucceedsIfServingStatusServing() {
addr := spiretest.StartGRPCServer(s.T(), func(srv *grpc.Server) {
grpc_health_v1.RegisterHealthServer(srv, withStatus(grpc_health_v1.HealthCheckResponse_SERVING))
})
code := s.cmd.Run([]string{common.AddrArg, common.GetAddr(addr)})
code := s.cmd.Run([]string{clitest.AddrArg, clitest.GetAddr(addr)})
s.Equal(0, code, "exit code")
s.Equal("Server is healthy.\n", s.stdout.String(), "stdout")
s.Equal("", s.stderr.String(), "stderr")
Expand All @@ -85,7 +85,7 @@ func (s *HealthCheckSuite) TestSucceedsIfServingStatusServingVerbose() {
addr := spiretest.StartGRPCServer(s.T(), func(srv *grpc.Server) {
grpc_health_v1.RegisterHealthServer(srv, withStatus(grpc_health_v1.HealthCheckResponse_SERVING))
})
code := s.cmd.Run([]string{common.AddrArg, common.GetAddr(addr), "-verbose"})
code := s.cmd.Run([]string{clitest.AddrArg, clitest.GetAddr(addr), "-verbose"})
s.Equal(0, code, "exit code")
s.Equal(`Checking server health...
Server is healthy.
Expand All @@ -97,7 +97,7 @@ func (s *HealthCheckSuite) TestFailsIfServiceStatusOther() {
addr := spiretest.StartGRPCServer(s.T(), func(srv *grpc.Server) {
grpc_health_v1.RegisterHealthServer(srv, withStatus(grpc_health_v1.HealthCheckResponse_NOT_SERVING))
})
code := s.cmd.Run([]string{common.AddrArg, common.GetAddr(addr), "-verbose"})
code := s.cmd.Run([]string{clitest.AddrArg, clitest.GetAddr(addr), "-verbose"})
s.NotEqual(0, code, "exit code")
s.Equal(`Checking server health...
`, s.stdout.String(), "stdout")
Expand Down
Loading

0 comments on commit 5521b42

Please sign in to comment.