Skip to content

Commit

Permalink
Remove usage of deprecated grpc-go methods
Browse files Browse the repository at this point in the history
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 <ryan.turner253@icloud.com>
  • Loading branch information
rturner3 committed Dec 17, 2024
1 parent 5c023a0 commit f4e5335
Show file tree
Hide file tree
Showing 65 changed files with 375 additions and 367 deletions.
13 changes: 7 additions & 6 deletions cmd/spire-agent/cli/api/api_test.go
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
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
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
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
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
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
6 changes: 3 additions & 3 deletions cmd/spire-server/cli/entry/util_test.go
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
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
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
6 changes: 3 additions & 3 deletions cmd/spire-server/cli/jwt/mint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ import (
"github.com/go-jose/go-jose/v4/jwt"
svidv1 "github.com/spiffe/spire-api-sdk/proto/spire/api/server/svid/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/assert"
"github.com/stretchr/testify/require"
Expand All @@ -35,7 +35,7 @@ qNV3lKIL59N7G2B4ojbhfSNneSIIpP448uPxUnaunaQZ+/m7+x9oobIp
availableFormats = []string{"pretty", "json"}
expectedUsage = `Usage of jwt mint:
-audience value
Audience claim that will be included in the SVID. Can be used more than once.` + common.AddrOutputUsage +
Audience claim that will be included in the SVID. Can be used more than once.` + clitest.AddrOutputUsage +
` -spiffeID string
SPIFFE ID of the JWT-SVID
-ttl duration
Expand Down Expand Up @@ -325,7 +325,7 @@ func TestMintRun(t *testing.T) {
BaseDir: dir,
})

args := []string{common.AddrArg, common.GetAddr(addr)}
args := []string{clitest.AddrArg, clitest.GetAddr(addr)}
if tt.spiffeID != "" {
args = append(args, "-spiffeID", tt.spiffeID)
}
Expand Down
Loading

0 comments on commit f4e5335

Please sign in to comment.