IP scrubber #2990

VeronikaSolovei9 · 2023-07-29T04:30:01Z

Added anonymizeIpv6 function to transmitPreciseGeo activitiy to match PBS-Java implementation

Addressed issue #2981

… PBS-Java implementation

privacy/scrubber.go

privacy/scrubber_test.go

privacy/scrubber.go

…rks both for ipv4 and ipv6

config/account.go

SyntaxNode · 2023-08-01T19:02:38Z

config/account.go

+}
+
+func (ipMasking *IpMasking) Validate(errs []error) []error {
+	errs = ipMasking.IpV6.validate(errs, Ipv6Bits, "ipv6")


You need to handle the case of ipMasking being nil.

Technically ipMasking is never nil here. Should I modify it to a non-pointer?

Yes, please. Let's add some defensive coding since nil is allowed here. Alternatively, you may be able to change this to a value type receiver.

privacy/enforcement.go

SyntaxNode · 2023-08-01T19:03:51Z

privacy/scrubber.go

@@ -76,14 +77,18 @@ type Scrubber interface {
 	ScrubUser(user *openrtb2.User, strategy ScrubStrategyUser, geo ScrubStrategyGeo) *openrtb2.User
 }

-type scrubber struct{}
+type scrubber struct {
+	ipMasking *config.IpMasking


Does this need to be passed as a pointer?

My main idea here was to load these values to a non-pointer config struct, because we always need them. And then pass it as a pointer downstream to avoid copying the struct.
But I can see now why this was not a good idea. I replaced it to non-pointer.

And then pass it as a pointer downstream to avoid copying the struct.

That is't a surefire premature optimization. Go has optimizations for passing arguments as values which may be much more efficient than using a pointer. It's a bit more reliable for large data structures like the BidRequest, but this struct is relatively small.

privacy/scrubber.go

config/account.go

…mask_bits`

AlexBVolcy · 2023-08-04T16:18:59Z

account/account.go

@@ -103,6 +103,11 @@ func GetAccount(ctx context.Context, cfg *config.Configuration, fetcher stored_r
 		return nil, errs
 	}

+	errs = account.Privacy.IPMasking.Validate(errs)
+	if len(errs) > 0 {
+		return nil, errs


Is there a reasonable way to add a test that throws this error? Or is it challenging with this Validate function?

Yes, added!

AlexBVolcy · 2023-08-04T16:20:59Z

config/account_test.go

@@ -910,3 +910,61 @@ func TestAccountPriceFloorsValidate(t *testing.T) {
 		})
 	}
 }
+
+func TestIPMaskingValidate(t *testing.T) {
+


Nitpick: Could you remove this blank line?

AlexBVolcy · 2023-08-04T16:24:08Z

config/account.go

@@ -40,7 +45,7 @@ type Account struct {
 	Validations             Validations                                 `mapstructure:"validations" json:"validations"`
 	DefaultBidLimit         int                                         `mapstructure:"default_bid_limit" json:"default_bid_limit"`
 	BidAdjustments          *openrtb_ext.ExtRequestPrebidBidAdjustments `mapstructure:"bidadjustments" json:"bidadjustments"`
-	Privacy                 *AccountPrivacy                             `mapstructure:"privacy" json:"privacy"`
+	Privacy                 AccountPrivacy                              `mapstructure:"privacy" json:"privacy"`


For my understanding, why is this converted to remove its pointer?

I changed it to non-pointer because we no have a new IPMasking field inside of the Privacy:

type AccountPrivacy struct { AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"` IPMasking IPMasking `mapstructure:"ip_masking" json:"ip_masking"` }

We need to have IPMasking to be always present and to be populated with default values from configs.
PBS host account_defaults configs and/or request.account config can overwrite these values.

This is why config.config.go has these lines:

v.SetDefault("account_defaults.privacy.ip_masking.ipv6.left_mask_bits", 56) v.SetDefault("account_defaults.privacy.ip_masking.ipv4.left_mask_bits", 24)

AlexBVolcy

Looks good! Just left one little nitpick

AlexBVolcy · 2023-08-07T21:53:31Z

privacy/scrubber_test.go

 			assert.Equal(t, test.expected, result, test.description)
 		})
 	}
 }

