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

router: Add configurable forward zones #35

Merged
merged 1 commit into from May 1, 2017
Merged

Conversation

nlsun
Copy link

@nlsun nlsun commented Apr 27, 2017

No description provided.

@nlsun
Copy link
Author

nlsun commented Apr 27, 2017

@GoelDeepak @drewkerrigan ready for review.

Also, ideas on how to test this change? I've only manually tested it for now.

@codecov-io
Copy link

codecov-io commented Apr 27, 2017

Codecov Report

Merging #35 into master will increase coverage by 0.51%.
The diff coverage is 48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #35      +/-   ##
==========================================
+ Coverage   59.61%   60.12%   +0.51%     
==========================================
  Files          12       12              
  Lines         463      484      +21     
==========================================
+ Hits          276      291      +15     
- Misses        187      193       +6
Impacted Files Coverage Δ
src/spartan_config.erl 89.65% <100%> (+0.36%) ⬆️
src/spartan_app.erl 49.09% <27.27%> (-5.46%) ⬇️
src/spartan_router.erl 63.88% <61.53%> (-6.49%) ⬇️
src/spartan_handler_fsm.erl 67.4% <0%> (+5.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0a78ff...e2583f0. Read the comment docs.

@nlsun nlsun force-pushed the configurable-upstream branch 2 times, most recently from 402d476 to c7027ac Compare May 1, 2017 15:47
@@ -7,7 +7,8 @@
-include_lib("dns/include/dns_records.hrl").

%% API
-export([upstreams_from_questions/1]).
%-export([upstreams_from_questions/1]).
-compile(export_all).

Choose a reason for hiding this comment

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

Any specific reason for using export_all?

find_custom_upstream(QueryLabels) ->
ForwardZones = spartan_config:forward_zones(),

UpstreamFilter =

Choose a reason for hiding this comment

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

Move this to its own function for readability. Somthing like mk_fun_upstream_filter..

@@ -125,6 +129,9 @@ load_json_config(FileBin) ->
process_config_tuple({<<"upstream_resolvers">>, UpstreamResolvers}) ->
UpstreamIPsAndPorts = lists:map(fun (Resolver) -> parse_ipv4_address_with_port(Resolver, 53) end, UpstreamResolvers),
application:set_env(?APP, upstream_resolvers, UpstreamIPsAndPorts);
process_config_tuple({<<"forward_zones">>, Upstreams0}) ->
Upstreams1 = lists:foldl(fun parse_upstream/2, #{}, Upstreams0),

Choose a reason for hiding this comment

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

My personal preference is to use maps:new() instead of #{} that way we safeguard ourselves from internal changes to maps module

@@ -28,6 +28,10 @@ tcp_port() ->
udp_port() ->
application:get_env(?APP, udp_port, 5454).

-spec(forward_zones() -> #{[dns:label()] => [{string(), integer()}]}).
forward_zones() ->
application:get_env(?APP, forward_zones, #{}).

Choose a reason for hiding this comment

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

maps:new()

@nlsun
Copy link
Author

nlsun commented May 1, 2017

@GoelDeepak addressed comments. also make a new type raw_upstream(). As well as pulled out the label parsing into a separate function.

@nlsun nlsun merged commit 218b64d into master May 1, 2017
@GoelDeepak GoelDeepak deleted the configurable-upstream branch May 18, 2017 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants