feat: BI-6829 dl obfuscator base package #1429
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a new base obfuscator package (dl_obfuscator) that provides functionality for obfuscating sensitive data like secrets and query parameters in various contexts (logs, Sentry, tracing, usage tracking, and inspector).
Key Changes:
- New
dl_obfuscatorpackage with core obfuscation engine and secret keeper registry - Support for context-aware obfuscation (different rules for logs vs. inspector)
- Extensible architecture with base obfuscator class for custom implementations
Reviewed changes
Copilot reviewed 13 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| metapkg/pyproject.toml | Adds dl-obfuscator to the metapackage dependencies |
| lib/dl_obfuscator/pyproject.toml | Package configuration defining dependencies and test settings |
| lib/dl_obfuscator/dl_obfuscator/secret_keeper.py | Registry for storing secrets and query parameters to be obfuscated |
| lib/dl_obfuscator/dl_obfuscator/engine.py | Core obfuscation engine with text and dict processing methods |
| lib/dl_obfuscator/dl_obfuscator/context.py | Enum defining obfuscation contexts (logs, sentry, tracing, etc.) |
| lib/dl_obfuscator/dl_obfuscator/obfuscators/base.py | Abstract base class for custom obfuscators |
| lib/dl_obfuscator/dl_obfuscator/init.py | Package initialization with public API exports |
| lib/dl_obfuscator/dl_obfuscator_tests/unit/test_secret_keeper.py | Unit tests for SecretKeeper functionality |
| lib/dl_obfuscator/dl_obfuscator_tests/unit/test_engine.py | Unit tests for ObfuscationEngine behavior |
| lib/dl_obfuscator/dl_obfuscator_tests/unit/conftest.py | Pytest fixtures for tests |
| lib/dl_obfuscator/README.md | Basic package documentation |
| lib/dl_obfuscator/LICENSE | Apache 2.0 license file |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/dl_obfuscator/dl_obfuscator_tests/unit/test_secret_keeper.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 17 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _obfuscators: list[BaseObfuscator] = attr.ib(factory=list, init=False) | ||
|
|
||
| def __init__(self, secret_keeper: SecretKeeper, obfuscators: list[BaseObfuscator] | None = None): | ||
| self.secret_keeper = secret_keeper | ||
| self._obfuscators = obfuscators or [] |
There was a problem hiding this comment.
The class uses @attr.s decorator but defines a custom __init__ method that bypasses attrs' generated initialization. This conflicts with the attr.ib() field definitions on lines 16-17. Either remove the @attr.s decorator and field definitions, or remove the custom __init__ and use attrs' initialization pattern with factory functions.
| _obfuscators: list[BaseObfuscator] = attr.ib(factory=list, init=False) | |
| def __init__(self, secret_keeper: SecretKeeper, obfuscators: list[BaseObfuscator] | None = None): | |
| self.secret_keeper = secret_keeper | |
| self._obfuscators = obfuscators or [] | |
| obfuscators: list[BaseObfuscator] | None = attr.ib(default=None, repr=False) | |
| _obfuscators: list[BaseObfuscator] = attr.ib(init=False, repr=False) | |
| def __attrs_post_init__(self) -> None: | |
| # Mirror the original __init__ behavior: if obfuscators is None, use an empty list. | |
| self._obfuscators = self.obfuscators or [] |
| # Core components | ||
| "SecretKeeper", | ||
| "ObfuscationContext", | ||
| "ObfuscationEngine", | ||
| # Obfuscators |
There was a problem hiding this comment.
let's not add comments in all block, it will be autosorted later on
| @@ -0,0 +1,13 @@ | |||
| import abc | |||
|
|
|||
| from ..context import ObfuscationContext | |||
There was a problem hiding this comment.
relative import ourside __init__.py is a bad idea by default
| def apply_replacement(text: str, secret_items: tuple[str, str]) -> str: | ||
| secret, replacement = secret_items | ||
| escaped_value = re.escape(secret) | ||
| pattern = rf"\b{escaped_value}\b" | ||
| replacement = replacement or "hidden" | ||
| replacement = f"***{replacement}***" | ||
| return re.sub(pattern, replacement, text) |
| if context != ObfuscationContext.INSPECTOR: | ||
| secrets.extend(self.secret_keeper.get_params().items()) | ||
|
|
||
| result = reduce(apply_replacement, secrets, text) |
There was a problem hiding this comment.
let's unwrap, reduce is not the most readable way to define things
| else: | ||
| result[key] = value |
There was a problem hiding this comment.
else not obfuscating? sounds wrong
| elif isinstance(data, dict): | ||
| return self.obfuscate_dict(data, context) | ||
| else: | ||
| return data |
There was a problem hiding this comment.
else not obfuscating? sounds wrong
| _secrets: dict[str, str] = attr.ib(factory=dict, init=False, repr=False) | ||
| _params: dict[str, str] = attr.ib(factory=dict, init=False, repr=False) |
| pattern = rf"\b{escaped_value}\b" | ||
| replacement = replacement or "hidden" | ||
| replacement = f"***{replacement}***" | ||
| return re.sub(pattern, replacement, text) |
There was a problem hiding this comment.
using re.sub for string replacement is overkill, str.replace should be faster
No description provided.