-func TestScrubIPV4(t *testing.T) {
+func TestScrubIp(t *testing.T) {


Nitpick: Could you capitalize the Ip here so it looks like TestScrubIP

AlexBVolcy

LGTM

AlexBVolcy

Left one comment. Could you also resolve the merge conflicts?

AlexBVolcy · 2023-08-22T18:22:30Z

config/account.go

+	// For the follow up PR, always keep first 64 bits
+	// AlwaysKeepBits int `mapstructure:"always-keep-bits" json:"always-keep-bits"`


Just want to confirm that we want to keep this commented code out here?

IMHO better to remove the comment and add as a note to the associated issue.

# Conflicts: # exchange/utils.go # privacy/enforcer.go

config/account.go

SyntaxNode · 2023-08-23T20:50:53Z

config/account.go

+	// For the follow up PR, always keep first 64 bits
+	// AlwaysKeepBits int `mapstructure:"always-keep-bits" json:"always-keep-bits"`


IMHO better to remove the comment and add as a note to the associated issue.

SyntaxNode · 2023-08-23T20:52:19Z

config/account.go

+	AnonKeepBits int `mapstructure:"anon-keep-bits" json:"anon-keep-bits"`
+}
+
+func (IPv6Config *IPv6) Validate(errs []error) []error {


Variables names should start with lower case characters. It's common to use a single character for a function receiver reference. Consider renaming to simply ip?

SyntaxNode · 2023-08-23T20:52:44Z

config/account.go

+}
+
+func (IPv6Config *IPv6) Validate(errs []error) []error {
+	if IPv6Config.AnonKeepBits > IPv6BitSize || IPv6Config.AnonKeepBits < 0 {


Since this is a pointer function receiver, you need to handle the nil case.

IPv6 and IPv4 are not pointers in AccountPrivacy:

type AccountPrivacy struct { AllowActivities *AllowActivities `mapstructure:"allowactivities" json:"allowactivities"` IPv6Config IPv6 `mapstructure:"ipv6" json:"ipv6"` IPv4Config IPv4 `mapstructure:"ipv4" json:"ipv4"` }

technically they will be always initialized. Should I add a nil check anyways?

config/account_test.go

config/account.go

SyntaxNode · 2023-08-23T20:58:06Z

privacy/scrubber_test.go

@@ -38,12 +39,12 @@ func TestScrubDevice(t *testing.T) {
 				MACMD5:   "",
 				IFA:      "",
 				IP:       "1.2.3.0",
-				IPv6:     "2001:0db8:0000:0000:0000:ff00:0:0",
+				IPv6:     "2001:db8:2233:4400::",


Should this segment still be 0db8?

In this case 0db8 and db8 are identical. Because we use net.ParseIP(ip) and then ipMasked.String() returns it in this form.

https://www.ciscopress.com/articles/article.asp?p=2803866 -> paragraph "Rule 1: Omit Leading 0s"

Gotcha. Maybe just add a the 0 to keep this straight forward?

Ooof, I see. This value is returned by the go net.IP functionality. In order to make this change we will need to compare net.IPs, not strings. This will require some assertions refactoring. We now assert the whole expected: &openrtb2.Device with the result.

We will need to change this(current implementation):

assert.Equal(t, test.expected, result, test.description)

to something like this:

assert.Equal(t, net.ParseIP(test.expected.IP).String(), net.ParseIP(result.IP).String(), test.description) assert.Equal(t, net.ParseIP(test.expected.IPv6).String(), net.ParseIP(result.IPv6).String(), test.description) assert.Equal(t, test.expected.DIDMD5, result.DIDMD5, test.description) assert.Equal(t, test.expected.IFA, result.IFA, test.description) ...

Are we good to go with this format?

How about we just change 0 to 1? My only intention here was to keep the test straight forward and not confuse the reader with the additional dimension of leading 0 optimization.

How about we change to 0 to a 1 to remove this issue entirely?

account/account.go

SyntaxNode · 2023-08-23T21:09:40Z

privacy/scrubber.go

@@ -199,14 +203,12 @@ func (scrubber) ScrubDevice(device *openrtb2.Device, id ScrubStrategyDeviceID, i

 	switch ipv4 {
 	case ScrubStrategyIPV4Lowest8:


Why do we still need ScrubStrategyIPV4Lowest8?

We need this to indicate we want to scrub IPv4. There are just 2 options:

const ( // ScrubStrategyIPV4None does not remove any part of an IPV4 address. ScrubStrategyIPV4None ScrubStrategyIPV4 = iota // ScrubStrategyIPV4Lowest8 zeroes out the last 8 bits of an IPV4 address. ScrubStrategyIPV4Lowest8 )

I think we should rename it to something like ScrubStrategyIPV4Default or better.

Ah ok. Yes, renaming would be good. Perhaps ScrubStrategyIPV4Subnet to match IPv6 naming?

# Conflicts: # config/config.go

AlexBVolcy · 2023-08-29T18:35:31Z

account/account.go

+	ipv6Errs = account.Privacy.IPv6Config.Validate(ipv6Errs)
+	if len(ipv6Errs) > 0 {
+		account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6BitSize
+		// record invalid ipv6 metric


I see you commented asking if this can be done in a separate PR, if so can this comment be removed?

AlexBVolcy · 2023-08-29T18:35:44Z

account/account.go

+	ipv4Errs = account.Privacy.IPv4Config.Validate(ipv4Errs)
+	if len(ipv4Errs) > 0 {
+		account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize
+		// record invalid ipv4 metric


Same point about removing metric related comment

SyntaxNode

Looking great. Left a few small nitpicks.

SyntaxNode · 2023-08-31T21:04:08Z

account/account.go

+	ipv4Errs = account.Privacy.IPv4Config.Validate(ipv4Errs)
+	if len(ipv4Errs) > 0 {
+		account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize
+	}


Nitpick: Could simplify the syntax...

if errs := account.Privacy.IPv6Config.Validate(nil); len(errs) > 0 { account.Privacy.IPv6Config.AnonKeepBits = iputil.IPv6BitSize } if errs := account.Privacy.IPv4Config.Validate(nil); len(errs) > 0 { account.Privacy.IPv4Config.AnonKeepBits = iputil.IPv4BitSize }

These lines show as having test coverage, but there doesn't seem to an assertion on setting their default values. I made a type locally where IPv4Config.AnonKeepBits = iputil.IPv6BitSize which is setting the ipv6 value for the ipv4 default and this mistake wasn't caught by a test.

Yes, this is tested in a general GetAccount test.
I added extra flag to assert IP config for account id "invalid_acct_ipv6_ipv4".
I also realized I set both ipv6 and ipv4 to max value, instead of default mask bits. I added 2 new constants:

IPv4DefaultMaskingBitSize = 24 IPv6DefaultMaskingBitSize = 56

For the simplified syntax I added new variables for errors to show explicitly we don't want to add IP config validation errors to errs.

And also tested this update end to end including test with publisher account id.

SyntaxNode · 2023-08-31T21:07:38Z

config/account.go

+
+type IPv4 struct {
+	AnonKeepBits int `mapstructure:"anon-keep-bits" json:"anon-keep-bits"`
+}


Nitpick: Let's use underscores to be consistent with the rest of this object: anon-keep-bits -> anon_keep_bits

SyntaxNode · 2023-08-31T21:10:30Z

privacy/scrubber_test.go

@@ -38,12 +39,12 @@ func TestScrubDevice(t *testing.T) {
 				MACMD5:   "",
 				IFA:      "",
 				IP:       "1.2.3.0",
-				IPv6:     "2001:0db8:0000:0000:0000:ff00:0:0",
+				IPv6:     "2001:db8:2233:4400::",


How about we change to 0 to a 1 to remove this issue entirely?

AlexBVolcy

LGTM

Added anonymizeIpv6 function to transmitPreciseGeo activitiy to match…

73cc1fe

… PBS-Java implementation

VeronikaSolovei9 commented Jul 29, 2023

View reviewed changes

privacy/scrubber.go Show resolved Hide resolved

SyntaxNode reviewed Jul 31, 2023

View reviewed changes

privacy/scrubber.go Show resolved Hide resolved

privacy/scrubber_test.go Outdated Show resolved Hide resolved

privacy/scrubber.go Outdated Show resolved Hide resolved

SyntaxNode self-assigned this Jul 31, 2023

VeronikaSolovei9 added 4 commits July 31, 2023 13:10

Rename and unit test refactoring

449f53b

Replaced existing ip scrubbing function with new scrubIp function. Wo…

0f3dd97

…rks both for ipv4 and ipv6

Minor refactoring

8a0179b

Added ipv4 and ipv6 scrubbing config

6e71825

SyntaxNode reviewed Aug 1, 2023

View reviewed changes

VeronikaSolovei9 added 3 commits August 1, 2023 16:06

Pointers refactoring

56ecfa5

Naming refactoring

89cc995

Unit tests

f3aef01

SyntaxNode reviewed Aug 2, 2023

View reviewed changes

config/account.go Outdated Show resolved Hide resolved

config/account.go Outdated Show resolved Hide resolved

config/account.go Show resolved Hide resolved

Naming refactoring

3d47811

hhhjort assigned AlexBVolcy Aug 3, 2023

hhhjort requested a review from AlexBVolcy August 3, 2023 17:45

VeronikaSolovei9 added 2 commits August 3, 2023 14:48

Merge branch 'master' into anonymizeIpv6

7a030e3

refactored IPv6 and IPv4 masking config to have just one value `left_…

5020713

…mask_bits`

AlexBVolcy reviewed Aug 4, 2023

View reviewed changes

Unit tests

9e19dd8

AlexBVolcy reviewed Aug 7, 2023

View reviewed changes

VeronikaSolovei9 changed the title ~~IPv6 scrubber~~ IP scrubber Aug 8, 2023

Test rename

0f3c0ee

AlexBVolcy previously approved these changes Aug 8, 2023

View reviewed changes

Config refactoring, rename left-mask-bits to anon-keep-bits

c48c25a

VeronikaSolovei9 dismissed AlexBVolcy’s stale review via c48c25a August 22, 2023 00:07

AlexBVolcy reviewed Aug 22, 2023

View reviewed changes

VeronikaSolovei9 added 2 commits August 22, 2023 12:22

Merge branch 'master' into anonymizeIpv6

0858cf3

# Conflicts: # exchange/utils.go # privacy/enforcer.go

Merge fixes

a6c4e18

SyntaxNode reviewed Aug 23, 2023

View reviewed changes

VeronikaSolovei9 added 3 commits August 28, 2023 15:26

Merge branch 'master' into anonymizeIpv6

82214f0

# Conflicts: # config/config.go

Refactoring

cd709cc

Refactoring

1332bd0

AlexBVolcy reviewed Aug 29, 2023

View reviewed changes

Constant rename and comments remove

83fbd9d

SyntaxNode reviewed Aug 31, 2023

View reviewed changes

Code refactoring and default IPv6 and IPv4 values set

a40290f

AlexBVolcy approved these changes Sep 1, 2023

View reviewed changes

SyntaxNode approved these changes Sep 5, 2023

View reviewed changes

SyntaxNode added the do not port label Sep 5, 2023

hhhjort merged commit 89f79e3 into master Sep 5, 2023
3 checks passed

SyntaxNode deleted the anonymizeIpv6 branch October 2, 2023 17:53

		// For the follow up PR, always keep first 64 bits
		// AlwaysKeepBits int `mapstructure:"always-keep-bits" json:"always-keep-bits"`

		@@ -199,14 +203,12 @@ func (scrubber) ScrubDevice(device *openrtb2.Device, id ScrubStrategyDeviceID, i

		switch ipv4 {
		case ScrubStrategyIPV4Lowest8:

IP scrubber #2990

IP scrubber #2990

Conversation

VeronikaSolovei9 commented Jul 29, 2023

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

VeronikaSolovei9 Aug 1, 2023 • edited Loading

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

AlexBVolcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

AlexBVolcy 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

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

VeronikaSolovei9 Aug 28, 2023 • edited Loading

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

SyntaxNode 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

AlexBVolcy left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 Aug 1, 2023 •

edited

Loading

VeronikaSolovei9 Aug 28, 2023 •

edited

Loading