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.

Sign up for GitHub

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

Jump to bottom

feat: move obfuscation and redaction to core (with specs) #3679

Merged
merged 0 commits into from
Feb 1, 2024
Merged

feat: move obfuscation and redaction to core (with specs) #3679

merged 0 commits into from
Feb 1, 2024

Conversation

xiangce
Copy link
Contributor

@xiangce xiangce commented Feb 16, 2023 •
edited
Loading

  • move the obfuscation/redaction from client to core:
    insights.core.spec_cleaner.Cleaner
  • add new "no_obfuscate" list option to the RegistryPoint,
    so the Cleaner could obfuscate lines according to the list
  • move file content redaction into Cleaner as well,
    and redact line by line instead of the whole file
  • add the obfuscated hostname and keyword information
    to rhsm facts in addition to IP
  • also support the legacy collection
  • refactor the tests correspondingly

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

Move the Obfuscation&Redaction from the client side to the core side.
For more details, see INSGHTCORE-184

@xiangce
Copy link
Contributor Author

xiangce commented Feb 16, 2023 •
edited
Loading

FYI, I will update all the other specs defined in the "insights/specs/__init__.py" soon after this migration got approved.

@subpop subpop requested a review from Glutexo February 17, 2023 14:45
@xiangce xiangce force-pushed the spec_obfuscate branch 5 times, most recently from f5370c5 to fb5db99 Compare March 13, 2023 09:50
@xiangce
Copy link
Contributor Author

xiangce commented Mar 21, 2023

test me

3 similar comments
@xiangce
Copy link
Contributor Author

xiangce commented Mar 21, 2023

test me

@xiangce
Copy link
Contributor Author

xiangce commented Mar 21, 2023

test me

@xiangce
Copy link
Contributor Author

xiangce commented Mar 21, 2023

test me

@xiangce
Copy link
Contributor Author

xiangce commented Mar 27, 2023

test me

2 similar comments
@xiangce
Copy link
Contributor Author

xiangce commented Mar 28, 2023

test me

@xiangce
Copy link
Contributor Author

xiangce commented Mar 28, 2023

test me

@xiangce xiangce force-pushed the spec_obfuscate branch 2 times, most recently from 17358e9 to 07a4ea3 Compare April 3, 2023 13:00
bfahr
bfahr requested changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@bfahr bfahr left a comment

Choose a reason for hiding this comment

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

@xiangce I don't know enough about the client to assess that code but generally, other than my comments, it looks good.

insights/collect.py Outdated Show resolved Hide resolved
insights/specs/__init__.py Outdated Show resolved Hide resolved
@subpop
Copy link
Collaborator

subpop commented Apr 6, 2023

Before we merge this, I would like to get some client QE to test an egg built with this code. How confident are we that this is a non-breaking change? Does this make any changes to the API boundary between core and client?

@xiangce
Copy link
Contributor Author

xiangce commented Apr 6, 2023

Before completing the modification to the insights/specs/default.py (adding the obfuscate option to the required specs), I think some of the existing tests of the IQE will be broken by this PR, it's expected (e.g. if we have test cases to the obfuscating result of some specified spec, but they were not updated yet in the default.py), I will do the modification asap after the peer review on the list the specs.

@xiangce xiangce mentioned this pull request Apr 13, 2023
Merged
3 tasks
@xiangce xiangce force-pushed the spec_obfuscate branch 3 times, most recently from dfc84da to 4a705fd Compare April 17, 2023 08:02
@xiangce xiangce force-pushed the spec_obfuscate branch 4 times, most recently from a68a762 to 08e39a2 Compare October 30, 2023 04:01
@xiangce
Copy link
Contributor Author

xiangce commented Nov 2, 2023

test me

@xiangce xiangce mentioned this pull request Nov 2, 2023
Closed
@xiangce xiangce force-pushed the spec_obfuscate branch 7 times, most recently from f763d1a to a2d5254 Compare November 10, 2023 05:34
@xiangce xiangce mentioned this pull request Nov 13, 2023
Closed
3 tasks
@xiangce xiangce force-pushed the spec_obfuscate branch 2 times, most recently from 52c62eb to 66f9e90 Compare November 30, 2023 13:59
@xiangce xiangce force-pushed the spec_obfuscate branch 3 times, most recently from d20948e to 1955264 Compare February 1, 2024 02:45
@xiangce xiangce merged commit e73628d into master Feb 1, 2024
7 of 11 checks passed
@xiangce xiangce deleted the spec_obfuscate branch February 1, 2024 05:11
xiangce added a commit that referenced this pull request Feb 1, 2024
@xiangce
feat: move obfuscation and redaction to core (with specs) (#3679)
e1a5b08 
* feat: move obfuscation and redaction to core (with specs)

- move the obfuscation/redaction from client to core:
- add "no_obfuscate" list to RegistryPoint ([] by default),
   the `Cleaner` could obfuscate lines accordingly
- move redaction into Cleaner as well,
   and redact line by line instead of the whole file
- add the obfuscated hostname and keywords information
   to rhsm facts in addition to IP
- also support the legacy collection
- refactor the tests correspondingly

Signed-off-by: Xiangce Liu <xiangceliu@redhat.com>

* Misc updates according to new updates

- add option "no_redact" to RegisterPoint for redaction,
  "no_redact=False" by default
- move processing of keywords (replacement) to _redact_line
  See https://access.redhat.com/documentation/en-us/red_hat_insights/2023/html/client_configuration_guide_for_red_hat_insights/con-insights-client-data-redaction_insights-cg-data-redaction#proc-insights-client-cg-redact-pattern-keyword-yaml_insights-cg-data-redaction
- process patterns and keywords in spec_cleaner for content-redaction,
   file-redcation is not processed in spec_cleaner
- update relevant tests

Signed-off-by: Xiangce Liu <xiangceliu@redhat.com>

* fix bugs and add tests for the spec_cleaner reports

- and move the test of spec_cleaner to insights/tests/core
- always generate the rhsm.facts per QE feedback
- fix the archive name in case hostname=localhost

* adjust the warning messages to the console per feedback from QE

Signed-off-by: Xiangce Liu <xiangceliu@redhat.com>
(cherry picked from commit e73628d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryan-blakley ryan-blakley ryan-blakley left review comments

@bfahr bfahr bfahr approved these changes

@subpop subpop Awaiting requested review from subpop

@chenlizhong chenlizhong Awaiting requested review from chenlizhong

@Glutexo Glutexo Awaiting requested review from Glutexo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xiangce @subpop @ryan-blakley @bfahr