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

Invalid host in HTTP header if setting host via service_name for channel inited by list or file #1940

Closed
thorneliu opened this issue Sep 30, 2022 · 6 comments

Comments

@thorneliu
Copy link
Contributor

thorneliu commented Sep 30, 2022

Describe the bug (描述bug)
In brpc::PROTOCOL_HTTP, we will set the host field by service_name when

  • host is not explicitly set
  • host is not set in the url
    if (_options.protocol == brpc::PROTOCOL_HTTP) {
        URI& uri = cntl->http_request().uri();
        if (uri.host().empty() && !_service_name.empty()) {
            uri.SetHostAndPort(_service_name);
        }
    }

However, when the channel is inited via na_url shceme = list or file, such _service_name is not valid host field which
results in 400 BadRequest

To Reproduce (复现方法)
Init a channel with ipport list and http_request uri with relative path, eg:

    brpc::Channel channel;
    brpc::ChannelOptions opt;
    opt.protocol = brpc::PROTOCOL_HTTP;

    std::string server("list://127.0.0.1:8000,127.0.0.1:8001");
    std::string lb("rr");
    const int ret = channel.Init(server.c_str(), lb.c_str(), &opt);

    brpc::Controller cntl;
    cntl.http_request().uri() = "/dummy";
    channel.CallMethod(nullptr, &cntl, nullptr, nullptr, nullptr);

In the http request, the host field will be set as :
host: 127.0.0.1:8000,127.0.0.1:8001

Similarly, when the ns_url is a file://conf/hosts, the host field in HTTP requeset will be:
host: conf

Expected behavior (期望行为)
host should be set as remote_side IP+Port in the senarios above

Versions (各种版本)
OS: Ubuntu 20.04
Compiler: g++ (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0
brpc: latest 59f5d77
protobuf: 3.6.1

Additional context/screenshots (更多上下文/截图)
issue

@thorneliu
Copy link
Contributor Author

We could

  • A: check if service name a valid host for HTTP
  • B: set host field by remote_side

option B would revert some changes dab62e4 by @guodongxiaren

@wwbmmm
Copy link
Contributor

wwbmmm commented Oct 14, 2022

We could

  • A: check if service name a valid host for HTTP
  • B: set host field by remote_side

option B would revert some changes dab62e4 by @guodongxiaren

I think we can check if ns_url shceme is 'http' or 'https', if true, set host field by service_name

@thorneliu
Copy link
Contributor Author

We could

  • A: check if service name a valid host for HTTP
  • B: set host field by remote_side

option B would revert some changes dab62e4 by @guodongxiaren

I think we can check if ns_url shceme is 'http' or 'https', if true, set host field by service_name

hmm. looks good. I can provide a patch based on this

@wasphin
Copy link
Member

wasphin commented Oct 28, 2022

Is it possible that we fix the host field after a server has been selected?

B: set host field by remote_side

If schema is not http(s), but protocol is HTTP.

@thorneliu
Copy link
Contributor Author

thorneliu commented Oct 30, 2022

Is it possible that we fix the host field after a server has been selected?

B: set host field by remote_side

If schema is not http(s), but protocol is HTTP.

Currently the host is set by remote side iff the host is not set. however such scheme info is not available when host is updated by remote_side inside http impl

@thorneliu
Copy link
Contributor Author

correction merged 3f8e1cc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants