#5077 closed task (complete)

Implement HTTP requests parser in libkea-http library

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.2
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: 0
Total Hours: 0 Internal?: no

Description

See http://kea.isc.org/wiki/ControlAPIDesign

We should add new Kea library: libkea-http which will provide mechanisms needed to setup HTTP service. This library should be then used by the Kea Control Agent to run HTTP service.

Subtickets

Change History (16)

comment:1 Changed 12 months ago by marcin

  • Milestone changed from Kea-proposed to Kea1.2

comment:2 Changed 12 months ago by marcin

  • Component changed from management API to remote-management

comment:3 Changed 12 months ago by marcin

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

comment:4 Changed 12 months ago by marcin

  • Summary changed from Implement HTTP service and use it within Control Agent to Implement HTTP requests parser in libkea-http library

comment:5 Changed 11 months ago by marcin

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

This ticket introduces libkea-http library and implements a couple of classes shown in the design: http://kea.isc.org/wiki/ControlAPIDesign#ProposedArchitecture. These are:

  • HttpRequestParser
  • HttpRequest
  • PostHttpRequest
  • PostHttpRequestJson
  • HttpRequestContext

This comes with no ChangeLog entry as it is partial functionality of the libkea-http. We'll provide a single changelog entry when we finish the implementation of the libkea-http lib.

comment:6 Changed 11 months ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:7 Changed 11 months ago by fdupont

The use of auto requires a specific check in configure.ac (it is a C++11 feature and some compilers, for instance macOS Xcode 8.2 clang, can show partial C++11 support...). Same for the lambda.

I believe in request.h the method context() requires a return doxygen comment.

buffer_ is a list<char> but postBuffer uses const uint8_t. I have no concern about the const but char is signed so I expected int8_t or simply char.

I fixed some spelling errors & co, and missing LDADDs.

comment:8 Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

comment:9 Changed 11 months ago by fdupont

  • Owner changed from marcin to fdupont

comment:10 Changed 11 months ago by fdupont

Take it back (in place of #5075).

Some extra notes:

  • even I like type inference I don't trust all C++ compilers about auto even in some trivial cases. IMHO we should discuss and add a new note about this in coding style guidelines.
  • this extends to lambdas where parameters are given without types. Again it is not really a required feature even it makes coding simpler (coding, not reading the code :-) so again it is something I should like (this is why I need more opinions).
  • I have to understand and check the role of context-length...

comment:11 follow-up: Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

I translated auto}}s into types (this showed types are useful because we have a mix of {{{iterators and const_iterators, which raises the question about using cbegin/cend, another C++11 feature :-).
I added a lambda (with auto so I got both :-) check in configure.ac into C++11 feature stuff (my clang requires the C++11 mode for this, remember it is not the case for unique_ptr).

About my 3 previous concerns:

  • if you prefer to use auto in place of a complex type please argue
  • I have now no objection using lambdas (obviously any alternative is worse)
  • I request an unit test for too short body (too short related to content-length) and a comment explaining what happens with a too long body.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 11 months ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

I translated auto}}s into types (this showed types are useful because we have a mix of {{{iterators and const_iterators, which raises the question about using cbegin/cend, another C++11 feature :-).
I added a lambda (with auto so I got both :-) check in configure.ac into C++11 feature stuff (my clang requires the C++11 mode for this, remember it is not the case for unique_ptr).

About my 3 previous concerns:

  • if you prefer to use auto in place of a complex type please argue
  • I have now no objection using lambdas (obviously any alternative is worse)
  • I request an unit test for too short body (too short related to content-length) and a comment explaining what happens with a too long body.

The first two we have discussed on jabber and we seem to be on the same page.

As for the last thing, this is really covered by the first parser test which provides the data in chunks. In such case the parser returns from poll() and waits for another call to HttpRequest::postBuffer. I don't think that another test would add a value. Instead, I have added a new test for too long buffer and a commentary in the class description.

comment:13 in reply to: ↑ 12 ; follow-up: Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

  • I request an unit test for too short body (too short related to content-length) and a comment explaining what happens with a too long body.

As for the last thing, this is really covered by the first parser test which provides the data in chunks. In such case the parser returns from poll() and waits for another call to HttpRequest::postBuffer. I don't think that another test would add a value.

=> if I understand well this means the next event is NEED_MORE_DATA_EVT and it will get back control only on the other side postBuffer but is there something designed when the other side is closed. Or with other words you answered about partial reads but not about unexpected end-of-file. Perhaps this will be clearer when the library will be used in an independent process with sockets.

Instead, I have added a new test for too long buffer and a commentary in the class description.

=> this is fine!

BTW:

  • I left some comments in comment:7. only the first one was solved.
  • as far as I know (i.e., even I don't understand why) this ticket is not a mozilla one so has lowest priority.

comment:14 in reply to: ↑ 13 Changed 11 months ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

Replying to marcin:

  • I request an unit test for too short body (too short related to content-length) and a comment explaining what happens with a too long body.

As for the last thing, this is really covered by the first parser test which provides the data in chunks. In such case the parser returns from poll() and waits for another call to HttpRequest::postBuffer. I don't think that another test would add a value.

=> if I understand well this means the next event is NEED_MORE_DATA_EVT and it will get back control only on the other side postBuffer but is there something designed when the other side is closed. Or with other words you answered about partial reads but not about unexpected end-of-file. Perhaps this will be clearer when the library will be used in an independent process with sockets.

The parser will be left with NEED_MORE_DATA_EVT. I suggest we wait until this class is used to process real requests to address your concern.

Instead, I have added a new test for too long buffer and a commentary in the class description.

=> this is fine!

BTW:

  • I left some comments in comment:7. only the first one was solved.
  • as far as I know (i.e., even I don't understand why) this ticket is not a mozilla one so has lowest priority.

I addressed comment 7 now.

comment:15 Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

OK to merge.

comment:16 Changed 11 months ago by marcin

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

Merged with commit cd72284b5b221e620770883db7e166c4d3ba7eb6

Note: See TracTickets for help on using tickets.