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

rgw: civetweb fixes for v1.1 upgrade #21123

Merged
merged 4 commits into from Apr 13, 2018

Conversation

theanalyst
Copy link
Member

@theanalyst theanalyst commented Mar 29, 2018

  • Update civetweb sha1
  • Resolve "validate_http_method" based on discussions in rgw: civetweb update to 1.10 civetweb#24
    (update: this was readded as swift tests depend on this)
    This changeset does the following:
  • mg_request_info::mg_header is no longer a type (it is just mg_header, used auto instead)
  • request_info->uri is deprecated, move to request_info->local_uri
  • tentatively dropping validate_http_method, needs discussion

@@ -55,7 +55,7 @@ int RGWCivetWebFrontend::run()
std::to_string(g_conf->rgw_thread_pool_size));
set_conf_default(conf_map, "decode_url", "no");
set_conf_default(conf_map, "enable_keep_alive", "yes");
set_conf_default(conf_map, "validate_http_method", "no");
//set_conf_default(conf_map, "validate_http_method", "no");
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: drop this once we decide whether it is necessary

@theanalyst
Copy link
Member Author

theanalyst commented Apr 5, 2018

Initial qe run: http://pulpito.ceph.com/abhi-2018-04-04_15:05:26-rgw-wip-civetweb-1.1-distro-basic-smithi/

We have failures in s3-tests with rfc2616 and swift failures (due to pip?), dropping the rfc2616 checks and rerunning as http://pulpito.ceph.com/abhi-2018-04-05_17:09:22-rgw-wip-civetweb-1.1-distro-basic-smithi/

The above suite failed for a few swift suites, re introduced the earlier downstream method validation drop and also added a downstream commit to allow unicode urls (civetweb was just allowing url encoded urls, however our swift tests sends both url encoded and non url encoded unicode urls), the new suite is running at http://pulpito.ceph.com/abhi-2018-04-09_17:18:28-rgw-wip-civetweb-1.1-distro-basic-smithi/

rerun of the failed jobs at http://pulpito.ceph.com/abhi-2018-04-11_08:17:08-rgw-wip-civetweb-1.1-distro-basic-smithi/, unfortunately the jobs failed due to env failures again, the valgrind failures seem to be the multisite ones, though the failures itself are with beast as a frontend and not running civetweb

@theanalyst
Copy link
Member Author

I have updated the ceph/civetweb#24 to correct branches. This PR needs to reference the correct sha1 once that merges

@theanalyst
Copy link
Member Author

jenkins test this please

@theanalyst
Copy link
Member Author

jenkins test this please (submodule sha not found as the remote didn't exist in ceph/civetewb)

@theanalyst
Copy link
Member Author

@cbodley @mattbenjamin I have pushed the last changeset based on version CMake tells us, should we do it this way or go the objdump route?

@cbodley
Copy link
Contributor

cbodley commented Apr 13, 2018

@cbodley @mattbenjamin I have pushed the last changeset based on version CMake tells us, should we do it this way or go the objdump route?

i think the cmake route is fine for mimic. let's revisit the linking strategy with @mdw-at-linuxbox later

@theanalyst theanalyst changed the title [DNM]: rgw: civetweb fixes for v1.1 upgrade rgw: civetweb fixes for v1.1 upgrade Apr 13, 2018
@theanalyst
Copy link
Member Author

err that didn't work, version_greater_equal was introduced in cmake 3.7 only, I'll rewrite to do a greater than or equal to myself

Introduces the following additions in rgw:
- allow_unicode_in_urls introduced with a corresponding downstream commit in
civetweb, as the newer version of civetweb validates that urls are url encoded
which swifttests do not follow, so introduce this as a configurable which we set
as true
- mg header struct changes in civetweb update, use auto here
- drop info->uri and use local_uri instead as the former is deprecated

wip: rgw: civetweb fixes for v1.1 upgrade

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
Since newer versions of civetweb are also strict on rfc2616 checks let's enforce
strict rfc2616 checks in s3tests

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
While not ideal, we set the civetweb openssl 1.1 conditional compile based on
the openssl version that cmake reports. In future we should make civetweb itself
do this based on OPENSSL_VERSION_COMPAT

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
Updating the sha1 to track ceph-master which currently updates civetweb to 1.1

Signed-off-by: Abhishek Lekshmanan <abhishek@suse.com>
@theanalyst
Copy link
Member Author

The submodule change probably requires @mattbenjamin / @yehudasa approval

@mattbenjamin mattbenjamin merged commit 7190e1b into ceph:master Apr 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants