fix(templates): Escape custom inline templates. (#579)

fix(templates): Escape custom inline templates. fix(templates): Escape all custom templates for standalone canary analysis as well.
spinnaker · Jun 28, 2019 · 26feb32 · 26feb32
1 parent d7fc103
commit 26feb32
Show file tree

Hide file tree

Showing 10 changed files with 105 additions and 15 deletions.
diff --git a/kayenta-core/src/main/java/com/netflix/kayenta/canary/CanaryMetricConfig.java b/kayenta-core/src/main/java/com/netflix/kayenta/canary/CanaryMetricConfig.java
@@ -23,7 +23,7 @@
 import java.util.List;
 import java.util.Map;
 
-@Builder
+@Builder(toBuilder = true)
 @ToString
 @NoArgsConstructor
 @AllArgsConstructor

diff --git a/kayenta-core/src/main/java/com/netflix/kayenta/canary/CanaryMetricSetQueryConfig.java b/kayenta-core/src/main/java/com/netflix/kayenta/canary/CanaryMetricSetQueryConfig.java
@@ -45,5 +45,9 @@ default String getCustomFilterTemplate() {
     return null;
   }
 
+  default CanaryMetricSetQueryConfig cloneWithEscapedInlineTemplate() {
+    return this;
+  }
+
   @NonNull String getServiceType();
 }
diff --git a/...nta-core/src/main/java/com/netflix/kayenta/canary/providers/metrics/QueryConfigUtils.java b/...nta-core/src/main/java/com/netflix/kayenta/canary/providers/metrics/QueryConfigUtils.java
@@ -18,6 +18,7 @@
 
 import com.google.common.annotations.VisibleForTesting;
 import com.netflix.kayenta.canary.CanaryConfig;
+import com.netflix.kayenta.canary.CanaryMetricConfig;
 import com.netflix.kayenta.canary.CanaryMetricSetQueryConfig;
 import com.netflix.kayenta.canary.CanaryScope;
 import freemarker.template.Configuration;
@@ -35,7 +36,9 @@
 import java.io.IOException;
 import java.io.StringReader;
 import java.lang.reflect.InvocationTargetException;
+import java.util.Collections;
 import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.stream.Collectors;
@@ -147,14 +150,36 @@ private static void populateTemplateBindings(Object bean,
 
   @VisibleForTesting
   public static CanaryConfig escapeTemplates(CanaryConfig canaryConfig) {
+    if (canaryConfig == null) {
+      return null;
+    }
+
+    Map<String, String> escapedTemplates;
+
     if (!CollectionUtils.isEmpty(canaryConfig.getTemplates())) {
-      Map<String, String> escapedTemplates =
+      escapedTemplates =
         canaryConfig.getTemplates()
           .entrySet()
           .stream()
           .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue().replace("${", "$\\{")));
+    } else {
+      escapedTemplates = Collections.emptyMap();
+    }
+
+    List<CanaryMetricConfig> escapedMetrics = null;
+
+    if (!CollectionUtils.isEmpty(canaryConfig.getMetrics())) {
+      escapedMetrics =
+        canaryConfig.getMetrics()
+          .stream()
+          .map(m -> m.toBuilder().query(m.getQuery().cloneWithEscapedInlineTemplate()).build())
+          .collect(Collectors.toList());
+    } else {
+      escapedMetrics = Collections.emptyList();
+    }
 
-      canaryConfig = canaryConfig.toBuilder().templates(escapedTemplates).build();
+    if (escapedTemplates != null || escapedMetrics != null) {
+      canaryConfig = canaryConfig.toBuilder().templates(escapedTemplates).clearMetrics().metrics(escapedMetrics).build();
     }
 
     return canaryConfig;

diff --git a/...nta-core/src/test/groovy/com/netflix/kayenta/canary/providers/QueryConfigUtilsSpec.groovy b/...nta-core/src/test/groovy/com/netflix/kayenta/canary/providers/QueryConfigUtilsSpec.groovy
@@ -17,7 +17,10 @@
 package com.netflix.kayenta.canary.providers
 
 import com.netflix.kayenta.canary.CanaryConfig
+import com.netflix.kayenta.canary.CanaryMetricConfig
+import com.netflix.kayenta.canary.CanaryMetricSetQueryConfig
 import com.netflix.kayenta.canary.providers.metrics.QueryConfigUtils
+import org.springframework.util.StringUtils
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -53,4 +56,41 @@ class QueryConfigUtilsSpec extends Specification {
     'A test: key1=${key1} key2=$\\{key2}.'   || 'A test: key1=${key1} key2=${key2}.'
     'A test: key1=key1.'                     || 'A test: key1=key1.'
   }
+
+  @Unroll
+  void "Custom inline template #customInlineTemplate is escaped to protect it from premature expression evaluation by orca"() {
+    expect:
+    CanaryMetricConfig canaryMetricConfig =
+      CanaryMetricConfig.builder().query(new TestCanaryMetricSetQueryConfig(customInlineTemplate: customInlineTemplate)).build()
+    CanaryConfig canaryConfig = CanaryConfig.builder().metric(canaryMetricConfig).build()
+    QueryConfigUtils.escapeTemplates(canaryConfig).getMetrics()[0].getQuery().getCustomInlineTemplate() ==
+      expectedEscapedCustomInlineTemplate
+
+    where:
+    customInlineTemplate                 || expectedEscapedCustomInlineTemplate
+    'A test: key1=${key1}.'              || 'A test: key1=$\\{key1}.'
+    'A test: key1=${key1} key2=${key2}.' || 'A test: key1=$\\{key1} key2=$\\{key2}.'
+    'A test: key1=val1.'                 || 'A test: key1=val1.'
+    null                                 || null
+    ""                                   || ""
+  }
 }
+
+class TestCanaryMetricSetQueryConfig implements CanaryMetricSetQueryConfig {
+
+  String customInlineTemplate
+
+  @Override
+  CanaryMetricSetQueryConfig cloneWithEscapedInlineTemplate() {
+    if (StringUtils.isEmpty(customInlineTemplate)) {
+      return this
+    } else {
+      return new TestCanaryMetricSetQueryConfig(customInlineTemplate: customInlineTemplate.replace('${', '$\\{'))
+    }
+  }
+
+  @Override
+  String getServiceType() {
+    "test-service"
+  }
+}
diff --git a/...java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java b/...java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java
@@ -44,7 +44,7 @@ public class InfluxdbCanaryMetricSetQueryConfig implements CanaryMetricSetQueryC
 
   @Getter
   private List<String> fields;
-  
+
   @Override
   public String getServiceType() {
     return SERVICE_TYPE;

diff --git a/...va/com/netflix/kayenta/canary/providers/metrics/PrometheusCanaryMetricSetQueryConfig.java b/...va/com/netflix/kayenta/canary/providers/metrics/PrometheusCanaryMetricSetQueryConfig.java
@@ -19,11 +19,12 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.netflix.kayenta.canary.CanaryMetricSetQueryConfig;
 import lombok.*;
+import org.springframework.util.StringUtils;
 
 import javax.validation.constraints.NotNull;
 import java.util.List;
 
-@Builder
+@Builder(toBuilder = true)
 @ToString
 @NoArgsConstructor
 @AllArgsConstructor
@@ -58,6 +59,15 @@ public class PrometheusCanaryMetricSetQueryConfig implements CanaryMetricSetQuer
   @Getter
   private String customFilterTemplate;
 
+  @Override
+  public CanaryMetricSetQueryConfig cloneWithEscapedInlineTemplate() {
+    if (StringUtils.isEmpty(customInlineTemplate)) {
+      return this;
+    } else {
+      return this.toBuilder().customInlineTemplate(customInlineTemplate.replace("${", "$\\{")).build();
+    }
+  }
+
   @Override
   public String getServiceType() {
     return SERVICE_TYPE;

diff --git a/...a/com/netflix/kayenta/canary/providers/metrics/StackdriverCanaryMetricSetQueryConfig.java b/...a/com/netflix/kayenta/canary/providers/metrics/StackdriverCanaryMetricSetQueryConfig.java
@@ -19,11 +19,12 @@
 import com.fasterxml.jackson.annotation.JsonTypeName;
 import com.netflix.kayenta.canary.CanaryMetricSetQueryConfig;
 import lombok.*;
+import org.springframework.util.StringUtils;
 
 import javax.validation.constraints.NotNull;
 import java.util.List;
 
-@Builder
+@Builder(toBuilder = true)
 @ToString
 @NoArgsConstructor
 @AllArgsConstructor
@@ -62,6 +63,15 @@ public class StackdriverCanaryMetricSetQueryConfig implements CanaryMetricSetQue
   @Getter
   private String customFilterTemplate;
 
+  @Override
+  public CanaryMetricSetQueryConfig cloneWithEscapedInlineTemplate() {
+    if (StringUtils.isEmpty(customInlineTemplate)) {
+      return this;
+    } else {
+      return this.toBuilder().customInlineTemplate(customInlineTemplate.replace("${", "$\\{")).build();
+    }
+  }
+
   @Override
   public String getServiceType() {
     return SERVICE_TYPE;

diff --git a/...ava/com/netflix/kayenta/canaryanalysis/controller/StandaloneCanaryAnalysisController.java b/...ava/com/netflix/kayenta/canaryanalysis/controller/StandaloneCanaryAnalysisController.java
@@ -17,6 +17,7 @@
 package com.netflix.kayenta.canaryanalysis.controller;
 
 import com.netflix.kayenta.canary.CanaryConfig;
+import com.netflix.kayenta.canary.providers.metrics.QueryConfigUtils;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisAdhocExecutionRequest;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisConfig;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisExecutionRequest;
@@ -135,7 +136,7 @@ public CanaryAnalysisExecutionResponse initiateCanaryAnalysis(
             .metricsAccountName(resolvedMetricsAccountName)
             .storageAccountName(resolvedStorageAccountName)
             .configurationAccountName(configurationAccountName)
-            .canaryConfig(canaryConfig)
+            .canaryConfig(QueryConfigUtils.escapeTemplates(canaryConfig))
             .build());
   }
 
@@ -195,7 +196,7 @@ public CanaryAnalysisExecutionResponse initiateCanaryAnalysisExecutionWithConfig
             .executionRequest(canaryAnalysisAdhocExecutionRequest.getExecutionRequest())
             .metricsAccountName(resolvedMetricsAccountName)
             .storageAccountName(resolvedStorageAccountName)
-            .canaryConfig(canaryAnalysisAdhocExecutionRequest.getCanaryConfig())
+            .canaryConfig(QueryConfigUtils.escapeTemplates(canaryAnalysisAdhocExecutionRequest.getCanaryConfig()))
             .build());
   }
 

diff --git a/...main/java/com/netflix/kayenta/canaryanalysis/orca/stage/SetupAndExecuteCanariesStage.java b/...main/java/com/netflix/kayenta/canaryanalysis/orca/stage/SetupAndExecuteCanariesStage.java
@@ -21,13 +21,13 @@
 import com.netflix.kayenta.canary.CanaryScope;
 import com.netflix.kayenta.canary.CanaryScopePair;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisConfig;
+import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisExecutionRequest;
+import com.netflix.kayenta.canaryanalysis.domain.RunCanaryContext;
 import com.netflix.spinnaker.orca.pipeline.StageDefinitionBuilder;
 import com.netflix.spinnaker.orca.pipeline.TaskNode;
 import com.netflix.spinnaker.orca.pipeline.WaitStage;
 import com.netflix.spinnaker.orca.pipeline.graph.StageGraphBuilder;
 import com.netflix.spinnaker.orca.pipeline.model.Stage;
-import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisExecutionRequest;
-import com.netflix.kayenta.canaryanalysis.domain.RunCanaryContext;
 import lombok.Data;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;

diff --git a/...lysis/src/main/java/com/netflix/kayenta/canaryanalysis/service/CanaryAnalysisService.java b/...lysis/src/main/java/com/netflix/kayenta/canaryanalysis/service/CanaryAnalysisService.java
@@ -20,17 +20,17 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisConfig;
-import com.netflix.spinnaker.orca.ExecutionStatus;
-import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher;
-import com.netflix.spinnaker.orca.pipeline.model.Execution;
-import com.netflix.spinnaker.orca.pipeline.model.PipelineBuilder;
-import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisExecutionResponse;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisExecutionResult;
 import com.netflix.kayenta.canaryanalysis.domain.CanaryAnalysisExecutionStatusResponse;
 import com.netflix.kayenta.canaryanalysis.domain.StageMetadata;
 import com.netflix.kayenta.canaryanalysis.orca.stage.GenerateCanaryAnalysisResultStage;
 import com.netflix.kayenta.canaryanalysis.orca.stage.SetupAndExecuteCanariesStage;
+import com.netflix.spinnaker.orca.ExecutionStatus;
+import com.netflix.spinnaker.orca.pipeline.ExecutionLauncher;
+import com.netflix.spinnaker.orca.pipeline.model.Execution;
+import com.netflix.spinnaker.orca.pipeline.model.PipelineBuilder;
+import com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;