feat(influxdb): enhancements and bug fixes (#899)

* feat(influxdb): bug fixes and enhancements * chore(build): Gradle 7 compatibility (#895) This change makes the project compatible with Gradle 7 * chore(dependencies): add explicit dependency on javax.validation:validation-api (#897) since spring cloud Hoxton.SR12 doesn't bring it in. See spinnaker/orca#4254 for more. * chore(dependencies): Autobump orcaVersion (#896) Co-authored-by: root <root@470565d33c7a> Co-authored-by: David Byron <82477955+dbyron-sf@users.noreply.github.com> * chore(influxdb): adding docs * chore(influxdb): update influxdb docs * chore(influxdb): fix spacing in docs * chore(influxdb): added comments Co-authored-by: IvanessChristleChiong <Ivaness.Chiong@target.com> Co-authored-by: Patrik Greco <pgreco@apple.com> Co-authored-by: David Byron <82477955+dbyron-sf@users.noreply.github.com> Co-authored-by: spinnakerbot <spinbot@spinnaker.io> Co-authored-by: root <root@470565d33c7a> Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com>
spinnaker · Oct 11, 2022 · a3c9ae7 · a3c9ae7
1 parent aa4c906
commit a3c9ae7
Show file tree

Hide file tree

Showing 7 changed files with 267 additions and 44 deletions.
diff --git a/kayenta-influxdb/docs/metric-set-query-config.md b/kayenta-influxdb/docs/metric-set-query-config.md
@@ -0,0 +1,26 @@
+### InfluxdbCanaryMetricSetQueryConfig (CanaryMetricSetQueryConfig)
+Influxdb specific query configurations.
+
+#### Properties
+- `metricName` (string, optional): The measurement name where metrics are stored. This field is **required** UNLESS using `customInlineTemplate`.
+
+   ```
+   "metricName": "cpu"
+   ```
+
+- `fields` (array[string], optional): The list of field names that need to be included in query. This field is **required** UNLESS using `customInlineTemplate`. See example below:
+
+   ```
+   fields: [
+      "count"
+   ]
+   ```
+
+- `customInlineTemplate` (string, optional): This allows you to write your own IQL statement. `${scope}` and `{timeFilter}` variables are **required** in the IQL statement. See example below:
+
+   ```
+   customInlineTemplate: "SELECT sum(count) FROM cpu WHERE host = 'value1' AND ${scope} AND ${timeFilter} GROUP BY time(1m)"
+   ```
+
+- `type` (enum[string], required)
+    - `influxdb`
diff --git a/...java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java b/...java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java
@@ -19,14 +19,15 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.netflix.kayenta.canary.CanaryMetricSetQueryConfig;
 import java.util.List;
-import javax.validation.constraints.NotNull;
+import javax.annotation.Nullable;
 import lombok.AllArgsConstructor;
 import lombok.Builder;
 import lombok.Getter;
 import lombok.NoArgsConstructor;
 import lombok.ToString;
+import org.springframework.util.StringUtils;
 
-@Builder
+@Builder(toBuilder = true)
 @ToString
 @NoArgsConstructor
 @AllArgsConstructor
@@ -35,9 +36,21 @@ public class InfluxdbCanaryMetricSetQueryConfig implements CanaryMetricSetQueryC
 
   public static final String SERVICE_TYPE = "influxdb";
 
-  @NotNull @Getter private String metricName;
+  @Nullable @Getter private String metricName;
 
   @Getter private List<String> fields;
+  @Nullable @Getter private String customInlineTemplate;
+
+  @Override
+  public CanaryMetricSetQueryConfig cloneWithEscapedInlineTemplate() {
+    if (StringUtils.isEmpty(customInlineTemplate)) {
+      return this;
+    } else {
+      return this.toBuilder()
+          .customInlineTemplate(customInlineTemplate.replace("${", "$\\{"))
+          .build();
+    }
+  }
 
   @Override
   public String getServiceType() {

diff --git a/...influxdb/src/main/java/com/netflix/kayenta/influxdb/config/InfluxDbResponseConverter.java b/...influxdb/src/main/java/com/netflix/kayenta/influxdb/config/InfluxDbResponseConverter.java
@@ -82,7 +82,8 @@ public Object fromBody(TypedInput body, Type type) throws ConversionException {
         List<Double> values = new ArrayList<>(seriesValues.size());
         for (List<Object> valueRow : seriesValues) {
           if (valueRow.get(i) != null) {
-            values.add(Double.valueOf((Integer) valueRow.get(i)));
+            String val = valueRow.get(i).toString();
+            values.add(Double.valueOf(val));
           }
         }
         influxDbResultsList.add(new InfluxDbResult(id, firstTimeMillis, stepMillis, null, values));

diff --git a/...a-influxdb/src/main/java/com/netflix/kayenta/influxdb/metrics/InfluxDbMetricsService.java b/...a-influxdb/src/main/java/com/netflix/kayenta/influxdb/metrics/InfluxDbMetricsService.java
@@ -31,6 +31,7 @@
 import com.netflix.spectator.api.Registry;
 import java.time.Instant;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
@@ -82,7 +83,7 @@ public List<MetricSet> queryMetrics(
     String metricSetName = canaryMetricConfig.getName();
     List<InfluxDbResult> influxDbResults = queryInfluxdb(remoteService, metricSetName, query);
 
-    return buildMetricSets(metricSetName, influxDbResults);
+    return buildMetricSets(metricSetName, influxDbResults, canaryScope, canaryMetricConfig, query);
   }
 
   private List<InfluxDbResult> queryInfluxdb(
@@ -101,8 +102,13 @@ private List<InfluxDbResult> queryInfluxdb(
   }
 
   private List<MetricSet> buildMetricSets(
-      String metricSetName, List<InfluxDbResult> influxDbResults) {
-    List<MetricSet> metricSets = new ArrayList<MetricSet>();
+      String metricSetName,
+      List<InfluxDbResult> influxDbResults,
+      CanaryScope canaryScope,
+      CanaryMetricConfig canaryMetricConfig,
+      String query) {
+    List<MetricSet> metricSetList = new ArrayList<>();
+
     if (influxDbResults != null) {
       for (InfluxDbResult influxDbResult : influxDbResults) {
         Instant endtime =
@@ -125,9 +131,25 @@ private List<MetricSet> buildMetricSets(
           metricSetBuilder.tags(tags);
         }
 
-        metricSets.add(metricSetBuilder.build());
+        metricSetList.add(metricSetBuilder.build());
       }
+    } else {
+      log.warn("Received no data from InfluxDB for query: {} scope: {}", query, canaryScope);
+      MetricSetBuilder metricSetBuilder =
+          MetricSet.builder()
+              .name(canaryMetricConfig.getName())
+              .startTimeMillis(canaryScope.getStart().toEpochMilli())
+              .startTimeIso(canaryScope.getStart().toString())
+              .endTimeMillis(canaryScope.getEnd().toEpochMilli())
+              .endTimeIso(canaryScope.getEnd().toString())
+              .stepMillis(TimeUnit.SECONDS.toMillis(canaryScope.getStep()))
+              .values(Collections.emptyList());
+
+      metricSetBuilder.attribute("query", query);
+      metricSetBuilder.tags(Collections.emptyMap());
+
+      metricSetList.add(metricSetBuilder.build());
     }
-    return metricSets;
+    return metricSetList;
   }
 }
diff --git a/...nta-influxdb/src/main/java/com/netflix/kayenta/influxdb/metrics/InfluxDbQueryBuilder.java b/...nta-influxdb/src/main/java/com/netflix/kayenta/influxdb/metrics/InfluxDbQueryBuilder.java
@@ -18,8 +18,7 @@
 
 import com.netflix.kayenta.canary.CanaryScope;
 import com.netflix.kayenta.canary.providers.metrics.InfluxdbCanaryMetricSetQueryConfig;
-import java.util.ArrayList;
-import java.util.List;
+import java.util.*;
 import java.util.stream.Collectors;
 import lombok.extern.slf4j.Slf4j;
 import org.apache.commons.lang3.StringUtils;
@@ -34,26 +33,31 @@ public class InfluxDbQueryBuilder {
   private static final String SCOPE_INVALID_FORMAT_MSG =
       "Scope expected in the format of 'name:value'. e.g. autoscaling_group:myapp-prod-v002, received: ";
 
-  // TODO(joerajeev): update to accept tags and groupby fields
   // TODO(joerajeev): protect against injection. Influxdb is supposed to support binding params,
   // https://docs.influxdata.com/influxdb/v1.5/tools/api/
   public String build(InfluxdbCanaryMetricSetQueryConfig queryConfig, CanaryScope canaryScope) {
-
-    validateManadtoryParams(queryConfig, canaryScope);
-
     StringBuilder query = new StringBuilder();
-    addBaseQuery(queryConfig.getMetricName(), handleFields(queryConfig), query);
-    addTimeRangeFilter(canaryScope, query);
-    addScopeFilter(canaryScope, query);
+    validateMandatoryParams(queryConfig, canaryScope);
+
+    if (!StringUtils.isEmpty(queryConfig.getCustomInlineTemplate())) {
+      buildCustomQuery(queryConfig, canaryScope, getTimeFilter(canaryScope), query);
+    } else {
+      addBaseQuery(queryConfig.getMetricName(), handleFields(queryConfig), query);
+      addTimeRangeFilter(getTimeFilter(canaryScope), query);
+      addScopeFilter(canaryScope, query);
+    }
 
     String builtQuery = query.toString();
+    validateQuery(builtQuery);
     log.debug("Built query: {} config: {} scope: {}", builtQuery, queryConfig, canaryScope);
+
     return builtQuery;
   }
 
-  private void validateManadtoryParams(
+  private void validateMandatoryParams(
       InfluxdbCanaryMetricSetQueryConfig queryConfig, CanaryScope canaryScope) {
-    if (StringUtils.isEmpty(queryConfig.getMetricName())) {
+    if (StringUtils.isEmpty(queryConfig.getMetricName())
+        && StringUtils.isEmpty(queryConfig.getCustomInlineTemplate())) {
       throw new IllegalArgumentException("Measurement is required to query metrics");
     }
     if (null == canaryScope) {
@@ -62,6 +66,15 @@ private void validateManadtoryParams(
     if (null == canaryScope.getStart() || null == canaryScope.getEnd()) {
       throw new IllegalArgumentException("Start and End times are required");
     }
+    // required variables when using customInlineTemplate
+    if (!StringUtils.isEmpty(queryConfig.getCustomInlineTemplate())) {
+      if (!queryConfig.getCustomInlineTemplate().contains("$\\{timeFilter}")) {
+        throw new IllegalArgumentException("${timeFilter} is required in query");
+      }
+      if (!queryConfig.getCustomInlineTemplate().contains("$\\{scope}")) {
+        throw new IllegalArgumentException("${scope} is required in query");
+      }
+    }
   }
 
   private List<String> handleFields(InfluxdbCanaryMetricSetQueryConfig queryConfig) {
@@ -78,16 +91,13 @@ private List<String> handleFields(InfluxdbCanaryMetricSetQueryConfig queryConfig
   private void addBaseQuery(String measurement, List<String> fields, StringBuilder query) {
     query.append("SELECT ");
     query.append(fields.stream().collect(Collectors.joining(", ")));
-    query.append(" FROM ");
-    query.append(measurement);
+    query.append(" FROM " + measurement + " ");
   }
 
-  private void addScopeFilter(CanaryScope canaryScope, StringBuilder sb) {
+  private void addScopeFilter(CanaryScope canaryScope, StringBuilder query) {
     String scope = canaryScope.getScope();
     if (scope != null) {
-      String[] scopeParts = validateAndExtractScope(scope);
-      sb.append(" AND ");
-      sb.append(scopeParts[0] + "='" + scopeParts[1] + "'");
+      query.append(" AND " + getScope(canaryScope));
     }
   }
 
@@ -102,10 +112,68 @@ private String[] validateAndExtractScope(String scope) {
     return scopeParts;
   }
 
-  private void addTimeRangeFilter(CanaryScope canaryScope, StringBuilder query) {
-    query.append(" WHERE");
-    query.append(" time >= '" + canaryScope.getStart().toString() + "'");
-    query.append(" AND");
-    query.append(" time < '" + canaryScope.getEnd().toString() + "'");
+  private String getScope(CanaryScope canaryScope) {
+    String scope = canaryScope.getScope();
+    String[] scopeParts = validateAndExtractScope(scope);
+    return scopeParts[0] + " = '" + scopeParts[1] + "'";
+  }
+
+  private void addTimeRangeFilter(String timeFilter, StringBuilder query) {
+    query.append("WHERE " + timeFilter);
+  }
+
+  private void buildCustomQuery(
+      InfluxdbCanaryMetricSetQueryConfig queryConfig,
+      CanaryScope canaryScope,
+      String timeFilter,
+      StringBuilder query) {
+    String inlineQuery = queryConfig.getCustomInlineTemplate();
+    validateQuery(inlineQuery);
+    String queryWithTimeFilter = inlineQuery.replace("$\\{timeFilter}", timeFilter);
+    String queryWithScope = queryWithTimeFilter.replace("$\\{scope}", getScope(canaryScope));
+    query.append(addOptionalStep(canaryScope, queryWithScope));
+  }
+
+  private String addOptionalStep(CanaryScope canaryScope, String query) {
+    if (query.contains("$\\{step}")) {
+      query = query.replace("$\\{step}", canaryScope.getStep().toString() + "s");
+    }
+    return query;
+  }
+
+  private String getTimeFilter(CanaryScope canaryScope) {
+    return "time >= '"
+        + canaryScope.getStart().toString()
+        + "' AND time < '"
+        + canaryScope.getEnd().toString()
+        + "'";
+  }
+
+  private void validateQuery(String query) {
+    List<String> blocked = new ArrayList<>();
+    blocked.add("SHOW CONTINUOUS QUERIES");
+    blocked.add("SHOW DATABASES");
+    blocked.add("SHOW DIAGNOSTICS");
+    blocked.add("SHOW FIELD");
+    blocked.add("SHOW TAG");
+    blocked.add("SHOW USERS");
+    blocked.add("SHOW GRANTS");
+    blocked.add("SHOW MEASUREMENT");
+    blocked.add("SHOW QUERIES");
+    blocked.add("SHOW RETENTION POLICIES");
+    blocked.add("SHOW SERIES");
+    blocked.add("SHOW SHARD");
+    blocked.add("SHOW STATS");
+    blocked.add("SHOW SUBSCRIPTIONS");
+    blocked.add(";"); // prevents having multiple queries that could lead to potential sql injection
+    blocked.add("--"); // prevents potential sql injection
+
+    blocked.stream()
+        .forEach(
+            (stmt) -> {
+              if (query.contains(stmt)) {
+                throw new IllegalArgumentException("Query type not allowed.");
+              }
+            });
   }
 }
diff --git a/...uxdb/src/test/java/com/netflix/kayenta/influxdb/config/InfluxDbResponseConverterTest.java b/...uxdb/src/test/java/com/netflix/kayenta/influxdb/config/InfluxDbResponseConverterTest.java
@@ -25,7 +25,6 @@
 import java.io.ByteArrayOutputStream;
 import java.util.ArrayList;
 import java.util.List;
-import org.junit.Before;
 import org.junit.Test;
 import retrofit.converter.ConversionException;
 import retrofit.mime.TypedByteArray;
@@ -35,12 +34,14 @@
 public class InfluxDbResponseConverterTest {
 
   private static final String MIME_TYPE = "application/json; charset=UTF-8";
-  private final String JSON =
+  private final String EXAMPLE_ALL_INTEGERS =
       "{\"results\":[{\"statement_id\":0,\"series\":[{\"name\":\"temperature\",\"columns\":[\"time\",\"external\",\"internal\"],\"values\":[[\"2018-05-27T04:50:44.105612486Z\",25,37],[\"2018-05-27T04:51:44.105612486Z\",25,37],[\"2018-05-27T04:52:06.585796188Z\",26,38]]}]}]}";
-  private List<InfluxDbResult> results = new ArrayList<>();
 
-  @Before
-  public void setup() {
+  private final String EXAMPLE_WITH_FLOATS =
+      "{\"results\":[{\"statement_id\":0,\"series\":[{\"name\":\"temperature\",\"columns\":[\"time\",\"external\",\"internal\"],\"values\":[[\"2018-05-27T04:50:44.105612486Z\",25.1,37.1],[\"2018-05-27T04:51:44.105612486Z\",25,37.2],[\"2018-05-27T04:52:06.585796188Z\",26.5,38]]}]}]}";
+
+  public List<InfluxDbResult> setupAllIntegers() {
+    List<InfluxDbResult> results = new ArrayList<>();
     List<Double> externalDataValues = new ArrayList<>();
     externalDataValues.add(25d);
     externalDataValues.add(25d);
@@ -56,19 +57,51 @@ public void setup() {
     InfluxDbResult internalTempResult =
         new InfluxDbResult("internal", 1527396644105L, 60000L, null, internalDataValues);
     results.add(internalTempResult);
+    return results;
+  }
+
+  public List<InfluxDbResult> setupWithFloats() {
+    List<InfluxDbResult> results = new ArrayList<>();
+    List<Double> externalDataValues = new ArrayList<>();
+    externalDataValues.add(25.1);
+    externalDataValues.add(25d);
+    externalDataValues.add(26.5);
+    InfluxDbResult externalTempResult =
+        new InfluxDbResult("external", 1527396644105L, 60000L, null, externalDataValues);
+    results.add(externalTempResult);
+
+    List<Double> internalDataValues = new ArrayList<>();
+    internalDataValues.add(37.1);
+    internalDataValues.add(37.2);
+    internalDataValues.add(38d);
+    InfluxDbResult internalTempResult =
+        new InfluxDbResult("internal", 1527396644105L, 60000L, null, internalDataValues);
+    results.add(internalTempResult);
+    return results;
   }
 
   private final InfluxDbResponseConverter influxDbResponseConverter =
       new InfluxDbResponseConverter(new ObjectMapper());
 
   @Test
   public void serialize() throws Exception {
+    List<InfluxDbResult> results = setupAllIntegers();
     assertThat(influxDbResponseConverter.toBody(results), is(nullValue()));
   }
 
   @Test
   public void deserialize() throws Exception {
-    TypedInput input = new TypedByteArray(MIME_TYPE, JSON.getBytes());
+    List<InfluxDbResult> results = setupAllIntegers();
+    TypedInput input = new TypedByteArray(MIME_TYPE, EXAMPLE_ALL_INTEGERS.getBytes());
+    List<InfluxDbResult> result =
+        (List<InfluxDbResult>) influxDbResponseConverter.fromBody(input, List.class);
+    assertThat(result, is(results));
+  }
+
+  @Test
+  public void deserializeWithFloatValues() throws Exception {
+    List<InfluxDbResult> results = setupWithFloats();
+    TypedInput input = new TypedByteArray(MIME_TYPE, EXAMPLE_WITH_FLOATS.getBytes());
     List<InfluxDbResult> result =
         (List<InfluxDbResult>) influxDbResponseConverter.fromBody(input, List.class);
     assertThat(result, is(results));