Add subdomain support

[matthewmcgarvey]

Purpose

Resolves #1166

Description

Adds subdomain support to Lucky.

Usage

For request requirement

abstract class AdminAction < Lucky::Action
  include Lucky::Subdomain

  register_subdomain "admin" # requires all requests to be made through admin.example.com
end

For subdomain access

class Payments::Create < BrowserAction
  include Lucky::Subdomain
  register_subdomain ["tenant1", "tenant2", ...]

  post "/payments" do
    tenant = TenantQuery.new.name(subdomain).first
    ...
  end
end

Checklist

• - An issue already exists detailing the issue/or feature request that this PR fixes
• - All specs are formatted with crystal tool format spec src
• - Inline documentation has been added and/or updated
• - Lucky builds on docker with ./script/setup
• - All builds and specs pass on docker with ./script/test

[jkthorne]

Is there a way to use Socket::IPAddress. This will handle IPV6 and IPV4 and it looks like it does not use a Regex so I think it might be faster.

[matthewmcgarvey]

I did try to look for something in the stdlib and didn't find anything. I didn't see this class. Unfortunately the docs for it say this:

IPAddress won't resolve domains, including localhost. If you must resolve an IP, or don't know whether a String contains an IP or a domain name, you should use Addrinfo.resolve instead.

And it looks like Addrinfo.resolve won't answer the question of whether or not the host uses an ip address.

[jkthorne]

That is a good point. You might looks at this to check if this is an IP. But it is not as nice as having using all stdlib tooling to fetch the sub domain.

https://crystal-lang.org/api/1.0.0/Socket.html#ip?(string:String)-class-method
https://play.crystal-lang.org/#/r/bicq

[jwoertink]

I agree with @wontruefree. This one may be a little tricky since IPV6 would get passed through, but since Addrinfo.resolve works on normal hosts, we may get a false positive. This Socket::IPAddress looks like it may work https://crystal-lang.org/api/1.0.0/Socket/IPAddress.html

If you pass in "localhost", it'll raise an exception. The only thing I don't like is that the IP would actually be the edge case here which means we're raising an exception and rescuing 90% of the time which is probably a performance penalty 😝 ...

Maybe this is a "fine enough for now" scenario, and we just open a separate issue to figure out a better way?

[matthewmcgarvey]

If you follow the rails link I left on the constant, the regex is exactly how Rails handles subdomains. It doesn't extract a subdomain if it matches the regex. I guess I'm not seeing how this wouldn't work.

[jwoertink]

It looks like rails uses that second regex https://github.com/rails/rails/blob/afc6abb674b51717dac39ea4d9e2252d7e40d060/actionpack/lib/action_dispatch/http/url.rb#L9 which handles IPV6

/(^[^:]+:\/\/)?(\[[^\]]+\]|[^:]+)(?::(\d+$))?/ =~ "fe80::aede:48ff:fe00:1122"

It seems they're only using it when building the host url though. So I think we can just ignore it for now. I've never actually seen anyone access a site using IPV6. Maybe API calls for hackers trying to find vulnerabilities? I think this is a "fine enough for now" scenario though.

[jwoertink]

I dig this direction, and I think it gives us some great flexibility. I left a few comments, but overall I'm happy with it.

[jwoertink]

I agree with @wontruefree. This one may be a little tricky since IPV6 would get passed through, but since Addrinfo.resolve works on normal hosts, we may get a false positive. This Socket::IPAddress looks like it may work https://crystal-lang.org/api/1.0.0/Socket/IPAddress.html

If you pass in "localhost", it'll raise an exception. The only thing I don't like is that the IP would actually be the edge case here which means we're raising an exception and rescuing 90% of the time which is probably a performance penalty 😝 ...

Maybe this is a "fine enough for now" scenario, and we just open a separate issue to figure out a better way?

[jwoertink]

I love all the different options. Thinking about how I currently use subdomains, I only need a subdomain on my routes when I'm in staging. In production, we don't use subdomains. Is that possible to do? I'm thinking when you do qa.luckyframework.org, that would only be on the QA environment, and not in production.. So we need to be able to say register_subdomain "qa" if LuckyEnv.qa? or whatever...

[matthewmcgarvey]

I have pushed an update to better clarify. The macro is now called require_subdomain.

In this situation you don't actually require a subdomain, you just do have a subdomain in certain scenarios. In those situations you wouldn't use require_subdomain but you can still check if a subdomain was provided by calling subdomain?. Calling subdomain will raise a compile time error though.

[jwoertink]

Cool. That gives us a nice escape hatch then too.

[jwoertink]

Does this work for multiple subdomain too? Like "staging.dashboard.foo"

[matthewmcgarvey]

My current job uses "multiple subdomains" as well because we have a tenant subdomain and the environment subdomain so in staging a url might look like tenant1.staging.example.com.

This subdomain support really focuses on 1 subdomain. Without any special handling, the subdomain for my example would be tenant1.staging and it would be up to me to add special logic to parse it further.

Another way to handle that would be to configure the tld_length to be 2 only in staging so that the subdomain would become tenant1 but I think that might be a sort of hack and of course only works if I don't care about that second subdomain part.

[jwoertink]

Ok, yeah, that's what my sites do too. staging.tenant1.example.com. That's fine though. We just would document that on the site saying if you use deep nested subdomains, then you'll need to do your own split and parse.

[jwoertink]

Alright! I think this is ready to go. Can you just open an issue on the website to document this after it's all merged in? Thanks!