Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461
GH-38460: [Java][FlightRPC] Add mTLS support for Flight SQL JDBC driver #38461
Changes from 15 commits
477e1c4
fa52fdc
6cfbe98
a41d551
c45c243
cc56b64
39ffc76
d852653
6ff25f6
953f5cd
7a04483
e9410ce
8e46cb9
40d0589
9697e50
1975786
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it be necessary to provide an optional client key password argument if this client private key is password-protected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello – it could be added, but the Java Flight client doesn’t yet support that to my knowledge – so I just wanted to catch up to that for now. We can add it in a later PR, unless you feel strongly that it should be included here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user calls this method repeatedly we will leak File handles. Need to close the current one if it exists. Perhaps it would be better to defer opening the physical file until starting the connection, though that delays notification if there's a problem opening it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jduo - I'm not a very good Java developer - but I'm learning (thanks for your patience). Would it be like this?
Thanks for your help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes this would work. However I'm leaning towards deferring opening mTlsCACert until the connection is made. It's odd for builders to have more logic than setting fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jduo just for my knowledge and to clarify a point, would it be more easier if we use try-with-resources here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hi @jduo - this is the FlightServer - I'd rather not change the overall design of the current class's builder. It already uses this approach for
useTls
- and I think my change is consistent with that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vibhatha , try-with-resources wouldn't help here since we need the stream to have a longer lifetime than this builder function. This is more of a case of tracking ownership.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks @jduo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jduo - I've added small changes that detect if the InputStream attributes are not null, and if so - closes them. Could you please re-review?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @prmoore77 minor comment about another possible error scenario in useTls() that applies here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as above about this potentially leaking a file. Also, now it is ambiguous whether the Flight client should close() mTlsCACert (presumably it should but there should be a comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mirrored the existing
useTls
method hereDoes that code have the issue as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. It looks to have this issue too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vibhatha @davisusanibar can you file an issue to fix this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lidavidm I will file one. Will study a bit and file one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that this issue will be filed separately - is it acceptable to approve/merge this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vibhatha @davisusanibar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #38586