Log blocking rule in resolver #1489

zc-devs · 2024-05-21T12:40:32Z

Follow up of #1460 (review)
Closes #1458

ThinkChaos

Thanks for following up with this!

I put comments about mostly minor things. but some of it does require changing a bunch of code so I tried to put suggestions those.

Could you also please add a couple checks in tests that return a positive mach for the rule? Alternatively, add some basic dedicated tests for that.
If it can be any help, the way I'd do it is update the tests to not assert anything. make the function print the rule before returning. and add an Expect(false) at the end so ginkgo prints the output and you can copy past the rules instead of typing them out.
Some kind of snapshot testing would be nice here, but that's more work and we don't have anything like that already.

And just to be explicit about this: nothing is set in stone feel free to push back on anything :)

cache/stringcache/in_memory_grouped_cache.go

ThinkChaos · 2024-05-21T22:34:46Z

cache/stringcache/string_caches.go

@@ -11,7 +11,7 @@ import (

 type stringCache interface {
 	elementCount() int
-	contains(searchString string) bool
+	contains(searchString string) (bool, string)


I think it'd be nice to swap the return values so it matches map:

Suggested change

contains(searchString string) (bool, string)

contains(searchString string) (string, bool)

I'll put suggestions for the other uses, hopefully I don't miss anything!

I would not do this with contains method. The name implies that the main return type should be boolean. Consider error return value:

contains(searchString string) (bool, err)

err should be at the end as it is not the main response from method.

Could we rename contains to match and get rid of boolean?

// returns rule if matched, otherwise empty string func match(searchString string) (string)

ThinkChaos · 2024-05-21T22:34:57Z

cache/stringcache/string_caches.go

@@ -36,12 +36,12 @@ func (cache stringMap) elementCount() int {
 	return count
 }

-func (cache stringMap) contains(searchString string) bool {
+func (cache stringMap) contains(searchString string) (bool, string) {


Suggested change

func (cache stringMap) contains(searchString string) (bool, string) {

func (cache stringMap) contains(searchString string) (string, bool) {

ThinkChaos · 2024-05-21T22:35:18Z

cache/stringcache/string_caches.go

 	normalized := normalizeEntry(searchString)
 	searchLen := len(normalized)

 	if searchLen == 0 {
-		return false
+		return false, ""


Suggested change

return false, ""

return "", false

ThinkChaos · 2024-05-21T22:36:05Z

cache/stringcache/string_caches.go

@@ -54,11 +54,11 @@ func (cache stringMap) contains(searchString string) bool {
 		if blockRule == normalized {
 			log.PrefixedLog("string_map").Debugf("block rule '%s' matched with '%s'", blockRule, searchString)

-			return true
+			return true, blockRule


Suggested change

return true, blockRule

return blockRule, true

ThinkChaos · 2024-05-21T23:27:06Z

resolver/blocking_resolver.go

-		if groups := r.matches(groupsToCheck, r.allowlistMatcher, domain); len(groups) > 0 {
-			logger.WithField("groups", groups).Debugf("domain is allowlisted")
+		if matches := r.matches(groupsToCheck, r.allowlistMatcher, domain); len(matches) > 0 {
+			logger.WithField("groups", maps.Keys(matches)).Debugf("domain is allowlisted")


I think we can change this to log all the info:

Suggested change

logger.WithField("groups", maps.Keys(matches)).Debugf("domain is allowlisted")

logger.WithField("matches", matches).Debug("domain is allowlisted")

ThinkChaos · 2024-05-21T23:29:01Z

resolver/blocking_resolver.go

-					return r.handleBlocked(logger, request, request.Req.Question[0], fmt.Sprintf("BLOCKED %s (%s)", tName,
-						strings.Join(groups, ",")))
+				if matches := r.matches(groupsToCheck, r.allowlistMatcher, entryToCheck); len(matches) > 0 {
+					logger.WithField("groups", maps.Keys(matches)).Debugf("%s is allowlisted", tName)


Suggested change

logger.WithField("groups", maps.Keys(matches)).Debugf("%s is allowlisted", tName)

logger.WithField("matches", matches).Debug("%s is allowlisted", tName)

ThinkChaos · 2024-05-21T23:37:05Z

resolver/blocking_resolver.go

+			var entries []string
+			for group, rule := range matches {
+				entries = append(entries, strings.Join([]string{group, rule}, ":"))
+			}
+
+			msg := fmt.Sprintf("BLOCKED (%s)", strings.Join(entries, ", "))
+			resp, err := r.handleBlocked(logger, request, question, msg)


Can you pass matches to handleBlock instead of msg so we can move the formatting there and deduplicate it with Resolve?

resolver/blocking_resolver.go:383 passes a string:

resp, err := r.handleBlocked(logger, request, question, "BLOCKED (ALLOWLIST ONLY)")

Could we return custom type from r.matches(...) and override String() method?

ThinkChaos · 2024-05-21T23:37:51Z

resolver/blocking_resolver.go

+					var entries []string
+					for group, rule := range matches {
+						entries = append(entries, strings.Join([]string{group, rule}, ":"))
+					}
+
+					msg := fmt.Sprintf("BLOCKED %s (%s)", tName, strings.Join(entries, ", "))
+
+					return r.handleBlocked(logger, request, request.Req.Question[0], msg)


Also pass maches to handleBlocked here.

resolver/blocking_resolver.go

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

codecov · 2024-05-25T19:51:21Z

Codecov Report

Attention: Patch coverage is 98.33333% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 94.28%. Comparing base (fe84ab8) to head (9e59fab).
Report is 77 commits behind head on main.

Files	Patch %	Lines
resolver/blocking_resolver.go	94.44%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1489      +/-   ##
==========================================
+ Coverage   93.88%   94.28%   +0.39%     
==========================================
  Files          78       78              
  Lines        6361     5018    -1343     
==========================================
- Hits         5972     4731    -1241     
+ Misses        300      196     -104     
- Partials       89       91       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zc-devs · 2024-05-25T19:56:21Z

Could you also please add a couple checks in tests ...
... copy past the rules instead of typing them out.

Sorry, I don't understand what you want. There are checks for the rules in blocking_resolver_test.go. And I've copy-pasted tests, not typed manually. Could you provide an example?

zc-devs added 2 commits May 21, 2024 15:37

log blocking rule in resolver

5dcb29c

linter

ad89678

ThinkChaos reviewed May 21, 2024

View reviewed changes

zc-devs and others added 5 commits May 23, 2024 17:29

pre-alloc

638528b

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

allocating

12e8877

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

nil

a0833fc

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

reworked trie

71822d2

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

Merge branch 'main' into 1458-improve-logging-2

abde77d

linter

9e59fab

0xERR0R added this to the v0.25 milestone May 26, 2024

0xERR0R assigned ThinkChaos May 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log blocking rule in resolver #1489

Log blocking rule in resolver #1489

zc-devs commented May 21, 2024

ThinkChaos left a comment

ThinkChaos May 21, 2024

zc-devs May 23, 2024

ThinkChaos May 21, 2024

ThinkChaos May 21, 2024

ThinkChaos May 21, 2024

ThinkChaos May 21, 2024

ThinkChaos May 21, 2024

ThinkChaos May 21, 2024

zc-devs May 23, 2024

ThinkChaos May 21, 2024

codecov bot commented May 25, 2024 •

edited

Loading

zc-devs commented May 25, 2024

	contains(searchString string) (bool, string)
	contains(searchString string) (string, bool)

	func (cache stringMap) contains(searchString string) (bool, string) {
	func (cache stringMap) contains(searchString string) (string, bool) {

	logger.WithField("groups", maps.Keys(matches)).Debugf("domain is allowlisted")
	logger.WithField("matches", matches).Debug("domain is allowlisted")

Log blocking rule in resolver #1489

Are you sure you want to change the base?

Log blocking rule in resolver #1489

Conversation

zc-devs commented May 21, 2024

ThinkChaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 25, 2024 • edited Loading

Codecov Report

zc-devs commented May 25, 2024

codecov bot commented May 25, 2024 •

edited

Loading