From dd8a91d0cf2bd10f2d58564f6e6730030e26f814 Mon Sep 17 00:00:00 2001 From: spinnakerbot Date: Fri, 28 Jun 2019 11:55:49 -0400 Subject: [PATCH] fix(templates): Escape custom inline templates. (#579) (#580) fix(templates): Escape custom inline templates. fix(templates): Escape all custom templates for standalone canary analysis as well. --- .../kayenta/canary/CanaryMetricConfig.java | 2 +- .../canary/CanaryMetricSetQueryConfig.java | 4 ++ .../providers/metrics/QueryConfigUtils.java | 29 +++++++++++++- .../providers/QueryConfigUtilsSpec.groovy | 40 +++++++++++++++++++ .../InfluxdbCanaryMetricSetQueryConfig.java | 2 +- .../PrometheusCanaryMetricSetQueryConfig.java | 12 +++++- ...StackdriverCanaryMetricSetQueryConfig.java | 12 +++++- .../StandaloneCanaryAnalysisController.java | 5 ++- .../stage/SetupAndExecuteCanariesStage.java | 4 +- .../service/CanaryAnalysisService.java | 10 ++--- 10 files changed, 105 insertions(+), 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 index fc84ae420..c159bec19 100644 --- 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 index 3f1b076ad..ac4f41cc5 100644 --- 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/kayenta-core/src/main/java/com/netflix/kayenta/canary/providers/metrics/QueryConfigUtils.java b/kayenta-core/src/main/java/com/netflix/kayenta/canary/providers/metrics/QueryConfigUtils.java index d45160d58..865aeb242 100644 --- a/kayenta-core/src/main/java/com/netflix/kayenta/canary/providers/metrics/QueryConfigUtils.java +++ b/kayenta-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 escapedTemplates; + if (!CollectionUtils.isEmpty(canaryConfig.getTemplates())) { - Map escapedTemplates = + escapedTemplates = canaryConfig.getTemplates() .entrySet() .stream() .collect(Collectors.toMap(e -> e.getKey(), e -> e.getValue().replace("${", "$\\{"))); + } else { + escapedTemplates = Collections.emptyMap(); + } + + List 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/kayenta-core/src/test/groovy/com/netflix/kayenta/canary/providers/QueryConfigUtilsSpec.groovy b/kayenta-core/src/test/groovy/com/netflix/kayenta/canary/providers/QueryConfigUtilsSpec.groovy index 2f9826de7..1cd87fd68 100644 --- a/kayenta-core/src/test/groovy/com/netflix/kayenta/canary/providers/QueryConfigUtilsSpec.groovy +++ b/kayenta-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" + } +} \ No newline at end of file diff --git a/kayenta-influxdb/src/main/java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java b/kayenta-influxdb/src/main/java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java index f41f4600f..8daa26576 100644 --- a/kayenta-influxdb/src/main/java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java +++ b/kayenta-influxdb/src/main/java/com/netflix/kayenta/canary/providers/metrics/InfluxdbCanaryMetricSetQueryConfig.java @@ -44,7 +44,7 @@ public class InfluxdbCanaryMetricSetQueryConfig implements CanaryMetricSetQueryC @Getter private List fields; - + @Override public String getServiceType() { return SERVICE_TYPE; diff --git a/kayenta-prometheus/src/main/java/com/netflix/kayenta/canary/providers/metrics/PrometheusCanaryMetricSetQueryConfig.java b/kayenta-prometheus/src/main/java/com/netflix/kayenta/canary/providers/metrics/PrometheusCanaryMetricSetQueryConfig.java index 8067ae8e2..cd4770add 100644 --- a/kayenta-prometheus/src/main/java/com/netflix/kayenta/canary/providers/metrics/PrometheusCanaryMetricSetQueryConfig.java +++ b/kayenta-prometheus/src/main/java/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/kayenta-stackdriver/src/main/java/com/netflix/kayenta/canary/providers/metrics/StackdriverCanaryMetricSetQueryConfig.java b/kayenta-stackdriver/src/main/java/com/netflix/kayenta/canary/providers/metrics/StackdriverCanaryMetricSetQueryConfig.java index 31344b16b..177bc9633 100644 --- a/kayenta-stackdriver/src/main/java/com/netflix/kayenta/canary/providers/metrics/StackdriverCanaryMetricSetQueryConfig.java +++ b/kayenta-stackdriver/src/main/java/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/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/controller/StandaloneCanaryAnalysisController.java b/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/controller/StandaloneCanaryAnalysisController.java index 5a35e8f96..ace4f7ab5 100644 --- a/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/controller/StandaloneCanaryAnalysisController.java +++ b/kayenta-standalone-canary-analysis/src/main/java/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/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/orca/stage/SetupAndExecuteCanariesStage.java b/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/orca/stage/SetupAndExecuteCanariesStage.java index d58920206..302e5138b 100644 --- a/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/orca/stage/SetupAndExecuteCanariesStage.java +++ b/kayenta-standalone-canary-analysis/src/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/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/service/CanaryAnalysisService.java b/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/service/CanaryAnalysisService.java index b2b162876..aea2cb8c6 100644 --- a/kayenta-standalone-canary-analysis/src/main/java/com/netflix/kayenta/canaryanalysis/service/CanaryAnalysisService.java +++ b/kayenta-standalone-canary-analysis/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;