Track how many times patterns are obfuscated #65

josefkarasek · 2021-09-16T15:12:20Z

Signed-off-by: Josef Karasek [email protected]

openshift-ci · 2021-09-16T15:12:21Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2021-09-16T15:12:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: josefkarasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [josefkarasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pkg/obfuscator/tracker.go

tjungblu · 2021-09-16T15:38:00Z

pkg/obfuscator/tracker.go

@@ -15,11 +15,11 @@ type ReplacementTracker interface {
 	Initialize(replacements map[string]string)

 	// Report returns a mapping of strings which were replaced.
-	Report() map[string]string
+	Report() map[string]Replacement


here's what I would return instead:

type ReplacementReport struct { replacements []Replacement } type Replacement { original string replaced string occurrences int } // for backwards compatibility in all our tests func (ReplacementReport) asMap() map[string]string { }

that way we can serialize the list in yaml correctly, can properly sort on the struct level and do diffs in the e2e tests.

in the implementation, you can just keep a second map for the counts and on Report() just collate, similar to the defensive copy we do

pkg/obfuscator/mac_address.go

pkg/obfuscator/tracker.go

pkg/obfuscator/keywords.go

pkg/obfuscator/mac_address.go

pkg/obfuscator/keywords_test.go

pkg/obfuscator/tracker.go

pkg/obfuscator/tracker_test.go

pkg/reporting/reporter.go

Signed-off-by: Josef Karasek <[email protected]>

pkg/obfuscator/ip.go

pkg/obfuscator/mac_address.go

Introduction of a hierarchical Report schema with normalized keys. ``` Canonical: "EB:A1:2A:B2:09:BF" Replacement: "x-mac-0000000001-x" Occurrences: - Original: "eb-a1-2a-b2-09-bf" Count: 5 - Original: "eb:a1:2a:b2:09:bf" Count: 15 ``` Normalized keys apply to: * IPv4 addresses - dots `'.'`: `255.255.255.255` * MAC addresses - upper case + `':'`: `29:7E:8C:8A:60:D9`

Occurences of values are tied to the specific format in which they appear in the source. For example: `192-168-1-10` and `192.168.1.10` are the same IP addresses. Just written differently. In the report both will be tied by the same normalized key `192.168.1.10` and each will have its specific number of occurrences

tjungblu · 2021-09-20T15:20:48Z

pkg/obfuscator/domain.go

@@ -48,6 +48,7 @@ func (d *domainObfuscator) replaceDomains(input string) string {
 				replacement = obfuscatedBaseDomain
 			}
 			output = strings.ReplaceAll(output, m[0], replacement)
+			d.ReplacementTracker.AddReplacement(baseDomain, baseDomain, obfuscatedBaseDomain)


shouldn't that be:

d.ReplacementTracker.AddReplacement(baseDomain, m[0], obfuscatedBaseDomain)

?

I thought that too, but the unit tests were failing when i tried it.

fmt.Println(m) [docs.okd.io docs. okd.io] ... expected: map[string]string{"okd.io":"domain0000000002", "openshift.com":"domain0000000001"} actual : map[string]string{"docs.okd.io":"domain0000000002", "openshift.com":"domain0000000001"}

Might it be that the unit tests have different expectations?

the map representation now changed a bit, the report will show the right thing. and there is also the side-effect of the generator changes below.

I reckon the tests here then should use the Report directly instead of the map.

I changed the unit tests to use the actual reports

tjungblu · 2021-09-20T15:21:34Z

pkg/obfuscator/ip.go

@@ -58,12 +58,12 @@ func (o *ipObfuscator) replace(s string) string {
 				continue
 			}

-			cleaned := strings.ReplaceAll(m, "-", ".")
+			cleaned := strings.ToUpper(strings.ReplaceAll(m, "-", "."))


not sure what toUpper does with numbers, what's the rationale to add that?

IPv6 addresses

dang, makes sense. another good reason to split this up. Is there no test case that needs adaptation to this?

I added 2 new test cases

pkg/obfuscator/ip_test.go

pkg/obfuscator/keywords.go

tjungblu · 2021-09-20T15:27:53Z

pkg/obfuscator/report_types.go

+	Count    uint   `yaml:"count,omitempty"`
+}
+
+func (r *Replacement) Increment(original string, count uint) Replacement {


you should definitely add some tests for this method

and why do you return the Replacement?

I mean, you already have a pointer to the replacement, you can just increment the counter directly without returning it.

👍 on tests

I'll review how Replacement is used

also below in the reporting is nothing else like a good old std::multimap. I'm sure there must be a good library for us to reuse?

tjungblu · 2021-09-20T15:36:47Z

pkg/obfuscator/tracker.go

 		return
 	}
-	s.mapping[original] = replacement
+	new := Replacement{Canonical: canonical, ReplacedWith: replacement}
+	new.Increment(original, count)


I'd add a NewReplacement method that would add the increment internally

tjungblu · 2021-09-20T15:37:15Z

pkg/obfuscator/tracker.go

-			klog.Exitf("'%s' already has a value reported as '%s', tried to report '%s'", original, val, replacement)
-		}
+	if r, ok := s.mapping[canonical]; ok {
+		s.mapping[canonical] = r.Increment(original, count)
 		return


that's a bit nitty, but instead of return you can also put the other clause into an else branch

tjungblu · 2021-09-20T15:39:05Z

pkg/obfuscator/tracker.go

-	r := generator()
-	s.mapping[key] = r
-	return r
+	return generator()


don't we have to save that in our mapping? Otherwise we will generate a different identifier for the same canonical key.

also be mindful of race conditions, since everything you return here is outside the lock

I modified AddReplacement so that it takes the canonical key (cleaned version for IP and MAC) along with the original key

AddReplacement(canonical, original string, replacement string)

the reason it was there, is because there can be a race condition where two goroutines find an empty mapping for the canonical string, generator then creates two representations for the same string.

having this here under a single locking was guaranteed that this could never happen. You also removed that exitf that was basically covering that case if it would happen. Put that exitf back and try it out on a couple of MGs, it should happen fairly frequently.

tjungblu · 2021-09-20T15:40:50Z

pkg/obfuscator/tracker.go

-	r := generator()
-	s.mapping[key] = r
-	return r
+	return generator()
 }

 func (s *SimpleTracker) Initialize(replacements map[string]string) {


I'm not sure whether this is currently used, probably not. But that ideally should take your ReplacementReport. I assume that also is touching @sairameshv a bit - fyi.

tjungblu · 2021-09-20T15:43:37Z

pkg/reporting/reporter.go

 	omissions    []string
 }

