Ability to add auth headers to Druid requests. #62

codingwhatever · 2016-10-10T22:07:12Z

We are adding security to our Druid cluster and need Fili to be able to send a token in a header for authenticating our Druid requests.

cdeszaq

Just a few small things. Overall, looks pretty good

cdeszaq · 2016-10-11T14:48:54Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

@@ -100,6 +100,7 @@
 import com.codahale.metrics.health.HealthCheckRegistry;
 import com.fasterxml.jackson.databind.ObjectMapper;

+import org.apache.commons.collections.map.HashedMap;


Can we use a built-in (ie. Java Collections) map instead of apache commons?

cdeszaq · 2016-10-11T14:50:04Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

-    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper) {
-        this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper);
+    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper,
+            Supplier<Map<String, String>> authHeaders


Wrapping param lists should "chop" the list, rather than just wrap it. eg.:

public AsyncDruidWebServiceImpl( DruidServiceConfig serviceConfig, ObjectMapper mapper, Supplier<Map<String, String>> authHeaders ) {

cdeszaq · 2016-10-11T14:50:20Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -70,8 +72,10 @@
     * @param serviceConfig  Configuration for the Druid Service
     * @param mapper  A shared jackson object mapper resource


Javadoc needed for the new parameter

Addressed this in the constructor below.

cdeszaq · 2016-10-11T14:50:44Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

-    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper) {
-        this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper);
+    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper,
+            Supplier<Map<String, String>> authHeaders


Can we re-name this? Rather than auth-specific, can this be named so that it can be any headers?

