Support for TLS

[bpinto]

Dalli already has support for connections with TLS, would it be possible to support the same for auto-discovery? Attempting to get the servers right now fails with: Errno::ECONNRESET: Connection reset by peer.

Perhaps the ssl_context we pass to Dalli through dalli_options could be used for the socket connection as well?

[petergoldstein]

So I'm not averse to adding TLS support for the endpoint, but I'm not sure we can safely reuse the ssl_context from Dalli.

Would adding an endpoint_options subhash to the options passed in the initializer be too clunky?

[bpinto]

I was thinking about this and wondering if there was a way to do that with keyword arguments but the solution I found is not particularly pretty either:

def my_method(config_endpoint, endpoint_options: nil, **dalli_options)
  puts "config_endpoint: #{config_endpoint}"
  puts "endpoint_options: #{endpoint_options}"
  puts "dalli_options: #{dalli_options}"
end

pry(main)> my_method('a', a: 1, ssl_context: 2, b: 2, endpoint_options: 2)
config_endpoint: a
endpoint_options: 2
dalli_options: {:a=>1, :ssl_context=>2, :b=>2}

So I guess using a subhash is likely the more readable approach. It's not pretty, but it doesn't cause backward incompatibilities so I guess that could be a valid reason to go forward with this approach.

[petergoldstein]

I hadn't considered in, but a keyword argument with a default empty hash might work well. Something like this in Dalli::Elasticache:

def initialize(endpoint_host_and_port, dalli_options = {}, endpoint_options: {})
  @endpoint = Dalli::Elasticache::AutoDiscovery::Endpoint.new(endpoint_host_and_port, endpoint_options)
  @options = dalli_options
end

# Clear all cached data from the cluster endpoint
def refresh
  host_and_port = "#{endpoint.host}:#{endpoint.port}"
  endpoint_options = endpoint.options
  @endpoint = Dalli::Elasticache::AutoDiscovery::Endpoint.new(host_and_port, endpoint_options)

   self
end

The endpoint could track its options, and push those down to commands when instantiating. The endpoint options could include ssl_context (which would be pretty easy to implement), as well as additional options like timeouts.

Thoughts @bpinto ?

[bpinto]

My main concern is breaking backwards compatibility with the Dalli::ElastiCache.new call. But if you are okay with breaking it, I think adding a keyword argument is a great change. We would probably need to convert dalli_options to keyword arguments as well to make it easier to use.

---

Existing code:

module Dalli
  class ElastiCache
    def initialize(config_endpoint, dalli_options = {})
      puts "config_endpoint: #{config_endpoint}"
      puts "dalli_options: #{dalli_options}"
    end
  end
end

Dalli::ElastiCache.new('url')
# config_endpoint: url
# dalli_options: {}
Dalli::ElastiCache.new('url', { expires_in: 24 * 60 * 60, namespace: 'my_app'})
# config_endpoint: url
# dalli_options: {:expires_in=>86400, :namespace=>"my_app"}
Dalli::ElastiCache.new('url', expires_in: 24 * 60 * 60, namespace: 'my_app')
# config_endpoint: url
# dalli_options: {:expires_in=>86400, :namespace=>"my_app"}

---

Proposed code:

module Dalli
  class ElastiCache
    def initialize(endpoint_host_and_port, dalli_options = {}, endpoint_options: {})
      puts "endpoint_host_and_port: #{endpoint_host_and_port}"
      puts "dalli_options: #{dalli_options}"
      puts "endpoint_options: #{endpoint_options}"
    end
  end
end

Dalli::ElastiCache.new('url')
# endpoint_host_and_port: url
# dalli_options: {}
# endpoint_options: {}
Dalli::ElastiCache.new('url', { expires_in: 24 * 60 * 60, namespace: 'my_app' })
# ArgumentError (unknown keywords: expires_in, namespace)
Dalli::ElastiCache.new('url', expires_in: 24 * 60 * 60, namespace: 'my_app')
# ArgumentError (unknown keywords: expires_in, namespace)

[petergoldstein]

Hmm...I see. For anyone who is passing in a bunch of trailing options and expecting them to be pulled into a Hash automatically, we'll break compatibility.

While not ideal, I'm actually not that worried about that particular issue if it comes with a version change. I'll do some more poking and give it some thought.

[bpinto]

For what it's worth, in order to make it clear on what each option will apply to, I think this is my favourite:

module Dalli
  class ElastiCache
    def initialize(endpoint_host_and_port, dalli_options: {}, endpoint_options: {})
      puts "endpoint_host_and_port: #{endpoint_host_and_port}"
      puts "dalli_options: #{dalli_options}"
      puts "endpoint_options: #{endpoint_options}"
    end
  end
end

Dalli::ElastiCache.new('url')
# endpoint_host_and_port: url
# dalli_options: {}
# endpoint_options: {}

Dalli::ElastiCache.new('url', dalli_options: { expires_in: 24 * 60 * 60, namespace: 'my_app' })
# endpoint_host_and_port: url
# dalli_options: {:expires_in=>86400, :namespace=>"my_app"}
# endpoint_options: {}

Dalli::ElastiCache.new('url', dalli_options: { expires_in: 24 * 60 * 60, namespace: 'my_app' }, endpoint_options: { ssl_context: '123' })
# endpoint_host_and_port: url
# dalli_options: {:expires_in=>86400, :namespace=>"my_app"}
# endpoint_options: {:ssl_context=>"123"}

[petergoldstein]

Upon reflection I'm inclined to group this with some Ruby version support changes and timeout support and call all of that a 2.0. I like the option you suggested in your last comment. Barring any objections, I think I'll make that the plan.

[danigirl329]

Hi- @petergoldstein I am bumping into this now with our networking infrastructure changing internally. Do you have any updates on this? Happy to help out. Thanks!

[petergoldstein]

@danigirl329 If you'd like to submit a PR implementing this I'd be happy to look at it. Right now I don't have time to work on this.