[hibernate-dev] Introducing a new API in Hibernate Search to "sign" HTTP requests to Elasticsearch

[Gunnar Morling]

2017-06-02 12:29 GMT+02:00 Sanne Grinovero <sanne at hibernate.org>:
> On 2 June 2017 at 10:58, Yoann Rodiere <yoann at hibernate.org> wrote:
>>> Did you instead consider to just let users provide their custom
>>> instance of org.elasticsearch.client.RestClient? It's still leaking an
>>> implementation detail of Hibernate Search, but at least it's one
>>> indirection less.
>>
>>
>> The only way to add AWS authentication to the RestClient is to use Apache
>> HTTP Client classes, so this solution would still bind users to the
>> implementation details. We'd just shove it under the carpet :)
>>
>> Also, I'd argue this would be an even bigger implementation leak: at least
>> with the current solution with can switch to an alternative Elasticsearch
>> client, as long as we still use Apache HTTP Client. If we expose RestClient,
>> we're stuck with it and whatever underlying technologies it uses.
>>
>> On the other hand, we could ask them to re-implement
>> org.hibernate.search.elasticsearch.client.impl.ElasticsearchClient, which is
>> our own façade over the Elasticsearch client. But I find it a bit much just
>> to tweak some settings or to add a new authentication scheme...
>> On top of that, this solution would not allow multiple third-party
>> customizations to work well together (for instance AWS authentication
>> provided by us or a third party + some performance tweaking by the user)...
>> which is something the SPI we're planning in the PR could allow.
>
> +1
>
> I think you nailed it design wise.
>
> Gunnar, I have the impression we're simply not aligned on the definition of SPI.
>
> How to do custom request signing on a *specific platform* (AWS) with a
> *specific client* (this ES client using the Apache HTTP client) is
> going to need a specific adaptor; the good news is that this is going
> to be isolated so the main user application code - entities and
> business logic - won't be affected.

Sure, I don't see where either proposal is different in that regard?

> That's SPI by the book and we're not giving guarantees of backwards
> compatibility beyond a minor.
>
> We know the Elasticsearch driver is using the Apache HTTP client and
> this won't change overnight; if they happen to change it, we'll change
> in a new minor release and people will have to adapt.

Understood that this is your rule. I don't find it a good one, though.
IMO, minor releases shouldn't break APIs nor SPIs for that matter.
YMMV.

>
> Too bad but it's practical. Let me also point out a drawback of your
> alternative solution: the user's code for those we re-implement the
> driver would not break but only if they fail to upgrade the driver,
> which is possibly another pandora box.

I don't follow. WDYM by "re-implementing the driver"? All I suggested
is letting people instantiate their own RestClient.

>
> If you feel strongly about it I'm ok to promote the
> `DefaultElasticsearchClientFactory` to SPI to give some more options
> but I see no much value in that. Send a PR if you want it ;)

I don't feel strongly about it - you asked for feedback on the
proposal, so that's what I gave. I'd personally avoid to tie our SPIs
to implementation specifics of dependencies. Nothing more, nothing
less.

>
> Thanks,
> Sanne
>
>>
>> Yoann Rodière
>> Hibernate NoORM Team
>> yoann at hibernate.org
>>
>> On 2 June 2017 at 10:30, Sanne Grinovero <sanne at hibernate.org> wrote:
>>>
>>> On Fri, 2 Jun 2017, 08:56 Gunnar Morling, <gunnar at hibernate.org> wrote:
>>>
>>> > Hi,
>>> >
>>> > I find the exposure of an implementation detail (usage of Apache HTTP
>>> > client) of the Elasticsearch client a bit problematic. If they change
>>> > this to another HTTP client, our SPI would break.
>>> >
>>>
>>> Yes the very point of exposing that detail is the reason for this thread.
>>>
>>> Still, our SPI being guaranteed only for a minor, that gives a lot of
>>> flexibility?
>>>
>>> The Elasticsearch client exposing this itself, I don't expect them to
>>> switch implementation in a micro release to make some bugfix. If they
>>> change it in a major or even minor version, we're ok to not support that
>>> version until our next minor.
>>>
>>>
>>>
>>> > Did you instead consider to just let users provide their custom
>>> > instance of org.elasticsearch.client.RestClient? It's still leaking an
>>> > implementation detail of Hibernate Search, but at least it's one
>>> > indirection less.
>>> >
>>> > People wishing to have a custom RestClient would have to implement a
>>> > few more bits themselves (the logic from
>>> > DefaultElasticsearchClientFactory#customizeHttpClientConfig()), but
>>> > I'd find that acceptable for the sake of a less detail-exposing SPI,
>>> > plus it grants more flexibility in terms of configuring the
>>> > RestClient.
>>> >
>>>
>>>
>>> N.B. The client factory already is a Service so any advanced user already
>>> can override it.
>>>
>>> We want to make it easier for cloud users. Focusing on AWS now as we've
>>> had
>>> user requests for this - not least our own CI - but I'd expect other
>>> clouds
>>> to have similar features (today or tomorrow). I just don't expect other
>>> use
>>> cases to need this so we might provide them all eventually, but at this
>>> point my goal is to leave an appealing SPI for contributors to join on
>>> that.
>>>
>>> With that I mean:
>>> 1# this might evolve but we need something simple to use for people to not
>>> get stuck.
>>> 2# I expect integrator implementors to contribute them back
>>> 3# People won't have this low level dependency in their projects for long
>>>
>>> Having them re-implement the client wouldn't encourage this ;)
>>>
>>> Thanks!
>>> Sanne
>>>
>>>
>>> > --Gunnar
>>> >
>>> >
>>> > 2017-06-01 19:11 GMT+02:00 Sanne Grinovero <sanne at hibernate.org>:
>>> > > Yoann has been working on allowing Hibernate Search users to use
>>> > > Elasticsearch on AWS.
>>> > >
>>> > > Specifically on AWS the Elasticsearch security can be configured to
>>> > > use application identities, which implies the requests need to be
>>> > > signed.
>>> > >
>>> > > A good background read can be found here [1].
>>> > >
>>> > > We planned to allow people to use this but were not planning to
>>> > > include AWS specific libraries as dependencies - but since Yoann
>>> > > implemented an actual AWS signer in the tests I suppose it would be
>>> > > selfish to not ship it..
>>> > >
>>> > > Please see the API proposal on github (with the PR):
>>> > >  - https://github.com/hibernate/hibernate-search/pull/1426
>>> > >
>>> > > Thanks,
>>> > > Sanne
>>> > >
>>> > > [1] -
>>> >
>>> > https://aws.amazon.com/blogs/security/how-to-control-access-to-your-amazon-elasticsearch-service-domain/
>>> > > _______________________________________________
>>> > > hibernate-dev mailing list
>>> > > hibernate-dev at lists.jboss.org
>>> > > https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>> >
>>> _______________________________________________
>>> hibernate-dev mailing list
>>> hibernate-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>>
>>
>
> _______________________________________________
> hibernate-dev mailing list
> hibernate-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/hibernate-dev