cdeszaq · 2016-10-11T14:51:06Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -102,7 +106,9 @@ private static AsyncHttpClient initializeWebClient(int requestTimeout) {
     * @param asyncHttpClient  the HTTP client
     * @param mapper  A shared jackson object mapper resource
     */
-    public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient asyncHttpClient, ObjectMapper mapper) {
+    public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient asyncHttpClient, ObjectMapper mapper,
+            Supplier<Map<String, String>> authHeaders


Same comments re: wrapping and name apply here as well

cdeszaq · 2016-10-11T14:51:19Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -61,7 +63,7 @@
    public static final String DRUID_WEIGHTED_QUERY_TIMER = DRUID_TIMER + "_W_";
    public static final String DRUID_SEGMENT_METADATA_TIMER = DRUID_TIMER + "_S_0";

-
+    private final Supplier<Map<String, String>> authHeaders;


can we rename this to indicate any headers?

cdeszaq · 2016-10-11T14:54:30Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -102,7 +106,9 @@ private static AsyncHttpClient initializeWebClient(int requestTimeout) {
     * @param asyncHttpClient  the HTTP client
     * @param mapper  A shared jackson object mapper resource
     */
-    public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient asyncHttpClient, ObjectMapper mapper) {
+    public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient asyncHttpClient, ObjectMapper mapper,
+            Supplier<Map<String, String>> authHeaders


We should also have constructors that default the header supplier for backwards compatibility. We can deprecate them so that people are required to supply a supplier eventually, but we should try to maintain backwards compatibility if we can.

The supplier can just return an empty map in the defaulting case.

codingwhatever · 2016-10-11T18:54:47Z

@cdeszaq Addressed all the comments.

archolewa · 2016-10-11T18:55:24Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -72,10 +73,27 @@
     * @param serviceConfig  Configuration for the Druid Service
     * @param mapper  A shared jackson object mapper resource
     */
-    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper,
-            Supplier<Map<String, String>> authHeaders
+    @Deprecated


If you could add the @deprecated annotation to the JavaDoc as well, with a brief explanation of why this constructor is being deprecated, and which constructor should be used instead, I would be grateful.

archolewa · 2016-10-11T18:57:00Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -70,8 +73,27 @@
     * @param serviceConfig  Configuration for the Druid Service
     * @param mapper  A shared jackson object mapper resource
     */
-    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper) {
-        this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper);
+    @Deprecated


The @deprecated annotation should also be added to the JavaDoc, with a brief explanation of why this is being deprecated, and a pointer to which constructor should be used instead.

archolewa · 2016-10-11T18:57:14Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -102,7 +124,29 @@ private static AsyncHttpClient initializeWebClient(int requestTimeout) {
     * @param asyncHttpClient  the HTTP client
     * @param mapper  A shared jackson object mapper resource
     */
-    public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient asyncHttpClient, ObjectMapper mapper) {
+    @Deprecated


archolewa · 2016-10-11T18:58:49Z

This also needs a CHANGELOG entry.

cdeszaq

Other than the minor JavaDoc update on the 2 deprecated constructors, and adding entries to the ChangeLog file (a bit in the ### Deprecated section and a bit in the ### Changed section), this PR looks like it's very close!

cdeszaq · 2016-10-11T18:57:31Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -70,8 +73,27 @@
     * @param serviceConfig  Configuration for the Druid Service
     * @param mapper  A shared jackson object mapper resource
     */
-    public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper) {
-        this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper);
+    @Deprecated


For these deprecations, we should add an @deprecated tag to the javadoc indicating why we're deprecating them (because you should specify your header supplier), and what you should use instead (the matching constructor with that new parameter)

Applies to the other @Deprecated instance in this PR as well

cdeszaq

Just a few thoughts around the extension hook for the Supplier, but otherwise that extension looks good.

PR still needs CHANGELOG entries and updates the the JavaDoc for those @Deprecated methods documenting the deprecation

cdeszaq · 2016-10-13T13:02:01Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

@@ -919,7 +924,24 @@ protected JobPayloadBuilder buildJobPayloadBuilder() {
     * @return A DruidWebService
     */
    protected DruidWebService buildDruidWebService(DruidServiceConfig druidServiceConfig, ObjectMapper mapper) {
-        return new AsyncDruidWebServiceImpl(druidServiceConfig, mapper, HashMap::new);
+        Supplier<Map<String, String>> supplier = HashMap::new;
+        String customSupplierClassString = SYSTEM_CONFIG.getStringProperty(DRUID_HEADER_SUPPLIER_CLASS, null);


@michael-mclawhorn does this usage fit with how we usually use systemConfig?

cdeszaq · 2016-10-13T13:04:00Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

+                Constructor<?> constructor = c.getConstructor();
+                supplier = (Supplier<Map<String, String>>) constructor.newInstance();
+            } catch (ClassNotFoundException | NoSuchMethodException |
+                    IllegalAccessException | InstantiationException | InvocationTargetException e) {


Rather than enumerate all of these, should we just catch Exception? If anything goes wrong loading this, we should just fail anyway, no matter what the exception is, right?

cdeszaq · 2016-10-13T13:05:59Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

+                );
+                throw new IllegalStateException(e);
+            }
+        }


I think we should pull the buildDruidWebServiceHeaderSupplier code into it's own method. That will give another (fairly easy) override-based config mechanism if someone's Supplier needs more than a no-param constructor.

I assumed we could just pull the other params from system_config in the supplier. I guess there could be more complicated use cases?

codingwhatever · 2016-10-13T18:20:22Z

@archolewa @cdeszaq Addressed all review comments.

cdeszaq

👍

michael-mclawhorn · 2016-10-13T23:14:02Z

I think Supplier<Map<String, String>> might be inferior as the type here to Iterable<Pair<String, String>>

michael-mclawhorn

I think the Supplier<Map<S,S>> type should be seriously reconsidered.

A better name for the additional headers and documentation of the semantics (adding vs replacing) on the request would be a good idea.

Also, tests.

At minimum, postDruidQuery should be tested to verify that the added headers get added.
Abstract binder factory being tested with actually building a header source from a config parameter and an empty one from null would be really good.

Alternatives to Supplier would be an Iterable or an rx Observable.

michael-mclawhorn · 2016-10-13T23:20:50Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -61,17 +64,38 @@
    public static final String DRUID_WEIGHTED_QUERY_TIMER = DRUID_TIMER + "_W_";
    public static final String DRUID_SEGMENT_METADATA_TIMER = DRUID_TIMER + "_S_0";

-
+    private final Supplier<Map<String, String>> anyHeaders;
    private final DruidServiceConfig serviceConfig;


I dislike the name of the variable. If I understand the scope of this, these are mandatory headers appended (or possibly prepended, it looks like the latter based on digging into the netty code) to the client request. So possible 'requestAppendHeaders' or 'clientAppendHeaders'.

Since we're adding the headers (and it looks like added headers strictly add, don't replace), I think a name like headersToAdd or headersToAppend helps clarify this.

michael-mclawhorn · 2016-10-13T23:21:36Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+                .addHeader("Content-Type", "application/json");
+
+        anyHeaders.get().forEach((k, v) -> requestBuilder.addHeader(k, v));
+


It does not appear that headers are maplike in that there can't be more than one of a given key value. Given this, you might be better served with an iterable here.

While the AsyncHttpClient allows for multi-value headers, I think it's OK for us to restrict it down to just being able to add single-valued headers. We can expand that later if we need to.

michael-mclawhorn · 2016-10-13T23:22:56Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

+                Constructor<?> constructor = c.getConstructor();
+                supplier = (Supplier<Map<String, String>>) constructor.newInstance();
+            } catch (Exception e) {
+                LOG.error(


Can we catch something more specific than 'Exception' here?

We were matching something more specific in an earlier version of this PR and I suggested replacing that list of roughly 5 exceptions with just Exception. Since we're re-throwing the exception anyways, catching everything so that we can log it (since it'll be fatal anyways) is (I think) fine, and lets the code be smaller / cleaner.

cdeszaq · 2016-10-14T20:51:25Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+                .setBody(entityBody)
+                .addHeader("Content-Type", "application/json");
+
+        anyHeaders.get().forEach((k, v) -> requestBuilder.addHeader(k, v));


Can be replaced with method reference (requestBuilder::addHeader)

michael-mclawhorn · 2016-10-17T15:04:11Z

To summarize my responses, since I distributed them across Rick's responses to my comments:
The only change I am currently asking for is the anyHeaders variable to be named slightly differently. Otherwise 👍 from me.

cdeszaq

Definitely need to test that we're adding any supplied headers on the Druid requests.

I don't know if we have a hook for that in our testing framework yet, but I don't know if there's a need for such a hook in the functional tests (it would be OK to have one). If we don't want to add one, I think we should be able to test the Async client in isolation.

To do so, we may need to either extend an existing or add a new TestServlet (which are the pieces we use to capture requests to "druid")

codingwhatever · 2016-10-17T23:26:37Z

@cdeszaq Addressed remaining comments and added a unit test.

cdeszaq

Very close, just a few minor things!

cdeszaq · 2016-10-18T15:36:23Z

.../test/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplSpec.groovy

@@ -0,0 +1,53 @@
+package com.yahoo.bard.webservice.druid.client.impl


Copyright needed

cdeszaq · 2016-10-18T15:41:53Z

.../test/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplSpec.groovy

+                DruidClientConfigHelper.getNonUiServiceConfig(),
+                new ObjectMapper(),
+                supplier
+        )


All of this stuff from here up that's in the when block should go up in the setup block, since it's still setting up the scenario.

If needed, you can use and: blocks (with an optional comment: and: "a set of expected headers to add") to break up any block.

cdeszaq · 2016-10-18T15:43:44Z

.../test/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplSpec.groovy

+        then:
+        HttpHeaders actualHeaders = webServiceImplWrapper.getHeaders()
+        for (Map.Entry<String, String> header : expectedHeaders) {
+            actualHeaders.get(header.getKey()) == header.getValue()


Only top-level expressions in a then block are treated as assertions. To treat non-top-level expressions as assertions, you need to explicitly add the assert keyword: assert actualHeaders.get(header.getKey()) == header.getValue()

cdeszaq · 2016-10-18T15:49:01Z

fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java

+    protected Supplier<Map<String, String>> buildDruidWebServiceHeaderSupplier() {
+        Supplier<Map<String, String>> supplier = HashMap::new;
+        String customSupplierClassString = SYSTEM_CONFIG.getStringProperty(DRUID_HEADER_SUPPLIER_CLASS, null);
+        if (customSupplierClassString != null && customSupplierClassString != "") {


We should fix this too.

codingwhatever · 2016-10-18T16:41:44Z

@cdeszaq Addressed the latest comments.

cdeszaq · 2016-10-18T19:24:24Z

👍 just needs to be squashed and rebased onto master and I think this is good to merge.

archolewa added NEED 2 REVIEWS REVIEWABLE labels Oct 11, 2016

cdeszaq requested changes Oct 11, 2016

View reviewed changes

archolewa requested changes Oct 11, 2016

View reviewed changes

cdeszaq requested changes Oct 11, 2016

View reviewed changes

archolewa added the NEED CHANGES label Oct 11, 2016

cdeszaq requested changes Oct 13, 2016

View reviewed changes

codingwhatever force-pushed the auth-headers branch 4 times, most recently from d2f5179 to fcdad74 Compare October 13, 2016 18:19

cdeszaq approved these changes Oct 13, 2016

View reviewed changes

cdeszaq added NEED 1 REVIEW and removed NEED 2 REVIEWS NEED CHANGES labels Oct 13, 2016

michael-mclawhorn reviewed Oct 13, 2016

View reviewed changes

michael-mclawhorn added the NEED CHANGES label Oct 13, 2016

cdeszaq reviewed Oct 14, 2016

View reviewed changes

michael-mclawhorn removed the NEED 1 REVIEW label Oct 17, 2016

codingwhatever force-pushed the auth-headers branch from fcdad74 to 0b6c2c4 Compare October 17, 2016 15:54

cdeszaq requested changes Oct 17, 2016

View reviewed changes

cdeszaq requested changes Oct 18, 2016

View reviewed changes

cdeszaq reviewed Oct 18, 2016

View reviewed changes

Ability to add headers to Druid requests.

6edbda2

codingwhatever force-pushed the auth-headers branch from 4caa0eb to 6edbda2 Compare October 18, 2016 20:31

cdeszaq merged commit b195343 into master Oct 18, 2016

cdeszaq deleted the auth-headers branch October 20, 2016 19:07

		@@ -70,8 +72,10 @@
		* @param serviceConfig Configuration for the Druid Service
		* @param mapper A shared jackson object mapper resource

		.addHeader("Content-Type", "application/json");

		anyHeaders.get().forEach((k, v) -> requestBuilder.addHeader(k, v));

		@@ -0,0 +1,53 @@
		package com.yahoo.bard.webservice.druid.client.impl

Ability to add auth headers to Druid requests. #62

Ability to add auth headers to Druid requests. #62

Conversation

codingwhatever commented Oct 10, 2016

cdeszaq left a comment

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

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

Choose a reason for hiding this comment

codingwhatever commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

archolewa commented Oct 11, 2016

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq left a comment

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

Choose a reason for hiding this comment

codingwhatever commented Oct 13, 2016

cdeszaq left a comment

Choose a reason for hiding this comment

michael-mclawhorn commented Oct 13, 2016 • edited

michael-mclawhorn left a comment

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

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

michael-mclawhorn commented Oct 17, 2016

cdeszaq left a comment

Choose a reason for hiding this comment

codingwhatever commented Oct 17, 2016

cdeszaq left a comment

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

Choose a reason for hiding this comment

codingwhatever commented Oct 18, 2016

cdeszaq commented Oct 18, 2016

michael-mclawhorn commented Oct 13, 2016 •

edited