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

Support subdomain and paths in process labels #123

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 68 additions & 6 deletions lib/invoker/dns_cache.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,22 +2,84 @@ module Invoker
class DNSCache
attr_accessor :dns_data

# INI:
# [app]
# port = 8000
# location = www2.app/blah
#
# [chat]
# port = 8001
# location www.app/chat
#
# [api]
# port = 8002
# location = www.app/api
#
# dns_data:
# {
# 'api' => [
# ['', 8002]
# ],
# 'app' => [
# ['', 8000]
# ],
# 'chat' => [
# ['', 8001]
# ],
# 'www.app' => [
# ['', 8000],
# ['/api, 8002],
# ['/chat, 8001]
# ],
# 'www2.app' => [
# ['/blah', 8000]
# ]
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment refers to still old proposed format isn't it? Lets update it?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if you're referring to the example INI or the dns_data object, but the INI is referring to the new format with location.

The dns_data object is simply an internal object representation.

# }
def initialize(config)
self.dns_data = {}
self.dns_data = Hash.new {|h,k| h[k] = []}
@dns_mutex = Mutex.new
Invoker.config.processes.each do |process|
if process.port
dns_data[process.label] = { 'port' => process.port }
add(process.label, process.port)
if process.location
process.location.split(' ').each do |loc|
add(loc, process.port)
end
end
end
end
end

def [](process_name)
@dns_mutex.synchronize { dns_data[process_name] }
def find_process(host, path)
@dns_mutex.synchronize {
until dns_data.include?(host) || host.nil?
host = host.split('.', 2)[1]
end

dns_data[host].reverse_each {|prefix, port|
if path_matches_prefix?(path, prefix)
return {:port => port}
end
}

nil
}
end

def add(location, port)
@dns_mutex.synchronize {
host, path_prefix = split_host_path(location)
(dns_data[host] << [path_prefix.to_s, port]).sort_by! &:length
}
end

private
def split_host_path(label)
label.split(%r{(?=/)}, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so this seems to be supporting both old and new format. Do you think it is best to support one or another? I would rather just have separate location field in config as proposed. Supporting both location and path in process label is somewhat unnecessary imo.

Copy link
Author

Choose a reason for hiding this comment

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

The path only refers to the section following the domain the URL, whereas location refers to the app name + path.

Although we are not explicitly adding path support to process labels, it's actually easier to support path than not, since both process labels and locations go through the same location parser. It also makes sense, in my opinion, since there's no clear way to not support paths in labels. We could, perhaps, add to code to error out, or add code to ignore any process labels that have paths, but why not just let it be handled in the same way location is handled?

end

def add(name, port)
@dns_mutex.synchronize { dns_data[name] = { 'port' => port } }
def path_matches_prefix?(path, prefix)
path.start_with?(prefix) && [nil, "/", "?"].member?(path[prefix.length])
end
end
end
7 changes: 2 additions & 5 deletions lib/invoker/ipc/dns_check_command.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@ module Invoker
module IPC
class DnsCheckCommand < BaseCommand
def run_command(message_object)
process_detail = Invoker.dns_cache[message_object.process_name]
process_detail = Invoker.dns_cache.find_process(message_object.host, message_object.path)

dns_check_response = Invoker::IPC::Message::DnsCheckResponse.new(
process_name: message_object.process_name,
port: process_detail ? process_detail['port'] : nil
)
dns_check_response = Invoker::IPC::Message::DnsCheckResponse.new(process_detail || {})
send_data(dns_check_response)
true
end
Expand Down
4 changes: 2 additions & 2 deletions lib/invoker/ipc/message.rb
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,12 @@ class Remove < Base

class DnsCheck < Base
include Serialization
message_attributes :process_name
message_attributes :host, :path
end

class DnsCheckResponse < Base
include Serialization
message_attributes :process_name, :port
message_attributes :port
end

class Ping < Base
Expand Down
1 change: 1 addition & 0 deletions lib/invoker/parsers/config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def make_pconfig(section)
cmd: section["command"]
}
pconfig['port'] = section['port'] if section['port']
pconfig['location'] = section['location'] if section['location']
pconfig['disable_autorun'] = section['disable_autorun'] if section['disable_autorun']

OpenStruct.new(pconfig)
Expand Down
39 changes: 29 additions & 10 deletions lib/invoker/power/balancer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,8 @@ def post_init

class Balancer
attr_accessor :connection, :http_parser, :session, :protocol
DEV_MATCH_REGEX = /([\w-]+)\.dev(\:\d+)?$/
XIP_IO_MATCH_REGEX = /([\w-]+)\.\d+\.\d+\.\d+\.\d+\.xip\.io(\:\d+)?$/
DEV_MATCH_REGEX = /([\w.-]+)\.dev(\:\d+)?$/
XIP_IO_MATCH_REGEX = /([\w.-]+)\.\d+\.\d+\.\d+\.\d+\.xip\.io(\:\d+)?$/

def self.run(options = {})
start_http_proxy(InvokerHttpProxy, 'http', options)
Expand All @@ -43,29 +43,47 @@ def initialize(connection, protocol)
@connection = connection
@protocol = protocol
@http_parser = HttpParser.new(protocol)
@response_http_parser = HttpParser.new(protocol)
@session = nil
@buffer = []
end

def install_callbacks
http_parser.on_url { |url| url_received(url) }
http_parser.on_headers_complete { |headers| headers_received(headers) }
http_parser.on_message_complete { |full_message| complete_message_received(full_message) }

