Skip to content

Commit 69769dc

Browse files
easwarseaswars
authored
xds: make it possible to create a StringMatcher from arguments outside of test code (#8723)
Changes in this PR: - Add a new constructors for StringMatcher that can be shared between test and non-test code. This will be used as part of an internal feature to support ext_authz. - Create new pointers to match strings instead of using the ones from the proto. This would ensure that the xDS proto structs (which are usually huge) can be garbage collected earlier that currently. - Fixes a bug involving the regex matcher, which should not be considering the ignore_case field, but was. RELEASE NOTES: * xds: Fix a bug in StringMatcher where regexes would match incorrectly when ignore_case is set to true.
1 parent a764d3f commit 69769dc

File tree

5 files changed

+157
-129
lines changed

5 files changed

+157
-129
lines changed

‎credentials/xds/xds_client_test.go‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ func newTestContextWithHandshakeInfo(parent context.Context, root, identity cert
226226
// NewSubConn().
227227
var sms []matcher.StringMatcher
228228
if sanExactMatch != "" {
229-
sms = []matcher.StringMatcher{matcher.StringMatcherForTesting(newStringP(sanExactMatch), nil, nil, nil, nil, false)}
229+
sms = []matcher.StringMatcher{matcher.NewExactStringMatcher(sanExactMatch, false)}
230230
}
231231
info := xdsinternal.NewHandshakeInfo(root, identity, sms, false)
232232
uPtr := unsafe.Pointer(info)
@@ -542,7 +542,7 @@ func (s) TestClientCredsProviderSwitch(t *testing.T) {
542542
// Create a root provider which will fail the handshake because it does not
543543
// use the correct trust roots.
544544
root1 := makeRootProvider(t, "x509/client_ca_cert.pem")
545-
handshakeInfo := xdsinternal.NewHandshakeInfo(root1, nil, []matcher.StringMatcher{matcher.StringMatcherForTesting(newStringP(defaultTestCertSAN), nil, nil, nil, nil, false)}, false)
545+
handshakeInfo := xdsinternal.NewHandshakeInfo(root1, nil, []matcher.StringMatcher{matcher.NewExactStringMatcher(defaultTestCertSAN, false)}, false)
546546
// We need to repeat most of what newTestContextWithHandshakeInfo() does
547547
// here because we need access to the underlying HandshakeInfo so that we
548548
// can update it before the next call to ClientHandshake().
@@ -568,7 +568,7 @@ func (s) TestClientCredsProviderSwitch(t *testing.T) {
568568
// Create a new root provider which uses the correct trust roots. And update
569569
// the HandshakeInfo with the new provider.
570570
root2 := makeRootProvider(t, "x509/server_ca_cert.pem")
571-
handshakeInfo = xdsinternal.NewHandshakeInfo(root2, nil, []matcher.StringMatcher{matcher.StringMatcherForTesting(newStringP(defaultTestCertSAN), nil, nil, nil, nil, false)}, false)
571+
handshakeInfo = xdsinternal.NewHandshakeInfo(root2, nil, []matcher.StringMatcher{matcher.NewExactStringMatcher(defaultTestCertSAN, false)}, false)
572572
// Update the existing pointer, which address attribute will continue to
573573
// point to.
574574
atomic.StorePointer(&uPtr, unsafe.Pointer(handshakeInfo))
@@ -596,7 +596,3 @@ func (s) TestClientClone(t *testing.T) {
596596
t.Fatal("return value from Clone() doesn't point to new credentials instance")
597597
}
598598
}
599-
600-
func newStringP(s string) *string {
601-
return &s
602-
}

‎internal/credentials/xds/handshake_info_test.go‎

Lines changed: 40 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -170,40 +170,40 @@ func TestMatchingSANExists_FailureCases(t *testing.T) {
170170
{
171171
desc: "exact match",
172172
sanMatchers: []matcher.StringMatcher{
173-
matcher.StringMatcherForTesting(newStringP("abcd.test.com"), nil, nil, nil, nil, false),
174-
matcher.StringMatcherForTesting(newStringP("http://golang"), nil, nil, nil, nil, false),
175-
matcher.StringMatcherForTesting(newStringP("HTTP://GOLANG.ORG"), nil, nil, nil, nil, false),
173+
matcher.NewExactStringMatcher("abcd.test.com", false),
174+
matcher.NewExactStringMatcher("http://golang", false),
175+
matcher.NewExactStringMatcher("HTTP://GOLANG.ORG", false),
176176
},
177177
},
178178
{
179179
desc: "prefix match",
180180
sanMatchers: []matcher.StringMatcher{
181-
matcher.StringMatcherForTesting(nil, newStringP("i-aint-the-one"), nil, nil, nil, false),
182-
matcher.StringMatcherForTesting(nil, newStringP("192.168.1.1"), nil, nil, nil, false),
183-
matcher.StringMatcherForTesting(nil, newStringP("FOO.BAR"), nil, nil, nil, false),
181+
matcher.NewPrefixStringMatcher("i-aint-the-one", false),
182+
matcher.NewPrefixStringMatcher("192.168.1.1", false),
183+
matcher.NewPrefixStringMatcher("FOO.BAR", false),
184184
},
185185
},
186186
{
187187
desc: "suffix match",
188188
sanMatchers: []matcher.StringMatcher{
189-
matcher.StringMatcherForTesting(nil, nil, newStringP("i-aint-the-one"), nil, nil, false),
190-
matcher.StringMatcherForTesting(nil, nil, newStringP("1::68"), nil, nil, false),
191-
matcher.StringMatcherForTesting(nil, nil, newStringP(".COM"), nil, nil, false),
189+
matcher.NewSuffixStringMatcher("i-aint-the-one", false),
190+
matcher.NewSuffixStringMatcher("1::68", false),
191+
matcher.NewSuffixStringMatcher(".COM", false),
192192
},
193193
},
194194
{
195195
desc: "regex match",
196196
sanMatchers: []matcher.StringMatcher{
197-
matcher.StringMatcherForTesting(nil, nil, nil, nil, regexp.MustCompile(`.*\.examples\.com`), false),
198-
matcher.StringMatcherForTesting(nil, nil, nil, nil, regexp.MustCompile(`192\.[0-9]{1,3}\.1\.1`), false),
197+
matcher.NewRegexStringMatcher(regexp.MustCompile(`.*\.examples\.com`)),
198+
matcher.NewRegexStringMatcher(regexp.MustCompile(`192\.[0-9]{1,3}\.1\.1`)),
199199
},
200200
},
201201
{
202202
desc: "contains match",
203203
sanMatchers: []matcher.StringMatcher{
204-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("i-aint-the-one"), nil, false),
205-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("2001:db8:1:1::68"), nil, false),
206-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("GRPC"), nil, false),
204+
matcher.NewContainsStringMatcher("i-aint-the-one", false),
205+
matcher.NewContainsStringMatcher("2001:db8:1:1::68", false),
206+
matcher.NewContainsStringMatcher("GRPC", false),
207207
},
208208
},
209209
}
@@ -248,65 +248,65 @@ func TestMatchingSANExists_Success(t *testing.T) {
248248
{
249249
desc: "exact match dns wildcard",
250250
sanMatchers: []matcher.StringMatcher{
251-
matcher.StringMatcherForTesting(nil, newStringP("192.168.1.1"), nil, nil, nil, false),
252-
matcher.StringMatcherForTesting(newStringP("https://github.com/grpc/grpc-java"), nil, nil, nil, nil, false),
253-
matcher.StringMatcherForTesting(newStringP("abc.example.com"), nil, nil, nil, nil, false),
251+
matcher.NewPrefixStringMatcher("192.168.1.1", false),
252+
matcher.NewExactStringMatcher("https://github.com/grpc/grpc-java", false),
253+
matcher.NewExactStringMatcher("abc.example.com", false),
254254
},
255255
},
256256
{
257257
desc: "exact match ignore case",
258258
sanMatchers: []matcher.StringMatcher{
259-
matcher.StringMatcherForTesting(newStringP("FOOBAR@EXAMPLE.COM"), nil, nil, nil, nil, true),
259+
matcher.NewExactStringMatcher("FOOBAR@EXAMPLE.COM", true),
260260
},
261261
},
262262
{
263263
desc: "prefix match",
264264
sanMatchers: []matcher.StringMatcher{
265-
matcher.StringMatcherForTesting(nil, nil, newStringP(".co.in"), nil, nil, false),
266-
matcher.StringMatcherForTesting(nil, newStringP("192.168.1.1"), nil, nil, nil, false),
267-
matcher.StringMatcherForTesting(nil, newStringP("baz.test"), nil, nil, nil, false),
265+
matcher.NewSuffixStringMatcher(".co.in", false),
266+
matcher.NewPrefixStringMatcher("192.168.1.1", false),
267+
matcher.NewPrefixStringMatcher("baz.test", false),
268268
},
269269
},
270270
{
271271
desc: "prefix match ignore case",
272272
sanMatchers: []matcher.StringMatcher{
273-
matcher.StringMatcherForTesting(nil, newStringP("BAZ.test"), nil, nil, nil, true),
273+
matcher.NewPrefixStringMatcher("BAZ.test", true),
274274
},
275275
},
276276
{
277277
desc: "suffix match",
278278
sanMatchers: []matcher.StringMatcher{
279-
matcher.StringMatcherForTesting(nil, nil, nil, nil, regexp.MustCompile(`192\.[0-9]{1,3}\.1\.1`), false),
280-
matcher.StringMatcherForTesting(nil, nil, newStringP("192.168.1.1"), nil, nil, false),
281-
matcher.StringMatcherForTesting(nil, nil, newStringP("@test.com"), nil, nil, false),
279+
matcher.NewRegexStringMatcher(regexp.MustCompile(`192\.[0-9]{1,3}\.1\.1`)),
280+
matcher.NewSuffixStringMatcher("192.168.1.1", false),
281+
matcher.NewSuffixStringMatcher("@test.com", false),
282282
},
283283
},
284284
{
285285
desc: "suffix match ignore case",
286286
sanMatchers: []matcher.StringMatcher{
287-
matcher.StringMatcherForTesting(nil, nil, newStringP("@test.COM"), nil, nil, true),
287+
matcher.NewSuffixStringMatcher("@test.COM", true),
288288
},
289289
},
290290
{
291291
desc: "regex match",
292292
sanMatchers: []matcher.StringMatcher{
293-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("https://github.com/grpc/grpc-java"), nil, false),
294-
matcher.StringMatcherForTesting(nil, nil, nil, nil, regexp.MustCompile(`192\.[0-9]{1,3}\.1\.1`), false),
295-
matcher.StringMatcherForTesting(nil, nil, nil, nil, regexp.MustCompile(`.*\.test\.com`), false),
293+
matcher.NewContainsStringMatcher("https://github.com/grpc/grpc-java", false),
294+
matcher.NewRegexStringMatcher(regexp.MustCompile(`192\.[0-9]{1,3}\.1\.1`)),
295+
matcher.NewRegexStringMatcher(regexp.MustCompile(`.*\.test\.com`)),
296296
},
297297
},
298298
{
299299
desc: "contains match",
300300
sanMatchers: []matcher.StringMatcher{
301-
matcher.StringMatcherForTesting(newStringP("https://github.com/grpc/grpc-java"), nil, nil, nil, nil, false),
302-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("2001:68::db8"), nil, false),
303-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("192.0.0"), nil, false),
301+
matcher.NewExactStringMatcher("https://github.com/grpc/grpc-java", false),
302+
matcher.NewContainsStringMatcher("2001:68::db8", false),
303+
matcher.NewContainsStringMatcher("192.0.0", false),
304304
},
305305
},
306306
{
307307
desc: "contains match ignore case",
308308
sanMatchers: []matcher.StringMatcher{
309-
matcher.StringMatcherForTesting(nil, nil, nil, newStringP("GRPC"), nil, true),
309+
matcher.NewContainsStringMatcher("GRPC", true),
310310
},
311311
},
312312
}
@@ -322,10 +322,6 @@ func TestMatchingSANExists_Success(t *testing.T) {
322322
}
323323
}
324324

