Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to add auth headers to Druid requests. #62

Merged
merged 1 commit into from
Oct 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,11 @@ Current

### Changed:

- [Allow configurable headers for Druid data requests](https:://github.com/yahoo/fili/pull/62)
- The `AsyncDruidWebServiceImpl` now accepts a `Supplier<Map<String, String>>` argument which specifies
the headers to add to the Druid data requests. This feature is made configurable through `SystemConfig` in
the `AbstractBinderFactory`

- [Improves error messages when querying Druid goes wrong](https:://github.com/yahoo/fili/pull/61)
- The `ResponseException` now includes a message that prints the `ResponseException`'s internal state
(i.e. the druid query and response code) using the error messages
Expand Down Expand Up @@ -138,6 +143,11 @@ Jobs resource. Here are the highlights of what's in this release:

### Deprecated:

- [Allow configurable headers for Druid data requests](https:://github.com/yahoo/fili/pull/62)
* Deprecated `AsyncDruidWebServiceImpl(DruidServiceConfig, ObjectMapper)` and
`AsyncDruidWebServiceImpl(DruidServiceConfig, AsyncHttpClient, ObjectMapper)` because we added new construstructors
that take a `Supplier` argument for Druid data request headers.

- [QueryTimeLookup Functionality Testing](https://github.com/yahoo/fili/pull/34)
* Deprecated `KeyValueDimensionLoader`, in favor of `TypeAwareDimensionLoader`

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@
import rx.subjects.PublishSubject;

import java.io.IOException;
import java.lang.reflect.Constructor;
import java.time.Clock;
import java.time.ZoneId;
import java.util.Arrays;
Expand All @@ -119,6 +120,7 @@
import java.util.Set;
import java.util.UUID;
import java.util.concurrent.TimeUnit;
import java.util.function.Supplier;
import java.util.stream.Collectors;
import java.util.stream.Stream;

Expand All @@ -144,6 +146,8 @@ public abstract class AbstractBinderFactory implements BinderFactory {
private static final String METER_SPLITS_TOTAL_RATIO = "queries.meter.split_queries.total_ratio";
private static final String METER_SPLITS_RATIO = "queries.meter.split_queries.ratio";

private static final String DRUID_HEADER_SUPPLIER_CLASS = "druid_header_supplier_class";

// Two minutes in milliseconds
public static final int HC_LAST_RUN_PERIOD_MILLIS_DEFAULT = 120 * 1000;
public static final int LOADER_SCHEDULER_THREAD_POOL_SIZE_DEFAULT = 4;
Expand Down Expand Up @@ -937,7 +941,33 @@ protected Class<? extends PhysicalTableResolver> getPhysicalTableResolver() {
* @return A DruidWebService
*/
protected DruidWebService buildDruidWebService(DruidServiceConfig druidServiceConfig, ObjectMapper mapper) {
return new AsyncDruidWebServiceImpl(druidServiceConfig, mapper);
Supplier<Map<String, String>> supplier = buildDruidWebServiceHeaderSupplier();
return new AsyncDruidWebServiceImpl(druidServiceConfig, mapper, supplier);
}

/**
* Build the Supplier for Druid data request headers.
*
* @return The Druid data request header Supplier.
*/
protected Supplier<Map<String, String>> buildDruidWebServiceHeaderSupplier() {
Supplier<Map<String, String>> supplier = HashMap::new;
String customSupplierClassString = SYSTEM_CONFIG.getStringProperty(DRUID_HEADER_SUPPLIER_CLASS, null);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (customSupplierClassString != null && customSupplierClassString.equals("")) {
try {
Class<?> c = Class.forName(customSupplierClassString);
Constructor<?> constructor = c.getConstructor();
supplier = (Supplier<Map<String, String>>) constructor.newInstance();
} catch (Exception e) {
LOG.error(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"Unable to load the Druid query header supplier, className: {}, exception: {}",
customSupplierClassString,
e
);
throw new IllegalStateException(e);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return supplier;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,10 @@

import java.io.IOException;
import java.io.InputStream;
import java.util.HashMap;
import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Supplier;

import javax.ws.rs.core.Response.Status;

Expand All @@ -61,17 +64,38 @@ public class AsyncDruidWebServiceImpl implements DruidWebService {
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>> headersToAppend;
private final DruidServiceConfig serviceConfig;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* Friendly non-DI constructor useful for manual tests.
*
* @param serviceConfig Configuration for the Druid Service
* @param mapper A shared jackson object mapper resource
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc needed for the new parameter

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed this in the constructor below.

* @deprecated We now require a header supplier parameter.
* Use {@link #AsyncDruidWebServiceImpl(DruidServiceConfig, ObjectMapper, Supplier)}
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public AsyncDruidWebServiceImpl(
DruidServiceConfig serviceConfig,
ObjectMapper mapper
) {
this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper, HashMap::new);
}

/**
* Friendly non-DI constructor useful for manual tests.
*
* @param serviceConfig Configuration for the Druid Service
* @param mapper A shared jackson object mapper resource
* @param headersToAppend Supplier for map of headers for Druid requests
*/
public AsyncDruidWebServiceImpl(DruidServiceConfig serviceConfig, ObjectMapper mapper) {
this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper);
public AsyncDruidWebServiceImpl(
DruidServiceConfig serviceConfig,
ObjectMapper mapper,
Supplier<Map<String, String>> headersToAppend
) {
this(serviceConfig, initializeWebClient(serviceConfig.getTimeout()), mapper, headersToAppend);
}

/**
Expand Down Expand Up @@ -101,8 +125,32 @@ private static AsyncHttpClient initializeWebClient(int requestTimeout) {
* @param config the configuration for this druid service
* @param asyncHttpClient the HTTP client
* @param mapper A shared jackson object mapper resource
* @deprecated We now require a header supplier parameter.
* Use {@link #AsyncDruidWebServiceImpl(DruidServiceConfig, AsyncHttpClient, ObjectMapper, Supplier)}
*/
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public AsyncDruidWebServiceImpl(
DruidServiceConfig config,
AsyncHttpClient asyncHttpClient,
ObjectMapper mapper
) {
this(config, asyncHttpClient, mapper, HashMap::new);
}

/**
* IOC constructor.
*
* @param config the configuration for this druid service
* @param asyncHttpClient the HTTP client
* @param mapper A shared jackson object mapper resource
* @param headersToAppend Supplier for map of headers for Druid requests
*/
public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient asyncHttpClient, ObjectMapper mapper) {
public AsyncDruidWebServiceImpl(
DruidServiceConfig config,
AsyncHttpClient asyncHttpClient,
ObjectMapper mapper,
Supplier<Map<String, String>> headersToAppend
) {
this.serviceConfig = config;

if (serviceConfig.getUrl() == null) {
Expand All @@ -112,6 +160,7 @@ public AsyncDruidWebServiceImpl(DruidServiceConfig config, AsyncHttpClient async
}

LOG.info("Configured with druid server config: {}", config.toString());
this.headersToAppend = headersToAppend;
this.webClient = asyncHttpClient;
this.writer = mapper.writer();
this.httpErrorMeter = REGISTRY.meter("druid.errors.http");
Expand Down Expand Up @@ -267,14 +316,18 @@ public void postDruidQuery(
timerName = DRUID_WEIGHTED_QUERY_TIMER + String.format(format, seqNum);
}

BoundRequestBuilder requestBuilder = webClient.preparePost(serviceConfig.getUrl())
.setBody(entityBody)
.addHeader("Content-Type", "application/json");

headersToAppend.get().forEach(requestBuilder::addHeader);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LOG.debug("druid json request: {}", entityBody);
sendRequest(
success,
error,
failure,
webClient.preparePost(serviceConfig.getUrl())
.setBody(entityBody)
.addHeader("Content-Type", "application/json"),
requestBuilder,
timerName,
outstanding
);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2016 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.druid.client.impl
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copyright needed


import com.yahoo.bard.webservice.druid.client.DruidClientConfigHelper
import com.yahoo.bard.webservice.druid.model.query.QueryContext
import com.yahoo.bard.webservice.druid.model.query.WeightEvaluationQuery

import com.fasterxml.jackson.databind.ObjectMapper

import io.netty.handler.codec.http.HttpHeaders
import spock.lang.Specification

import java.util.function.Supplier

class AsyncDruidWebServiceImplSpec extends Specification {
def "Ensure that headersToAppend are added"() {
setup:
WeightEvaluationQuery weightEvaluationQuery = Mock(WeightEvaluationQuery)
QueryContext queryContext = Mock(QueryContext)
weightEvaluationQuery.getContext() >> { queryContext }
queryContext.numberOfQueries() >> { 1 }
queryContext.getSequenceNumber() >> { 1 }

and:
Map<String, String> expectedHeaders = new HashMap<>()
expectedHeaders.put("k1", "v1")
expectedHeaders.put("k2", "v2")
Supplier<Map<String, String>> supplier = new Supplier<Map<String, String>>() {
@Override
Map<String, String> get() {
return expectedHeaders
}
}
AsyncDruidWebServiceImplWrapper webServiceImplWrapper = new AsyncDruidWebServiceImplWrapper(
DruidClientConfigHelper.getNonUiServiceConfig(),
new ObjectMapper(),
supplier
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


when:
webServiceImplWrapper.postDruidQuery(
null,
null,
null,
null,
weightEvaluationQuery
);

then:
HttpHeaders actualHeaders = webServiceImplWrapper.getHeaders()
for (Map.Entry<String, String> header : expectedHeaders) {
assert actualHeaders.get(header.getKey()) == header.getValue()
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
// Copyright 2016 Yahoo Inc.
// Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
package com.yahoo.bard.webservice.druid.client.impl;

import com.yahoo.bard.webservice.druid.client.DruidServiceConfig;
import com.yahoo.bard.webservice.druid.client.FailureCallback;
import com.yahoo.bard.webservice.druid.client.HttpErrorCallback;
import com.yahoo.bard.webservice.druid.client.SuccessCallback;

import com.fasterxml.jackson.databind.ObjectMapper;

import org.asynchttpclient.BoundRequestBuilder;
import org.asynchttpclient.Request;

import io.netty.handler.codec.http.HttpHeaders;

import java.util.Map;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.Supplier;

/**
* Used for testing AsyncDruidWebServiceImpl.
*/
public class AsyncDruidWebServiceImplWrapper extends AsyncDruidWebServiceImpl {
public Request request;

/**
* Constructor wrapper.
*
* @param serviceConfig Service config
* @param mapper Mapper
* @param headersToAppend Headers
*/
public AsyncDruidWebServiceImplWrapper(
DruidServiceConfig serviceConfig,
ObjectMapper mapper,
Supplier<Map<String, String>> headersToAppend
) {
super(serviceConfig, mapper, headersToAppend);
}

/**
* Capture arguments to test for expected values.
*
* @param success callback for handling successful requests.
* @param error callback for handling http errors.
* @param failure callback for handling exception failures.
* @param requestBuilder The bound request builder for the request to be sent.
* @param timerName The name that distinguishes this request as part of a druid query or segment metadata request
* @param outstanding The counter that keeps track of the outstanding (in flight) requests for the top level query
*/
@Override
protected void sendRequest(
final SuccessCallback success,
final HttpErrorCallback error,
final FailureCallback failure,
final BoundRequestBuilder requestBuilder,
final String timerName,
final AtomicLong outstanding
) {
this.request = requestBuilder.build();
}

public HttpHeaders getHeaders() {
return request.getHeaders();
}
}