Tristan instacart/add basic ssl support #73
Conversation
|
Investigating test failure |
|
Sorry for the test error, copy paste issue from a monkeypatch version I based the PR on, fixed now. |
lib/temporal/client/grpc_client.rb
Outdated
| @@ -379,7 +379,7 @@ def cancel_polling_request | |||
| def client | |||
| @client ||= Temporal::Api::WorkflowService::V1::WorkflowService::Stub.new( | |||
| url, | |||
| :this_channel_is_insecure, | |||
| Temporal.configuration.grpc_ssl_config, | |||
There was a problem hiding this comment.
Would you mind passing this as the argument (similar to host/port) to the client to avoid this extra dependency?
There was a problem hiding this comment.
Still pull from config but do so here yeah?
temporal-ruby/lib/temporal/client.rb
Line 18 in a7665ee
|
And now we have learned the valuable lesson of running the rspec locally :) |
|
Refactored, then tested against an env in AWS behind an SSL enabled ALB to ensure it is connecting via SSL successfully. |
|
@tristan-instacart can you please rebase it against the master? There are a few conflict, but otherwise looking good to go 👍 |
tristan-instacart
force-pushed
the
tristan-instacart/add-basic-ssl-support
branch
from
46f00c5
to
f984a3b
Compare
|
Thank you for the feedback @antstorm |
| client_class.new(host, port, identity, Temporal.configuration.grpc_ssl_config) | ||
| else | ||
| client_class.new(host, port, identity) | ||
| end |
There was a problem hiding this comment.
Can you please add a test case for this?
|
|
||
| def client | ||
| @client ||= Temporal::Api::WorkflowService::V1::WorkflowService::Stub.new( | ||
| url, | ||
| :this_channel_is_insecure, | ||
| channel_creds, |
| @@ -55,6 +55,7 @@ Temporal.configure do |config| | |||
| config.port = 7233 | |||
| config.namespace = 'ruby-samples' | |||
| config.task_queue = 'hello-world' | |||
| config.channel_creds = :this_channel_is_insecure | |||
There was a problem hiding this comment.
nit: Ruby folks are ok with full names, let's probably call this just credentials unless you think it might conflict with some other config we'd want to add in the future
|
|
||
| def grpc_ssl_config | ||
| if @channel_creds.nil? | ||
| :this_channel_is_insecure |
There was a problem hiding this comment.
also would be great to have a spec for this and also put the value in a constant
This change enables the use of mTLS for client<>server communication. https://docs.temporal.io/docs/server/security/#encryption-in-transit-with-mtls An update of coinbase#73 to close coinbase#70
…_WOFLO-310 Expose scheduled_time and current_attempt_scheduled_time on activity metadata
This Pull Request references issue #70
It is a simple implementation for allowing SSL support by allowing the consumer of the library to pass channel credentials. A very simple use case is added to the README for reference.
It maintains the default behavior of the SDK for backwards compatibility.