Skip to content

Fix Java & Go SDK TLS support#986

Merged
feast-ci-bot merged 7 commits intofeast-dev:masterfrom
mrzzy:fix-java-go-sdk-tls
Sep 7, 2020
Merged

Fix Java & Go SDK TLS support#986
feast-ci-bot merged 7 commits intofeast-dev:masterfrom
mrzzy:fix-java-go-sdk-tls

Conversation

@mrzzy
Copy link
Copy Markdown
Collaborator

@mrzzy mrzzy commented Sep 7, 2020 •
edited
Loading

What this PR does / why we need it:
Follow up of #971:

  • implements missing TLS support for Java SDK.
  • Fixes Go SDK issue where it does not use system certificates if TLS enabled but no certificate path is specified.
  • Rename Go and Java SDK secure client creation methods to include "secure" instead of "authenticated"
    • Go SDK: NewAuthGrpcClient() -> NewSecureGrpcClient()
    • Java SDK: createdAuthenticated() -> createSecure()

Continuation of #504

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

implements TLS support for Java SDK.
 Rename Go and Java SDK secure client creation methods to include "secure" instead of "authenticated"
     - Go SDK: `NewAuthGrpcClient()` -> `NewSecureGrpcClient()`  
     - Java SDK: `createdAuthenticated()` -> `createSecure()`

@pyalex
Copy link
Copy Markdown
Collaborator

pyalex commented Sep 7, 2020

/lgtm

@pyalex
Copy link
Copy Markdown
Collaborator

pyalex commented Sep 7, 2020

/retest

woop
woop reviewed Sep 7, 2020
View reviewed changes
sdk/go/client.go Outdated
Copy link
Copy Markdown
Member

@woop woop Sep 7, 2020

Choose a reason for hiding this comment

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

Should this maybe be an else? the predicate is redundant.

Copy link
Copy Markdown
Collaborator Author

@mrzzy mrzzy Sep 7, 2020

Choose a reason for hiding this comment

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

Updated.

woop
woop reviewed Sep 7, 2020
View reviewed changes
sdk/java/src/main/java/com/gojek/feast/FeastClient.java Outdated
Copy link
Copy Markdown
Member

@woop woop Sep 7, 2020

Choose a reason for hiding this comment

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

Is there a reason to rethrow without any attached message?

Copy link
Copy Markdown
Collaborator Author

@mrzzy mrzzy Sep 7, 2020

Choose a reason for hiding this comment

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

Updated createSecure() to throw IllegalArgumentException when provided with bad certificate.

  • Removed redundant try catch in create()

@pyalex
Copy link
Copy Markdown
Collaborator

pyalex commented Sep 7, 2020

/hold

@feast-ci-bot
Copy link
Copy Markdown
Collaborator

feast-ci-bot commented Sep 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrzzy, pyalex, woop

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@woop
Copy link
Copy Markdown
Member

woop commented Sep 7, 2020

/lgtm

@mrzzy
Copy link
Copy Markdown
Collaborator Author

mrzzy commented Sep 7, 2020

/unhold

@feast-ci-bot feast-ci-bot merged commit 1156db4 into feast-dev:master Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pyalex pyalex pyalex approved these changes

@woop woop woop approved these changes

@davidheryanto davidheryanto Awaiting requested review from davidheryanto

@khorshuheng khorshuheng Awaiting requested review from khorshuheng

@zhilingc zhilingc Awaiting requested review from zhilingc

Assignees

@pyalex pyalex

@woop woop

Labels

approved area/sdks kind/bug lgtm size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mrzzy @pyalex @feast-ci-bot @woop