Skip to content

[HUDI-4991] Allow kafka-like configs to set truststore and keystore for the SchemaProvider#7576

Merged
nsivabalan merged 1 commit intoapache:masterfrom
jonvex:fix_deltastreamer_ssl
Jan 25, 2023
Merged

[HUDI-4991] Allow kafka-like configs to set truststore and keystore for the SchemaProvider#7576
nsivabalan merged 1 commit intoapache:masterfrom
jonvex:fix_deltastreamer_ssl

Conversation

@jonvex
Copy link
Contributor

@jonvex jonvex commented Dec 28, 2022

Change Logs

Update the SchemaRegistryProvider to take in kafka-like configs for truststore and keystore.

Testing:
To test this, I modified the examples from Oracle's Sample Code Illustrating a Secure Socket Connection Between a Client and a Server to work similarly to the SchemaRegistryProvider code. To set up the client and server truststore and keystore, I followed the guide from this comment. Note that you should make common name 'localhost'. In step 4, the password that goes in the command line parameter is the PEM pass phrase. Additionally in steps 7 and 8 you are setting the truststore passwords even though you will be prompted to set a keystore password.
ClassFileServer.java
ClassServer.java
SSLSocketClient.java

Impact

User requested this feature in this issue

Risk level (write none, low medium or high below)

medium
The testing above was done to make sure this change will work

Documentation Update

N/A

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@jonvex jonvex changed the title attempt at ssl implementation [HUDI-4991] Allow kafka-like configs to set truststore and keystore for the SchemaProvider Jan 2, 2023
@jonvex jonvex marked this pull request as ready for review January 2, 2023 22:54
Copy link
Contributor

@nsivabalan nsivabalan left a comment

Choose a reason for hiding this comment

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

LGTM.
@umehrot2 @CTTY : Can you folks review the patch.

@nsivabalan nsivabalan added the priority:blocker Production down; release blocker label Jan 6, 2023
@nsivabalan nsivabalan self-assigned this Jan 6, 2023
@nsivabalan
Copy link
Contributor

Can you check CI failures.

Copy link
Contributor

@CTTY CTTY left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! This LGTM in general, left some minor comments.

@jonvex jonvex requested review from CTTY and nsivabalan and removed request for CTTY and nsivabalan January 17, 2023 15:20
}
sslSocketFactory = sslContextBuilder.build().getSocketFactory();
} catch (UnrecoverableKeyException | IOException | KeyStoreException | NoSuchAlgorithmException | CertificateException | KeyManagementException e) {
throw new RuntimeException(e);
Copy link
Member

Choose a reason for hiding this comment

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

Should we wrap in HoodieIOException instead of RuntimeException, after all it's file i/o?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@jonvex jonvex force-pushed the fix_deltastreamer_ssl branch from 777e992 to 0a0d47d Compare January 24, 2023 22:55
@jonvex jonvex requested review from codope and removed request for nsivabalan January 24, 2023 22:59
@hudi-bot
Copy link
Collaborator

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@nsivabalan
Copy link
Contributor

CI is green
Screen Shot 2023-01-24 at 11 54 45 PM

@nsivabalan nsivabalan merged commit f49b5d3 into apache:master Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:blocker Production down; release blocker

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants