From 687ca05294cf0ba02a4dd45b07706a3efe4a85f9 Mon Sep 17 00:00:00 2001 From: Peter Kingswell <1403866+sming@users.noreply.github.com> Date: Tue, 26 Jan 2021 16:07:38 -0500 Subject: [PATCH] Enforce Mandatory Client ID (Github Issue #704) (#727) this is the implementation of x-client-id header enforcement in Heroic (as opposed to the just-reverted Envoy implementation) --- .../heroic/aggregation/simple/Module.java | 3 +- .../simple/FilterAggregationTest.java | 5 +- .../simple/FilterKAreaAggregationTest.java | 13 +-- .../simple/TdigestAggregationTest.java | 2 - .../simple/ValueBucketIntegrationTest.java | 4 - .../consumer/pubsub/PubSubConsumer.java | 2 +- docs/_layouts/api-endpoint.html | 4 +- .../heroic/aggregation/AbstractBucket.java | 3 +- .../spotify/heroic/aggregation/AnyBucket.java | 1 - .../metric/DistributionPointDeserialize.java | 2 - .../heroic/metric/HeroicDistribution.java | 6 +- .../com/spotify/heroic/metric/MetricType.java | 1 - .../com/spotify/heroic/CoreQueryManager.java | 1 - .../com/spotify/heroic/http/HttpServer.java | 3 + .../servlet/MandatoryClientIdFilter.java | 88 +++++++++++++++ .../heroic/metric/LocalMetricManagerTest.java | 16 ++- .../servlet/MandatoryClientIdFilterTest.java | 103 ++++++++++++++++++ .../heroic/AbstractClusterQueryIT.java | 1 + .../heroic/HeroicConfigurationTest.java | 20 ++-- .../heroic/HeroicDistributionGenerator.java | 4 +- .../heroic/test/DistributionPoints.java | 3 +- .../heroic/test/AbstractSuggestBackendIT.java | 17 ++- .../metric/bigtable/BigtableBackend.java | 1 + .../metric/bigtable/BigtableMetricModule.java | 8 +- system-tests/test_heroic.py | 15 ++- 25 files changed, 262 insertions(+), 64 deletions(-) create mode 100644 heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java create mode 100644 heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java diff --git a/aggregation/simple/src/main/java/com/spotify/heroic/aggregation/simple/Module.java b/aggregation/simple/src/main/java/com/spotify/heroic/aggregation/simple/Module.java index 3a561c658..f1f3d68dc 100644 --- a/aggregation/simple/src/main/java/com/spotify/heroic/aggregation/simple/Module.java +++ b/aggregation/simple/src/main/java/com/spotify/heroic/aggregation/simple/Module.java @@ -35,10 +35,9 @@ import com.spotify.heroic.grammar.IntegerExpression; import dagger.Component; import eu.toolchain.serializer.SerializerFramework; - +import java.util.Optional; import javax.inject.Inject; import javax.inject.Named; -import java.util.Optional; public class Module implements HeroicModule { @Override diff --git a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterAggregationTest.java b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterAggregationTest.java index 0eb307436..6d22fd680 100644 --- a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterAggregationTest.java +++ b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterAggregationTest.java @@ -1,15 +1,14 @@ package com.spotify.heroic.aggregation.simple; +import static org.junit.Assert.assertEquals; + import com.fasterxml.jackson.annotation.JsonTypeInfo; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.jsontype.NamedType; import com.spotify.heroic.aggregation.AggregationInstance; import com.spotify.heroic.test.FakeModuleLoader; import org.junit.Before; import org.junit.Test; -import static org.junit.Assert.assertEquals; - public class FilterAggregationTest { private ObjectMapper mapper; diff --git a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterKAreaAggregationTest.java b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterKAreaAggregationTest.java index 034cfd3d5..153fec1e8 100644 --- a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterKAreaAggregationTest.java +++ b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/FilterKAreaAggregationTest.java @@ -1,5 +1,7 @@ package com.spotify.heroic.aggregation.simple; +import static org.junit.Assert.assertEquals; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.spotify.heroic.aggregation.AggregationInstance; @@ -12,17 +14,14 @@ import com.spotify.heroic.common.DateRange; import com.spotify.heroic.common.Series; import com.spotify.heroic.metric.Point; -import org.junit.Assert; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.mockito.runners.MockitoJUnitRunner; - import java.util.HashSet; import java.util.List; import java.util.Optional; import java.util.Set; - -import static org.junit.Assert.assertEquals; +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.runners.MockitoJUnitRunner; @RunWith(MockitoJUnitRunner.class) public class FilterKAreaAggregationTest { diff --git a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/TdigestAggregationTest.java b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/TdigestAggregationTest.java index 0bb53e66b..37c0b4f46 100644 --- a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/TdigestAggregationTest.java +++ b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/TdigestAggregationTest.java @@ -1,9 +1,7 @@ package com.spotify.heroic.aggregation.simple; - import static org.junit.Assert.assertEquals; - import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.protobuf.ByteString; diff --git a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/ValueBucketIntegrationTest.java b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/ValueBucketIntegrationTest.java index 76adfc2b4..a3fcf7697 100644 --- a/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/ValueBucketIntegrationTest.java +++ b/aggregation/simple/src/test/java/com/spotify/heroic/aggregation/simple/ValueBucketIntegrationTest.java @@ -6,11 +6,8 @@ import com.spotify.heroic.aggregation.DoubleBucket; import com.spotify.heroic.aggregation.TDigestBucket; import com.spotify.heroic.metric.DistributionPoint; -import com.spotify.heroic.metric.HeroicDistribution; import com.spotify.heroic.metric.Point; -import com.tdunning.math.stats.MergingDigest; import com.tdunning.math.stats.TDigest; -import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collection; import java.util.List; @@ -25,7 +22,6 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; -import com.google.protobuf.ByteString; public abstract class ValueBucketIntegrationTest { private static final int NCPU = Runtime.getRuntime().availableProcessors(); diff --git a/consumer/pubsub/src/main/java/com/spotify/heroic/consumer/pubsub/PubSubConsumer.java b/consumer/pubsub/src/main/java/com/spotify/heroic/consumer/pubsub/PubSubConsumer.java index 878d18c55..197045d7b 100644 --- a/consumer/pubsub/src/main/java/com/spotify/heroic/consumer/pubsub/PubSubConsumer.java +++ b/consumer/pubsub/src/main/java/com/spotify/heroic/consumer/pubsub/PubSubConsumer.java @@ -21,11 +21,11 @@ package com.spotify.heroic.consumer.pubsub; +import com.google.common.collect.ImmutableMap; import com.spotify.heroic.common.Statistics; import com.spotify.heroic.consumer.Consumer; import com.spotify.heroic.lifecycle.LifeCycleRegistry; import com.spotify.heroic.lifecycle.LifeCycles; -import com.google.common.collect.ImmutableMap; import eu.toolchain.async.AsyncFramework; import eu.toolchain.async.AsyncFuture; import eu.toolchain.async.Managed; diff --git a/docs/_layouts/api-endpoint.html b/docs/_layouts/api-endpoint.html index c7298f1f0..bf493ad10 100644 --- a/docs/_layouts/api-endpoint.html +++ b/docs/_layouts/api-endpoint.html @@ -44,7 +44,9 @@

Example Request

Example Curl


           {%- capture headers %}
-          {%- unless page.empty %}-H "Content-Type: application/json" {% endunless %}
+          {%- unless page.empty %}-H "X-Client-Id: my_app_name" -H "Content-Type: application/json"
+        {%
+        endunless %}
           {%- endcapture %}
           $ curl -X{{ page.method }} {{ headers -}}
           http://localhost:8080{{ page.endpoint }}
diff --git a/heroic-component/src/main/java/com/spotify/heroic/aggregation/AbstractBucket.java b/heroic-component/src/main/java/com/spotify/heroic/aggregation/AbstractBucket.java
index 993a06fc7..b30349d78 100644
--- a/heroic-component/src/main/java/com/spotify/heroic/aggregation/AbstractBucket.java
+++ b/heroic-component/src/main/java/com/spotify/heroic/aggregation/AbstractBucket.java
@@ -22,11 +22,10 @@
 package com.spotify.heroic.aggregation;
 
 import com.spotify.heroic.metric.DistributionPoint;
-import com.spotify.heroic.metric.Payload;
 import com.spotify.heroic.metric.MetricGroup;
+import com.spotify.heroic.metric.Payload;
 import com.spotify.heroic.metric.Point;
 import com.spotify.heroic.metric.Spread;
-
 import com.spotify.heroic.metric.TdigestPoint;
 import java.util.Map;
 
diff --git a/heroic-component/src/main/java/com/spotify/heroic/aggregation/AnyBucket.java b/heroic-component/src/main/java/com/spotify/heroic/aggregation/AnyBucket.java
index 9c9475dcb..af35873f6 100644
--- a/heroic-component/src/main/java/com/spotify/heroic/aggregation/AnyBucket.java
+++ b/heroic-component/src/main/java/com/spotify/heroic/aggregation/AnyBucket.java
@@ -27,7 +27,6 @@
 import com.spotify.heroic.metric.Payload;
 import com.spotify.heroic.metric.Point;
 import com.spotify.heroic.metric.Spread;
-
 import com.spotify.heroic.metric.TdigestPoint;
 import java.util.Map;
 
diff --git a/heroic-component/src/main/java/com/spotify/heroic/metric/DistributionPointDeserialize.java b/heroic-component/src/main/java/com/spotify/heroic/metric/DistributionPointDeserialize.java
index ed041e8f1..fc803b0b9 100644
--- a/heroic-component/src/main/java/com/spotify/heroic/metric/DistributionPointDeserialize.java
+++ b/heroic-component/src/main/java/com/spotify/heroic/metric/DistributionPointDeserialize.java
@@ -25,9 +25,7 @@
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.DeserializationContext;
 import com.fasterxml.jackson.databind.JsonNode;
-
 import com.fasterxml.jackson.databind.deser.std.StdDeserializer;
-
 import com.google.protobuf.ByteString;
 import java.io.IOException;
 
diff --git a/heroic-component/src/main/java/com/spotify/heroic/metric/HeroicDistribution.java b/heroic-component/src/main/java/com/spotify/heroic/metric/HeroicDistribution.java
index 8ff8214b3..bcc735aeb 100644
--- a/heroic-component/src/main/java/com/spotify/heroic/metric/HeroicDistribution.java
+++ b/heroic-component/src/main/java/com/spotify/heroic/metric/HeroicDistribution.java
@@ -21,9 +21,9 @@
 
 package com.spotify.heroic.metric;
 
-    import com.google.auto.value.AutoValue;
-    import com.google.protobuf.ByteString;
-    import java.nio.ByteBuffer;
+import com.google.auto.value.AutoValue;
+import com.google.protobuf.ByteString;
+import java.nio.ByteBuffer;
 
 @AutoValue
 public abstract class HeroicDistribution implements Distribution {
diff --git a/heroic-component/src/main/java/com/spotify/heroic/metric/MetricType.java b/heroic-component/src/main/java/com/spotify/heroic/metric/MetricType.java
index 2b0fbacb0..eeff260a8 100644
--- a/heroic-component/src/main/java/com/spotify/heroic/metric/MetricType.java
+++ b/heroic-component/src/main/java/com/spotify/heroic/metric/MetricType.java
@@ -22,7 +22,6 @@
 package com.spotify.heroic.metric;
 
 import com.google.common.collect.ImmutableMap;
-
 import java.util.Arrays;
 import java.util.Map;
 import java.util.Optional;
diff --git a/heroic-core/src/main/java/com/spotify/heroic/CoreQueryManager.java b/heroic-core/src/main/java/com/spotify/heroic/CoreQueryManager.java
index 5deea12c7..8fd916e26 100644
--- a/heroic-core/src/main/java/com/spotify/heroic/CoreQueryManager.java
+++ b/heroic-core/src/main/java/com/spotify/heroic/CoreQueryManager.java
@@ -25,7 +25,6 @@
 import static io.opencensus.trace.AttributeValue.longAttributeValue;
 import static io.opencensus.trace.AttributeValue.stringAttributeValue;
 
-
 import com.google.common.base.Stopwatch;
 import com.google.common.collect.ImmutableSortedSet;
 import com.spotify.heroic.aggregation.Aggregation;
diff --git a/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java b/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
index 9e068e9e5..d77c76aff 100644
--- a/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
+++ b/heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
@@ -32,6 +32,7 @@
 import com.spotify.heroic.jetty.JettyServerConnector;
 import com.spotify.heroic.lifecycle.LifeCycleRegistry;
 import com.spotify.heroic.lifecycle.LifeCycles;
+import com.spotify.heroic.servlet.MandatoryClientIdFilter;
 import com.spotify.heroic.servlet.ShutdownFilter;
 import com.spotify.heroic.tracing.TracingConfig;
 import eu.toolchain.async.AsyncFramework;
@@ -212,6 +213,7 @@ private HandlerCollection setupHandler() throws Exception {
 
         context.addServlet(jerseyServlet, "/*");
         context.addFilter(new FilterHolder(new ShutdownFilter(stopping, mapper)), "/*", null);
+        context.addFilter(new FilterHolder(new MandatoryClientIdFilter()), "/*", null);
         context.setErrorHandler(new JettyJSONErrorHandler(mapper));
 
         final RequestLogHandler requestLogHandler = new RequestLogHandler();
@@ -221,6 +223,7 @@ private HandlerCollection setupHandler() throws Exception {
         final RewriteHandler rewrite = new RewriteHandler();
         makeRewriteRules(rewrite);
 
+
         final HandlerCollection handlers = new HandlerCollection();
         handlers.setHandlers(new Handler[]{rewrite, context, requestLogHandler});
 
diff --git a/heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java b/heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java
new file mode 100644
index 000000000..ca8149eb6
--- /dev/null
+++ b/heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java
@@ -0,0 +1,88 @@
+/*
+ * Copyright (c) 2015 Spotify AB.
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.spotify.heroic.servlet;
+
+import com.google.common.base.Strings;
+import java.io.IOException;
+import javax.servlet.Filter;
+import javax.servlet.FilterChain;
+import javax.servlet.FilterConfig;
+import javax.servlet.ServletException;
+import javax.servlet.ServletRequest;
+import javax.servlet.ServletResponse;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import javax.ws.rs.core.Response.Status;
+
+/**
+ * Rejects anonymous requests. That is, requests to any API endpoint that are
+ * missing a non-null X-Client-Id HTTP header.
+ */
+// Note that this @Suppress has to go here even though it's just for the doFilter
+// method, else the JavaDoc for it doesn't render. Weird.
+@SuppressWarnings("checkstyle:LineLength")
+public class MandatoryClientIdFilter implements Filter {
+
+    public static final String X_CLIENT_ID_HEADER_NAME = "X-Client-Id";
+
+    @Override
+    public void init(FilterConfig filterConfig) throws ServletException { /* intentionally empty
+    */ }
+
+    /**
+     * Reject (with a 400) the request, if the X-Client-Id HTTP header is not present
+     * or is non-null/empty.

+ * Calling {@link javax.servlet.FilterChain#doFilter} + * effectively "passes" this filter and the next + * filter gets a stab at it.

+ * Conversely, not calling doFilter halts "happy path" processing altogether + * and that's the mechanism with which we stop anonymous requests.

+ * Finally, using + * {@link javax.servlet.http.HttpServletResponse#sendError(int, java.lang.String)} to + * return a status message didn't work and instead sent text of "internal error" back + * to the client. + */ + @Override + public void doFilter( + ServletRequest request, ServletResponse response, FilterChain chain + ) throws IOException, ServletException { + if (passesFilter(request)) { + chain.doFilter(request, response); + } else { + var httpResponse = HttpServletResponse.class.cast(response); + httpResponse.setStatus(Status.BAD_REQUEST.getStatusCode()); + } + } + + /** + * Returns true if the HTTP header X-Client-Id is present and non-null and not empty. + * @param request request to pluck X-Client-Id's value from + * @return see above + */ + public static boolean passesFilter(ServletRequest request) { + var req = HttpServletRequest.class.cast(request); + return !Strings.isNullOrEmpty(req.getHeader(X_CLIENT_ID_HEADER_NAME)); + } + + @Override + public void destroy() { /* intentionally empty */ } +}; diff --git a/heroic-core/src/test/java/com/spotify/heroic/metric/LocalMetricManagerTest.java b/heroic-core/src/test/java/com/spotify/heroic/metric/LocalMetricManagerTest.java index 4b03b5626..11ded3d1a 100644 --- a/heroic-core/src/test/java/com/spotify/heroic/metric/LocalMetricManagerTest.java +++ b/heroic-core/src/test/java/com/spotify/heroic/metric/LocalMetricManagerTest.java @@ -66,9 +66,19 @@ public void setup() { final QueryLoggerFactory queryLoggerFactory = mock(QueryLoggerFactory.class); when(queryLoggerFactory.create(any())).thenReturn(queryLogger); - manager = new LocalMetricManager(groupLimit, seriesLimit, aggregationLimit, dataLimit, - concurrentQueriesBackoff, fetchParallelism, failOnLimits, async, groupSet, metadata, - reporter, queryLoggerFactory); + manager = new LocalMetricManager( + groupLimit, + seriesLimit, + aggregationLimit, + dataLimit, + concurrentQueriesBackoff, + fetchParallelism, + failOnLimits, + async, + groupSet, + metadata, + reporter, + queryLoggerFactory); } @Test diff --git a/heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java b/heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java new file mode 100644 index 000000000..c02c0dd9b --- /dev/null +++ b/heroic-core/src/test/java/com/spotify/heroic/servlet/MandatoryClientIdFilterTest.java @@ -0,0 +1,103 @@ +package com.spotify.heroic.servlet; + +import static org.junit.Assert.assertEquals; +import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import java.io.IOException; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletResponse; +import javax.ws.rs.core.Response.Status; +import org.eclipse.jetty.server.HttpChannel; +import org.eclipse.jetty.server.HttpInput; +import org.eclipse.jetty.server.HttpOutput; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Response; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; +import org.mockito.junit.MockitoJUnitRunner; + +/** + * straightforward tests to show that the passesFilter method is correct and that + * `chain.doFilter(...)` is only called when the x-client-id header is passed, + * which proves that only such requests are not rejected. + */ +@RunWith(MockitoJUnitRunner.class) +public class MandatoryClientIdFilterTest { + + private static MandatoryClientIdFilter filter; + + @Mock + private Request request; + + private HttpServletResponse response; + + @Mock + private FilterChain chain; + + @Mock + private HttpChannel channel; + + @Mock + private HttpInput input; + + @Mock + private HttpOutput out; + + @BeforeClass + public static void setUpClass() { + filter = new MandatoryClientIdFilter(); + } + + @Before + public void setUp() { + MockitoAnnotations.initMocks(this); + response = new Response(channel, out); + } + + @Test + public void doNoHeaderTest() throws IOException, ServletException { + Mockito + .doReturn(null) + .when(request) + .getHeader(MandatoryClientIdFilter.X_CLIENT_ID_HEADER_NAME); + + filter.doFilter(request, response, chain); + + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + verify(chain, never()).doFilter(same(request), same(response)); + } + + @Test + public void doEmptyStringHeaderTest() throws IOException, ServletException { + Mockito + .doReturn("") + .when(request) + .getHeader(MandatoryClientIdFilter.X_CLIENT_ID_HEADER_NAME); + + filter.doFilter(request, response, chain); + + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + verify(chain, never()).doFilter(same(request), same(response)); + } + + @Test + public void doCorrectHeaderTest() throws IOException, ServletException { + Mockito + .doReturn("fred") + .when(request) + .getHeader(MandatoryClientIdFilter.X_CLIENT_ID_HEADER_NAME); + + filter.doFilter(request, response, chain); + + assertEquals(Status.OK.getStatusCode(), response.getStatus()); + verify(chain, Mockito.times(1)).doFilter(same(request), same(response)); + } +} diff --git a/heroic-dist/src/test/java/com/spotify/heroic/AbstractClusterQueryIT.java b/heroic-dist/src/test/java/com/spotify/heroic/AbstractClusterQueryIT.java index d6a82da25..f09d5365e 100644 --- a/heroic-dist/src/test/java/com/spotify/heroic/AbstractClusterQueryIT.java +++ b/heroic-dist/src/test/java/com/spotify/heroic/AbstractClusterQueryIT.java @@ -17,6 +17,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoMoreInteractions; + import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import com.google.common.collect.ImmutableSortedMap; diff --git a/heroic-dist/src/test/java/com/spotify/heroic/HeroicConfigurationTest.java b/heroic-dist/src/test/java/com/spotify/heroic/HeroicConfigurationTest.java index 2b826e0cb..6cebc3fd5 100644 --- a/heroic-dist/src/test/java/com/spotify/heroic/HeroicConfigurationTest.java +++ b/heroic-dist/src/test/java/com/spotify/heroic/HeroicConfigurationTest.java @@ -126,22 +126,22 @@ public void testDatastaxConfiguration() throws Exception { public void testConditionalFeatures() throws Exception { final HeroicCoreInstance instance = testConfiguration("heroic-conditional-features.yml"); - final HttpContext httpContext1 = mock(HttpContext.class); - doReturn(Optional.of("bar")).when(httpContext1).getClientId(); - final HttpContext httpContext2 = mock(HttpContext.class); - doReturn(Optional.empty()).when(httpContext2).getClientId(); + final HttpContext httpContextClientIdBar = mock(HttpContext.class); + doReturn(Optional.of("bar")).when(httpContextClientIdBar).getClientId(); + final HttpContext httpContextEmptyClientId = mock(HttpContext.class); + doReturn(Optional.empty()).when(httpContextEmptyClientId).getClientId(); - final QueryContext context1 = mock(QueryContext.class); - doReturn(Optional.of(httpContext1)).when(context1).httpContext(); - final QueryContext context2 = mock(QueryContext.class); - doReturn(Optional.of(httpContext2)).when(context2).httpContext(); + final QueryContext queryContextClientIdBar = mock(QueryContext.class); + doReturn(Optional.of(httpContextClientIdBar)).when(queryContextClientIdBar).httpContext(); + final QueryContext queryContextAnonymous = mock(QueryContext.class); + doReturn(Optional.of(httpContextEmptyClientId)).when(queryContextAnonymous).httpContext(); instance.inject(coreComponent -> { final ConditionalFeatures conditional = coreComponent.conditionalFeatures().get(); assertEquals(FeatureSet.of(Feature.CACHE_QUERY), - conditional.match(context1)); - assertEquals(FeatureSet.empty(), conditional.match(context2)); + conditional.match(queryContextClientIdBar)); + assertEquals(FeatureSet.empty(), conditional.match(queryContextAnonymous)); return null; }); diff --git a/heroic-dist/src/test/java/com/spotify/heroic/HeroicDistributionGenerator.java b/heroic-dist/src/test/java/com/spotify/heroic/HeroicDistributionGenerator.java index 9378c46b1..a154221f3 100644 --- a/heroic-dist/src/test/java/com/spotify/heroic/HeroicDistributionGenerator.java +++ b/heroic-dist/src/test/java/com/spotify/heroic/HeroicDistributionGenerator.java @@ -1,8 +1,5 @@ package com.spotify.heroic; -import static com.google.common.math.Quantiles.percentiles; - - import com.google.protobuf.ByteString; import com.spotify.heroic.metric.HeroicDistribution; import com.tdunning.math.stats.MergingDigest; @@ -67,6 +64,7 @@ public static class Data { static List getParetoData(int count) { ParetoDistribution pareto = new ParetoDistribution(5, 1); List list = new ArrayList<>(); + //noinspection AssignmentToMethodParameter while (count-- > 0) { list.add((long)pareto.sample()); } diff --git a/heroic-dist/src/test/java/com/spotify/heroic/test/DistributionPoints.java b/heroic-dist/src/test/java/com/spotify/heroic/test/DistributionPoints.java index 8c574f3bf..fbf70b68f 100644 --- a/heroic-dist/src/test/java/com/spotify/heroic/test/DistributionPoints.java +++ b/heroic-dist/src/test/java/com/spotify/heroic/test/DistributionPoints.java @@ -1,13 +1,12 @@ package com.spotify.heroic.test; - import com.google.common.collect.ImmutableList; import com.google.protobuf.ByteString; import com.spotify.heroic.metric.DistributionPoint; import com.spotify.heroic.metric.HeroicDistribution; import com.spotify.heroic.metric.MetricCollection; import com.tdunning.math.stats.TDigest; -import java.nio.ByteBuffer;; +import java.nio.ByteBuffer; import java.util.List; public class DistributionPoints { diff --git a/heroic-test/src/main/java/com/spotify/heroic/test/AbstractSuggestBackendIT.java b/heroic-test/src/main/java/com/spotify/heroic/test/AbstractSuggestBackendIT.java index 521efcae6..335330ec1 100644 --- a/heroic-test/src/main/java/com/spotify/heroic/test/AbstractSuggestBackendIT.java +++ b/heroic-test/src/main/java/com/spotify/heroic/test/AbstractSuggestBackendIT.java @@ -102,7 +102,6 @@ public abstract class AbstractSuggestBackendIT { protected SuggestBackend backend; private HeroicCoreInstance core; - protected abstract SuggestModule setupModule() throws Exception; @Before @@ -121,7 +120,7 @@ public final void abstractSetup() throws Exception { backend = core.inject( c -> c.suggestManager().groupSet().inspectAll().stream() .map(GroupMember::getMember).findFirst()).orElseThrow( - () -> new IllegalStateException("Failed to find backend")); + () -> new IllegalStateException("Failed to find backend")); } @After @@ -274,7 +273,6 @@ public void tagValueSuggestLimited() throws Exception { assertEquals(REQ_SUGGESTION_ENTITY_LIMIT, result.getValues().size()); } - @Test public void keySuggest() throws Exception { var et = EntityType.KEY; @@ -377,12 +375,14 @@ private AsyncFuture checks(final Series s, DateRange range) { matchKey(s.getKey()), range, OptionalLimit.empty(), MatchOptions.builder().build(), Optional.empty())).directTransform(result -> { - if (result.getSuggestions().isEmpty()) { - throw new IllegalStateException("No key suggestion available for the given series"); - } + if (result.getSuggestions().isEmpty()) { + throw new IllegalStateException( + "No key suggestion available for the given series"); + } - return null; - })); + return null; + } + )); return async.collectAndDiscard(checks); } @@ -503,7 +503,6 @@ private static long getUniqueTimestamp() { return t; } - private static List> createTestSeriesData(int numKeys, int tagsAndTagValuesPerKey, long timestamp, EntityType et) { diff --git a/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableBackend.java b/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableBackend.java index 8150f0de6..84d07453d 100644 --- a/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableBackend.java +++ b/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableBackend.java @@ -121,6 +121,7 @@ public class BigtableBackend extends AbstractMetricBackend implements LifeCycles private final Meter written = new Meter(); private final int maxWriteBatchSize; + @Inject public BigtableBackend( final AsyncFramework async, diff --git a/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableMetricModule.java b/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableMetricModule.java index 18d0cd846..5986661be 100644 --- a/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableMetricModule.java +++ b/metric/bigtable/src/main/java/com/spotify/heroic/metric/bigtable/BigtableMetricModule.java @@ -141,11 +141,11 @@ public BigtableMetricModule( } @Override - public Exposed module(PrimaryComponent primary, Depends depends, String id) { + public Exposed module(PrimaryComponent primary, Depends backend, String id) { return DaggerBigtableMetricModule_C .builder() .primaryComponent(primary) - .depends(depends) + .depends(backend) .m(new M()) .build(); } @@ -179,9 +179,9 @@ public AsyncFuture construct() { } @Override - public AsyncFuture destruct(final BigtableConnection value) { + public AsyncFuture destruct(final BigtableConnection t) { return async.call(() -> { - value.close(); + t.close(); return null; }); } diff --git a/system-tests/test_heroic.py b/system-tests/test_heroic.py index 532d83deb..f4755d413 100644 --- a/system-tests/test_heroic.py +++ b/system-tests/test_heroic.py @@ -2,10 +2,10 @@ import pytest import requests -from requests.adapters import HTTPAdapter +import time import urllib.parse +from requests.adapters import HTTPAdapter from urllib3.util.retry import Retry -import time pytest_plugins = ['docker_compose'] @@ -44,16 +44,25 @@ def api_session(session_scoped_container_getter): backoff_factor=0.1, status_forcelist=[500, 502, 503, 504]) request_session.mount('http://', HTTPAdapter(max_retries=retries)) + return request_session def test_loading(api_session): + api_session.headers.update({'x-client-id': 'Heroic--System-Tests'}) resp = api_session.get('/status', timeout=10) + assert resp.ok - status = resp.json() + status = resp.json() assert status['ok'] assert status['consumers']['ok'] assert status['backends']['ok'] assert status['metadataBackend']['ok'] assert status['cluster']['ok'] + +def test_loading_no_client_id(api_session): + api_session.headers.update({'x-client-id': ''}) + resp = api_session.get('/status', timeout=10) + assert resp.status_code == requests.codes['bad_request'] +