Address PR review feedback for search expression feature

Author: jaredbrook

docs/search_expressions.md

@@ -8,14 +8,14 @@ When a CloudFormation stack replaces an ASG on deployment, the physical ASG name
 
 ## How It Works
 
-Instead of emitting a CloudWatch alarm with fixed `Dimensions`, `MetricName`, `Namespace`, and `Statistic` properties, a search expression alarm emits `MetricDataQueries` with:
+Instead of emitting a CloudWatch alarm with fixed `Dimensions`, `MetricName`, `Namespace`, and `Statistic` properties, a search expression alarm emits the CloudFormation `Metrics` property (a list of `MetricDataQuery` objects) with:
 
 1. A **SEARCH()** expression that dynamically matches metrics by partial or exact name
 2. An **aggregation function** (e.g. `MAX`, `AVG`, `SUM`) that reduces the matched metrics to a single time series for threshold evaluation
 
 ## Configuration
 
-Add `SearchExpression` and optionally `SearchAggregation` to an alarm template. When `SearchExpression` is set, the `Dimensions`, `MetricName`, `Namespace`, `Statistic`, and `Period` properties are not used since CloudWatch treats these as mutually exclusive with `MetricDataQueries`.
+Add `SearchExpression` and optionally `SearchAggregation` to an alarm template. When `SearchExpression` is set, the `Dimensions`, `MetricName`, `Namespace`, `Statistic`, and `Period` properties are not used since CloudWatch treats these as mutually exclusive with the alarm `Metrics` property.
 
 ### Properties
 

lib/cfnguardian/compile.rb

@@ -191,8 +191,17 @@ def validate_resources()
         case resource.type
         when 'Alarm'
           if resource.search_expression
-            if resource.search_expression.empty?
-              @errors << "CfnGuardian::AlarmPropertyError - alarm #{resource.name} for resource #{resource.resource_id} has an empty SearchExpression."
+            if !resource.search_expression.is_a?(String) || resource.search_expression.strip.empty?
+              @errors << "CfnGuardian::AlarmPropertyError - alarm #{resource.name} for resource #{resource.resource_id} has an invalid SearchExpression. Must be a non-empty string."
+            end
+            if resource.search_aggregation
+              valid_aggregations = %w(MAX MIN AVG SUM)
+              normalized = resource.search_aggregation.to_s.upcase
+              if valid_aggregations.include?(normalized)
+                resource.search_aggregation = normalized
+              else
+                @errors << "CfnGuardian::AlarmPropertyError - alarm #{resource.name} for resource #{resource.resource_id} has invalid SearchAggregation '#{resource.search_aggregation}'. Must be one of: #{valid_aggregations.join(', ')}."
+              end
             end
           else
             %w(metric_name namespace).each do |property|

lib/cfnguardian/resources/base.rb

@@ -126,8 +126,8 @@ def get_alarms(group,overides={})
 
         # String interpolation for search expressions
         if alarm.search_expression.is_a?(String)
-          alarm.search_expression = alarm.search_expression.gsub(/\${Resource::([A-Za-z]+)}/) do
-            resource_key = $1
+          alarm.search_expression = alarm.search_expression.gsub(/\${Resource::([A-Za-z0-9_]+)}/) do
+            resource_key = Regexp.last_match(1)
             if @resource.has_key?(resource_key)
               logger.debug "interpolating search_expression variable '#{resource_key}' with value '#{@resource[resource_key]}' for alarm #{alarm.name}"
               @resource[resource_key]

lib/cfnguardian/stacks/resources.rb

@@ -33,67 +33,45 @@ def build_template(resources)
       def add_alarm(alarm)
         actions = alarm.alarm_action.kind_of?(Array) ? alarm.alarm_action.map{|action| Ref(action)} : [Ref(alarm.alarm_action)]
         actions.concat alarm.maintenance_groups.map {|mg| Ref(mg)} if alarm.maintenance_groups.any?
+        use_search = alarm.search_expression ? true : false
 
-        if alarm.search_expression
-          add_search_expression_alarm(alarm, actions)
-        else
-          add_standard_alarm(alarm, actions)
-        end
-      end
-
-      def add_standard_alarm(alarm, actions)
         @template.declare do
           CloudWatch_Alarm("#{alarm.resource_hash}#{alarm.group}#{alarm.name.gsub(/[^0-9a-zA-Z]/i, '')}#{alarm.type}"[0..255]) do
             ActionsEnabled true
             AlarmDescription "Guardian alarm #{alarm.name} for the resource #{alarm.resource_id} in alarm group #{alarm.group}"
             AlarmName CfnGuardian::CloudWatch.get_alarm_name(alarm)
             ComparisonOperator alarm.comparison_operator
-            Dimensions alarm.dimensions.map {|k,v| {Name: k, Value: v}} unless alarm.dimensions.nil?
             EvaluationPeriods alarm.evaluation_periods
-            Statistic alarm.statistic if alarm.extended_statistic.nil?
-            Period alarm.period
             Threshold alarm.threshold
-            MetricName alarm.metric_name
-            Namespace alarm.namespace
             AlarmActions actions
             OKActions actions unless alarm.ok_action_disabled
             TreatMissingData alarm.treat_missing_data unless alarm.treat_missing_data.nil?
             DatapointsToAlarm alarm.datapoints_to_alarm unless alarm.datapoints_to_alarm.nil?