@response_http_parser.on_message_complete { response_complete }

connection.on_data { |data| upstream_data(data) }
connection.on_response { |backend, data| backend_data(backend, data) }
connection.on_finish { |backend, name| frontend_disconnect(backend, name) }
end

def response_complete
# disconnect from backend after each response, since subsequent
# requests from persistent HTTP connections might be proxied to a stale
# backend
EventMachine.next_tick do # still needs chance to send data
connection.unbind_backend(@session)
@session = nil
@response_http_parser.reset
http_parser.reset
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sort of unrelated to this change isn't it? Do you think this should be a good idea in general or the path matching specifically made something worse, thereby highlighting a bug we haven't seen before?

Copy link
Author

Choose a reason for hiding this comment

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

This is actually required since the previously, there was a one-to-one mapping between incoming domain and backend. Now, since a single incoming domain could have multiple backends depending on the path, we need to make sure that the backend is not persisted across the same request coming in from the same domain.


def complete_message_received(full_message)
connection.relay_to_servers(full_message)
http_parser.reset
end

def url_received(url)
@path = url
end

def headers_received(headers)
if @session
return
end
@session = UUID.generate()
dns_check_response = select_backend_config(headers['Host'])
dns_check_response = select_backend_config(headers['Host'], @path)
if dns_check_response && dns_check_response.port
connection.server(session, host: '0.0.0.0', port: dns_check_response.port)
else
Expand All @@ -88,6 +106,7 @@ def append_for_http_parsing(data)
end

def backend_data(backend, data)
@response_http_parser << data
@backend_data = true
data
end
Expand All @@ -107,11 +126,11 @@ def extract_host_from_domain(host)

private

def select_backend_config(host)
matching_string = extract_host_from_domain(host)
def select_backend_config(domain, path)
matching_string = extract_host_from_domain(domain)
return nil unless matching_string
if selected_app = matching_string[1]
dns_check(process_name: selected_app)
if host = matching_string[1]
dns_check(host: host, path: path)
else
nil
end
Expand Down
11 changes: 11 additions & 0 deletions lib/invoker/power/http_parser.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,18 @@ def initialize(protocol)
@parser = HTTP::Parser.new
@header = {}
initialize_message_content
parser.on_url { |url| url_received(url) }
parser.on_headers_complete { complete_headers_received }
parser.on_header_field { |field_name| @last_key = field_name }
parser.on_header_value { |field_value| header_value_received(field_value) }

parser.on_message_complete { complete_message_received }
end

def on_url(&block)
@on_url_callback = block
end

# define a callback for invoking when complete header is parsed
def on_headers_complete(&block)
@on_headers_complete_callback = block
Expand Down Expand Up @@ -63,6 +68,12 @@ def complete_headers_received
@on_headers_complete_callback.call(@header)
end
end

def url_received(url)
if @on_url_callback
@on_url_callback.call(url)
end
end
end
end
end
2 changes: 1 addition & 1 deletion spec/invoker/config_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@

expect(dns_cache.dns_data).to_not be_empty
expect(dns_cache.dns_data['web']).to_not be_empty
expect(dns_cache.dns_data['web']['port']).to eql 9001
expect(dns_cache.dns_data['web'].first[1]).to eql 9001
ensure
File.delete("/tmp/Procfile")
end
Expand Down
8 changes: 4 additions & 4 deletions spec/invoker/ipc/dns_check_command_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
let(:client) { Invoker::IPC::ClientHandler.new(client_socket) }

describe "dns check for valid process" do
let(:message_object) { MM::DnsCheck.new(process_name: 'lolbro') }
let(:message_object) { MM::DnsCheck.new(host: 'lolbro', path: '/') }
it "should response with dns check response" do
invoker_dns_cache.expects(:[]).returns('port' => 9000)
invoker_dns_cache.expects(:find_process).returns('process_name' => 'lolbro', 'port' => 9000)
client_socket.string = message_object.encoded_message

client.read_and_execute
Expand All @@ -18,9 +18,9 @@
end

describe "dns check for invalid process" do
let(:message_object) { MM::DnsCheck.new(process_name: 'foo') }
let(:message_object) { MM::DnsCheck.new(host: 'foo', path: '/') }
it "should response with dns check response" do
invoker_dns_cache.expects(:[]).returns('port' => nil)
invoker_dns_cache.expects(:find_process).returns(nil)
client_socket.string = message_object.encoded_message

client.read_and_execute
Expand Down
4 changes: 2 additions & 2 deletions spec/invoker/power/balancer_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
expect(match).to_not be_nil

matching_string = match[1]
expect(matching_string).to eq("bar")
expect(matching_string).to eq("emacs.bar")
end

it "should match hello-world.dev" do
Expand Down Expand Up @@ -61,7 +61,7 @@
expect(match).to_not be_nil

matching_string = match[1]
expect(matching_string).to eq("bar")
expect(matching_string).to eq("emacs.bar")
end

it "should match hello-world.10.0.0.1.xip.io" do
Expand Down
10 changes: 10 additions & 0 deletions spec/invoker/power/http_parser_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,16 @@

describe "complete message received" do
before { parser.reset }
it "should call url received with url" do
@header, @path = nil
parser.on_headers_complete { |header| @header = header }
parser.on_url { |url| @path = url }
parser << "GET /blah HTTP/1.1\r\n"
parser << "Host: localhost\r\n"

expect(@path).to eql "/blah"
end

it "should call header received with full header" do
@header = nil
parser.on_headers_complete { |header| @header = header }
Expand Down