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

Add ability to mask object key prefixes in logs #402

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

ivanyu
Copy link
Contributor

@ivanyu ivanyu commented Sep 27, 2023

See individual commits for details.

@ivanyu ivanyu force-pushed the ivanyu/mask-object-key branch 5 times, most recently from 027e2ef to 5e01385 Compare September 28, 2023 07:08

import static org.assertj.core.api.Assertions.assertThat;

class ObjectKeyFactoryTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed from ObjectKeyTest.java + prefixMasking* tests added.


import static org.assertj.core.api.Assertions.assertThat;

class ObjectKeyTest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed into ObjectKeyFactoryTest

@ivanyu ivanyu force-pushed the ivanyu/mask-object-key branch from 5e01385 to b024730 Compare September 28, 2023 07:22
Comment on lines -67 to +68
throw new KeyNotFoundException(this, key, e);
throw new KeyNotFoundException(this, key);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to satisfy the new tests for masking in BaseStorageTest. I don't think we that badly need this inner exception in the file system storage implementation to make the test more complex, so I just decided to remove it. The same below.

return "masked-key";
}
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deletions don't throw exceptions when keys don't exist => not tested here.

integrationTestImplementation "org.wiremock:wiremock:$wireMockVersion"

integrationTestImplementation("org.wiremock:wiremock:$wireMockVersion") {
exclude group: "org.slf4j"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It screws up logging in s3 tests otherwise. Doing in this PR because I needed it for visual validation of masking.

@ivanyu ivanyu marked this pull request as ready for review September 28, 2023 08:22
@ivanyu ivanyu requested a review from a team as a code owner September 28, 2023 08:22
AnatolyPopov
AnatolyPopov previously approved these changes Sep 28, 2023
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

LGTM, apart from some nits.

@AnatolyPopov AnatolyPopov merged commit e6f322a into main Sep 29, 2023
6 checks passed
@AnatolyPopov AnatolyPopov deleted the ivanyu/mask-object-key branch September 29, 2023 06:56
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.

3 participants