> On Jun 11, 2015, at 8:09 AM, Susan Hinrichs
> <shinr...@network-geographics.com> wrote:
>
> James, thanks for the comments. I had some old references and as you noted
> did not discuss how memory is tracked through the API's.
Thanks Susan. I actually find it pretty difficult to review API changes from
documentation. It would be much easier to see the actual API diffs, with the
documentation as supporting material.
Looking at the diffs in your branch
- apidefs.h should not be pulling in openssl/ssl.h
- The TS_EVENT_SESSION_* names should be more specific, eg.
TS_EVENT_SSL_SESSION_GET
- TSSslSessionID probably should be an opaque type
- TSSslSessionID_t should be removed
- The API type TSSslSessionID should not leak into SSLSessionCache.h
- The use of TSSslSessionID* an an input parameter is inconsistent with
other APIs
I think that the TS_EVENT_SESSION_GET is interesting. There's a number of
things I could imagine a plugin wanting to do, but the way it is described this
is just a notification. If you have the SSL VC, then you could allow the plugin
to decide whether to use a cached session or not.
AFAICT this is in support of only server-side session caching? What happens
with session tickets?
I'm wondering whether this should really be a HTTP hook in the first place. If
you can use it to manage SSL sessions or tickets for a specific VC, then I
think it is. If it is only for events from the session cache, then maybe it is
not. For example, does it make sense to pass TS_EVENT_SSL_SESSION_GET to
TSHttpSsnHookAdd? That seems to be legal with this API construction, but not
particularly useful.
Random code review comments
- SSLSessionBucket::getSessionBuffer should return the byte count so
you don't need the true_len output parameter
- How is a plugin supposed to know how large a buffer it needs for
TSSslSessionGetBuffer?
> I've updated the document
> http://network-geographics.com/ats/docs/ssl-session-api.en.html
>
> On my branch, I have a very simple example plugin which exercises the new hook
>
> https://github.com/shinrich/trafficserver/blob/TS-3527/example/ssl-session/ssl-session.cc
>
> I've also created a pull request on my working branch.
>
> https://github.com/apache/trafficserver/pull/217https://github.com/apache/trafficserver/pull/217z
>
>
> On 6/10/2015 6:11 PM, James Peach wrote:
>>> On Jun 10, 2015, at 3:24 PM, Susan Hinrichs
>>> <shinr...@network-geographics.com> wrote:
>>>
>>> I haven't heard anything about this. We did discuss it at the Austin
>>> Summit. There was general agreement to the idea. I've updated the
>>> document to reflect what I have implemented. I will create a pull request
>>> with the code changes later this evening.
>>>
>>> The details of the API are at
>>> http://network-geographics.com/ats/docs/ssl-session-api.en.html
>>>
>>> Please share your comments and concerns.
>> How do you register this hook? What is TSSslSessionCurrentInsert()?
>>
>> Many of the naming is inconsistent. For example, TSSslSessionId_t. We "Id"
>> should be spelled "ID", etc.
>>
>> This API needs to talk about the lifecycle of the session and what locking
>> is involved. Who owns the object returned by TSSslSessionGet()? Does
>> TSSslSessionInsert() replace an existing entry?
>>
>>> On 3/18/2015 10:52 AM, Susan Hinrichs wrote:
>>>> The motivation is to leverage the core SSL session support of ATS while
>>>> enabling others to add things like multi-ATS session state sharing,
>>>> additional stats, additional checks, etc.
>>>>
>>>> Proposed interface write up here
>>>> http://network-geographics.com/ats/docs/ssl-session-api.en.html
>>>>
>>>> Please give me your comments.
>>>>
>>>> Thanks,
>>>> Susan
>>>>
>>>>
>>>>
>
