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.

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

trace-obfuscation: Add is_card_number func to identify credit cards #117

Merged
merged 8 commits into from
Mar 17, 2023

Conversation

ajgajg1134
Copy link
Contributor

What does this PR do?

A brief description of the change being made with this pull request.

Motivation

As part of R&D week I've been playing with libdatadog and FFI in go, I realized some of the obfuscation the agent does was missing from libdatadog so I decided to try and port the datadog-agent's "IsCreditCard" function. For now this won't be used, but can serve as a starting point for additional obfuscation in libdatadog

Additional Notes

My rust is a bit... rusty... 😎 . So please make sure what I'm doing is idiomatic and I'm not making any silly mistakes / missing any good refactors here. The original go code it pretty tough to read and I believe this new implementation is far more readable, however I avoided changing the design too much to reduce the likelihood I introduce any bugs in the port.

How to test the change?

All the unit tests from datadog-agent were brought over (And a few new tests for my own benefit were added)

@ajgajg1134 ajgajg1134 requested a review from a team March 2, 2023 20:59
@ajgajg1134 ajgajg1134 requested a review from a team as a code owner March 2, 2023 20:59
@pawelchcki
Copy link
Contributor

Overall looks good to me.
I'd suggest setting up a criterion benchmark for is_card_number too - just so we can track the 🚤 in the future

@ajgajg1134
Copy link
Contributor Author

ajgajg1134 commented Mar 3, 2023

Overall looks good to me. I'd suggest setting up a criterion benchmark for is_card_number too - just so we can track the 🚤 in the future

I added some criterion benchmarks which are hopefully about what you expected? I've never written criterion benchmarks before but these seemed to be "good enough". Let me know if I shouldn't be using a slice of data and grouping and just do a single test of 1 valid credit card.

Also I'm not sure if just adding an extra [[bench]] section to the cargo toml was the right way to add this as an extra file 🤔

@thedavl
Copy link
Contributor

thedavl commented Mar 3, 2023

you're going to want to add the path to your new bench file here to fix that alpine docker build CI issue!

@ajgajg1134 ajgajg1134 requested a review from a team as a code owner March 6, 2023 14:50
Copy link
Contributor

@pawelchcki pawelchcki left a comment

Choose a reason for hiding this comment

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

One nit from me and one important comment from Levi.

Otherwise it looks good to me.

Thanks @ajgajg1134 🙇

trace-obfuscation/src/credit_cards.rs Outdated Show resolved Hide resolved
@ajgajg1134 ajgajg1134 force-pushed the andrew.glaude/AddCreditCardChecker branch from 1fdf28e to c49a51e Compare March 9, 2023 15:06
@r1viollet
Copy link
Contributor

Should we merge this one ?

@morrisonlevi
Copy link
Contributor

@r1viollet, I think it's waiting on a review from the serverless team, since it's part of a folder they own.

Copy link
Contributor

@thedavl thedavl left a comment

Choose a reason for hiding this comment

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

Aside from the clippy errors, everything looks good! Just fix those before merging

@ajgajg1134
Copy link
Contributor Author

Aside from the clippy errors, everything looks good! Just fix those before merging

So it looks like those clippy failures are outside this PR? They seem to be in the trace-normalization code which isn't touched here? Do you still want me to fix those in this PR?

@thedavl
Copy link
Contributor

thedavl commented Mar 17, 2023

Aside from the clippy errors, everything looks good! Just fix those before merging

So it looks like those clippy failures are outside this PR? They seem to be in the trace-normalization code which isn't touched here? Do you still want me to fix those in this PR?

Ah, sorry. I missed what files it was failing on. Must have been some clippy change, as it was green before.

That's fine then, you're good to merge!

@ajgajg1134
Copy link
Contributor Author

Aside from the clippy errors, everything looks good! Just fix those before merging

So it looks like those clippy failures are outside this PR? They seem to be in the trace-normalization code which isn't touched here? Do you still want me to fix those in this PR?

Ah, sorry. I missed what files it was failing on. Must have been some clippy change, as it was green before.

That's fine then, you're good to merge!

Sweet! I don't have permissions to merge so hopefully someone else can do so :)

@ivoanjo
Copy link
Member

ivoanjo commented Mar 17, 2023

Thanks for the great contribution y'all, pressing the magic button!

@ivoanjo ivoanjo merged commit 5c92dee into main Mar 17, 2023
@bantonsson bantonsson deleted the andrew.glaude/AddCreditCardChecker branch March 7, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants