Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
IP scrubber #2990
IP scrubber #2990
Changes from 16 commits
73cc1fe
449f53b
0f3dd97
8a0179b
6e71825
56ecfa5
89cc995
f3aef01
3d47811
7a030e3
5020713
9e19dd8
0f3c0ee
c48c25a
0858cf3
a6c4e18
82214f0
cd709cc
1332bd0
83fbd9d
a40290f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a reasonable way to add a test that throws this error? Or is it challenging with this Validate function?
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.
Yes, added!
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.
Nitpick: Could simplify the syntax...
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.
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.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.
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:
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
.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.
And also tested this update end to end including test with publisher account id.
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.
For my understanding, why is this converted to remove its pointer?
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 changed it to non-pointer because we no have a new
IPMasking
field inside of the Privacy: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:
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 want to confirm that we want to keep this commented code out here?
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.
IMHO better to remove the comment and add as a note to the associated issue.
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.
Nitpick: Let's use underscores to be consistent with the rest of this object:
anon-keep-bits
->anon_keep_bits
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.
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
?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.
Since this is a pointer function receiver, you need to handle the
nil
case.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.
IPv6 and IPv4 are not pointers in AccountPrivacy:
technically they will be always initialized. Should I add a nil check anyways?