Set hostname rather than ip when channel Init by hostname but not host in http_requst().uri() #1529

guodongxiaren · 2021-08-16T14:28:57Z

当前brpc的实现中，如果http请求，uri没有设置全地址，比如"/get_result?a=b"，那么会默认用ip+port作为HTTP 报头的header字段。
但是这样有问题，即使我的Channel初始化的时候使用的是域名，比如：http://www.foo.com，最后对对端看到的还是ip+端口。但像Apache/Nginx这种Web Server都是只是单地址多服务的，http://www.foo.com 和 www.bar.com 可能是同一个ip和端口（对应的是Apache的地址），然后Web Server会通过Host字段来路由到具体的服务。如果Apache看到的是Ip+端口，则不知道该把请求路由到哪个服务。

当前只能通过发送请求之时给uri填写完整的路径来解决，比如http://www.foo.com/get_result?a=b，但是感觉还是应该框架补全能力比较好。
因为比如我有多个环境，有测试、生产环境。他们访问下游HTTP服务的时候，每个环境的服务请求的接口地址是不同的。
比如生产环境是：http://www.foo.com/get_result?a=b，而测试环境是http://www.foo-test.com/get_result?a=b，我的服务在给Channel初始化的时候已经依据环境来用不同的域名地址进行了初始化。但是当实际给下游发请求的时候（CallMethod之前构造http_request的时候），还需要再判断一遍系统运行环境，再给cntl->http_request().uri() 视环境来赋不同的值。有些冗余，我感觉还是应该框架做好，减少冗余配置和冗余判断。

当然要实现这一feature，可实现的方案有很多。我这里采用的方案是也是当前感觉改动最小，影响最小但能达到目的的一个。受限于Brpc的历史设计，导致从URL解析出来的域名（或机器名，hostname）没有被存储。
因此我这里重新做了ParseUrl和hostname2ip的操作。虽然Init过程中其实之前就有间接调用到这两个函数，但是确实无法被Channel拿到结果。

guodongxiaren · 2021-08-16T16:57:56Z

@zyearn

guodongxiaren · 2021-08-19T10:17:20Z

@jamesge @zyearn 辛苦看看

zyearn · 2021-08-19T21:37:51Z

嗯感谢反馈，这两天我们看一下

guodongxiaren · 2021-08-20T00:59:51Z

嗯感谢反馈，这两天我们看一下

嗯嗯，Apache有个VirtualHost的功能，就是让一个端口，根据域名路由到不同服务

guodongxiaren · 2021-08-25T11:48:10Z

@zyearn 请问看了吗

zyearn · 2021-08-26T20:07:17Z

src/brpc/channel.cpp

+                _hostname.swap(host);
+            }
+        }
+    }


这段可以直接写在InitSingle里吗？这样就不用重复写在两个Init里了

另外这段可以封装成函数，因为在single和集群同时用到了

感觉可以不用判断协议，对所有协议而言把这个hostname存下来没什么坏处，以后拓展协议用到的话，也更容易。

zyearn · 2021-08-26T20:17:06Z

src/brpc/channel.cpp

+    if (_options.protocol == brpc::PROTOCOL_HTTP) {
+        const URI& uri = cntl->http_request().uri();
+        if (uri.host().empty() && !_hostname.empty()) {
+            const_cast<URI&>(uri).set_host(_hostname);


这里直接拿到mutable uri就不需要const转化了吧？

哦。对。我搞错了，以为拿到的是const的，其实就是mutable的。我改下

guodongxiaren · 2021-08-27T15:51:54Z

@zyearn 修改了，但是CI的UT测试不稳定。在我没涉及的地方出问题，而且每次不一样。。

zyearn · 2021-08-27T20:46:04Z

src/brpc/channel.cpp

@@ -332,6 +333,7 @@ int Channel::Init(const char* ns_url,
                     NULL, &_options.mutable_ssl_options()->sni_name, NULL);
        }
    }
+    ParseHostname(ns_url);


如果这里返回码不重要的话，原函数需要返回int吗

src/brpc/channel.cpp

guodongxiaren · 2021-08-28T09:40:38Z

@zyearn 已经根据反馈修改了代码。有几个地方和你确认一下。

Init用命名服务初始化的重载函数中，我还是判断了HTTP协议，因为如果是file:// 或者consul:// ，那么参数中的ns_url进行ParseHostname无意义。在InitSingle中没有判断协议了。
当前的实现如果初始化的URL中是机器IP，调用ParseHostname（间接调用hostname2ip）也会解析成功，最后把ip存到 _hostname中，不知道是否需要额外规避掉这种情况，让 _hostname只存储主机名或者网络域名？

zyearn · 2021-08-28T13:21:06Z

@zyearn 已经根据反馈修改了代码。有几个地方和你确认一下。

Init用命名服务初始化的重载函数中，我还是判断了HTTP协议，因为如果是file:// 或者consul:// ，那么参数中的ns_url进行ParseHostname无意义。在InitSingle中没有判断协议了。

当前的实现如果初始化的URL中是机器IP，调用ParseHostname（间接调用hostname2ip）也会解析成功，最后把ip存到 _hostname中，不知道是否需要额外规避掉这种情况，让 _hostname只存储主机名或者网络域名？

这样的话两处代码就不统一了。另外_hostname这个名字在channel中改成_service_name是不是更通用一些(https://github.com/apache/incubator-brpc/blob/master/src/brpc/naming_service.h#L53 )？然后在HTTP协议中把_service_name作为hostname，在别的协议中可能担任另外的作用（也可以不用，例如你说的file/consul），这样似乎就不用判断协议了。
如果按照1改成_service_name的话，就不存在这个问题了，因为这里预期存的是用户的输入。域名就存域名，ip的话就存ip

What do you think?

guodongxiaren · 2021-08-28T14:54:47Z

@zyearn 改成_service_name了，也取消了协议的判断。另外给 src/brpc/details/http_message.cpp 中从uri.host()给Header补Host字段的逻辑，加上了补port。

因为我这个逻辑走到这里，uri.port()还是 -1 不会被追加到Host字段中（Host只有域名）。当然Host字段中的port在Http协议中也不是必须的，不过我PR里之前修改的文档描述中写了，对方收到的Host会包含端口。所以我这里和我的文档做一下对齐。

wasphin · 2021-08-28T15:20:55Z

src/brpc/channel.cpp

@@ -332,6 +333,7 @@ int Channel::Init(const char* ns_url,
                     NULL, &_options.mutable_ssl_options()->sni_name, NULL);
        }
    }
+    ParseServiceName(ns_url);


这里能拿到 service_name 吗？拿到的会是什么样？

拿到的就是 file:// ，list://, consul:// 这些前缀后面的部分，我加了一个用 file:// 初始化channel的UT ,你可以看看。
ParseHostname就是调用了一下ParseURL，提取了 xxx:// 之后，端口号: 或路径/ 之前的部分，作为service_name

这一整段可以合并一下，少一次parse：

Parseurl... if (_options.protocol == brpc::PROTOCOL_HTTP && ::strncmp(ns_url, "https://", 8) == 0) { if (_options.mutable_ssl_options()->sni_name.empty()) { _options.mutable_ssl_options()->sni_name = _service_name; } }

zyearn · 2021-08-28T16:15:28Z

因为我这个逻辑走到这里，uri.port()还是 -1 不会被追加到Host字段中（Host只有域名）。当然Host字段中的port在Http协议中也不是必须的，不过我PR里之前修改的文档描述中写了，对方收到的Host会包含端口。所以我这里和我的文档做一下对齐。

Hi，这个改动打破backward compatibility了，不知道会有什么影响，另外文档里也说“若用户没有填且URL中包含host，比如http://www.foo.com/path，则http request中会包含"Host: www.foo.com"，我们最好可以不改这个。

zyearn · 2021-08-28T16:19:17Z

src/brpc/channel.cpp

@@ -332,6 +333,7 @@ int Channel::Init(const char* ns_url,
                     NULL, &_options.mutable_ssl_options()->sni_name, NULL);
        }
    }
+    ParseServiceName(ns_url);


这一整段可以合并一下，少一次parse：

Parseurl... if (_options.protocol == brpc::PROTOCOL_HTTP && ::strncmp(ns_url, "https://", 8) == 0) { if (_options.mutable_ssl_options()->sni_name.empty()) { _options.mutable_ssl_options()->sni_name = _service_name; } }

zyearn · 2021-08-28T16:21:27Z

src/brpc/channel.cpp

+void Channel::ParseServiceName(const char* server_addr) {
+    if (ParseURL(server_addr, NULL, &_service_name, NULL) != 0) {
+        _service_name.clear();
+    }


当时是觉得函数太长，如果目前这个函数这么短了，感觉就不需要封装ParseServiceName了，在调用的地方直接ParseURL(...)即可，如果失败的话_service_name都不会被设置（参考ParseURL的实现），所以_service_name.clear()也不需要了。

zyearn · 2021-08-28T16:24:08Z

src/brpc/channel.cpp

@@ -295,6 +295,7 @@ int Channel::InitSingle(const butil::EndPoint& server_addr_and_port,
                     NULL, &_options.mutable_ssl_options()->sni_name, NULL);
        }
    }
+    ParseServiceName(raw_server_address);


和下面的comment一样，移到前面去可以合并一下

zyearn · 2021-08-28T16:25:18Z

src/brpc/details/http_message.cpp

@@ -567,6 +567,8 @@ void MakeRawHttpRequest(butil::IOBuf* request,
            os << uri.host();
            if (uri.port() >= 0) {
                os << ':' << uri.port();
+            } else if (remote_side.port != 0) {
+                os << ':' << remote_side.port;


As comments said, we can't afford to break the backward compatibility.

guodongxiaren · 2021-08-28T17:32:44Z

@zyearn 哦，那个if 我忘了合并，明天我改完，你再merge吧。

zyearn · 2021-08-28T19:12:32Z

@zyearn 哦，那个if 我忘了合并，明天我改完，你再merge吧。

Okay

guodongxiaren · 2021-08-29T02:32:20Z

@zyearn 再看下

guodongxiaren · 2021-08-29T10:41:16Z

@zyearn 有空看下，CI也过了

zyearn · 2021-08-29T19:31:09Z

docs/cn/http_client.md

@@ -114,7 +114,9 @@ URL的一般形式如下图：

 若用户没有填且URL中包含host，比如http://www.foo.com/path，则http request中会包含"Host: www.foo.com"。

-若用户没有填且URL不包含host，比如"/index.html?name=value"，则框架会以目标server的ip和port为Host，地址为10.46.188.39:8989的http server将会看到"Host: 10.46.188.39:8989"。
+若用户没有填且URL不包含host，比如"/index.html?name=value"，但如果Channel初始化的地址包含域名，则框架会以域名作为Host，比如"http://www.foo.com"，该http server将会看到"Host: www.foo.com"。如果地址是"http://www.foo.com:8989"，则该http server将会看到"Host: www.foo.com"。
+


最后这个Host是不是应该是"Host: www.foo.com:8989"?

不是。在http_message.cpp中：

if (h->GetHeader("host") == NULL) { os << "Host: "; if (!uri.host().empty()) { os << uri.host(); if (uri.port() >= 0) { os << ':' << uri.port(); } } else if (remote_side.port != 0) { os << remote_side; } os << BRPC_CRLF; }

URL不包含host，但是channel初始化的时候包含的情况，我给uri的host字段赋了值。会走到：if (!uri.host().empty())的if 逻辑中。但是下面的uri.port() 是-1，所以不会把:port 追加到host中。
我前面有个commit是改了http_message.cpp，改成：

if (h->GetHeader("host") == NULL) { os << "Host: "; if (!uri.host().empty()) { os << uri.host(); if (uri.port() >= 0) { os << ':' << uri.port(); } else if (remote_side.port != 0) { // 加了这个逻辑 os << ':' << remote_side.port; } } else if (remote_side.port != 0) { os << remote_side; } os << BRPC_CRLF; }

从remote_side中拿port拼接到Host字段。不过你说可能有兼容性问题，所以我又去掉了。

Hi, 我之前说的谦容性问题是，实现将Host: www.aa.com 变成了 Host: www.aa.com:80。现在这样实现的话 www.aa.com和www.aa.com:8899对应到请求全部都是Host: www.aa.com，不太符合预期 https://datatracker.ietf.org/doc/html/rfc2616#section-14.23

符合谦容性的实现是：

channel.Init("www.aa.com", ...) -> Host: www.aa.com 不变

channel.Init("www.aa.com:8899", ...) -> Host: www.aa.com:8899

要想个办法把ParseUrl出来的port，也在set_host的时候顺便set_port，这样http_message.cpp就不用改了。简单看了下，似乎把parse出来的port追加到_service_name后面，然后替换set_host为uri.SetHostAndPort(_service_name)就可以了。看看可行么？

有一点问题，如果调用的是 channel.Init("www.aa.com", 8899)，那么对方收到的Host还是不包含端口号的。因为InitSingle中解析的原始地址，是Init的第一个参数。
感觉不存在十全十美的解决方案，我感觉还是要在CallMethod的时候，当uri不包含host的时候，把host和port都拼上。不影响uri中包含host时的处理逻辑。

另外一个方案就是在两个Init重载中，记录一下(新增bool成员变量存储标记，或者直接存原始port)，InitSingle中判断是否单独设置了port参数。

zyearn · 2021-08-29T19:34:05Z

docs/en/http_client.md

-If user does not set host header and the URL does not have host as well,  for example "/index.html?name=value", framework sets `Host` header with IP and port of the target server. A http server at 10.46.188.39:8989 should see `Host: 10.46.188.39:8989`.
+If user does not set host header and the URL does not have host as well, for example "/index.html?name=value", but if the address initialized by the channel contains domain name. framework sets `Host` header with domain name of the target server. if this address is "http://www.foo.com", this http server should see `Host: www.foo.com`. if this address is "http://www.foo.com:8989", this http server should be see `Host: www.foo.com`.
+
+If user does not set host header and the URL does not have host as well,  for example "/index.html?name=value", and the address initialized by the channel doesn't contain domain name. framework sets `Host` header with IP and port of the target server. A http server at 10.46.188.39:8989 should see `Host: 10.46.188.39:8989`.


There is an extra space before 'for example'

it should be ',' instead of '.' after 'if...'

guodongxiaren · 2021-08-30T05:45:55Z

@zyearn 你的评论我回复了。看下

guodongxiaren

@zyearn 我改了一下InitSingle的函数参数，多了一个raw_port参数（默认值-1），让InitSingle可以感知是从哪个Init重载调用进来的。

如果raw_port不等于-1，那么就是用它拼接到hostname后面作为 _service_name。
如果raw_port为-1，就去解析 raw_server_address(其实Init的第一个参数）字符串，看能不能解析到port。能的话就追加到hostname后面作为 _service_name，否则，直接用 hostname作为 _service_name。

CallMethod中调用SetHostAndPort

guodongxiaren · 2021-09-02T14:02:04Z

src/brpc/channel.cpp

@@ -256,7 +256,7 @@ int Channel::Init(const char* server_addr, int port,
            return -1;
        }
    }
-    return InitSingle(point, server_addr, options);
+    return InitSingle(point, server_addr, options, port);


channel.Init("www.aa.com", 8899，...) 这个Init函数会调用这个带port参数的InitSingle

guodongxiaren · 2021-09-02T14:03:03Z

src/brpc/channel.cpp

-    if (_options.protocol == brpc::PROTOCOL_HTTP &&
-        ::strncmp(raw_server_address, "https://", 8) == 0) {
+    std::string scheme;
+    int* port_out = raw_port == -1 ? &raw_port: NULL;


如果raw_port 是默认值-1，那么就取它的地址，否则取NULL（表示raw_port已经有值了，不需要再从raw_server_address中解析）

guodongxiaren · 2021-09-02T14:03:33Z

src/brpc/channel.cpp

+    int* port_out = raw_port == -1 ? &raw_port: NULL;
+    ParseURL(raw_server_address, &scheme, &_service_name, port_out);
+    if (raw_port != -1) {
+        _service_name.append(":").append(std::to_string(raw_port));


hostname:port 组合成 _service_name

guodongxiaren · 2021-09-02T14:04:22Z

src/brpc/channel.cpp

-        ::strncmp(ns_url, "https://", 8) == 0) {
+    std::string scheme;
+    int raw_port = -1;
+    ParseURL(ns_url, &scheme, &_service_name, &raw_port);


如果是name_service的Init，那么直接取尝试从ns_url中解析port

guodongxiaren · 2021-09-02T14:04:52Z

src/brpc/channel.cpp

+    if (_options.protocol == brpc::PROTOCOL_HTTP) {
+        URI& uri = cntl->http_request().uri();
+        if (uri.host().empty() && !_service_name.empty()) {
+            uri.SetHostAndPort(_service_name);


一次性给uri的Host，port赋值

guodongxiaren · 2021-09-02T14:05:07Z

src/brpc/channel.h

@@ -213,8 +213,10 @@ friend class SelectiveChannel;
    int InitChannelOptions(const ChannelOptions* options);
    int InitSingle(const butil::EndPoint& server_addr_and_port,
                   const char* raw_server_address,
-                   const ChannelOptions* options);
+                   const ChannelOptions* options,
+                   int raw_port = -1);


InitSingle增加了一个带默认值的参数，raw_port

zyearn · 2021-09-04T09:59:10Z

LGTM

Set host not ip when not in url

dab62e4

zyearn reviewed Aug 26, 2021

View reviewed changes

ParseHostname

95606cc

guodongxiaren closed this Aug 27, 2021

guodongxiaren reopened this Aug 27, 2021

guodongxiaren closed this Aug 27, 2021

guodongxiaren reopened this Aug 27, 2021

guodongxiaren closed this Aug 27, 2021

guodongxiaren reopened this Aug 27, 2021

zyearn reviewed Aug 27, 2021

View reviewed changes

update PraseHostname && add ut

486de0d

guodongxiaren added 2 commits August 28, 2021 22:20

update _hostname to _service_name

5085c5e

set port

a5c72a0

wasphin reviewed Aug 28, 2021

View reviewed changes

zyearn reviewed Aug 28, 2021

View reviewed changes

rm ParseHostname and restore http_message.cpp

3cd4840

reduce ParseUrl() for Init()

b1ebfe5

guodongxiaren closed this Aug 29, 2021

guodongxiaren reopened this Aug 29, 2021

zyearn reviewed Aug 29, 2021

View reviewed changes

update en/http_client.mp

93b4efe

guodongxiaren added 2 commits September 2, 2021 21:17

redesign InitSingle

1d04a27

update docs

a3c02fa

guodongxiaren commented Sep 2, 2021

View reviewed changes

zyearn approved these changes Sep 4, 2021

View reviewed changes

zyearn merged commit bc343db into apache:master Sep 6, 2021

Set hostname rather than ip when channel Init by hostname but not host in http_requst().uri() #1529

Set hostname rather than ip when channel Init by hostname but not host in http_requst().uri() #1529

Conversation

guodongxiaren commented Aug 16, 2021 • edited

guodongxiaren commented Aug 16, 2021

guodongxiaren commented Aug 19, 2021

zyearn commented Aug 19, 2021

guodongxiaren commented Aug 20, 2021

guodongxiaren commented Aug 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guodongxiaren commented Aug 27, 2021

Choose a reason for hiding this comment

guodongxiaren commented Aug 28, 2021 • edited

zyearn commented Aug 28, 2021 • edited

guodongxiaren commented Aug 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyearn commented Aug 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guodongxiaren commented Aug 28, 2021 • edited

zyearn commented Aug 28, 2021

guodongxiaren commented Aug 29, 2021

guodongxiaren commented Aug 29, 2021

Choose a reason for hiding this comment

guodongxiaren Aug 30, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guodongxiaren commented Aug 30, 2021

guodongxiaren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyearn commented Sep 4, 2021

guodongxiaren commented Aug 16, 2021 •

edited

guodongxiaren commented Aug 28, 2021 •

edited

zyearn commented Aug 28, 2021 •

edited

guodongxiaren commented Aug 28, 2021 •

edited

guodongxiaren Aug 30, 2021 •

edited