Skip to content

Commit d3b9bc9

Browse files
committed
address review comments: make grpcservice a pure parser, relocate access token creds, and simplify channel API
1 parent 979d438 commit d3b9bc9

14 files changed

Lines changed: 285 additions & 169 deletions

File tree

internal/xds/bootstrap/accesstokencreds/call_creds.go renamed to internal/xds/grpcservice/accesstokencreds/call_creds.go

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,12 @@ import (
3232
// token to outgoing RPCs. The config must be a JSON object of the form
3333
// {"token": <non-empty string>}.
3434
//
35-
// The caller is expected to invoke the cancel function when they are done
36-
// using the returned call creds. This cancel function is idempotent.
35+
// The token is only ever sent on connections that provide privacy and
36+
// integrity; on weaker connections it is withheld without failing the RPC.
37+
//
38+
// The returned cleanup function is a no-op since these credentials hold no
39+
// resources; it exists to satisfy the registry's CallCredentials Build
40+
// contract, and is idempotent.
3741
func NewCallCredentials(configJSON json.RawMessage) (credentials.PerRPCCredentials, func(), error) {
3842
var cfg struct {
3943
Token string `json:"token"`
@@ -57,7 +61,7 @@ type callCreds struct {
5761

5862
// GetRequestMetadata returns the token as an authorization header, but only
5963
// when the connection provides privacy and integrity. On weaker connections
60-
// the token is withheld without failing the RPC, as per gRFC A102.
64+
// the token is withheld without failing the RPC.
6165
func (c *callCreds) GetRequestMetadata(ctx context.Context, _ ...string) (map[string]string, error) {
6266
ri, ok := credentials.RequestInfoFromContext(ctx)
6367
if !ok || credentials.CheckSecurityLevel(ri.AuthInfo, credentials.PrivacyAndIntegrity) != nil {
@@ -66,9 +70,10 @@ func (c *callCreds) GetRequestMetadata(ctx context.Context, _ ...string) (map[st
6670
return map[string]string{"authorization": "Bearer " + c.token}, nil
6771
}
6872

69-
// RequireTransportSecurity returns false. The credentials may be used on any
70-
// connection, but GetRequestMetadata withholds the token on connections that
71-
// do not provide privacy and integrity.
73+
// RequireTransportSecurity indicates whether the credentials requires
74+
// transport security. It returns false: the credentials may be added to any
75+
// connection, but the token is withheld (not sent) on connections that do
76+
// not provide privacy and integrity.
7277
func (c *callCreds) RequireTransportSecurity() bool {
7378
return false
7479
}

internal/xds/bootstrap/accesstokencreds/call_creds_test.go renamed to internal/xds/grpcservice/accesstokencreds/call_creds_test.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,8 @@ func (s) TestGetRequestMetadata(t *testing.T) {
101101
t.Fatalf("GetRequestMetadata() on a secure connection returned authorization header %q, want %q", got, want)
102102
}
103103

104-
// The token must be withheld, without error, on an insecure connection.
104+
// The token must be withheld, without error, on a connection that does
105+
// not provide privacy and integrity.
105106
insecureCtx := credentials.NewContextWithRequestInfo(ctx, credentials.RequestInfo{
106107
AuthInfo: &testAuthInfo{secLevel: credentials.NoSecurity},
107108
})

internal/xds/grpcservice/grpcservice.go

Lines changed: 33 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,14 @@ import (
2727
"strings"
2828
"time"
2929

30-
v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
31-
access_tokenpb "github.com/envoyproxy/go-control-plane/envoy/extensions/grpc_service/call_credentials/access_token/v3"
32-
xdspb "github.com/envoyproxy/go-control-plane/envoy/extensions/grpc_service/channel_credentials/xds/v3"
3330
imetadata "google.golang.org/grpc/internal/metadata"
3431
"google.golang.org/grpc/internal/xds/bootstrap"
3532
"google.golang.org/grpc/metadata"
3633
"google.golang.org/grpc/resolver"
34+
35+
v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
36+
accesstokenpb "github.com/envoyproxy/go-control-plane/envoy/extensions/grpc_service/call_credentials/access_token/v3"
37+
xdspb "github.com/envoyproxy/go-control-plane/envoy/extensions/grpc_service/channel_credentials/xds/v3"
3738
"google.golang.org/protobuf/proto"
3839
"google.golang.org/protobuf/types/known/anypb"
3940
)
@@ -49,21 +50,6 @@ const (
4950
maxHeaderValueLen = 16384
5051
)
5152

52-
// GrpcService parses GrpcService protos in the context of a bootstrap
53-
// configuration and a trust level for the delivering xDS server.
54-
type GrpcService struct {
55-
config *bootstrap.Config
56-
trusted bool
57-
}
58-
59-
// New returns a GrpcService that parses GrpcService protos against the given
60-
// bootstrap configuration. The trusted argument indicates whether the xDS
61-
// server that delivered the resource is configured with the trusted_xds_server
62-
// server feature.
63-
func New(config *bootstrap.Config, trusted bool) *GrpcService {
64-
return &GrpcService{config: config, trusted: trusted}
65-
}
66-
6753
// Config is the parsed form of a GrpcService proto.
6854
type Config struct {
6955
// TargetURI is the gRPC target URI of the side-channel service.
@@ -73,75 +59,55 @@ type Config struct {
7359
Timeout time.Duration
7460
// InitialMetadata is the metadata to add to RPCs on the side channel.
7561
InitialMetadata metadata.MD
76-
// ChannelCredentials are the channel credentials used to create the
77-
// side channel. Set only on the trusted path.
62+
// ChannelCredentials are the channel credentials extracted from the
63+
// proto's channel credentials plugins. Empty if the proto configures no
64+
// supported channel credentials.
7865
ChannelCredentials bootstrap.ChannelCreds
79-
// CallCredentials are the call credentials to apply to RPCs sent on the
80-
// side channel. Set only on the trusted path.
66+
// CallCredentials are the call credentials extracted from the proto's
67+
// call credentials plugins, preserving order.
8168
CallCredentials []bootstrap.CallCredsConfig
8269
}
8370

8471
// Parse parses and validates a GrpcService proto into a Config.
8572
//
86-
// When the delivering server is trusted, the credentials are taken from the
87-
// proto; otherwise the target URI must be present in the allowed_grpc_services
88-
// map, and the credentials are resolved later at channel creation time and left
89-
// empty here.
90-
func (g *GrpcService) Parse(gs *v3corepb.GrpcService) (Config, error) {
73+
// Parsing is independent of the trust status of the xDS server that delivered
74+
// the proto: the credentials configured in the proto are always extracted
75+
// into the returned Config, and it is up to the caller to decide whether they
76+
// may be used.
77+
func Parse(gs *v3corepb.GrpcService) (*Config, error) {
9178
googleGrpc := gs.GetGoogleGrpc()
9279
if googleGrpc == nil {
93-
return Config{}, fmt.Errorf("grpcservice: only google_grpc GrpcService config is supported")
80+
return nil, fmt.Errorf("grpcservice: only google_grpc GrpcService config is supported")
9481
}
9582

9683
targetURI := googleGrpc.GetTargetUri()
9784
if targetURI == "" {
98-
return Config{}, fmt.Errorf("grpcservice: target_uri must be non-empty")
85+
return nil, fmt.Errorf("grpcservice: target_uri must be non-empty")
9986
}
10087
if err := validateTargetURI(targetURI); err != nil {
101-
return Config{}, err
88+
return nil, err
10289
}
10390

104-
var channelCreds bootstrap.ChannelCreds
105-
var callCreds []bootstrap.CallCredsConfig
106-
if g.trusted {
107-
var err error
108-
if channelCreds, err = extractChannelCredentials(googleGrpc.GetChannelCredentialsPlugin()); err != nil {
109-
return Config{}, fmt.Errorf("grpcservice: failed to extract channel credentials: %v", err)
110-
}
111-
if callCreds, err = extractCallCredentials(googleGrpc.GetCallCredentialsPlugin()); err != nil {
112-
return Config{}, fmt.Errorf("grpcservice: failed to extract call credentials: %v", err)
113-
}
114-
} else {
115-
// For untrusted servers we ignore the credentials in the proto.
116-
// The target must be present in the allowed_grpc_services
117-
// allowlist, but the credentials themselves are resolved later,
118-
// at channel creation time; they are left empty in the parsed
119-
// config here. A nil bootstrap config has no allowlist, so all
120-
// targets are rejected.
121-
var allowedSvc *bootstrap.AllowedGRPCService
122-
var ok bool
123-
if g.config != nil {
124-
allowedSvc, ok = g.config.AllowedGRPCService(targetURI)
125-
}
126-
if !ok {
127-
return Config{}, fmt.Errorf("grpcservice: target_uri %q is not present in allowed_grpc_services", targetURI)
128-
}
129-
if allowedSvc == nil {
130-
return Config{}, fmt.Errorf("grpcservice: allowed gRPC service %q has nil configuration", targetURI)
131-
}
91+
channelCreds, err := extractChannelCredentials(googleGrpc.GetChannelCredentialsPlugin())
92+
if err != nil {
93+
return nil, fmt.Errorf("grpcservice: failed to extract channel credentials: %v", err)
94+
}
95+
callCreds, err := extractCallCredentials(googleGrpc.GetCallCredentialsPlugin())
96+
if err != nil {
97+
return nil, fmt.Errorf("grpcservice: failed to extract call credentials: %v", err)
13298
}
13399

134100
timeout, err := parseTimeout(gs)
135101
if err != nil {
136-
return Config{}, err
102+
return nil, err
137103
}
138104

139105
initialMetadata, err := parseInitialMetadata(gs.GetInitialMetadata())
140106
if err != nil {
141-
return Config{}, err
107+
return nil, err
142108
}
143109

144-
return Config{
110+
return &Config{
145111
TargetURI: targetURI,
146112
Timeout: timeout,
147113
InitialMetadata: initialMetadata,
@@ -212,9 +178,10 @@ func parseInitialMetadata(headers []*v3corepb.HeaderValue) (metadata.MD, error)
212178
return md, nil
213179
}
214180

215-
// extractChannelCredentials returns the first supported channel credential from
216-
// the plugin list. It is an error if none of the configured plugins are
217-
// supported.
181+
// extractChannelCredentials returns the first supported channel credential
182+
// from the plugin list. If none of the configured plugins are supported, it
183+
// returns empty credentials without error; whether credentials are required
184+
// is a policy decision left to the caller.
218185
func extractChannelCredentials(plugins []*anypb.Any) (bootstrap.ChannelCreds, error) {
219186
for _, cred := range plugins {
220187
if cred == nil {
@@ -250,7 +217,7 @@ func extractChannelCredentials(plugins []*anypb.Any) (bootstrap.ChannelCreds, er
250217
continue
251218
}
252219
}
253-
return bootstrap.ChannelCreds{}, fmt.Errorf("no supported channel credentials found in plugins")
220+
return bootstrap.ChannelCreds{}, nil
254221
}
255222

256223
// extractCallCredentials returns the supported call credentials from the plugin
@@ -265,7 +232,7 @@ func extractCallCredentials(plugins []*anypb.Any) ([]bootstrap.CallCredsConfig,
265232
if cred.GetTypeUrl() != accessTokenCredsTypeURL {
266233
continue
267234
}
268-
var accessToken access_tokenpb.AccessTokenCredentials
235+
var accessToken accesstokenpb.AccessTokenCredentials
269236
if err := anypb.UnmarshalTo(cred, &accessToken, proto.UnmarshalOptions{}); err != nil {
270237
return nil, fmt.Errorf("failed to unmarshal AccessTokenCredentials: %v", err)
271238
}

internal/xds/grpcservice/grpcservice_test.go

Lines changed: 57 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -23,37 +23,27 @@ import (
2323
"strings"
2424
"testing"
2525

26-
v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
2726
"github.com/google/go-cmp/cmp"
28-
"google.golang.org/grpc/internal/envconfig"
29-
"google.golang.org/grpc/internal/testutils"
27+
"google.golang.org/grpc/internal/grpctest"
3028
"google.golang.org/grpc/internal/xds/bootstrap"
3129
"google.golang.org/grpc/metadata"
30+
31+
v3corepb "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
32+
accesstokenpb "github.com/envoyproxy/go-control-plane/envoy/extensions/grpc_service/call_credentials/access_token/v3"
3233
"google.golang.org/protobuf/types/known/anypb"
3334
"google.golang.org/protobuf/types/known/durationpb"
3435
)
3536

36-
const target = "dns:///my-service:443"
37+
type s struct {
38+
grpctest.Tester
39+
}
3740

38-
// bootstrapConfig builds a bootstrap Config whose allowed_grpc_services is set
39-
// to the provided JSON (a map from target URI to allowed service config).
40-
func bootstrapConfig(t *testing.T, allowed string) *bootstrap.Config {
41-
t.Helper()
42-
contents, err := bootstrap.NewContentsForTesting(bootstrap.ConfigOptionsForTesting{
43-
Servers: json.RawMessage(`[{"server_uri":"td.googleapis.com:443","channel_creds":[{"type":"insecure"}]}]`),
44-
Node: json.RawMessage(`{}`),
45-
AllowedGRPCServices: json.RawMessage(allowed),
46-
})
47-
if err != nil {
48-
t.Fatalf("NewContentsForTesting() failed: %v", err)
49-
}
50-
cfg, err := bootstrap.NewConfigFromContents(contents)
51-
if err != nil {
52-
t.Fatalf("NewConfigFromContents() failed: %v", err)
53-
}
54-
return cfg
41+
func Test(t *testing.T) {
42+
grpctest.RunSubTests(t, s{})
5543
}
5644

45+
const target = "dns:///my-service:443"
46+
5747
func googleGrpcService(target string, channelPlugins []*anypb.Any, timeout *durationpb.Duration) *v3corepb.GrpcService {
5848
return &v3corepb.GrpcService{
5949
TargetSpecifier: &v3corepb.GrpcService_GoogleGrpc_{
@@ -66,62 +56,59 @@ func googleGrpcService(target string, channelPlugins []*anypb.Any, timeout *dura
6656
}
6757
}
6858

69-
func TestParse(t *testing.T) {
70-
// The allowed_grpc_services bootstrap field is parsed only when a
71-
// consuming feature is enabled.
72-
testutils.SetEnvConfig(t, &envconfig.XDSClientExtProcEnabled, true)
59+
func accessTokenPlugin(t *testing.T, token string) *anypb.Any {
60+
t.Helper()
61+
a, err := anypb.New(&accesstokenpb.AccessTokenCredentials{Token: token})
62+
if err != nil {
63+
t.Fatalf("Failed to marshal AccessTokenCredentials: %v", err)
64+
}
65+
return a
66+
}
7367

68+
func (s) TestParse(t *testing.T) {
7469
insecurePlugin := &anypb.Any{TypeUrl: insecureCredsTypeURL}
75-
allowedInsecure := `{"dns:///my-service:443":{"channel_creds":[{"type":"insecure"}]}}`
7670

7771
tests := []struct {
7872
name string
7973
gs *v3corepb.GrpcService
80-
trusted bool
81-
config *bootstrap.Config
82-
want Config
74+
want *Config
8375
wantErr string
8476
}{
8577
{
86-
name: "trusted_insecure_channel_creds",
87-
gs: googleGrpcService(target, []*anypb.Any{insecurePlugin}, nil),
88-
trusted: true,
89-
config: bootstrapConfig(t, "{}"),
90-
want: Config{TargetURI: target, ChannelCredentials: bootstrap.ChannelCreds{Type: "insecure"}},
78+
name: "insecure_channel_creds",
79+
gs: googleGrpcService(target, []*anypb.Any{insecurePlugin}, nil),
80+
want: &Config{TargetURI: target, ChannelCredentials: bootstrap.ChannelCreds{Type: "insecure"}},
9181
},
9282
{
93-
name: "untrusted_allowlisted_leaves_creds_empty",
94-
gs: googleGrpcService(target, nil, nil),
95-
trusted: false,
96-
config: bootstrapConfig(t, allowedInsecure),
97-
want: Config{TargetURI: target},
83+
name: "no_channel_creds_left_empty",
84+
gs: googleGrpcService(target, nil, nil),
85+
want: &Config{TargetURI: target},
9886
},
9987
{
100-
name: "untrusted_not_allowlisted",
101-
gs: googleGrpcService(target, nil, nil),
102-
trusted: false,
103-
config: bootstrapConfig(t, "{}"),
104-
wantErr: "not present in allowed_grpc_services",
88+
name: "unsupported_channel_creds_left_empty",
89+
gs: googleGrpcService(target, []*anypb.Any{{TypeUrl: "type.googleapis.com/unsupported.Credentials"}}, nil),
90+
want: &Config{TargetURI: target},
10591
},
10692
{
10793
name: "missing_google_grpc",
10894
gs: &v3corepb.GrpcService{},
109-
trusted: true,
110-
config: bootstrapConfig(t, "{}"),
11195
wantErr: "only google_grpc",
11296
},
97+
{
98+
name: "empty_target_uri",
99+
gs: googleGrpcService("", nil, nil),
100+
wantErr: "target_uri must be non-empty",
101+
},
113102
{
114103
name: "zero_timeout_rejected",
115104
gs: googleGrpcService(target, []*anypb.Any{insecurePlugin}, durationpb.New(0)),
116-
trusted: true,
117-
config: bootstrapConfig(t, "{}"),
118105
wantErr: "timeout must be strictly positive",
119106
},
120107
}
121108

122109
for _, test := range tests {
123110
t.Run(test.name, func(t *testing.T) {
124-
got, err := New(test.config, test.trusted).Parse(test.gs)
111+
got, err := Parse(test.gs)
125112
if test.wantErr != "" {
126113
if err == nil || !strings.Contains(err.Error(), test.wantErr) {
127114
t.Fatalf("Parse() error = %v, want substring %q", err, test.wantErr)
@@ -138,13 +125,32 @@ func TestParse(t *testing.T) {
138125
}
139126
}
140127

141-
func TestParseInitialMetadata(t *testing.T) {
128+
func (s) TestParseCallCredentials(t *testing.T) {
129+
gs := googleGrpcService(target, []*anypb.Any{{TypeUrl: insecureCredsTypeURL}}, nil)
130+
gs.GetGoogleGrpc().CallCredentialsPlugin = []*anypb.Any{accessTokenPlugin(t, "test-token")}
131+
got, err := Parse(gs)
132+
if err != nil {
133+
t.Fatalf("Parse() returned unexpected error: %v", err)
134+
}
135+
want := []bootstrap.CallCredsConfig{{Type: "access_token", Config: json.RawMessage(`{"token":"test-token"}`)}}
136+
if diff := cmp.Diff(want, got.CallCredentials); diff != "" {
137+
t.Errorf("Parse() CallCredentials mismatch (-want +got):\n%s", diff)
138+
}
139+
140+
// An empty token must be rejected.
141+
gs.GetGoogleGrpc().CallCredentialsPlugin = []*anypb.Any{accessTokenPlugin(t, "")}
142+
if _, err := Parse(gs); err == nil || !strings.Contains(err.Error(), "access token must be non-empty") {
143+
t.Fatalf("Parse() error = %v, want substring %q", err, "access token must be non-empty")
144+
}
145+
}
146+
147+
func (s) TestParseInitialMetadata(t *testing.T) {
142148
gs := googleGrpcService(target, []*anypb.Any{{TypeUrl: insecureCredsTypeURL}}, nil)
143149
gs.InitialMetadata = []*v3corepb.HeaderValue{
144150
{Key: "key-b", Value: "b"},
145151
{Key: "key-a", Value: "legacy", RawValue: []byte("raw-a")},
146152
}
147-
got, err := New(bootstrapConfig(t, "{}"), true).Parse(gs)
153+
got, err := Parse(gs)
148154
if err != nil {
149155
t.Fatalf("Parse() returned unexpected error: %v", err)
150156
}

internal/xds/httpfilter/ext_authz/ext_authz.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ func init() {
4444
}
4545

4646
var (
47-
// TODO: Parse via grpcservice.GrpcService with the filter parse context,
47+
// TODO: Parse via grpcservice.Parse with the filter parse options,
4848
// as ext_proc does, when ext_authz is wired up for gRFC A102.
4949
parseGRPCServiceConfig = func(*v3corepb.GrpcService) (grpcservice.Config, error) {
5050
return grpcservice.Config{}, fmt.Errorf("parseGRPCServiceConfig not implemented")

0 commit comments

Comments
 (0)