325-
func newStringP(s string) *string {
326-
return &s
327-
}
328-
329325
func TestEqual(t *testing.T) {
330326
tests := []struct {
331327
desc string
@@ -354,31 +350,31 @@ func TestEqual(t *testing.T) {
354350
{
355351
desc: "same providers, same SAN matchers",
356352
hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
357-
matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
353+
matcher.NewExactStringMatcher("foo.com", false),
358354
}, false),
359355
hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
360-
matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
356+
matcher.NewExactStringMatcher("foo.com", false),
361357
}, false),
362358
wantMatch: true,
363359
},
364360
{
365361
desc: "same providers, different SAN matchers",
366362
hi1: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
367-
matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
363+
matcher.NewExactStringMatcher("foo.com", false),
368364
}, false),
369365
hi2: NewHandshakeInfo(testCertProvider{}, testCertProvider{}, []matcher.StringMatcher{
370-
matcher.StringMatcherForTesting(newStringP("bar.com"), nil, nil, nil, nil, false),
366+
matcher.NewExactStringMatcher("bar.com", false),
371367
}, false),
372368
wantMatch: false,
373369
},
374370
{
375371
desc: "same SAN matchers with different content",
376372
hi1: NewHandshakeInfo(&testCertProvider{}, &testCertProvider{}, []matcher.StringMatcher{
377-
matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
373+
matcher.NewExactStringMatcher("foo.com", false),
378374
}, false),
379375
hi2: NewHandshakeInfo(&testCertProvider{}, &testCertProvider{}, []matcher.StringMatcher{
380-
matcher.StringMatcherForTesting(newStringP("foo.com"), nil, nil, nil, nil, false),
381-
matcher.StringMatcherForTesting(newStringP("bar.com"), nil, nil, nil, nil, false),
376+
matcher.NewExactStringMatcher("foo.com", false),
377+
matcher.NewExactStringMatcher("bar.com", false),
382378
}, false),
383379
wantMatch: false,
384380
},

