Fix typo, improve destination validations (#832)

* Fix typo, improve destination validations Signed-off-by: Pete Wall <pete.wall@grafana.com> * Improve linting and testing Signed-off-by: Pete Wall <pete.wall@grafana.com> * Also prevent multiple platform tests from running at the same time. Signed-off-by: Pete Wall <pete.wall@grafana.com> * Need to set something that evaluates to something, even if the matrix is empty Signed-off-by: Pete Wall <pete.wall@grafana.com> --------- Signed-off-by: Pete Wall <pete.wall@grafana.com>
grafana · Oct 31, 2024 · eafe32e · eafe32e
1 parent a63dbb3
commit eafe32e
Show file tree

Hide file tree

Showing 14 changed files with 151 additions and 24 deletions.
diff --git a/.github/workflows/platform-test.yml b/.github/workflows/platform-test.yml
@@ -46,6 +46,9 @@ jobs:
       matrix:
         test: ${{ fromJson(needs.list-tests.outputs.tests) }}
       fail-fast: false
+    concurrency:
+      group: ${{ matrix.test || 'no-platform-test' }}
+      cancel-in-progress: false
     steps:
       - name: Checkout code
         uses: actions/checkout@v4

diff --git a/Makefile b/Makefile
@@ -23,7 +23,7 @@ build:
 	make -C charts/k8s-monitoring-v1 build
 
 .PHONY: test
-test: build
+test: build lint
 	helm repo update
 	set -e && \
 	for chart in $(FEATURE_CHARTS); do \
@@ -49,6 +49,15 @@ endif
 install:
 	yarn install
 
+node_modules/.bin/alex: package.json yarn.lock
+	yarn install
+
+node_modules/.bin/markdownlint-cli2: package.json yarn.lock
+	yarn install
+
+node_modules/.bin/textlint: package.json yarn.lock
+	yarn install
+
 ####################################################################
 #                           Linting                                #
 ####################################################################
@@ -57,28 +66,28 @@ lint: lint-sh lint-md lint-txt lint-yml lint-alex lint-misspell lint-actionlint
 
 # Shell Linting for checking shell scripts
 lint-sh lint-shell:
-	@./scripts/lint-shell.sh || true
+	@./scripts/lint-shell.sh
 
 # Markdown Linting for checking markdown files
-lint-md lint-markdown:
-	@./scripts/lint-markdown.sh || true
+lint-md lint-markdown: node_modules/.bin/markdownlint-cli2
+	@./scripts/lint-markdown.sh
 
 # Text Linting for checking text files
-lint-txt lint-text:
-	@./scripts/lint-text.sh || true
+lint-txt lint-text: node_modules/.bin/textlint
+	@./scripts/lint-text.sh
 
 # Yaml Linting for checking yaml files
 lint-yml lint-yaml:
-	@./scripts/lint-yaml.sh || true
+	@./scripts/lint-yaml.sh
 
 # Alex Linting for checking insensitive language
-lint-alex:
+lint-alex: node_modules/.bin/alex
 	@./scripts/lint-alex.sh || true
 
 # Misspell Linting for checking common spelling mistakes
 lint-misspell:
-	@./scripts/lint-misspell.sh || true
+	@./scripts/lint-misspell.sh
 
 # Actionlint Linting for checking GitHub Actions
 lint-al lint-actionlint:
-	@./scripts/lint-actionlint.sh || true
+	@./scripts/lint-actionlint.sh
diff --git a/charts/k8s-monitoring/Makefile b/charts/k8s-monitoring/Makefile
@@ -28,6 +28,7 @@ schema-mods/destination-list.json: $(DESTINATION_VALUES_FILES)
 	@echo '      "type": "array",' >> $@
 	@echo '      "items": {' >> $@
 	@echo '        "anyOf": [' >> $@
+	@echo '          { "$$ref": "#/definitions/invalid-destination"},' >> $@
 	@count=0; \
 	for file in $(DESTINATION_VALUES_FILES); do \
 		count=$$((count + 1)); \
@@ -148,7 +149,7 @@ lint-helm: build
 	helm lint .
 	ct lint --config .ct.yaml --lint-conf ../../.configs/lintconf.yaml --check-version-increment=false --charts .
 
-unittest: build
+unittest: values.schema.json templates/destinations/_destination_types.tpl
 ifdef HAS_HELM_UNITTEST
 	helm unittest .
 else

diff --git a/charts/k8s-monitoring/schema-mods/definitions/invalid-destination.schema.json b/charts/k8s-monitoring/schema-mods/definitions/invalid-destination.schema.json
@@ -0,0 +1,8 @@
+{
+    "type": "object",
+    "properties": {
+        "type": {
+          "type": "string"
+        }
+    }
+}
diff --git a/charts/k8s-monitoring/schema-mods/destination-list.json b/charts/k8s-monitoring/schema-mods/destination-list.json
@@ -4,6 +4,7 @@
       "type": "array",
       "items": {
         "anyOf": [
+          { "$ref": "#/definitions/invalid-destination"},
           { "$ref": "#/definitions/loki-destination"},
           { "$ref": "#/definitions/otlp-destination"},
           { "$ref": "#/definitions/prometheus-destination"},

diff --git a/charts/k8s-monitoring/templates/_helpers.tpl b/charts/k8s-monitoring/templates/_helpers.tpl
@@ -1,9 +1,9 @@
 {{ define "helper.k8s_name" }}
-{{- . | lower -}}
+{{- . | lower | replace " " "-" -}}
 {{ end }}
 
 {{ define "helper.alloy_name" }}
-{{- . | lower | replace "-" "_" -}}
+{{- . | lower | replace " " "-" | replace "-" "_" -}}
 {{ end }}
 
 {{- define "helper.fullname" -}}
@@ -32,6 +32,19 @@
 {{- end }}
 {{- end }}
 
+{{- define "english_list_or" }}
+{{- if eq (len .) 0 }}
+{{- else if eq (len .) 1 }}
+{{- index . 0 }}
+{{- else if eq (len .) 2 }}
+{{- index . 0 }} and {{ index . 1 }}
+{{- else }}
+{{- $last := index . (sub (len .) 1) }}
+{{- $rest := slice . 0 (sub (len .) 1) }}
+{{- join ", " $rest }}, or {{ $last }}
+{{- end }}
+{{- end }}
+
 {{- define "label_list" }}
 {{- $labels := list }}
 {{- range $key, $value := . }}

diff --git a/charts/k8s-monitoring/templates/collectors/_collector_validations.tpl b/charts/k8s-monitoring/templates/collectors/_collector_validations.tpl
@@ -62,7 +62,7 @@
 {{- if (index .Values .collectorName).enabled }}
   {{- if (index .Values .collectorName).liveDebugging.enabled }}
     {{- if not (eq (index .Values .collectorName).alloy.stabilityLevel "experimental") }}
-      {{- $msg := list "" "The live debugging feature requires Alloy to use the \"experiemenal\" stability level. Please set:" }}
+      {{- $msg := list "" "The live debugging feature requires Alloy to use the \"experimental\" stability level. Please set:" }}
       {{- $msg = append $msg (printf "%s:" .collectorName ) }}
       {{- $msg = append $msg "  alloy:" }}
       {{- $msg = append $msg "    stabilityLevel: experimental" }}

diff --git a/charts/k8s-monitoring/templates/destinations/_destination_validations.tpl b/charts/k8s-monitoring/templates/destinations/_destination_validations.tpl
@@ -6,14 +6,17 @@
       {{ fail (printf "\nDestination #%d does not have a name.\nPlease set:\ndestinations:\n  - name: my-destination-name" $i) }}
     {{- end }}
 
+    {{- if (regexFind "[^-_ a-zA-Z0-9]" $destination.name) }}
+      {{ fail (printf "\nDestination #%d (%s) has invalid characters in its name.\nPlease only use alphanumeric, underscores, dashes, or spaces" $i $destination.name) }}
+    {{- end }}
+
     {{- $types := (include "destinations.types" . ) | fromYamlArray }}
     {{- if not $destination.type }}
-      {{ fail (printf "\nDestination \"%s\" does not have a type.\nPlease set:\ndestinations:\n  - name: %s\n    type: %s" $destination.name $destination.name ($types | join "|")) }}
+      {{ fail (printf "\nDestination #%d (%s) does not have a type.\nPlease set:\ndestinations:\n  - name: %s\n    type: %s" $i $destination.name $destination.name (include "english_list_or" $types)) }}
     {{- end }}
 
     {{- if not (has $destination.type $types) }}
-      {{ fail (printf "\nDestination \"%s\" is using an unknown type (%s).\nPlease set:\ndestinations:\n  - name: %s\n    type: \"[%s]\"" $destination.name $destination.type $destination.name ($types | join "|")) }}
+      {{ fail (printf "\nDestination #%d (%s) is using an unknown type (%s).\nPlease set:\ndestinations:\n  - name: %s\n    type: \"[%s]\"" $i $destination.name $destination.type $destination.name (include "english_list_or" $types)) }}
     {{- end }}
   {{- end }}
 {{- end }}
-
diff --git a/charts/k8s-monitoring/templates/validations.yaml b/charts/k8s-monitoring/templates/validations.yaml
@@ -1,14 +1,14 @@
 {{- include "validations.cluster_name" . }}
 {{- include "validations.platform" . }}
 
+{{- include "destinations.validate" . -}}
+
 {{- /* Feature Validations*/}}
 {{- include "validations.features_enabled" . }}
 {{- range $feature := ((include "features.list" .) | fromYamlArray) }}
   {{- include (printf "features.%s.validate" $feature) (dict "Values" $.Values) }}
 {{- end }}
 
-{{- include "destinations.validate" . -}}
-
 {{- include "collectors.validate.featuresEnabled" . }}
 {{- range $collectorName := ((include "collectors.list.enabled" .) | fromYamlArray) }}
   {{- include "collectors.validate.liveDebugging" (dict "collectorName" $collectorName "Values" $.Values) }}

diff --git a/charts/k8s-monitoring/tests/cluster_events_test.yaml b/charts/k8s-monitoring/tests/cluster_events_test.yaml
@@ -13,7 +13,7 @@ tests:
     asserts:
       - failedTemplate:
           errorMessage: |-
-            execution error at (k8s-monitoring/templates/validations.yaml:7:6): 
+            execution error at (k8s-monitoring/templates/validations.yaml:9:6): 
             No destinations found that can accept logs from Kubernetes Cluster events
             Please add a destination with logs support.
             See https://github.com/grafana/k8s-monitoring-helm/blob/main/charts/k8s-monitoring/docs/destinations/README.md for more details.
@@ -31,7 +31,7 @@ tests:
     asserts:
       - failedTemplate:
           errorMessage: |-
-            execution error at (k8s-monitoring/templates/validations.yaml:7:6): 
+            execution error at (k8s-monitoring/templates/validations.yaml:9:6): 
             The Kubernetes Cluster events feature requires the use of the alloy-singleton collector.
             
             Please enable it by setting:

diff --git a/charts/k8s-monitoring/tests/destination_validations_test.yaml b/charts/k8s-monitoring/tests/destination_validations_test.yaml
@@ -0,0 +1,77 @@
+# yamllint disable rule:document-start rule:line-length rule:trailing-spaces
+suite: Test destination validations
+templates:
+  - validations.yaml
+tests:
+  - it: asks you to set the name of a destination
+    set:
+      cluster: {name: test-cluster}
+      clusterMetrics: {enabled: true}
+      destinations:
+        - type: prometheus
+    asserts:
+      - failedTemplate:
+          errorMessage: |-
+            execution error at (k8s-monitoring/templates/validations.yaml:4:4): 
+            Destination #0 does not have a name.
+            Please set:
+            destinations:
+              - name: my-destination-name
+
+  - it: asks you to set a valid name for the destination
+    set:
+      cluster: {name: test-cluster}
+      clusterMetrics: {enabled: true}
+      destinations:
+        - name: prøm3thüs!
+          type: prometheus
+    asserts:
+      - failedTemplate:
+          errorMessage: |-
+            execution error at (k8s-monitoring/templates/validations.yaml:4:4): 
+            Destination #0 (prøm3thüs!) has invalid characters in its name.
+            Please only use alphanumeric, underscores, dashes, or spaces
+
+  - it: requires a destination type
+    set:
+      cluster: {name: test-cluster}
+      clusterMetrics: {enabled: true}
+      destinations:
+        - name: a destination with no type
+    asserts:
+      - failedTemplate:
+          errorMessage: |-
+            execution error at (k8s-monitoring/templates/validations.yaml:4:4): 
+            Destination #0 (a destination with no type) does not have a type.
+            Please set:
+            destinations:
+              - name: a destination with no type
+                type: loki, otlp, prometheus, or pyroscope
+
+  - it: validates the destination type
+    set:
+      cluster: {name: test-cluster}
+      clusterMetrics: {enabled: true}
+      destinations:
+        - name: a destination with an invalid type
+          type: invalidType
+    asserts:
+      - failedTemplate:
+          errorMessage: |-
+            execution error at (k8s-monitoring/templates/validations.yaml:4:4): 
+            Destination #0 (a destination with an invalid type) is using an unknown type (invalidType).
+            Please set:
+            destinations:
+              - name: a destination with an invalid type
+                type: "[loki, otlp, prometheus, or pyroscope]"
+
+  - it: allows destination names with alphanumeric, underscores, dashes, and spaces
+    set:
+      cluster: {name: test-cluster}
+      clusterMetrics: {enabled: true}
+      alloy-metrics: {enabled: true}
+      destinations:
+        - name: This is my _Prometheus_ destination-1
+          type: prometheus
+    asserts:
+      - notFailedTemplate: {}
diff --git a/charts/k8s-monitoring/tests/validations_test.yaml b/charts/k8s-monitoring/tests/validations_test.yaml
@@ -3,7 +3,7 @@ suite: Test validations
 templates:
   - validations.yaml
 tests:
-  - it: asks you to set the cluster
+  - it: asks you to set the cluster name
     asserts:
       - failedTemplate:
           errorMessage: |-
@@ -12,14 +12,15 @@ tests:
             Please set:
             cluster:
               name: my-cluster-name
+
   - it: asks you to set a feature
     set:
       cluster:
         name: test-cluster
     asserts:
       - failedTemplate:
           errorMessage: |-
-            execution error at (k8s-monitoring/templates/validations.yaml:5:4): 
+            execution error at (k8s-monitoring/templates/validations.yaml:7:4): 
             No features are enabled. Please choose a feature to start monitoring. For example:
             clusterMetrics:
               enabled: true

diff --git a/charts/k8s-monitoring/values.schema.json b/charts/k8s-monitoring/values.schema.json
@@ -1013,6 +1013,14 @@
         }
     },
     "definitions": {
+        "invalid-destination": {
+            "type": "object",
+            "properties": {
+                "type": {
+                    "type": "string"
+                }
+            }
+        },
         "loki-destination": {
             "type": "object",
             "properties": {
@@ -1669,6 +1677,9 @@
             "type": "array",
             "items": {
                 "anyOf": [
+                    {
+                        "$ref": "#/definitions/invalid-destination"
+                    },
                     {
                         "$ref": "#/definitions/loki-destination"
                     },

diff --git a/scripts/lint-alex.sh b/scripts/lint-alex.sh
@@ -9,7 +9,7 @@ heading "Kubernetes Monitoring Helm" "Performing Text Linting using alex"
 dir=$(pwd || true)
 
 # check to see if remark is installed
-if [[ ! -f "${dir}"/node_modules/.bin/textlint ]]; then
+if [[ ! -f "${dir}"/node_modules/.bin/alex ]]; then
   emergency "alex node module is not installed, please run: make install";
 fi