Configurable headers for all elasticsearch requests #7996

epixa · 2016-08-13T16:51:23Z

A new server-side configuration, elasticsearch.customHeaders, allows
people to configure any number of custom headers that will get sent
along to all requests to Elasticsearch that are made via the proxy or
exposed client.

This allows for advanced architectures that do things such as dynamic
routing based on install-specific headers.

Closes #7964

ycombinator · 2016-08-13T17:15:32Z

Please add a section in config/kibana.yml similar to https://github.com/elastic/kibana/blob/master/config/kibana.yml#L64-L67.

ycombinator · 2016-08-13T17:18:45Z

How do you feel about combining customHeaders with requestHeadersWhitelist into a single setting, given that a header in the former will take precedence over the same header in the latter.

Such a configuration setting would take an array of strings or objects. I think it would make it a tad more obvious to the person making the configuration what's going on, particularly if they would've otherwise inadvertently defined the same header in both settings at different points in time.

epixa · 2016-08-13T17:41:44Z

They are really two different things though: one is the name of headers that should be proxied through from the clieny, the other is essentially hardcoded header names and values and is not exposed to the client at all.

On Aug 13, 2016, at 1:19 PM, Shaunak Kashyap notifications@github.com wrote:

How do you feel about combining customHeaders with requestHeadersWhitelist, given that a header in the former will take precedence over the same header in the latter. The configuration setting would take an array of strings or objects. I think it would make it a tad more obvious to the person making the configuration what's going on, particularly if they would've otherwise inadvertently defined the same header in both settings at different points in time.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

ycombinator · 2016-08-13T18:42:09Z

Alright. I think then we should make it clear in the inline config
documentation for the new setting that it's header values take precedence
over the whitelist header values or client header values.
On Sat, Aug 13, 2016 at 12:42 PM Court Ewing notifications@github.com
wrote:

They are really two different things though: one is the name of headers
that should be proxied through from the clieny, the other is essentially
hardcoded header names and values and is not exposed to the client at all.

On Aug 13, 2016, at 1:19 PM, Shaunak Kashyap notifications@github.com
wrote:

How do you feel about combining customHeaders with
requestHeadersWhitelist, given that a header in the former will take
precedence over the same header in the latter. The configuration setting
would take an array of strings or objects. I think it would make it a tad
more obvious to the person making the configuration what's going on,
particularly if they would've otherwise inadvertently defined the same
header in both settings at different points in time.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

—
You are receiving this because you were assigned.

Reply to this email directly, view it on GitHub
#7996 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADHdZV3jcoEWi9fETiyX3u5T3giRoOrks5qfgHrgaJpZM4JjtrY
.

epixa · 2016-08-14T23:34:19Z

@ycombinator Good call. I updated the kibana.yml as well as docs.

ycombinator · 2016-08-15T12:15:17Z

config/kibana.yml

@@ -65,6 +65,10 @@
 # headers, set this value to [] (an empty list).
 # elasticsearch.requestHeadersWhitelist: [ authorization ]

+# Header names and values that are sent to Elasticsearch. Any custom headers cannot be
+# overwritten by client-side headers, regardless of requestHeadersWhitelist configuration.


Nit: change requestHeadersWhitelist to elasticsearch.requestHeadersWhitelist for consistency.

epixa · 2016-08-15T13:57:55Z

@ycombinator Updated

ycombinator · 2016-08-16T11:33:36Z

src/core_plugins/elasticsearch/lib/set_headers.js

