-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regexp support for node lables #2262
Conversation
lib/services/role.go
Outdated
return false, fmt.Sprintf("no value match: got '%v' want: '%v'", targetVal, selectorValues) | ||
|
||
if !utils.SliceContainsStr(selectorValues, Wildcard) { | ||
result, err := utils.SliceContainsRegexStr(targetVal, selectorValues) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SliceMatchesRegex
is a better name. Str
is irrelevant because regex
is not Str
and Contains
is not really contains, but Matches
if err != nil { | ||
return false, "", trace.Wrap(err) | ||
} else if !result { | ||
return false, fmt.Sprintf("no value match: got '%v' want: '%v'", targetVal, selectorValues), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just use %q
that will properly quote the string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used to do this, but then I noticed when customer paste us logs from systemd they end up being quoted and escaped which makes logs difficult to read.
if !hasKey { | ||
return false, fmt.Sprintf("no key match: '%v'", key) | ||
return false, fmt.Sprintf("no key match: '%v'", key), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use %q
instead of '%v'
lib/services/role.go
Outdated
return false, fmt.Sprintf("no value match: got '%v' want: '%v'", targetVal, selectorValues) | ||
|
||
if !utils.SliceContainsStr(selectorValues, Wildcard) { | ||
result, err := utils.SliceContainsRegexStr(targetVal, selectorValues) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to compile regex once and match multiple times, but before doing this, what are performance implications of the match on let's say 4K nodes? Is it computationally expensive? If yes, I would improve this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each expression is only compiled once already? We loop over a slice and compile each once at the moment.
I compared the CheckAccessToServer
benchmark I wrote on master and this branch this branch is about 1 ms slower.
cbb9598
to
ed6d35a
Compare
Purpose
Building off the work of @ken5scal in #2206, add regexp support for node labels.
Implementation
Related Issues
Fixes #2205
Fixes #2206