‎internal/xds/matcher/string_matcher.go‎

Lines changed: 77 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -48,24 +48,49 @@ type StringMatcher struct {
4848

4949
// Match returns true if input matches the criteria in the given StringMatcher.
5050
func (sm StringMatcher) Match(input string) bool {
51-
if sm.ignoreCase {
52-
input = strings.ToLower(input)
53-
}
5451
switch {
5552
case sm.exactMatch != nil:
53+
if sm.ignoreCase {
54+
input = strings.ToLower(input)
55+
}
5656
return input == *sm.exactMatch
5757
case sm.prefixMatch != nil:
58+
if sm.ignoreCase {
59+
input = strings.ToLower(input)
60+
}
5861
return strings.HasPrefix(input, *sm.prefixMatch)
5962
case sm.suffixMatch != nil:
63+
if sm.ignoreCase {
64+
input = strings.ToLower(input)
65+
}
6066
return strings.HasSuffix(input, *sm.suffixMatch)
61-
case sm.regexMatch != nil:
62-
return grpcutil.FullMatchWithRegex(sm.regexMatch, input)
6367
case sm.containsMatch != nil:
68+
if sm.ignoreCase {
69+
input = strings.ToLower(input)
70+
}
6471
return strings.Contains(input, *sm.containsMatch)
72+
case sm.regexMatch != nil:
73+
return grpcutil.FullMatchWithRegex(sm.regexMatch, input)
6574
}
6675
return false
6776
}
6877

78+
// newStrPtr allocates a new string that holds the value of input and returns a
79+
// pointer to it. ignoreCase controls if a lower case version of input is used.
80+
func newStrPtr(input *string, ignoreCase bool) *string {
81+
if input == nil {
82+
return nil
83+
}
84+
85+
s := new(string)
86+
if ignoreCase {
87+
*s = strings.ToLower(*input)
88+
} else {
89+
*s = *input
90+
}
91+
return s
92+
}
93+
6994
// StringMatcherFromProto is a helper function to create a StringMatcher from
7095
// the corresponding StringMatcher proto.
7196
//
@@ -78,26 +103,17 @@ func StringMatcherFromProto(matcherProto *v3matcherpb.StringMatcher) (StringMatc
78103
matcher := StringMatcher{ignoreCase: matcherProto.GetIgnoreCase()}
79104
switch mt := matcherProto.GetMatchPattern().(type) {
80105
case *v3matcherpb.StringMatcher_Exact:
81-
matcher.exactMatch = &mt.Exact
82-
if matcher.ignoreCase {
83-
*matcher.exactMatch = strings.ToLower(*matcher.exactMatch)
84-
}
106+
matcher.exactMatch = newStrPtr(&mt.Exact, matcher.ignoreCase)
85107
case *v3matcherpb.StringMatcher_Prefix:
86108
if matcherProto.GetPrefix() == "" {
87109
return StringMatcher{}, errors.New("empty prefix is not allowed in StringMatcher")
88110
}
89-
matcher.prefixMatch = &mt.Prefix
90-
if matcher.ignoreCase {
91-
*matcher.prefixMatch = strings.ToLower(*matcher.prefixMatch)
92-
}
111+
matcher.prefixMatch = newStrPtr(&mt.Prefix, matcher.ignoreCase)
93112
case *v3matcherpb.StringMatcher_Suffix:
94113
if matcherProto.GetSuffix() == "" {
95114
return StringMatcher{}, errors.New("empty suffix is not allowed in StringMatcher")
96115
}
97-
matcher.suffixMatch = &mt.Suffix
98-
if matcher.ignoreCase {
99-
*matcher.suffixMatch = strings.ToLower(*matcher.suffixMatch)
100-
}
116+
matcher.suffixMatch = newStrPtr(&mt.Suffix, matcher.ignoreCase)
101117
case *v3matcherpb.StringMatcher_SafeRegex:
102118
regex := matcherProto.GetSafeRegex().GetRegex()
103119
re, err := regexp.Compile(regex)
@@ -109,40 +125,59 @@ func StringMatcherFromProto(matcherProto *v3matcherpb.StringMatcher) (StringMatc
109125
if matcherProto.GetContains() == "" {
110126
return StringMatcher{}, errors.New("empty contains is not allowed in StringMatcher")
111127
}
112-
matcher.containsMatch = &mt.Contains
113-
if matcher.ignoreCase {
114-
*matcher.containsMatch = strings.ToLower(*matcher.containsMatch)
115-
}
128+
matcher.containsMatch = newStrPtr(&mt.Contains, matcher.ignoreCase)
116129
default:
117130
return StringMatcher{}, fmt.Errorf("unrecognized string matcher: %+v", matcherProto)
118131
}
119132
return matcher, nil
120133
}
121134

122-
// StringMatcherForTesting is a helper function to create a StringMatcher based
123-
// on the given arguments. Intended only for testing purposes.
124-
func StringMatcherForTesting(exact, prefix, suffix, contains *string, regex *regexp.Regexp, ignoreCase bool) StringMatcher {
125-
sm := StringMatcher{
126-
exactMatch: exact,
127-
prefixMatch: prefix,
128-
suffixMatch: suffix,
129-
regexMatch: regex,
130-
containsMatch: contains,
135+
// NewExactStringMatcher creates a string matcher that requires the input string
136+
// to exactly match the pattern specified here. The match will be case
137+
// insensitive if ignore_case is true.
138+
func NewExactStringMatcher(pattern string, ignoreCase bool) StringMatcher {
139+
return StringMatcher{
140+
exactMatch: newStrPtr(&pattern, ignoreCase),
141+
ignoreCase: ignoreCase,
142+
}
143+
}
144+
145+
// NewPrefixStringMatcher creates a string matcher that requires the input
146+
// string to contain the prefix specified here. The match will be case
147+
// insensitive if ignore_case is true.
148+
func NewPrefixStringMatcher(prefix string, ignoreCase bool) StringMatcher {
149+
return StringMatcher{
150+
prefixMatch: newStrPtr(&prefix, ignoreCase),
151+
ignoreCase: ignoreCase,
152+
}
153+
}
154+
155+
// NewSuffixStringMatcher creates a string matcher that requires the input
156+
// string to contain the suffix specified here. The match will be case
157+
// insensitive if ignore_case is true.
158+
func NewSuffixStringMatcher(suffix string, ignoreCase bool) StringMatcher {
159+
return StringMatcher{
160+
suffixMatch: newStrPtr(&suffix, ignoreCase),
161+
ignoreCase: ignoreCase,
162+
}
163+
}
164+
165+
// NewContainsStringMatcher creates a string matcher that requires the input
166+
// string to contain the pattern specified here. The match will be case
167+
// insensitive if ignore_case is true.
168+
func NewContainsStringMatcher(pattern string, ignoreCase bool) StringMatcher {
169+
return StringMatcher{
170+
containsMatch: newStrPtr(&pattern, ignoreCase),
131171
ignoreCase: ignoreCase,
132172
}
133-
if ignoreCase {
134-
switch {
135-
case sm.exactMatch != nil:
136-
*sm.exactMatch = strings.ToLower(*exact)
137-
case sm.prefixMatch != nil:
138-
*sm.prefixMatch = strings.ToLower(*prefix)
139-
case sm.suffixMatch != nil:
140-
*sm.suffixMatch = strings.ToLower(*suffix)
141-
case sm.containsMatch != nil:
142-
*sm.containsMatch = strings.ToLower(*contains)
143-
}
173+
}
174+
175+
// NewRegexStringMatcher creates a string matcher that requires the input string
176+
// to match the regular expression specified here.
177+
func NewRegexStringMatcher(regex *regexp.Regexp) StringMatcher {
178+
return StringMatcher{
179+
regexMatch: regex,
144180
}
145-
return sm
146181
}
147182

148183
// ExactMatch returns the value of the configured exact match or an empty string

0 commit comments

Comments
 (0)