-            ExtendedStatistic alarm.extended_statistic unless alarm.extended_statistic.nil?
-            EvaluateLowSampleCountPercentile alarm.evaluate_low_sample_count_percentile unless alarm.evaluate_low_sample_count_percentile.nil?
-            Unit alarm.unit unless alarm.unit.nil?
-          end
-        end
-      end
 
-      def add_search_expression_alarm(alarm, actions)
-        search_expr = alarm.search_expression
-        aggregation = alarm.search_aggregation || 'MAX'
-
-        @template.declare do
-          CloudWatch_Alarm("#{alarm.resource_hash}#{alarm.group}#{alarm.name.gsub(/[^0-9a-zA-Z]/i, '')}#{alarm.type}"[0..255]) do
-            ActionsEnabled true
-            AlarmDescription "Guardian alarm #{alarm.name} for the resource #{alarm.resource_id} in alarm group #{alarm.group}"
-            AlarmName CfnGuardian::CloudWatch.get_alarm_name(alarm)
-            ComparisonOperator alarm.comparison_operator
-            EvaluationPeriods alarm.evaluation_periods
-            Threshold alarm.threshold
-            AlarmActions actions
-            OKActions actions unless alarm.ok_action_disabled
-            TreatMissingData alarm.treat_missing_data unless alarm.treat_missing_data.nil?
-            DatapointsToAlarm alarm.datapoints_to_alarm unless alarm.datapoints_to_alarm.nil?
-            Metrics [
-              {
-                Id: 'search_expression',
-                Expression: search_expr,
-                ReturnData: false
-              },
-              {
-                Id: 'aggregate',
-                Expression: "#{aggregation}(search_expression)",
-                ReturnData: true
-              }
-            ]
+            if use_search
+              aggregation = alarm.search_aggregation || 'MAX'
+              Metrics [
+                {
+                  Id: 'search_expression',
+                  Expression: alarm.search_expression,
+                  ReturnData: false
+                },
+                {
+                  Id: 'aggregate',
+                  Expression: "#{aggregation}(search_expression)",
+                  ReturnData: true
+                }
+              ]
+            else
+              Dimensions alarm.dimensions.map {|k,v| {Name: k, Value: v}} unless alarm.dimensions.nil?
+              Statistic alarm.statistic if alarm.extended_statistic.nil?
+              Period alarm.period
+              MetricName alarm.metric_name
+              Namespace alarm.namespace
+              ExtendedStatistic alarm.extended_statistic unless alarm.extended_statistic.nil?
+              EvaluateLowSampleCountPercentile alarm.evaluate_low_sample_count_percentile unless alarm.evaluate_low_sample_count_percentile.nil?
+              Unit alarm.unit unless alarm.unit.nil?
+            end
           end
         end
       end

spec/search_expression_spec.rb

@@ -1,6 +1,8 @@
 require 'spec_helper'
 require 'json'
 require 'yaml'
+require 'fileutils'
+require 'tmpdir'
 require 'term/ansicolor'
 require 'cfnguardian/log'
 require 'cfnguardian/models/alarm'
@@ -179,35 +181,30 @@
   end
 
   describe 'Validation' do
-    let(:config_dir) { File.join(File.dirname(__FILE__), 'fixtures') }
-
     context 'when search expression alarm has no metric_name or namespace' do
       it 'does not raise validation errors' do
-        fixture = File.join(config_dir, 'search_expression_alarms.yaml')
-        FileUtils.mkdir_p(config_dir)
-        File.write(fixture, {
-          'Resources' => {
-            'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
-          },
-          'Templates' => {
-            'AutoScalingGroup' => {
-              'CPUUtilizationHighBase' => {
-                'SearchExpression' => "SEARCH('{AWS/EC2,AutoScalingGroupName} MetricName=\"CPUUtilization\" my-app', 'Minimum', 60)",
-                'SearchAggregation' => 'MAX'
-              },
-              'StatusCheckFailed' => false
+        Dir.mktmpdir do |tmpdir|
+          fixture = File.join(tmpdir, 'search_expression_alarms.yaml')
+          File.write(fixture, {
+            'Resources' => {
+              'AutoScalingGroup' => [{ 'Id' => 'my-app-AsgGroup-abc123' }]
+            },
+            'Templates' => {
+              'AutoScalingGroup' => {
+                'CPUUtilizationHighBase' => {
+                  'SearchExpression' => "SEARCH('{AWS/EC2,AutoScalingGroupName} MetricName=\"CPUUtilization\" my-app', 'Minimum', 60)",
+                  'SearchAggregation' => 'MAX'
+                },
+                'StatusCheckFailed' => false
+              }
             }
-          }
-        }.to_yaml)
+          }.to_yaml)
 
-        begin
           compile = CfnGuardian::Compile.new(fixture, false)
           compile.get_resources
           search_alarms = compile.alarms.select { |a| a.search_expression }
           expect(search_alarms.length).to eq(1)
           expect(search_alarms.first.search_expression).to include('SEARCH')
-        ensure
-          FileUtils.rm_f(fixture)
         end
       end
     end