Skip to content

Modify LogReader to print just the crypto params#2935

Merged
milleruntime merged 1 commit intoapache:mainfrom
milleruntime:crypto-WALtest
Sep 22, 2022
Merged

Modify LogReader to print just the crypto params#2935
milleruntime merged 1 commit intoapache:mainfrom
milleruntime:crypto-WALtest

Conversation

@milleruntime
Copy link
Copy Markdown
Contributor

@milleruntime milleruntime commented Sep 16, 2022

  • Added the -e option to LogReader to not read the whole WAL and just print the crypto params
  • Make the LogReader options extend ConfigOpts to allow passing in properties and changed the -p option to --regex to differentiate from the properties option
  • Added checkWALEncryption() to PerTableCryptoIT that calls the LogReader
  • Rename a test in CryptoTest
  • Supports Per Table Crypto Follow On Tasks #2930

* Added the -e option to LogReader to not read the whole WAL and just
print the crypto params
* Added a new check to PerTableCryptoIT that calls the LogReader to
check for encryption present in the WALs
* Also make the LogReader options extend ConfigOpts to allow passing in
properties
* Rename a test in CryptoTest
try {
input.readFully(magicBuffer);
if (Arrays.equals(magicBuffer, magic4)) {
byte[] cryptoParams = CryptoUtils.readParams(input);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if you should throw a CryptoException here instead of letting this fall through to a RuntimeException. If you caught the CryptoException in the call to this method, then you could continue processing the next file after logging the error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to throw a CryptoException when we are expecting a file to be encrypted and get an IOException. I am not sure if this is the best place though.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think improvements to the exception handling would be good to look at with the follow on task "Make CryptoException extend GeneralSecurityException" I mentioned in #2930

@milleruntime
Copy link
Copy Markdown
Contributor Author

Full ITs passed in my fork.

@milleruntime milleruntime merged commit 735baa1 into apache:main Sep 22, 2022
@milleruntime milleruntime deleted the crypto-WALtest branch September 22, 2022 16:23
@ctubbsii ctubbsii added this to the 2.1.0 milestone Jul 12, 2024
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