Changes to include apikey configuration for elastic search rest client

[sriram22]

Adding support for apikey to elasticsearch low level rest client
Fixes #45490

[quarkus-bot]

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

• title should preferably start with an uppercase character (if it makes sense!)
• description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

_{This message is automatically generated by a bot.}

[quarkus-bot]

/cc @gsmet (elasticsearch), @loicmathieu (elasticsearch), @marko-bekhta (elasticsearch)

[marko-bekhta]

Hey @sriram22,

Thanks for the PR. I've added a comment inline and you might also want to take a look at that message from the quarkus bot 😉😃.

[marko-bekhta]

We probably should add a bit more validation... e.g. we should check that only one auth method is configured, and if there are both, we should let the user know (since we'd only apply one). And we probably would want to check that outside of the customize method 🤔

Also, maybe it's worth creating a small enum with auth methods, e.g. NONE, BASIC, API_KEY where you'd have a method that applies required transformations and here in the customizer you'd just make a call authMethod.apply(builder, config )

[manofthepeace]

Since both apiKeyId and apiKeySecret are needed or not I think this should be in a nested interface like this;

@WithParentName
Optional<EsAuth> esAuth()

interface EsAuth {
    String apiKeyId
    String apiKeySecret
}

This way the code can just check for esAuth.isPresent()

esAuth is a quick name just as an example. WithParentName only if you do not want to have an extra level to the config.

[sriram22]

used this as reference for configuration https://www.elastic.co/guide/en/elasticsearch/client/java-api-client/current/_other_authentication_methods.html#_elasticsearch_api_keys but looks like the apikey generated from elastic search already has base64 encoded apikeyid and secret. nevertheless i have made updates

[marko-bekhta]

Thanks for the updated PR! 🎉
I've added a few questions inline 😃.

[marko-bekhta]

since you are returning an auth type after you've checked the presence of the properties these very outer ifs aren't necessary anymore, right?

[marko-bekhta]

Suggested change

LOG.info("API Key authentication is enabled.");

I'd say let's be consistent here, since we aren't logging at info for the basic auth, we shouldn't log here too.

[marko-bekhta]

I'd still say it's better to throw an exception here instead of logging a warning since this is a "misconfiguration", and a user would want to be precise about the auth methods.

[marko-bekhta]

since it's a single property.. do we want to wrap it in the interface? This would probably make more sense for the basic auth where there's a pair of properties... if we want to have auth grouped somehow maybe we can consider going with something like:

# basic
quarkus.elasticsearch.auth.basic.username=foo
quarkus.elasticsearch.auth.basic.password=bar
# or API key:
quarkus.elasticsearch.auth.api-key=foobar

but that would mean deprecating existing username/password properties and creating new alternatives for them too. @yrodiere do you think it's worth doing ?

[yrodiere]

I think the way it's done is fine, if unnecessarily complex. @WithParentName means the interface is basically ignored, and the effective property key is quarkus.elasticsearch.api-key, which is fine by me.

So... no change needed here?