Skip to content
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

Reuse Redis connection in allow_domain method #38

Closed
waynerobinson opened this issue Jan 7, 2017 · 8 comments
Closed

Reuse Redis connection in allow_domain method #38

waynerobinson opened this issue Jan 7, 2017 · 8 comments
Milestone

Comments

@waynerobinson
Copy link

Sorry for a potentially dumb question, I've got no experience with Lua.

But is it possible to reuse the Redis connection within the allow_domain method for looking up permissable domains from a set stored there?

We've played with a couple of different options, but would really like to keep a whitelist of domains in a DB somewhere it refers to and since there's already Redis support baked in, we may as well use it.

@GUI
Copy link
Collaborator

GUI commented Jan 13, 2017

Not a dumb question at all!

You could establish a new redis connection inside the allow_domain callback, but it doesn't look like we have a super-easy way to reuse the same Redis connection that the storage mechanism would already have instantiated. But I think that's a good idea, so we could certainly look at ways to better expose that part of the storage API to the allow_domain callback for a future release (or pull requests are always welcome).

In the meantime, here's a workaround that I think might work to allow you to leverage the same connection in both places:

local function get_redis_instance(redis_options)
  local instance = ngx.ctx.auto_ssl_redis_instance
  if instance then
    return instance
  end

  instance = redis:new()
  local ok, err

  if redis_options["socket"] then
    ok, err = instance:connect(redis_options["socket"])
  else
    ok, err = instance:connect(redis_options["host"], redis_options["port"])
  end
  if not ok then
    return false, err
  end

  if redis_options["auth"] then
    ok, err = instance:auth(redis_options["auth"])
    if not ok then
      return false, err
    end
  end

  ngx.ctx.auto_ssl_redis_instance = instance
  return instance
end

auto_ssl = (require "resty.auto-ssl").new()
local redis_options = {
  host = "10.10.10.1"
}
auto_ssl:set("redis", redis_options)
auto_ssl:set("allow_domain", function(domain)
  local redis_instance, instance_err = get_redis_instance(redis_options)
  if instance_err then
    return nil, instance_err
  end

  local res, err = redis_instance:get("WHATEVER...")
  -- Do whatever else you need with redis_instance.
end)
auto_ssl:init()

It's a bit ugly, but basically I'm just copying the private get_redis_instance method out of the Redis storage adapter: https://github.com/GUI/lua-resty-auto-ssl/blob/v0.10.3/lib/resty/auto-ssl/storage_adapters/redis.lua#L5-L32 Since both functions will cache the redis connection on the ngx.ctx.auto_ssl_redis_instance context variable, then which ever version of get_redis_instance gets called first should be the only one that actually establishes a connection.

But again, I think providing a cleaner way to get at this connection object directly from the storage API would be better long term.

@waynerobinson
Copy link
Author

Thanks for this! I'll check it out.

@aviatrix
Copy link

aviatrix commented Jun 3, 2017

Hey @GUI , has anything been done to enable this ?

@luto
Copy link
Collaborator

luto commented Jun 5, 2017

@aviatrix I don't know of any changes regarding this. Right now nobody knows what reusing that object would even look like, for a users point of view. Is there an interface idea you'd like to work out and propose? I'll be glad to give you some pointers on how to implement it.

@aviatrix
Copy link

aviatrix commented Jun 5, 2017

@luto Right now i've settled for the get_redis_instance example given by @GUI in the comments above, however that is sub optimal in my opinion. On first thought maybe exposing the get_redis_instance as public static method should suffice so we can call that function inside the auto_ssl like this : auto_ssl:get_redis_instance(). I'm not familiar with Lua or how it works or if this is even possible.

@aviatrix
Copy link

One small caveat : if you don't put local redis = require "resty.redis" on the top of init_by_lua_block there is a chance it will blow up when trying to init redis :)

@GUI GUI added this to the v0.12.0 milestone Feb 5, 2018
GUI added a commit that referenced this issue Feb 5, 2018
The auto_ssl instance is now passed as the second argument to
the `allow_domain` callback. If using the Redis storage adapter, then
now the Redis connection can be accessed by
`auto_ssl.storage.adapter:get_connection()`. This allows for more easily
accessing the same redis connection in this callback function (or
anywhere else the auto_ssl instance is available).

This rolls back the change in 9703684
to rename the storage adapter instance inside the storage library, so
it continues to just be `adapter` (instead of `storage_adapter`). This
makes this access a bit easier and also keeps backwards compatibility
with the previous release.

See: #38
@GUI
Copy link
Collaborator

GUI commented Feb 5, 2018

Well, better late than never, but this should finally be easier in the v0.12.0 release. You can now access the redis connection via auto_ssl.storage.adapter:get_connection(), and the auto_ssl instance is passed as the second argument to the allow_domain callback. Thanks for the idea!

@GUI GUI closed this as completed Feb 5, 2018
@gohai
Copy link
Contributor

gohai commented Feb 28, 2018

@GUI Is it possible to get to ssl_options through the auto_ssl instance? Thanks!

rsumnerz pushed a commit to rsumnerz/lua-resty-auto-ssl that referenced this issue Aug 21, 2018
The auto_ssl instance is now passed as the second argument to
the `allow_domain` callback. If using the Redis storage adapter, then
now the Redis connection can be accessed by
`auto_ssl.storage.adapter:get_connection()`. This allows for more
easily
accessing the same redis connection in this callback function (or
anywhere else the auto_ssl instance is available).

This rolls back the change in 9703684
to rename the storage adapter instance inside the storage library, so
it continues to just be `adapter` (instead of `storage_adapter`). This
makes this access a bit easier and also keeps backwards compatibility
with the previous release.

See: auto-ssl/lua-resty-auto-ssl#38

Conflicts:
	README.md
	lib/resty/auto-ssl/ssl_certificate.lua
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants