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.

Sign up for GitHub

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

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

Merged
merged 10 commits into from
Sep 6, 2021
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dab62e4
Set host not ip when not in url
guodongxiaren Aug 16, 2021
95606cc
ParseHostname
guodongxiaren Aug 27, 2021
486de0d
update PraseHostname && add ut
guodongxiaren Aug 28, 2021
5085c5e
update _hostname to _service_name
guodongxiaren Aug 28, 2021
a5c72a0
set port
guodongxiaren Aug 28, 2021
3cd4840
rm ParseHostname and restore http_message.cpp
guodongxiaren Aug 28, 2021
b1ebfe5
reduce ParseUrl() for Init()
guodongxiaren Aug 29, 2021
93b4efe
update en/http_client.mp
guodongxiaren Aug 30, 2021
1d04a27
redesign InitSingle
guodongxiaren Sep 2, 2021
a3c02fa
update docs
guodongxiaren Sep 2, 2021
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
4 changes: 3 additions & 1 deletion docs/cn/http_client.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"。

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@guodongxiaren guodongxiaren Aug 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

不是。在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字段。不过你说可能有兼容性问题，所以我又去掉了。

Copy link
Member

Choose a reason for hiding this comment

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

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

符合谦容性的实现是：

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

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

对应的字段在h2中叫":authority"。

Expand Down
4 changes: 3 additions & 1 deletion docs/en/http_client.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,9 @@ If user already sets `Host` header(case insensitive), framework makes no change.

If user does not set `Host` header and the URL has host, for example http://www.foo.com/path, the http request contains "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", 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`.
Copy link
Member

Choose a reason for hiding this comment

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

  1. There is an extra space before 'for example'
  2. it should be ',' instead of '.' after 'if...'

Copy link
Member Author

Choose a reason for hiding this comment

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

改了


The header is named ":authority" in h2.

Expand Down
22 changes: 14 additions & 8 deletions src/brpc/channel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,11 @@ int Channel::InitSingle(const butil::EndPoint& server_addr_and_port,
if (InitChannelOptions(options) != 0) {
return -1;
}
if (_options.protocol == brpc::PROTOCOL_HTTP &&
::strncmp(raw_server_address, "https://", 8) == 0) {
std::string scheme;
ParseURL(raw_server_address, &scheme, &_service_name, NULL);
if (_options.protocol == brpc::PROTOCOL_HTTP && scheme == "https://") {
if (_options.mutable_ssl_options()->sni_name.empty()) {
ParseURL(raw_server_address,
NULL, &_options.mutable_ssl_options()->sni_name, NULL);
_options.mutable_ssl_options()->sni_name = _service_name;
}
}
const int port = server_addr_and_port.port;
Expand Down Expand Up @@ -325,11 +325,11 @@ int Channel::Init(const char* ns_url,
if (InitChannelOptions(options) != 0) {
return -1;
}
if (_options.protocol == brpc::PROTOCOL_HTTP &&
::strncmp(ns_url, "https://", 8) == 0) {
std::string scheme;
ParseURL(ns_url, &scheme, &_service_name, NULL);
if (_options.protocol == brpc::PROTOCOL_HTTP && scheme == "https://") {
if (_options.mutable_ssl_options()->sni_name.empty()) {
ParseURL(ns_url,
NULL, &_options.mutable_ssl_options()->sni_name, NULL);
_options.mutable_ssl_options()->sni_name = _service_name;
}
}
LoadBalancerWithNaming* lb = new (std::nothrow) LoadBalancerWithNaming;
Expand Down Expand Up @@ -386,6 +386,12 @@ void Channel::CallMethod(const google::protobuf::MethodDescriptor* method,
CHECK(cntl->protocol_param().empty());
cntl->protocol_param() = _options.protocol.param();
}
if (_options.protocol == brpc::PROTOCOL_HTTP) {
URI& uri = cntl->http_request().uri();
if (uri.host().empty() && !_service_name.empty()) {
uri.set_host(_service_name);
}
}
cntl->_preferred_index = _preferred_index;
cntl->_retry_policy = _options.retry_policy;
if (_options.enable_circuit_breaker) {
Expand Down
2 changes: 2 additions & 0 deletions src/brpc/channel.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,8 @@ friend class SelectiveChannel;
const char* raw_server_address,
const ChannelOptions* options);

// May be ip, hostname or name_service
std::string _service_name;
butil::EndPoint _server_address;
SocketId _server_id;
Protocol::SerializeRequest _serialize_request;
Expand Down
56 changes: 56 additions & 0 deletions test/brpc_channel_unittest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1958,6 +1958,62 @@ TEST_F(ChannelTest, init_using_naming_service) {
// `lb' should be destroyed after
}

TEST_F(ChannelTest, parse_hostname) {
brpc::ChannelOptions opt;
opt.succeed_without_server = false;
brpc::Channel channel;

ASSERT_EQ(-1, channel.Init("", 8888, &opt));
ASSERT_EQ("", channel._service_name);
ASSERT_EQ(-1, channel.Init("", &opt));
ASSERT_EQ("", channel._service_name);

ASSERT_EQ(0, channel.Init("127.0.0.1", 8888, &opt));
ASSERT_EQ("127.0.0.1", channel._service_name);
ASSERT_EQ(0, channel.Init("127.0.0.1:8888", &opt));
ASSERT_EQ("127.0.0.1", channel._service_name);

ASSERT_EQ(0, channel.Init("localhost", 8888, &opt));
ASSERT_EQ("localhost", channel._service_name);
ASSERT_EQ(0, channel.Init("localhost:8888", &opt));
ASSERT_EQ("localhost", channel._service_name);

opt.protocol = brpc::PROTOCOL_HTTP;
ASSERT_EQ(0, channel.Init("http://baidu.com", &opt));
ASSERT_EQ("baidu.com", channel._service_name);
ASSERT_EQ(0, channel.Init("http://baidu.com", 80, &opt));
ASSERT_EQ("baidu.com", channel._service_name);
ASSERT_EQ(0, channel.Init("https://baidu.com", &opt));
ASSERT_EQ("baidu.com", channel._service_name);
ASSERT_EQ(0, channel.Init("https://baidu.com", 443, &opt));
ASSERT_EQ("baidu.com", channel._service_name);

ASSERT_EQ(0, channel.Init("http://baidu.com", "rr", &opt));
ASSERT_EQ("baidu.com", channel._service_name);
ASSERT_EQ(0, channel.Init("https://baidu.com", "rr", &opt));
ASSERT_EQ("baidu.com", channel._service_name);

const char *address_list[] = {
"10.127.0.1:1234",
"10.128.0.1:1234 enable",
"10.129.0.1:1234",
"localhost:1234",
"baidu.com:1234"
};
butil::TempFile tmp_file;
{
FILE* fp = fopen(tmp_file.fname(), "w");
for (size_t i = 0; i < ARRAY_SIZE(address_list); ++i) {
ASSERT_TRUE(fprintf(fp, "%s\n", address_list[i]));
}
fclose(fp);
}
brpc::Channel ns_channel;
std::string ns = std::string("file://") + tmp_file.fname();
ASSERT_EQ(0, ns_channel.Init(ns.c_str(), "rr", &opt));
ASSERT_EQ(tmp_file.fname(), ns_channel._service_name);
}

TEST_F(ChannelTest, connection_failed) {
for (int i = 0; i <= 1; ++i) { // Flag SingleServer
for (int j = 0; j <= 1; ++j) { // Flag Asynchronous
Expand Down