You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2020/11/23 22:43:19 UTC
[GitHub] [mesos] zorro786 opened a new pull request #371: [MESOS-10199] Fix Host field in client headers
zorro786 opened a new pull request #371:
URL: https://github.com/apache/mesos/pull/371
This PR replaces empty host with actual IP/Port of the message recipient in Host field. IP+Port has been used as per RFC guidelines.
An earlier PR made host field empty https://reviews.apache.org/r/27865
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-742224818
Thanks @vinodkone. I have run the tests on master branch, two tests fail:
`HttpServeTest.Discard` and `HttpServeTest.Pipelining`.
Same tests fail on this PR change too. Rest all pass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] vinodkone merged pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
vinodkone merged pull request #371:
URL: https://github.com/apache/mesos/pull/371
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 commented on a change in pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 commented on a change in pull request #371:
URL: https://github.com/apache/mesos/pull/371#discussion_r539812380
##########
File path: 3rdparty/libprocess/src/tests/process_tests.cpp
##########
@@ -1463,12 +1465,17 @@ TEST_F(ProcessTest, Remote)
message.name = "handler";
message.from = UPID("sender", sender.get());
message.to = process.self();
+ message.body = "hello world";
const string data = MessageEncoder::encode(message);
AWAIT_READY(socket.send(data));
- AWAIT_READY(handler);
+ AWAIT_READY(body);
+ ASSERT_EQ("hello world", body.get());
+
+ AWAIT_READY(pid);
+ ASSERT_EQ(message.from, pid.get());
Review comment:
No it won't, it is just the pid of the sender process. Regardless of what the header is set this should succeed.
I actually wanted to write a Unit Test to verify host header being set at the receiver, however it is not straightforward to me from code to get host headers from libprocess' process. Message handlers in `process/process.hpp` seem to only care about process id and message body. I added the above as an Integration Test to verify connectivity/data transfer with changed header.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-732492130
Ran `make check` in Linux. `HttpServeTest.Discard` test seems flaky, it fails with or without this change.
Also verified agent-master registration/communication behind nginx proxy.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-738489609
Is there any CI job which verifies build for my sanity? I haven't been successful in running all tests due to the flaky HttpServeTest.Discard
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 edited a comment on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 edited a comment on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-732492130
Ran `make check` in Linux. `HttpServeTest.Discard` test seems flaky, it fails with or without this change.
Also verified agent-master registration/communication behind nginx proxy in a cluster.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] bmahler commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
bmahler commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-737557425
Thanks for fixing this, a couple of things, maybe @vinodkone or @asekretenko can have a look after these updates:
* Can you follow the existing logic for the http client? See: https://github.com/apache/mesos/blob/1.11.0/3rdparty/libprocess/src/http.cpp#L1092-L1108
* Can you remove the extra .gitignore file? It's not related to this change and we like to keep our commits tidy with only a single logical change involved
* Can you add or update one of the existing tests in libprocess?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] vinodkone commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
vinodkone commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-742138197
@zorro786 If you believe HTTPDiscard test is flaky you can run the test suite by filter out that test (e.g., make check GTEST_FILTER=-"HttpServeTest.Discard")
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-738476472
> * Can you follow the existing logic for the http client? See: https://github.com/apache/mesos/blob/1.11.0/3rdparty/libprocess/src/http.cpp#L1092-L1108
I find the existing logic redundant in this case, as `message.to.address` is an Inet address not a URL, which already includes an IP and port. `message.to.address` essentially includes the following code block already:
```
else if (request.url.ip.isSome()) {
headers["Host"] = stringify(request.url.ip.get());
}
// Add port for non-standard ports.
if (request.url.port.isSome() &&
request.url.port != 80 &&
request.url.port != 443) {
headers["Host"] += ":" + stringify(request.url.port.get());
}
```
Please let me know if you meant something else.
> * Can you remove the extra .gitignore file? It's not related to this change and we like to keep our commits tidy with only a single logical change involved
Done
> * Can you add or update one of the existing tests in libprocess?
Done
cc: @vinodkone @asekretenko
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] zorro786 edited a comment on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
zorro786 edited a comment on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-742224818
Thanks @vinodkone. I have run the tests on master branch in Ubuntu 16.04, two tests fail:
`HttpServeTest.Discard` and `HttpServeTest.Pipelining`.
Same tests fail on this PR change too. Rest all pass.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] vinodkone commented on pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
vinodkone commented on pull request #371:
URL: https://github.com/apache/mesos/pull/371#issuecomment-744636884
LGTM. I merged it. Thanks for the fix @zorro786
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] vinodkone commented on a change in pull request #371: [MESOS-10199] Fix Host field in client headers
Posted by GitBox <gi...@apache.org>.
vinodkone commented on a change in pull request #371:
URL: https://github.com/apache/mesos/pull/371#discussion_r539734862
##########
File path: 3rdparty/libprocess/src/tests/process_tests.cpp
##########
@@ -1463,12 +1465,17 @@ TEST_F(ProcessTest, Remote)
message.name = "handler";
message.from = UPID("sender", sender.get());
message.to = process.self();
+ message.body = "hello world";
const string data = MessageEncoder::encode(message);
AWAIT_READY(socket.send(data));
- AWAIT_READY(handler);
+ AWAIT_READY(body);
+ ASSERT_EQ("hello world", body.get());
+
+ AWAIT_READY(pid);
+ ASSERT_EQ(message.from, pid.get());
Review comment:
Would this assert fail if HOST header is not set with a real ip port?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org