+import { isObject } from 'lodash';
+
+export default function setHeaders(originalHeaders, newHeaders) {
+  if (!isObject(originalHeaders)) {


Consider using isPlainObject as it is a stricter check than isObject.

I considered that, but spread should work just fine with any enumerable object, so I couldn't think of a reason to only allow plain objects.

I was thinking more that you probably don't want either of these to be arrays or functions, for example, right? I know they could be and the spread operator would work fine but, from a validation perspective for what this function does, you only want headers to be plain objects, right?

I agree with @ycombinator here. If the intention is to verify that the arguments are compatible with the spread operator then so be it, but if the intention is to make sure that setHeaders() is being used as intended, a more strict check is probably appropriate.

Someone sending an instance of RegExp, or an array, is probably a mistake that would not be caught by these helpful checks.

ycombinator · 2016-08-16T12:04:57Z

Left a couple of comments in the code. Functionality LGTM, though:

With only `elasticsearch.customHeaders` set:

With `elasticsearch.customHeaders` and `elasticsearch.requestHeaderWhitelist` set to same header

![screen shot 2016-08-16 at 7 58 35 am](https://cloud.githubusercontent.com/assets/51061/17698257/fe33bad6-6387-11e6-9313-920af248d911.png)

kimchy · 2016-08-17T09:46:39Z

can we see if this gets merged in time to backporting to the upcoming 4.x release as well?

epixa · 2016-08-17T11:17:46Z

@kimchy The backporting is likely to take longer than the remaining time to get this merged, but that's the goal.

spalger · 2016-08-17T20:59:10Z

src/core_plugins/elasticsearch/lib/expose_client.js

+    const host = {
+      host: uri.hostname,
+      port: uri.port,
+      protocol: uri.protocol.replace(':', ''),


replacing the ":" isn't necessary, and protocol will be null if the parsed uri is just //localhost or something... just a thought

Is the unnecessary : a tested feature of the elasticsearch client?

Yeah, https://github.com/elastic/elasticsearch-js/blob/master/test/unit/specs/host.js#L118-L119. Docs should be updated with that fact

A new server-side configuration, elasticsearch.customHeaders, allows people to configure any number of custom headers that will get sent along to all requests to Elasticsearch that are made via the proxy or exposed client. This allows for advanced architectures that do things such as dynamic routing based on install-specific headers.

epixa · 2016-08-18T22:07:00Z

@spalger @ycombinator All outstanding feedback should be addressed now.

spalger · 2016-08-19T15:02:13Z

LGTM

BigFunger · 2016-08-19T17:13:50Z

LGTM

--------- **Commit 1:** Configurable headers for all elasticsearch requests A new server-side configuration, elasticsearch.customHeaders, allows people to configure any number of custom headers that will get sent along to all requests to Elasticsearch that are made via the proxy or exposed client. This allows for advanced architectures that do things such as dynamic routing based on install-specific headers. * Original sha: d00d177 * Authored by Court Ewing <court@epixa.com> on 2016-08-13T16:46:54Z

[backport] PR #7996 to 4.x - Configurable headers for all elasticsearch requests

Configurable headers for all elasticsearch requests Former-commit-id: bca4732

epixa added review v5.0.0 v5.0.0-beta1 labels Aug 13, 2016

epixa force-pushed the 7964-esclientheaders branch from 8e3a3e3 to 78b178d Compare August 13, 2016 17:05

epixa assigned spalger and ycombinator Aug 13, 2016

epixa force-pushed the 7964-esclientheaders branch from 78b178d to 906feb5 Compare August 14, 2016 23:31

ycombinator reviewed Aug 15, 2016
View reviewed changes

epixa force-pushed the 7964-esclientheaders branch from 906feb5 to e076ed0 Compare August 15, 2016 13:57

ycombinator reviewed Aug 16, 2016
View reviewed changes

epixa added the v4.6.0 label Aug 17, 2016

spalger reviewed Aug 17, 2016
View reviewed changes

epixa force-pushed the 7964-esclientheaders branch from e076ed0 to d00d177 Compare August 18, 2016 21:58

epixa assigned BigFunger Aug 19, 2016

spalger removed their assignment Aug 19, 2016

BigFunger removed their assignment Aug 19, 2016

epixa merged commit bca4732 into elastic:master Aug 19, 2016

epixa deleted the 7964-esclientheaders branch August 19, 2016 17:14

elastic-jasper mentioned this pull request Aug 19, 2016

[backport] PR #7996 to 4.x - Configurable headers for all elasticsearch requests #8032

Merged

epixa added a commit that referenced this pull request Aug 22, 2016

Merge pull request #8032 from elastic/jasper/backport/7996/4.x

a41a308

[backport] PR #7996 to 4.x - Configurable headers for all elasticsearch requests

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#7996 from epixa/7964-esclientheaders

bb4b4e5

Configurable headers for all elasticsearch requests Former-commit-id: bca4732

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Configurable headers for all elasticsearch requests #7996

Configurable headers for all elasticsearch requests #7996

epixa commented Aug 13, 2016

ycombinator commented Aug 13, 2016

ycombinator commented Aug 13, 2016 •

edited

Loading

epixa commented Aug 13, 2016

ycombinator commented Aug 13, 2016

epixa commented Aug 14, 2016

ycombinator Aug 15, 2016

epixa commented Aug 15, 2016

ycombinator Aug 16, 2016

epixa Aug 16, 2016

ycombinator Aug 16, 2016 •

edited

Loading

spalger Aug 17, 2016 •

edited

Loading

epixa Aug 18, 2016

ycombinator commented Aug 16, 2016

kimchy commented Aug 17, 2016

epixa commented Aug 17, 2016

spalger Aug 17, 2016

epixa Aug 17, 2016

spalger Aug 17, 2016

epixa Aug 18, 2016

epixa commented Aug 18, 2016

spalger commented Aug 19, 2016

BigFunger commented Aug 19, 2016

Configurable headers for all elasticsearch requests #7996

Configurable headers for all elasticsearch requests #7996

Conversation

epixa commented Aug 13, 2016

ycombinator commented Aug 13, 2016

ycombinator commented Aug 13, 2016 • edited Loading

epixa commented Aug 13, 2016

ycombinator commented Aug 13, 2016

epixa commented Aug 14, 2016

Choose a reason for hiding this comment

epixa commented Aug 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator Aug 16, 2016 • edited Loading

Choose a reason for hiding this comment

spalger Aug 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycombinator commented Aug 16, 2016

With only elasticsearch.customHeaders set:

With elasticsearch.customHeaders and elasticsearch.requestHeaderWhitelist set to same header

kimchy commented Aug 17, 2016

epixa commented Aug 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epixa commented Aug 18, 2016

spalger commented Aug 19, 2016

BigFunger commented Aug 19, 2016

ycombinator commented Aug 13, 2016 •

edited

Loading

ycombinator Aug 16, 2016 •

edited

Loading

spalger Aug 17, 2016 •

edited

Loading

With only `elasticsearch.customHeaders` set:

With `elasticsearch.customHeaders` and `elasticsearch.requestHeaderWhitelist` set to same header