Merge pull request #76 from zalando-incubator/refactor-parsing

Prevent panic when parsing HPAs

pkg/collector/aws_collector.go

@@ -56,11 +56,11 @@ func NewAWSSQSCollector(sessions map[string]*session.Session, config *MetricConf
 		return nil, fmt.Errorf("selector for queue is not specified")
 	}
 
-	name, ok := config.Metric.Selector.MatchLabels[sqsQueueNameLabelKey]
+	name, ok := config.Config[sqsQueueNameLabelKey]
 	if !ok {
 		return nil, fmt.Errorf("sqs queue name not specified on metric")
 	}
-	region, ok := config.Metric.Selector.MatchLabels[sqsQueueRegionLabelKey]
+	region, ok := config.Config[sqsQueueRegionLabelKey]
 	if !ok {
 		return nil, fmt.Errorf("sqs queue region is not specified on metric")
 	}

pkg/collector/collector.go

@@ -208,7 +208,9 @@ func ParseHPAMetrics(hpa *autoscalingv2.HorizontalPodAutoscaler) ([]*MetricConfi
 			Config:          map[string]string{},
 		}
 
-		if metric.Type == autoscalingv2.ExternalMetricSourceType {
+		if metric.Type == autoscalingv2.ExternalMetricSourceType &&
+			metric.External.Metric.Selector != nil &&
+			metric.External.Metric.Selector.MatchLabels != nil {
 			config.Config = metric.External.Metric.Selector.MatchLabels
 		}
 
@@ -217,7 +219,11 @@ func ParseHPAMetrics(hpa *autoscalingv2.HorizontalPodAutoscaler) ([]*MetricConfi
 			config.CollectorName = annotationConfigs.CollectorName
 			config.Interval = annotationConfigs.Interval
 			config.PerReplica = annotationConfigs.PerReplica
-			config.Config = annotationConfigs.Configs
+			// configs specified in annotations takes precedence
+			// over labels
+			for k, v := range annotationConfigs.Configs {
+				config.Config[k] = v
+			}
 		}
 		metricConfigs = append(metricConfigs, config)
 	}

pkg/collector/prometheus_collector.go

@@ -91,7 +91,11 @@ func NewPrometheusCollector(client kubernetes.Interface, promAPI promv1.API, hpa
 			return nil, fmt.Errorf("no prometheus query defined")
 		}
 	case autoscalingv2.ExternalMetricSourceType:
-		queryName, ok := config.Metric.Selector.MatchLabels[prometheusQueryNameLabelKey]
+		if config.Metric.Selector == nil {
+			return nil, fmt.Errorf("selector for prometheus query is not specified")
+		}
+
+		queryName, ok := config.Config[prometheusQueryNameLabelKey]
 		if !ok {
 			return nil, fmt.Errorf("query name not specified on metric")
 		}

pkg/collector/zmon_collector.go

@@ -44,11 +44,7 @@ func NewZMONCollectorPlugin(zmon zmon.ZMON) (*ZMONCollectorPlugin, error) {
 func (c *ZMONCollectorPlugin) NewCollector(hpa *autoscalingv2.HorizontalPodAutoscaler, config *MetricConfig, interval time.Duration) (Collector, error) {
 	switch config.Metric.Name {
 	case ZMONCheckMetric:
-		annotations := map[string]string{}
-		if hpa != nil {
-			annotations = hpa.Annotations
-		}
-		return NewZMONCollector(c.zmon, config, annotations, interval)
+		return NewZMONCollector(c.zmon, config, interval)
 	}
 
 	return nil, fmt.Errorf("metric '%s' not supported", config.Metric.Name)
@@ -68,7 +64,11 @@ type ZMONCollector struct {
 }
 
 // NewZMONCollector initializes a new ZMONCollector.
-func NewZMONCollector(zmon zmon.ZMON, config *MetricConfig, annotations map[string]string, interval time.Duration) (*ZMONCollector, error) {
+func NewZMONCollector(zmon zmon.ZMON, config *MetricConfig, interval time.Duration) (*ZMONCollector, error) {
+	if config.Metric.Selector == nil {
+		return nil, fmt.Errorf("selector for zmon-check is not specified")
+	}
+
 	checkIDStr, ok := config.Config[zmonCheckIDLabelKey]
 	if !ok {
 		return nil, fmt.Errorf("ZMON check ID not specified on metric")
@@ -86,11 +86,6 @@ func NewZMONCollector(zmon zmon.ZMON, config *MetricConfig, annotations map[stri
 		key = k
 	}
 
-	// annotations takes precedence over label
-	if k, ok := annotations[zmonKeyAnnotationKey]; ok {
-		key = k
-	}
-
 	duration := defaultQueryDuration
 
 	// parse optional duration value
@@ -110,16 +105,6 @@ func NewZMONCollector(zmon zmon.ZMON, config *MetricConfig, annotations map[stri
 		}
 	}
 
-	// parse tags from annotations
-	// tags defined in annotations takes precedence over tags defined in
-	// the labels.
-	for k, v := range annotations {
-		if strings.HasPrefix(k, zmonTagPrefixAnnotationKey) {
-			key := strings.TrimPrefix(k, zmonTagPrefixAnnotationKey)
-			tags[key] = v
-		}
-	}
-
 	// default aggregator is last
 	aggregators := []string{"last"}
 	if k, ok := config.Config[zmonAggregatorsLabelKey]; ok {

pkg/collector/zmon_collector_test.go

@@ -50,20 +50,6 @@ func TestZMONCollectorNewCollector(t *testing.T) {
 	require.Equal(t, []string{"max"}, zmonCollector.aggregators)
 	require.Equal(t, map[string]string{"alias": "cluster_alias"}, zmonCollector.tags)
 
-	// check that annotations overwrites labels
-	hpa.ObjectMeta = metav1.ObjectMeta{
-		Annotations: map[string]string{
-			zmonKeyAnnotationKey:                 "annotation_key",
-			zmonTagPrefixAnnotationKey + "alias": "cluster_alias_annotation",
-		},
-	}
-	collector, err = collectPlugin.NewCollector(hpa, config, 1*time.Second)
-	require.NoError(t, err)
-	require.NotNil(t, collector)
-	zmonCollector = collector.(*ZMONCollector)
-	require.Equal(t, "annotation_key", zmonCollector.key)
-	require.Equal(t, map[string]string{"alias": "cluster_alias_annotation"}, zmonCollector.tags)
-
 	// should fail if the metric name isn't ZMON
 	config.Metric = newMetricIdentifier("non-zmon-check")
 	_, err = collectPlugin.NewCollector(nil, config, 1*time.Second)
@@ -131,7 +117,7 @@ func TestZMONCollectorGetMetrics(tt *testing.T) {
 				dataPoints: ti.dataPoints,
 			}
 
-			zmonCollector, err := NewZMONCollector(z, config, nil, 1*time.Second)
+			zmonCollector, err := NewZMONCollector(z, config, 1*time.Second)
 			require.NoError(t, err)
 
 			metrics, _ := zmonCollector.GetMetrics()