Skip to content

Commit 206b758

Browse files
mbissambissa
committed
cleanup
1 parent 7e9e127 commit 206b758

4 files changed

Lines changed: 60 additions & 40 deletions

File tree

‎internal/xds/credentials/credentials.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -171,9 +171,6 @@ func (c *ChannelCreds) Bundle() credentials.Bundle {
171171

172172
// Equal reports whether c and other were built from the same configuration.
173173
func (c *ChannelCreds) Equal(other *ChannelCreds) bool {
174-
if c == nil || other == nil {
175-
return c == other
176-
}
177174
return c.identity.Equal(other.identity)
178175
}
179176

@@ -211,9 +208,6 @@ func (c *CallCreds) Credentials() credentials.PerRPCCredentials {
211208

212209
// Equal reports whether c and other were built from the same configuration.
213210
func (c *CallCreds) Equal(other *CallCreds) bool {
214-
if c == nil || other == nil {
215-
return c == other
216-
}
217211
return c.identity.Equal(other.identity)
218212
}
219213

‎internal/xds/grpcservice/grpcservice.go‎

Lines changed: 19 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -77,29 +77,30 @@ func (c *Config) Equal(other *Config) bool {
7777
// no-op for credentials owned by another component (e.g. the allowlisted
7878
// credentials owned by the bootstrap config).
7979
func (c *Config) Close() {
80-
// ChannelCredentials is nil only when a mid-parse failure closes a
81-
// partially populated config.
82-
if c.ChannelCredentials != nil {
83-
c.ChannelCredentials.Close()
84-
}
80+
c.ChannelCredentials.Close()
8581
for _, cc := range c.CallCredentials {
8682
cc.Close()
8783
}
8884
}
8985

9086
// Dial creates a channel to the side-channel service, using the channel and
91-
// call credentials from the config along with the provided dial options.
87+
// call credentials from the config along with the provided dial options. The
88+
// call credentials are attached to the channel; since they are part of the
89+
// config's identity, configs whose call credentials differ do not share a
90+
// channel.
9291
//
9392
// Dial does not take ownership of the config: the caller releases the
9493
// config's credentials via Close when the config is no longer needed, after
9594
// closing any channel created from it.
9695
func (c *Config) Dial(opts ...grpc.DialOption) (*grpc.ClientConn, error) {
96+
// The credentials are appended after the provided options so that they
97+
// cannot be overridden.
9798
dialOpts := make([]grpc.DialOption, 0, len(opts)+len(c.CallCredentials)+1)
99+
dialOpts = append(dialOpts, opts...)
98100
dialOpts = append(dialOpts, grpc.WithCredentialsBundle(c.ChannelCredentials.Bundle()))
99101
for _, cc := range c.CallCredentials {
100102
dialOpts = append(dialOpts, grpc.WithPerRPCCredentials(cc.Credentials()))
101103
}
102-
dialOpts = append(dialOpts, opts...)
103104
return grpc.NewClient(c.TargetURI, dialOpts...)
104105
}
105106

@@ -117,7 +118,7 @@ func (c *Config) Dial(opts ...grpc.DialOption) (*grpc.ClientConn, error) {
117118
// The credentials in the returned Config are built and ready to use. The
118119
// caller owns the Config and releases its credentials via Close when it is no
119120
// longer needed, after closing any channel dialed from it.
120-
func Parse(gs *v3corepb.GrpcService, bc *bootstrap.Config, sc *bootstrap.ServerConfig) (_ *Config, err error) {
121+
func Parse(gs *v3corepb.GrpcService, bc *bootstrap.Config, sc *bootstrap.ServerConfig) (*Config, error) {
121122
googleGrpc := gs.GetGoogleGrpc()
122123
if googleGrpc == nil {
123124
return nil, fmt.Errorf("grpcservice: only google_grpc GrpcService config is supported")
@@ -132,18 +133,22 @@ func Parse(gs *v3corepb.GrpcService, bc *bootstrap.Config, sc *bootstrap.ServerC
132133
}
133134

134135
cfg := &Config{TargetURI: targetURI}
135-
// Release any credentials built before a mid-parse failure.
136-
defer func() {
137-
if err != nil {
138-
cfg.Close()
139-
}
140-
}()
136+
var err error
137+
if cfg.Timeout, err = parseTimeout(gs); err != nil {
138+
return nil, err
139+
}
140+
if cfg.InitialMetadata, err = parseInitialMetadata(gs.GetInitialMetadata()); err != nil {
141+
return nil, err
142+
}
141143

144+
// Credentials are built last, so that no error path can drop built
145+
// credentials.
142146
if sc.ServerFeaturesTrustedXDSServer() {
143147
if cfg.ChannelCredentials, err = buildChannelCredentials(googleGrpc.GetChannelCredentialsPlugin(), bc); err != nil {
144148
return nil, err
145149
}
146150
if cfg.CallCredentials, err = buildCallCredentials(googleGrpc.GetCallCredentialsPlugin()); err != nil {
151+
cfg.ChannelCredentials.Close()
147152
return nil, err
148153
}
149154
} else {
@@ -156,13 +161,6 @@ func Parse(gs *v3corepb.GrpcService, bc *bootstrap.Config, sc *bootstrap.ServerC
156161
// them.
157162
cfg.ChannelCredentials, cfg.CallCredentials = svc.SideChannelCredentials()
158163
}
159-
160-
if cfg.Timeout, err = parseTimeout(gs); err != nil {
161-
return nil, err
162-
}
163-
if cfg.InitialMetadata, err = parseInitialMetadata(gs.GetInitialMetadata()); err != nil {
164-
return nil, err
165-
}
166164
return cfg, nil
167165
}
168166

‎internal/xds/httpfilter/ext_authz/ext_authz_test.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"google.golang.org/grpc/codes"
2929
"google.golang.org/grpc/internal/grpctest"
3030
"google.golang.org/grpc/internal/testutils"
31+
xdscreds "google.golang.org/grpc/internal/xds/credentials"
3132
"google.golang.org/grpc/internal/xds/grpcservice"
3233
"google.golang.org/grpc/internal/xds/httpfilter"
3334
"google.golang.org/grpc/internal/xds/matcher"
@@ -76,6 +77,18 @@ var cmpOpts = []cmp.Option{
7677
grpcservice.Config{},
7778
fraction{},
7879
),
80+
cmp.Comparer(func(a, b *xdscreds.ChannelCreds) bool {
81+
if a == nil || b == nil {
82+
return a == b
83+
}
84+
return a.Equal(b)
85+
}),
86+
cmp.Comparer(func(a, b *xdscreds.CallCreds) bool {
87+
if a == nil || b == nil {
88+
return a == b
89+
}
90+
return a.Equal(b)
91+
}),
7992
cmp.Transformer("RegexpToString", func(r *regexp.Regexp) string {
8093
if r == nil {
8194
return ""

‎internal/xds/httpfilter/extproc/config_test.go‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,18 @@ var cmpOpts = []cmp.Option{
139139
optional.Optional[processingModes]{},
140140
optional.Optional[bool]{},
141141
),
142-
cmp.Comparer((*xdscreds.ChannelCreds).Equal),
143-
cmp.Comparer((*xdscreds.CallCreds).Equal),
142+
cmp.Comparer(func(a, b *xdscreds.ChannelCreds) bool {
143+
if a == nil || b == nil {
144+
return a == b
145+
}
146+
return a.Equal(b)
147+
}),
148+
cmp.Comparer(func(a, b *xdscreds.CallCreds) bool {
149+
if a == nil || b == nil {
150+
return a == b
151+
}
152+
return a.Equal(b)
153+
}),
144154
protocmp.Transform(),
145155
cmp.Transformer("RegexpToString", func(r *regexp.Regexp) string {
146156
if r == nil {
@@ -811,9 +821,10 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
811821
responseBodyMode: modeSkip,
812822
},
813823
server: grpcservice.Config{
814-
TargetURI: testBaseURI,
815-
Timeout: time.Second,
816-
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
824+
TargetURI: testBaseURI,
825+
ChannelCredentials: allowlistInsecureCreds,
826+
Timeout: time.Second,
827+
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
817828
},
818829
mutationRules: httpfilter.HeaderMutationRules{
819830
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
@@ -837,7 +848,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
837848
responseBodyMode: modeSend,
838849
}),
839850
server: optional.New(grpcservice.Config{
840-
TargetURI: "override-uri",
851+
TargetURI: "override-uri",
852+
ChannelCredentials: allowlistInsecureCreds,
841853
}),
842854
},
843855
wantConfig: baseConfig{
@@ -861,7 +873,8 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
861873
responseBodyMode: modeSend,
862874
},
863875
server: grpcservice.Config{
864-
TargetURI: "override-uri",
876+
TargetURI: "override-uri",
877+
ChannelCredentials: allowlistInsecureCreds,
865878
},
866879
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
867880
disallowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("disallow-header", false)},
@@ -884,9 +897,10 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
884897
responseBodyMode: modeSkip,
885898
},
886899
server: grpcservice.Config{
887-
TargetURI: testBaseURI,
888-
Timeout: time.Second,
889-
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
900+
TargetURI: testBaseURI,
901+
ChannelCredentials: allowlistInsecureCreds,
902+
Timeout: time.Second,
903+
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
890904
},
891905
mutationRules: httpfilter.HeaderMutationRules{
892906
AllowExpr: regexp.MustCompile("^(?:allow-.*)$"),
@@ -921,9 +935,10 @@ func (s) TestBuildClientInterceptor_Success(t *testing.T) {
921935
responseBodyMode: modeSkip,
922936
},
923937
server: grpcservice.Config{
924-
TargetURI: testBaseURI,
925-
Timeout: time.Second,
926-
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
938+
TargetURI: testBaseURI,
939+
ChannelCredentials: allowlistInsecureCreds,
940+
Timeout: time.Second,
941+
InitialMetadata: metadata.MD(metadata.Pairs("key1", "value1")),
927942
},
928943
allowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("allow-header", false)},
929944
disallowedHeaders: []matcher.StringMatcher{matcher.NewExactStringMatcher("disallow-header", false)},

0 commit comments

Comments
 (0)