From cfcc3e48fc788910d1943c86ae49e895461c73b0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 27 Jan 2017 11:06:02 -0300 Subject: [PATCH 1/3] Add striping to some channel settings values This should help a bit dealing with user entry errors while configuring channels. Regarding #783, 2 out of 3 channels which fail to resolve the IP address of the domain have a whitespace at the start/end of the domain, which makes the DNS query fail. Plus, we've had authentication errors in the past due to extra whitespace in the username and password. --- app/models/channel.rb | 4 ++++ app/models/channels/sip.rb | 8 ++++---- config/initializers/active_record.rb | 2 ++ 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/models/channel.rb b/app/models/channel.rb index d19d89b42..e09fdfd55 100644 --- a/app/models/channel.rb +++ b/app/models/channel.rb @@ -40,6 +40,10 @@ class Channel < ActiveRecord::Base broker_cached + def name=(value) + super(value.try(:strip)) + end + def config self[:config] ||= {} end diff --git a/app/models/channels/sip.rb b/app/models/channels/sip.rb index 3952aa78d..13c95f817 100644 --- a/app/models/channels/sip.rb +++ b/app/models/channels/sip.rb @@ -18,12 +18,12 @@ class Channels::Sip < Channel validate :server_username_uniqueness - config_accessor :username - config_accessor :password - config_accessor :domain + config_accessor :username, strip: true + config_accessor :password, strip: true + config_accessor :domain, strip: true config_accessor :direction config_accessor :register - config_accessor :number + config_accessor :number, strip: true def register? register == true || register == "1" diff --git a/config/initializers/active_record.rb b/config/initializers/active_record.rb index f746640b9..0bed6f6f9 100644 --- a/config/initializers/active_record.rb +++ b/config/initializers/active_record.rb @@ -19,6 +19,7 @@ class ActiveRecord::Base def self.config_accessor(*names) options = names.extract_options! default = options[:default] + strip = options[:strip] names.each do |name| self.config_attrs << name @@ -31,6 +32,7 @@ def self.config_accessor(*names) respond_to?(:encrypted_config_will_change!) ? encrypted_config_will_change! : config_will_change! self.config ||= {} + value = value.try(:strip) if strip self.config[name.to_s] = value # this is needed so attr_encrypted encrypts the value correctly From 17471ef9f2dda77d3057ac19a00f97ab66121c3d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 27 Jan 2017 12:18:45 -0300 Subject: [PATCH 2/3] Skip generating config for channels with unresolved domains If the domain name of the channel cannot be resolved when generating the configuration file for Asterisk, skip it entirely. Asterisk needs the resolved IP addresses for matching incoming calls from the channel (ie. the `identify` sections). Also, the domain needs to be resolvable for Asterisk to attempt the registration. We may consider relaxing this restriction if the channel should not be registered and is only used for outgoing calls. As of today, the broker doesn't care for the channel's direction. --- broker/src/asterisk/asterisk_config.erl | 161 +++++++++++++----------- 1 file changed, 90 insertions(+), 71 deletions(-) diff --git a/broker/src/asterisk/asterisk_config.erl b/broker/src/asterisk/asterisk_config.erl index 4299451b0..2a61f60de 100644 --- a/broker/src/asterisk/asterisk_config.erl +++ b/broker/src/asterisk/asterisk_config.erl @@ -75,90 +75,109 @@ generate_config([Channel | Rest], ConfigFile, ResolvCache, ChannelIndex, Registr generate_config(Rest, ConfigFile, ResolvCache, ChannelIndex, RegistryIndex); _ -> - HasAuth = length(Username) > 0 andalso length(Password) > 0, - UserOrNumber = case length(Username) > 0 of - true -> Username; - _ -> Number - end, - - % Registration - NewRegistryIndex = case channel:register(Channel) of + {Expanded, NewCache} = expand_domain(Domain, ResolvCache), + % TODO: if we consider the channel's direction (ie. inbound/outbound) we + % may relax this condition since we only need a set of resolved IP + % addresses to match incoming calls. Although, the domain needs to be + % resolvable also if registration is enabled. + case is_resolved(Expanded) of true -> + HasAuth = length(Username) > 0 andalso length(Password) > 0, + UserOrNumber = case length(Username) > 0 of + true -> Username; + _ -> Number + end, + + % Registration + NewRegistryIndex = case channel:register(Channel) of + true -> + file:write(ConfigFile, ["[", Section, "]\n"]), + file:write(ConfigFile, "type=registration\n"), + if HasAuth -> + file:write(ConfigFile, ["outbound_auth=", Section, "\n"]); + true -> ok + end, + file:write(ConfigFile, ["server_uri=sip:", Domain, "\n"]), + file:write(ConfigFile, ["client_uri=sip:", UserOrNumber, "@", Domain, "\n"]), + %% When Asterisk Version > 13.1... + % file:write(ConfigFile, "line=yes\n"), + % file:write(ConfigFile, ["endpoint=", Section, "\n"]), + file:write(ConfigFile, "\n"), + dict:store({Username, Domain}, Channel#channel.id, RegistryIndex); + + _ -> RegistryIndex + end, + + % Endpoint file:write(ConfigFile, ["[", Section, "]\n"]), - file:write(ConfigFile, "type=registration\n"), + file:write(ConfigFile, "type=endpoint\n"), + file:write(ConfigFile, "context=verboice\n"), + file:write(ConfigFile, ["aors=", Section, "\n"]), if HasAuth -> file:write(ConfigFile, ["outbound_auth=", Section, "\n"]); true -> ok end, - file:write(ConfigFile, ["server_uri=sip:", Domain, "\n"]), - file:write(ConfigFile, ["client_uri=sip:", UserOrNumber, "@", Domain, "\n"]), - %% When Asterisk Version > 13.1... - % file:write(ConfigFile, "line=yes\n"), - % file:write(ConfigFile, ["endpoint=", Section, "\n"]), + file:write(ConfigFile, ["from_user=", UserOrNumber, "\n"]), + file:write(ConfigFile, ["from_domain=", Domain, "\n"]), + file:write(ConfigFile, "disallow=all\n"), + file:write(ConfigFile, "allow=ulaw\n"), + file:write(ConfigFile, "allow=gsm\n"), file:write(ConfigFile, "\n"), - dict:store({Username, Domain}, Channel#channel.id, RegistryIndex); - - _ -> RegistryIndex - end, - - % Endpoint - file:write(ConfigFile, ["[", Section, "]\n"]), - file:write(ConfigFile, "type=endpoint\n"), - file:write(ConfigFile, "context=verboice\n"), - file:write(ConfigFile, ["aors=", Section, "\n"]), - if HasAuth -> - file:write(ConfigFile, ["outbound_auth=", Section, "\n"]); - true -> ok - end, - file:write(ConfigFile, ["from_user=", UserOrNumber, "\n"]), - file:write(ConfigFile, ["from_domain=", Domain, "\n"]), - file:write(ConfigFile, "disallow=all\n"), - file:write(ConfigFile, "allow=ulaw\n"), - file:write(ConfigFile, "allow=gsm\n"), - file:write(ConfigFile, "\n"), - - % Auth - if HasAuth -> - file:write(ConfigFile, ["[", Section, "]\n"]), - file:write(ConfigFile, "type=auth\n"), - file:write(ConfigFile, "auth_type=userpass\n"), - file:write(ConfigFile, ["username=", Username, "\n"]), - file:write(ConfigFile, ["password=", Password, "\n"]), - file:write(ConfigFile, "\n"); - true -> ok - end, - % AOR - file:write(ConfigFile, ["[", Section, "]\n"]), - file:write(ConfigFile, "type=aor\n"), - file:write(ConfigFile, "qualify_frequency=60\n"), - file:write(ConfigFile, ["contact=sip:", Domain, "\n"]), - file:write(ConfigFile, "\n"), + % Auth + if HasAuth -> + file:write(ConfigFile, ["[", Section, "]\n"]), + file:write(ConfigFile, "type=auth\n"), + file:write(ConfigFile, "auth_type=userpass\n"), + file:write(ConfigFile, ["username=", Username, "\n"]), + file:write(ConfigFile, ["password=", Password, "\n"]), + file:write(ConfigFile, "\n"); + true -> ok + end, - % Identify - file:write(ConfigFile, ["[", Section, "]\n"]), - file:write(ConfigFile, "type=identify\n"), - file:write(ConfigFile, ["endpoint=", Section, "\n"]), + % AOR + file:write(ConfigFile, ["[", Section, "]\n"]), + file:write(ConfigFile, "type=aor\n"), + file:write(ConfigFile, "qualify_frequency=60\n"), + file:write(ConfigFile, ["contact=sip:", Domain, "\n"]), + file:write(ConfigFile, "\n"), - {Expanded, NewCache} = expand_domain(Domain, ResolvCache), - {NewChannelIndex, _} = lists:foldl(fun ({_Host, IPs, Port}, {R1, I}) -> - case Port of - undefined -> ok; - _ -> ok - % file:write(ConfigFile, ["port=", integer_to_list(Port), "\n"]) - end, + % Identify + file:write(ConfigFile, ["[", Section, "]\n"]), + file:write(ConfigFile, "type=identify\n"), + file:write(ConfigFile, ["endpoint=", Section, "\n"]), + + {NewChannelIndex, _} = lists:foldl(fun ({_Host, IPs, Port}, {R1, I}) -> + case Port of + undefined -> ok; + _ -> ok + % file:write(ConfigFile, ["port=", integer_to_list(Port), "\n"]) + end, + + R3 = lists:foldl(fun (IP, R2) -> + file:write(ConfigFile, ["match=", IP, "\n"]), + dict:append({util:to_string(IP), Number}, Channel#channel.id, R2) + end, R1, IPs), + {R3, I + 1} + end, {ChannelIndex, 0}, Expanded), + file:write(ConfigFile, "\n"), - R3 = lists:foldl(fun (IP, R2) -> - file:write(ConfigFile, ["match=", IP, "\n"]), - dict:append({util:to_string(IP), Number}, Channel#channel.id, R2) - end, R1, IPs), - {R3, I + 1} - end, {ChannelIndex, 0}, Expanded), - file:write(ConfigFile, "\n"), + generate_config(Rest, ConfigFile, NewCache, NewChannelIndex, NewRegistryIndex); - generate_config(Rest, ConfigFile, NewCache, NewChannelIndex, NewRegistryIndex) + false -> + % Domain could not be resolved, we cannot generate the endpoint + % information for Asterisk + generate_config(Rest, ConfigFile, NewCache, ChannelIndex, RegistryIndex) + end end. +is_resolved([]) -> + false; +is_resolved([{_Host, [_Ip|_], _Port}|_]) -> + true; +is_resolved([_|Rest]) -> + is_resolved(Rest). + expand_domain(<<>>, ResolvCache) -> {[], ResolvCache}; expand_domain(Domain, ResolvCache) -> case dict:find(Domain, ResolvCache) of @@ -191,7 +210,7 @@ expand_domain(Domain) -> {ok, #hostent{h_addr_list = IpList}} -> IPs = map_ips(IpList), [{Domain, IPs, undefined}]; - _ -> [{Domain, [Domain], undefined}] + _ -> [{Domain, [], undefined}] end end. From 7969d5b206694559fa1fefa54dc5b8dac7bda7c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20Gir=C3=A1ldez?= Date: Fri, 27 Jan 2017 15:30:58 -0300 Subject: [PATCH 3/3] Report back Asterisk configuration errors to the UI If a channel is not configured in Asterisk due to the domain name resolution failing, report that error back to the UI. --- broker/src/asterisk/asterisk_channel_srv.erl | 61 +++++++++++++------- broker/src/asterisk/asterisk_config.erl | 17 +++--- 2 files changed, 51 insertions(+), 27 deletions(-) diff --git a/broker/src/asterisk/asterisk_channel_srv.erl b/broker/src/asterisk/asterisk_channel_srv.erl index 6c37cf911..9378bd6d1 100644 --- a/broker/src/asterisk/asterisk_channel_srv.erl +++ b/broker/src/asterisk/asterisk_channel_srv.erl @@ -21,7 +21,11 @@ % channel_status: dict(channel_id, {channel_id, registration_ok, error_message}) % Registration statuses % --record(state, {channels, dynamic_channels, registry, channel_status, config_job_state = idle, status_job_state = idle}). +% config_channel_status: dict(channel_id, {channel_id, registration_ok, error_message}) +% Channel configuration errors; if a channel fails to configure correctly, +% there will be an entry for it here +% +-record(state, {channels, dynamic_channels, registry, channel_status, config_channel_status, config_job_state = idle, status_job_state = idle}). start_link() -> gen_server:start_link({local, ?SERVER}, ?MODULE, {}, []). @@ -83,21 +87,18 @@ handle_call({register_channel, ChannelId, PeerIp}, _From, State) -> {reply, ok, NewState}; handle_call({get_channel_status, ChannelIds}, _From, State) -> - case State#state.channel_status of - undefined -> {reply, [], State}; - Status -> - Result = lists:foldl(fun(ChannelId, R) -> - case dict:find(ChannelId, Status) of - {ok, ChannelStatus} -> [ChannelStatus | R]; - _ -> R - end - end, [], ChannelIds), - {reply, Result, State} - end; + Result = lists:foldl(fun(ChannelId, R) -> + case find_channel_status(ChannelId, State) of + undefined -> R; + ChannelStatus -> [ChannelStatus | R] + end + end, [], ChannelIds), + {reply, Result, State}; handle_call(_Request, _From, State) -> {reply, {error, unknown_call}, State}. + %% @private handle_cast(regenerate_config, State = #state{config_job_state = JobState}) -> NewState = case JobState of @@ -105,11 +106,11 @@ handle_cast(regenerate_config, State = #state{config_job_state = JobState}) -> spawn_link(fun() -> {ok, BaseConfigPath} = application:get_env(asterisk_config_dir), ConfigFilePath = filename:join(BaseConfigPath, "pjsip_verboice.conf"), - {ChannelIndex, RegistryIndex} = asterisk_config:generate(ConfigFilePath), + {ChannelIndex, RegistryIndex, ConfigErrors} = asterisk_config:generate(ConfigFilePath), ami_client:sip_reload(), - gen_server:cast(?MODULE, {set_channels, ChannelIndex, RegistryIndex}) + gen_server:cast(?MODULE, {set_channels, ChannelIndex, RegistryIndex, ConfigErrors}) end), - State#state{config_job_state = working}; + State#state{config_job_state = working, config_channel_status = undefined}; working -> State#state{config_job_state = must_regenerate}; must_regenerate -> @@ -117,15 +118,18 @@ handle_cast(regenerate_config, State = #state{config_job_state = JobState}) -> end, {noreply, NewState}; -handle_cast({set_channels, ChannelIndex, RegistryIndex}, State = #state{config_job_state = JobState}) -> - lager:info("Updated Asterisk Channel registry: ~B IP-number and ~B SIP registrations", - [dict:size(ChannelIndex), dict:size(RegistryIndex)]), - lager:debug("Asterisk Channel registry: ~n~p~n~p~n", [ChannelIndex, RegistryIndex]), +handle_cast({set_channels, ChannelIndex, RegistryIndex, ConfigErrors}, State = #state{config_job_state = JobState}) -> + lager:info("Updated Asterisk Channel registry: ~B IP-number and ~B SIP registrations, ~B errors", + [dict:size(ChannelIndex), dict:size(RegistryIndex), length(ConfigErrors)]), + lager:debug("Asterisk Registrations: ~p", [RegistryIndex]), + lager:debug("Asterisk Channels: ~p", [ChannelIndex]), + lager:debug("Asterisk Configuration Errors: ~p", [ConfigErrors]), case JobState of must_regenerate -> regenerate_config(); _ -> ok end, - {noreply, State#state{channels = ChannelIndex, registry = RegistryIndex, config_job_state = idle}}; + ConfigChannelStatus = dict:from_list(lists:map(fun(Value = {ChannelId, _, _}) -> {ChannelId, Value} end, ConfigErrors)), + {noreply, State#state{channels = ChannelIndex, registry = RegistryIndex, config_channel_status = ConfigChannelStatus, config_job_state = idle}}; handle_cast({set_channel_status, Status}, State = #state{channel_status = PrevStatus}) -> channel:log_broken_channels(PrevStatus, Status), @@ -174,3 +178,20 @@ find_registered_channel(ChannelIds, ChannelStatus) -> Result end, undefined, ChannelIds). +find_channel_status(ChannelId, #state{channel_status = ChannelStatus, config_channel_status = ConfigChannelStatus}) -> + case find_channel_status(ChannelId, ChannelStatus) of + undefined -> + find_channel_status(ChannelId, ConfigChannelStatus); + Status -> + Status + end; + +find_channel_status(_, undefined) -> + undefined; +find_channel_status(ChannelId, StatusDict) -> + case dict:find(ChannelId, StatusDict) of + {ok, Status} -> + Status; + _ -> + undefined + end. diff --git a/broker/src/asterisk/asterisk_config.erl b/broker/src/asterisk/asterisk_config.erl index 2a61f60de..c6be30931 100644 --- a/broker/src/asterisk/asterisk_config.erl +++ b/broker/src/asterisk/asterisk_config.erl @@ -23,17 +23,18 @@ generate(ConfigFilePath) -> % user. generate_config(ConfigFile) -> Channels = channel:find_all_sip(), - generate_config(Channels, ConfigFile, dict:new(), dict:new(), dict:new()). + generate_config(Channels, ConfigFile, dict:new(), dict:new(), dict:new(), []). -generate_config([], _, _, ChannelIndex, RegistryIndex) -> {ChannelIndex, RegistryIndex}; -generate_config([Channel | Rest], ConfigFile, ResolvCache, ChannelIndex, RegistryIndex) -> +generate_config([], _, _, ChannelIndex, RegistryIndex, ConfigErrors) -> {ChannelIndex, RegistryIndex, ConfigErrors}; +generate_config([Channel | Rest], ConfigFile, ResolvCache, ChannelIndex, RegistryIndex, ConfigErrors) -> HeaderLines = binary:split(erlang:iolist_to_binary(pretty_print(Channel)), <<"\n">>, [global]), lists:foreach(fun(Line) -> file:write(ConfigFile, ["; ", Line, "\n"]) end, HeaderLines), file:write(ConfigFile, "\n"), - Section = ["verboice_", integer_to_list(Channel#channel.id)], + ChannelId = Channel#channel.id, + Section = ["verboice_", integer_to_list(ChannelId)], Username = channel:username(Channel), Password = channel:password(Channel), Domain = channel:domain(Channel), @@ -72,7 +73,7 @@ generate_config([Channel | Rest], ConfigFile, ResolvCache, ChannelIndex, Registr file:write(ConfigFile, "remove_existing=yes\n"), file:write(ConfigFile, "\n"), - generate_config(Rest, ConfigFile, ResolvCache, ChannelIndex, RegistryIndex); + generate_config(Rest, ConfigFile, ResolvCache, ChannelIndex, RegistryIndex, ConfigErrors); _ -> {Expanded, NewCache} = expand_domain(Domain, ResolvCache), @@ -162,12 +163,14 @@ generate_config([Channel | Rest], ConfigFile, ResolvCache, ChannelIndex, Registr end, {ChannelIndex, 0}, Expanded), file:write(ConfigFile, "\n"), - generate_config(Rest, ConfigFile, NewCache, NewChannelIndex, NewRegistryIndex); + generate_config(Rest, ConfigFile, NewCache, NewChannelIndex, NewRegistryIndex, ConfigErrors); false -> % Domain could not be resolved, we cannot generate the endpoint % information for Asterisk - generate_config(Rest, ConfigFile, NewCache, ChannelIndex, RegistryIndex) + Message = iolist_to_binary(["Error: could not resolve domain ", Domain]), + NewConfigErrors = [{ChannelId, false, [Message]}|ConfigErrors], + generate_config(Rest, ConfigFile, NewCache, ChannelIndex, RegistryIndex, NewConfigErrors) end end.