Skip to content
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.

Sign up for GitHub

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

Make stats and string matching case-insensitive #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Make stats and string matching case-insensitive #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e477ae9
Make names lowercase to avoid ballooning metrics
ltning Aug 31, 2023
029e58f
Import strings
ltning Aug 31, 2023
c6b0287
Move lowercasing to inc/excl file read and matching
Sep 1, 2023
d181a73
Whitespace fix
Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion collectors/names_collector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package collectors
import (
"bufio"
"os"
"strings"

"github.com/DRuggeri/bind_query_exporter/util"
"github.com/prometheus/client_golang/prometheus"
Expand Down Expand Up @@ -126,7 +127,7 @@ func makeList(fileName string) (map[string]bool, error) {
scanner := bufio.NewScanner(file)
for scanner.Scan() {
log.Debugln(" ", scanner.Text())
result[scanner.Text()] = true
result[strings.ToLower(scanner.Text())] = true
}

if err := scanner.Err(); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion util/LogMatcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func (m LogMatcher) ExtractInfo(line string) LogMatch {
if len(match) > 0 {
result.Matched = true
result.QueryClient = match[1]
result.QueryName = match[2]
result.QueryName = strings.ToLower(match[2])
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! I think the idea is great, but I detect a possible bug here. Since BIND will log the query exactly as received, I think we want to fold the entire input down to lower case first or make the match case insensitive. If we don't, we'll have an issue under the following circumstances:

  • The list of matches includes "FOOBAR.com" (but this is folded to "foobar.com") by the changes in names_collector.go
  • The client queries for "FooBar.com" -> therefore, no match against "foobar"

Conversely, if we fold the whole line down to lower case (maybe on line 39 with line = strings.ToLower(line), then we'll have a successful match. Since DNS RFC says that DNS names are not case sensitive, a non-case match is indeed a match... I think this is fine.

To me, it feels like it's more obvious to fold to lower case than to check/enforce the regex to have the case-insensitive prepend flag. WDYT?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, I'm not sure what I missed? Wouldn't my PR make sure that 'foobar.com' is inserted in the array and by ToLower-ing the name when looking it up, isn't this going to cover both cases?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're close - the only issue is that the string coming into this function (in line) is coming straight from the log file. I did a quick test and confirmed that BIND will log the value exactly as it is received. So, earlier on in the PR, you're correctly coaxing the search strings to lower case - but at this point, the lower case string is compared to a mixed case value in line if the user queried for "FoObAr". So I'm thinking that if we convert line to lower case around line 39, it'd be all set.

result.QueryType = match[3]

/* Check if we should avoid a DNS lookup since this name is not
Expand Down