nickva commented on code in PR #5983: URL: https://github.com/apache/couchdb/pull/5983#discussion_r3159019034
########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 +]). + +-type dns_override() :: {binary(), binary()}. + +-spec resolve_host(string()) -> {string(), string() | undefined}. +resolve_host(Host) -> + case find_override(unicode:characters_to_binary(Host), get_overrides()) of Review Comment: Why use unicode here instead of list_to_binary. DNS hostnames should be ASCII I think? ########## src/couch_replicator/src/couch_replicator_httpc.erl: ########## @@ -113,6 +114,10 @@ send_ibrowse_req(#httpdb{headers = BaseHeaders} = HttpDb0, Params) -> {Headers2, HttpDb} = couch_replicator_auth:update_headers(HttpDb0, Headers1), Url = full_url(HttpDb, Params), Body = get_value(body, Params, []), + + % Apply DNS override using connect_to ibrowse option + #url{host = Host, protocol = Protocol} = ibrowse_lib:parse_url(Url), Review Comment: We'd be parsing the url on every request, I wonder if it would work to cache in the httpdb record the host already parsed? Or maybe we could cache the override ssl / connect_to settings in `#httpd{}`... ########## src/couch_replicator/src/couch_replicator_httpc.erl: ########## @@ -535,6 +564,15 @@ merge_headers(Headers1, Headers2) when is_list(Headers1), is_list(Headers2) -> Merged = mochiweb_headers:enter_from_list(Headers1 ++ Headers2, Empty), mochiweb_headers:to_list(Merged). +%% Add SNI to SSL options +add_sni_option(IbrowseOpts, Host) -> + SslOpts = proplists:get_value(ssl_options, IbrowseOpts, []), + SslOpts1 = [ + {server_name_indication, Host} Review Comment: Can all values for Host be a SNI? Like what if it's an ipv6 or ipv4 address, it's not a "name" technically ########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 Review Comment: This is exported for testing mostly? I don't think it's used otherwise, if so we could skip exporting ########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 +]). + +-type dns_override() :: {binary(), binary()}. + +-spec resolve_host(string()) -> {string(), string() | undefined}. +resolve_host(Host) -> + case find_override(unicode:characters_to_binary(Host), get_overrides()) of + {ok, Target} -> + {binary_to_list(Target), Host}; + not_found -> + {Host, undefined} + end. + +-spec get_overrides() -> [dns_override()]. +get_overrides() -> Review Comment: It would be nice to have to reparse everything on each resolve. We could use a persistent_term perhaps, but it would add a more code here... ########## src/couch_replicator/src/couch_replicator_auth_session.erl: ########## @@ -311,11 +311,44 @@ refresh(#state{session_url = Url, user = User, pass = Pass} = State) -> {ok, string(), headers(), binary()} | {error, term()}. http_request(#state{httpdb_pool = Pool} = State, Url, Headers, Method, Body) -> Timeout = State#state.httpdb_timeout, - Opts = [ + + % Apply DNS override using connect_to ibrowse option + {TargetHost, OriginalHost, Proto} = + case ibrowse_lib:parse_url(Url) of Review Comment: Could we match the `#url{}` record, it's a bit nicer than element-wise tuple access? Or it's to avoid importing an extra module. ########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 +]). + +-type dns_override() :: {binary(), binary()}. + +-spec resolve_host(string()) -> {string(), string() | undefined}. +resolve_host(Host) -> + case find_override(unicode:characters_to_binary(Host), get_overrides()) of + {ok, Target} -> + {binary_to_list(Target), Host}; + not_found -> + {Host, undefined} + end. + +-spec get_overrides() -> [dns_override()]. +get_overrides() -> + case config:get("replicator", "dns_overrides", undefined) of + undefined -> + []; + ConfigStr -> + parse_config(ConfigStr) + end. + +-spec parse_config(string()) -> [dns_override()]. +parse_config(ConfigStr) -> + ConfigBin = unicode:characters_to_binary(ConfigStr), Review Comment: Maybe we don't need unicode? ########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 +]). + +-type dns_override() :: {binary(), binary()}. + +-spec resolve_host(string()) -> {string(), string() | undefined}. +resolve_host(Host) -> + case find_override(unicode:characters_to_binary(Host), get_overrides()) of + {ok, Target} -> + {binary_to_list(Target), Host}; + not_found -> + {Host, undefined} + end. + +-spec get_overrides() -> [dns_override()]. +get_overrides() -> + case config:get("replicator", "dns_overrides", undefined) of + undefined -> + []; + ConfigStr -> + parse_config(ConfigStr) + end. + +-spec parse_config(string()) -> [dns_override()]. +parse_config(ConfigStr) -> + ConfigBin = unicode:characters_to_binary(ConfigStr), + Entries = binary:split(ConfigBin, <<",">>, [global, trim]), + lists:filtermap(fun parse_entry/1, Entries). + +parse_entry(<<>>) -> + false; +parse_entry(Entry0) -> + Entry = string:trim(Entry0), + case binary:split(Entry, <<":">>) of + [Pattern0, Target0] -> + Pattern = string:trim(Pattern0), + Target = string:trim(Target0), + case {Pattern, Target} of + {<<>>, _} -> invalid_entry(Entry); + {_, <<>>} -> invalid_entry(Entry); + _ -> {true, {Pattern, Target}} + end; + _ -> + invalid_entry(Entry) + end. + +invalid_entry(Entry) -> + couch_log:warning("Invalid dns_override entry: ~ts", [Entry]), + false. + +find_override(_Host, []) -> + not_found; +find_override(Host, [{Pattern, Target} | Rest]) -> + case match_pattern(Host, Pattern) of + true -> + {ok, Target}; + false -> + find_override(Host, Rest) + end. + +-spec match_pattern(binary(), binary()) -> boolean(). +match_pattern(Host, <<"*", Suffix/binary>>) when is_binary(Host) -> + % wildcard match: extract last N bytes from Host and compare to Suffix Review Comment: We support only leading wildcards? Can't have them in the middle of the term, that's fine just that we should ensure we mention that in the docs and etc ini comments ########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 +]). + +-type dns_override() :: {binary(), binary()}. + +-spec resolve_host(string()) -> {string(), string() | undefined}. +resolve_host(Host) -> + case find_override(unicode:characters_to_binary(Host), get_overrides()) of + {ok, Target} -> + {binary_to_list(Target), Host}; + not_found -> + {Host, undefined} + end. + +-spec get_overrides() -> [dns_override()]. +get_overrides() -> + case config:get("replicator", "dns_overrides", undefined) of + undefined -> + []; + ConfigStr -> + parse_config(ConfigStr) + end. + +-spec parse_config(string()) -> [dns_override()]. +parse_config(ConfigStr) -> + ConfigBin = unicode:characters_to_binary(ConfigStr), + Entries = binary:split(ConfigBin, <<",">>, [global, trim]), + lists:filtermap(fun parse_entry/1, Entries). + +parse_entry(<<>>) -> + false; +parse_entry(Entry0) -> + Entry = string:trim(Entry0), + case binary:split(Entry, <<":">>) of + [Pattern0, Target0] -> + Pattern = string:trim(Pattern0), + Target = string:trim(Target0), + case {Pattern, Target} of + {<<>>, _} -> invalid_entry(Entry); + {_, <<>>} -> invalid_entry(Entry); + _ -> {true, {Pattern, Target}} + end; + _ -> + invalid_entry(Entry) + end. + +invalid_entry(Entry) -> + couch_log:warning("Invalid dns_override entry: ~ts", [Entry]), + false. + +find_override(_Host, []) -> + not_found; +find_override(Host, [{Pattern, Target} | Rest]) -> + case match_pattern(Host, Pattern) of + true -> + {ok, Target}; + false -> + find_override(Host, Rest) + end. + +-spec match_pattern(binary(), binary()) -> boolean(). +match_pattern(Host, <<"*", Suffix/binary>>) when is_binary(Host) -> Review Comment: Technically I think dns names are case-insensitive, may we could string:lowercase/1 them before matching? ########## src/couch_replicator/src/couch_replicator_dns.erl: ########## @@ -0,0 +1,93 @@ +% Licensed under the Apache License, Version 2.0 (the "License"); you may not +% use this file except in compliance with the License. You may obtain a copy of +% the License at +% +% http://www.apache.org/licenses/LICENSE-2.0 +% +% Unless required by applicable law or agreed to in writing, software +% distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +% WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +% License for the specific language governing permissions and limitations under +% the License. + +-module(couch_replicator_dns). + +-export([ + resolve_host/1, + parse_config/1, + match_pattern/2, + get_overrides/0 +]). + +-type dns_override() :: {binary(), binary()}. + +-spec resolve_host(string()) -> {string(), string() | undefined}. +resolve_host(Host) -> + case find_override(unicode:characters_to_binary(Host), get_overrides()) of + {ok, Target} -> + {binary_to_list(Target), Host}; + not_found -> + {Host, undefined} + end. + +-spec get_overrides() -> [dns_override()]. +get_overrides() -> + case config:get("replicator", "dns_overrides", undefined) of + undefined -> + []; + ConfigStr -> + parse_config(ConfigStr) + end. + +-spec parse_config(string()) -> [dns_override()]. +parse_config(ConfigStr) -> + ConfigBin = unicode:characters_to_binary(ConfigStr), + Entries = binary:split(ConfigBin, <<",">>, [global, trim]), + lists:filtermap(fun parse_entry/1, Entries). + +parse_entry(<<>>) -> + false; +parse_entry(Entry0) -> + Entry = string:trim(Entry0), + case binary:split(Entry, <<":">>) of Review Comment: the `:` is used in port separators (but maybe we don't have to worry about that here) but ipv6 addresses have them a bunch, too, wonder if that's would make using this separator a tricky a bit -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
