#5451 closed enhancement (complete)

Create HTTP client classes in libkea-http

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.4
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 12
Total Hours: 12 Internal?: no

Description

High Availability in Kea requires that the DHCP servers communicate with each other using the Control Channel. The DHCP server must be able to act as a controlling client and send commands to the Control Agent of a partner server. Currently, libkea-http contains server specific classes. We need to add classes that can be used by the client side.

Subtickets

Change History (6)

comment:1 Changed 23 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:2 Changed 22 months ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

I have implemented the HTTP client classes in libkea-http. There are numerous changes/addons to the code:

  • Existing HttpResponse class had to be modified to allow for parsing it from the wire
  • Existing HttpRequest class had to be modified to allow for creating it from the arbitrary parameters (not only from the wire)
  • Introduced common base classes for responses and requests
  • Created response parser as it is required that the client can parse it from the wire
  • Created HttpClient class which can maintain multiple connections to multiple servers and communicate simulataneously with them
  • Created URL class for parsing and representing URLs.

Overall, this is a significant change in terms of the number of lines of code. However, we have so many tickets for HA that it didn't seem practical for me to further split this work.

Proposed ChangeLog entry:

13XX.	[func]		marcin
	Implemented HTTP client classes in libkea-http.
	(Trac #5451, git cafe)

comment:3 Changed 22 months ago by tomek

  • Owner changed from UnAssigned to tomek

comment:4 Changed 22 months ago by fdupont

En passant: obviously missing a "not" in "The connection is usable if the peer has closed it."

comment:5 Changed 21 months ago by tomek

  • Add Hours to Ticket changed from 0 to 12
  • Owner changed from tomek to marcin
  • Total Hours changed from 0 to 12

General comment: Happy New Year. Hope you had a great time celebrating
new year, but I'm afraid it comes with the necessity to update
copyright years. But does it really? If you wrote the code in 2017 and it
was reviewed in 2018 and it wasn't changed, I think it's ok to keep
2017 in there.

I have corrected couple things. Please pull and review.

tcp_socket.h

isUsable() - comment says "The connection is usable if the peer has
closed it.". That should be "has NOT closed it", right?

url.cc
The names (e, p , h) were confusing in Url::parse(). I renamed them
to something more meaningful. Please pull and review.

If the path is not specifed (e.g. http://isc.org, shouldn't the path
default to /?

ca_response_creator.h
The comment for createstockHttpResponseInternal should explain what an
unfinalized response is. This should probably be just a pointer to
HttpResponseJson? class and the life cycle should be explained there.
I don't ask for much, maybe a paragraph of text. Also, unfinalized
means something that was finalized, but is now broken :)
Source: https://www.urbandictionary.com/define.php?term=unfinalized

http_message_parser_base.h
I'm not fond of having names longer than necessary. Would it be ok
to rename HttpMessageParserBaseError to HttpParseError? Yes, it's less
specific, but at the same time it gives a better intuitive
understanding what failed.

http_message_parser_base.cc
The whole postBuffer() method is ran when buf_size > 0. Is there any
reasonable case to call this method when a buf_size is zero? When
the connection is being destroyed prematurely or similar.

What's the benefit of having dedicated exceptions for request parsers
and response parsers? Couldn't you use the same exception everywhere?

Do you expect to have a code that would potentially throw both of
those exceptions and there would be a value in making the distinction
between them?

response.cc|h
I don't understand the concept behind setGenericBody(). Since it's
non-virtual, all calls from HttpResponse? class will always end up
calling the no-op function. Any calls from a derived class that have
its own setGenericBody will only result in calling that derived
class' implementation. So what's the point in having it in the base
class at all?

This is not strictly to the new code, but don't you think the
CallSetGenericBody? is a bit overkill? You have a class (ok, a struct,
but that's a class with the access modifiers being public) with proper
getters, setters just to convey a boolean value? Why doesn't the code
simply use boolean? I'm concerned with the amount of new code being
written that results in bigger binary and slower compilation. Both
being a problem of increasing importance nowadays.

http_message.cc|h
I don't like the fact that there are two methods to get headers:
getHeader and getHeaderSafe. I think there should be one and the whole
code should use it.

client.h
The HttpClient? class definitely deserves a better description. The
general purpose is obvious, but the text should explain how the
connection and transmission is expected to happen, what's the typical
control flow, when the connection is established and when it is
terminated etc. When you send one request to a server it's not clear
whether the connection is closed immediately after the request is
completed, is closed after certain inactivity time or maintained
indefinitely until the server closes it.

There are two asyncSendRequest methods. Please merge them into one
method with the timeout being the last parameter with a default value.

timerCallback should be renamed to timeoutCallback.

Line wraps in Connection:doSend() and do Receive(). Coding guidelines
say that we should limit the line to 80 if we can, but given the
lengths in C++ names it's ok to go up to 100 columns. I won't insist
or wrapping the code one way or another. It just looks weird - it's
over 80 cols, but is wrapped anyway, athough it could fit in 100 cols.

client.cc
There's a compilation warning that void
{anonymous}::Connection::stop()’ is defined but not used.

post_request_json_unittest.cc
The env name expected_response is misleading. There's no transmission
of any kind in this test, so it's not a response, just a text version
of the request.

response_parser_unittest.cc
The HttpResponseParserTest?.responseWithJson test should be refactored
to use a method like with similar signature as this:

testHttpResponseParser(const std::string& header, const std::string&
body)

The http interfaces seems to be very popular and I hope to see many
more cases to be expected to be handled in the future, like the
request we got recently to make stats available with GET method.
Having such a method would make writing new tests trivial.


A generic comment about file naming. Many files don't have http in the
file names, e.g. HttpClient? stored in client.cc|h files) as the http
is implied by the http/ directory. However, some files don't follow
that. It should be unified. I don't have a strong preference, but
if you don't have one either, the shorter names (without http_ prefix)
are easier to use. This doesn't have to be done in this ticket if
there are other branches that depend on it.


The proposed changelog is ok.

The code builds and unit-tests pass on Ubuntu 17.04 x64.

Since the changes are generally simple, I don't need to see this ticket
again, unless you strongly disagree with my comments and want to discuss.

comment:6 Changed 21 months ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

I addressed review comments and merged with commit 94267e252b372650e4235389251b49d6f5501322

Note: See TracTickets for help on using tickets.