Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix typo, improve destination validations #832

Merged
merged 4 commits into from
Oct 31, 2024
Merged

Fix typo, improve destination validations #832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efe3cd6
Fix typo, improve destination validations
petewall Oct 31, 2024
b29759d
Improve linting and testing
petewall Oct 31, 2024
4f2a209
Also prevent multiple platform tests from running at the same time.
petewall Oct 31, 2024
1adf6dd
Need to set something that evaluates to something, even if the matrix…
petewall Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/platform-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 19 additions & 10 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 \
Expand All @@ -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 #
####################################################################
Expand All @@ -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
3 changes: 2 additions & 1 deletion charts/k8s-monitoring/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)); \
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions charts/k8s-monitoring/docs/examples/remote-config/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
## Values

```yaml
---
cluster:
name: remote-config-example-cluster

Expand Down
1 change: 1 addition & 0 deletions charts/k8s-monitoring/docs/examples/remote-config/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
---
cluster:
name: remote-config-example-cluster

Expand Down
8 changes: 8 additions & 0 deletions charts/k8s-monitoring/schema-mods/definitions/invalid-destination.schema.json
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows for invalid types to pass schema validation... only to be caught by the template validation. It greatly improves the quality of the error message if you're missing a type or use an invalid type.

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "object",
"properties": {
"type": {
"type": "string"
}
}
}
1 change: 1 addition & 0 deletions charts/k8s-monitoring/schema-mods/destination-list.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"type": "array",
"items": {
"anyOf": [
{ "$ref": "#/definitions/invalid-destination"},
{ "$ref": "#/definitions/loki-destination"},
{ "$ref": "#/definitions/otlp-destination"},
{ "$ref": "#/definitions/prometheus-destination"},
Expand Down
17 changes: 15 additions & 2 deletions charts/k8s-monitoring/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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" -}}
Expand Down Expand Up @@ -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 := . }}
Expand Down
2 changes: 1 addition & 1 deletion charts/k8s-monitoring/templates/collectors/_collector_validations.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" }}
Expand Down
9 changes: 6 additions & 3 deletions charts/k8s-monitoring/templates/destinations/_destination_validations.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}

4 changes: 2 additions & 2 deletions charts/k8s-monitoring/templates/validations.yaml
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing the destination validation before feature validation to catch these issues earlier.
For example, if your values file had this:

destinations:
  - name: prom
    type: prom   # invalid type
clusterMetrics:
  enabled: true

The error message will say that Cluster Metrics requires a metrics destination, not that "prom" is not a valid destination type.

Original file line number Diff line number Diff line change
@@ -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) }}
Expand Down
4 changes: 2 additions & 2 deletions charts/k8s-monitoring/tests/cluster_events_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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:
Expand Down
77 changes: 77 additions & 0 deletions charts/k8s-monitoring/tests/destination_validations_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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: {}
5 changes: 3 additions & 2 deletions charts/k8s-monitoring/tests/validations_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: |-
Expand All @@ -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
Expand Down
11 changes: 11 additions & 0 deletions charts/k8s-monitoring/values.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1013,6 +1013,14 @@
}
},
"definitions": {
"invalid-destination": {
"type": "object",
"properties": {
"type": {
"type": "string"
}
}
},
"loki-destination": {
"type": "object",
"properties": {
Expand Down Expand Up @@ -1669,6 +1677,9 @@
"type": "array",
"items": {
"anyOf": [
{
"$ref": "#/definitions/invalid-destination"
},
{
"$ref": "#/definitions/loki-destination"
},
Expand Down
2 changes: 1 addition & 1 deletion scripts/lint-alex.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down