+var _ Reporter = (*SimpleReporter)(nil)


what does that do? should we typedef this thing?

Compile time check member functions of SimpleReporter implement functions of interface Reporter

mind blown, why are we not returning the Reporter interface in the New func below? that should do the same 👯

Good point. I'll change that.

pair programming

tjungblu · 2021-09-21T15:35:39Z

pkg/obfuscator/tracker.go

@@ -85,12 +87,22 @@ func (s *SimpleTracker) GenerateIfAbsent(canonical string, original string, coun
 	return g
 }

-func (s *SimpleTracker) Initialize(replacements map[string]string) {
+func (s *SimpleTracker) Initialize(report ReplacementReport) {
+	close(onlyOneInit) // panics when called twice


tjungblu · 2021-09-21T15:37:47Z

/label tide/merge-method-squash

tjungblu · 2021-09-21T15:38:25Z

/lgtm

tjungblu · 2021-09-21T15:38:35Z

/hold cancel

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2021

josefkarasek changed the title ~~Track how many times patters was obfuscated~~ Track how many times pattern was obfuscated Sep 16, 2021

josefkarasek changed the title ~~Track how many times pattern was obfuscated~~ Track how many times patterns are obfuscated Sep 16, 2021

openshift-ci bot requested review from ayesha54 and sairameshv September 16, 2021 15:12

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2021

tjungblu reviewed Sep 16, 2021

View reviewed changes

pkg/obfuscator/tracker.go Outdated Show resolved Hide resolved

tjungblu reviewed Sep 16, 2021

View reviewed changes

pkg/obfuscator/mac_address.go Outdated Show resolved Hide resolved