vmalert: remove notifier null check (#813)

* vmalert: remove notifier null check, since `-notifier.url` is optional and is needed only if there are alerting rules. * fix vmauth EmbeddedIngress sanity check * update doc * fix ut
VictoriaMetrics · Nov 28, 2023 · 7965dba · 7965dba
1 parent f628bee
commit 7965dba
Show file tree

Hide file tree

Showing 14 changed files with 147 additions and 66 deletions.
diff --git a/Makefile b/Makefile
@@ -198,7 +198,7 @@ fix_crd_nulls_yaml:
 
 
 doc: install-develop-tools
-	cat hack/doc_header.md > doc_api.md
+	cat hack/doc_header.md > docs/api.md
 	doc-print --paths=\
 	$(APIS_BASE_PATH)/vmalertmanager_types.go,\
 	$(APIS_BASE_PATH)/vmalertmanagerconfig_types.go,\
@@ -216,7 +216,7 @@ doc: install-develop-tools
 	$(APIS_BASE_PATH)/vmstaticscrape_types.go,\
 	$(APIS_BASE_PATH)/vmprobe_types.go \
 	--owner VictoriaMetrics \
-	>> doc_api.md
+	>> docs/api.md
 
 operator-conf: install-develop-tools
 	cat hack/doc_vars_header.md > vars.md

diff --git a/api/v1beta1/vmalert_types.go b/api/v1beta1/vmalert_types.go
@@ -166,13 +166,13 @@ type VMAlertSpec struct {
 	// +optional
 	Port string `json:"port,omitempty"`
 
-	// Notifier prometheus alertmanager endpoint spec. Required at least one of  notifier or notifiers. e.g. http://127.0.0.1:9093
+	// Notifier prometheus alertmanager endpoint spec. Required at least one of notifier or notifiers when there are alerting rules. e.g. http://127.0.0.1:9093
 	// If specified both notifier and notifiers, notifier will be added as last element to notifiers.
 	// only one of notifier options could be chosen: notifierConfigRef or notifiers +  notifier
 	// +optional
 	Notifier *VMAlertNotifierSpec `json:"notifier,omitempty"`
 
-	// Notifiers prometheus alertmanager endpoints. Required at least one of  notifier or notifiers. e.g. http://127.0.0.1:9093
+	// Notifiers prometheus alertmanager endpoints. Required at least one of notifier or notifiers when there are alerting rules. e.g. http://127.0.0.1:9093
 	// If specified both notifier and notifiers, notifier will be added as last element to notifiers.
 	// only one of notifier options could be chosen: notifierConfigRef or notifiers +  notifier
 	// +optional

diff --git a/api/v1beta1/vmalert_webhook.go b/api/v1beta1/vmalert_webhook.go
@@ -27,10 +27,6 @@ func (r *VMAlert) sanityCheck() error {
 		return fmt.Errorf("spec.datasource.url cannot be empty")
 	}
 
-	if r.Spec.Notifier == nil && len(r.Spec.Notifiers) == 0 && r.Spec.NotifierConfigRef == nil {
-		return fmt.Errorf("notifier is not defined, provide valid config with spec.notifier or spec.notifiers or spec.notifierConfigRef")
-	}
-
 	if r.Spec.Notifier != nil {
 		if r.Spec.Notifier.URL == "" && r.Spec.Notifier.Selector == nil {
 			return fmt.Errorf("spec.notifier.url and spec.notifier.selector cannot be empty at the same time, provide at least one setting")

diff --git a/api/v1beta1/vmalert_webhook_test.go b/api/v1beta1/vmalert_webhook_test.go
@@ -5,7 +5,6 @@ import (
 )
 
 func TestVMAlert_sanityCheck(t *testing.T) {
-
 	tests := []struct {
 		name    string
 		spec    VMAlertSpec
@@ -23,7 +22,7 @@ func TestVMAlert_sanityCheck(t *testing.T) {
 		{
 			name:    "wo notifiers",
 			spec:    VMAlertSpec{Datasource: VMAlertDatasourceSpec{URL: "http://some-url"}},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
 			name: "wo notifier url",

diff --git a/api/v1beta1/vmauth_webhook.go b/api/v1beta1/vmauth_webhook.go
@@ -39,13 +39,16 @@ func (r *VMAuth) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Validator = &VMAuth{}
 
 func (cr *VMAuth) sanityCheck() error {
-
 	if cr.Spec.Ingress != nil {
 		// check ingress
+		// TlsHosts and TlsSecretName are both needed if one of them is used
 		ing := cr.Spec.Ingress
 		if len(ing.TlsHosts) > 0 && ing.TlsSecretName == "" {
 			return fmt.Errorf("spec.ingress.tlsSecretName cannot be empty with non-empty spec.ingress.tlsHosts")
 		}
+		if ing.TlsSecretName != "" && len(ing.TlsHosts) == 0 {
+			return fmt.Errorf("spec.ingress.tlsHosts cannot be empty with non-empty spec.ingress.tlsSecretName")
+		}
 	}
 	return nil
 }

diff --git a/api/victoriametrics/v1beta1/vmalert_types.go b/api/victoriametrics/v1beta1/vmalert_types.go
@@ -166,13 +166,13 @@ type VMAlertSpec struct {
 	// +optional
 	Port string `json:"port,omitempty"`
 
-	// Notifier prometheus alertmanager endpoint spec. Required at least one of  notifier or notifiers. e.g. http://127.0.0.1:9093
+	// Notifier prometheus alertmanager endpoint spec. Required at least one of notifier or notifiers when there are alerting rules. e.g. http://127.0.0.1:9093
 	// If specified both notifier and notifiers, notifier will be added as last element to notifiers.
 	// only one of notifier options could be chosen: notifierConfigRef or notifiers +  notifier
 	// +optional
 	Notifier *VMAlertNotifierSpec `json:"notifier,omitempty"`
 
-	// Notifiers prometheus alertmanager endpoints. Required at least one of  notifier or notifiers. e.g. http://127.0.0.1:9093
+	// Notifiers prometheus alertmanager endpoints. Required at least one of notifier or notifiers when there are alerting rules. e.g. http://127.0.0.1:9093
 	// If specified both notifier and notifiers, notifier will be added as last element to notifiers.
 	// only one of notifier options could be chosen: notifierConfigRef or notifiers +  notifier
 	// +optional

diff --git a/api/victoriametrics/v1beta1/vmalert_webhook.go b/api/victoriametrics/v1beta1/vmalert_webhook.go
@@ -27,10 +27,6 @@ func (r *VMAlert) sanityCheck() error {
 		return fmt.Errorf("spec.datasource.url cannot be empty")
 	}
 
-	if r.Spec.Notifier == nil && len(r.Spec.Notifiers) == 0 && r.Spec.NotifierConfigRef == nil {
-		return fmt.Errorf("notifier is not defined, provide valid config with spec.notifier or spec.notifiers or spec.notifierConfigRef")
-	}
-
 	if r.Spec.Notifier != nil {
 		if r.Spec.Notifier.URL == "" && r.Spec.Notifier.Selector == nil {
 			return fmt.Errorf("spec.notifier.url and spec.notifier.selector cannot be empty at the same time, provide at least one setting")

diff --git a/api/victoriametrics/v1beta1/vmalert_webhook_test.go b/api/victoriametrics/v1beta1/vmalert_webhook_test.go
@@ -5,7 +5,6 @@ import (
 )
 
 func TestVMAlert_sanityCheck(t *testing.T) {
-
 	tests := []struct {
 		name    string
 		spec    VMAlertSpec
@@ -23,7 +22,7 @@ func TestVMAlert_sanityCheck(t *testing.T) {
 		{
 			name:    "wo notifiers",
 			spec:    VMAlertSpec{Datasource: VMAlertDatasourceSpec{URL: "http://some-url"}},
-			wantErr: true,
+			wantErr: false,
 		},
 		{
 			name: "wo notifier url",

diff --git a/api/victoriametrics/v1beta1/vmauth_webhook.go b/api/victoriametrics/v1beta1/vmauth_webhook.go
@@ -39,13 +39,16 @@ func (r *VMAuth) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Validator = &VMAuth{}
 
 func (cr *VMAuth) sanityCheck() error {
-
 	if cr.Spec.Ingress != nil {
 		// check ingress
+		// TlsHosts and TlsSecretName are both needed if one of them is used
 		ing := cr.Spec.Ingress
 		if len(ing.TlsHosts) > 0 && ing.TlsSecretName == "" {
 			return fmt.Errorf("spec.ingress.tlsSecretName cannot be empty with non-empty spec.ingress.tlsHosts")
 		}
+		if ing.TlsSecretName != "" && len(ing.TlsHosts) == 0 {
+			return fmt.Errorf("spec.ingress.tlsHosts cannot be empty with non-empty spec.ingress.tlsSecretName")
+		}
 	}
 	return nil
 }

diff --git a/config/crd/bases/operator.victoriametrics.com_vmalerts.yaml b/config/crd/bases/operator.victoriametrics.com_vmalerts.yaml
@@ -447,10 +447,11 @@ spec:
                 type: object
               notifier:
                 description: 'Notifier prometheus alertmanager endpoint spec. Required
-                  at least one of  notifier or notifiers. e.g. http://127.0.0.1:9093
-                  If specified both notifier and notifiers, notifier will be added
-                  as last element to notifiers. only one of notifier options could
-                  be chosen: notifierConfigRef or notifiers +  notifier'
+                  at least one of notifier or notifiers when there are alerting rules.
+                  e.g. http://127.0.0.1:9093 If specified both notifier and notifiers,
+                  notifier will be added as last element to notifiers. only one of
+                  notifier options could be chosen: notifierConfigRef or notifiers
+                  +  notifier'
                 properties:
                   basicAuth:
                     description: BasicAuth allow an endpoint to authenticate over
@@ -722,10 +723,11 @@ spec:
                 x-kubernetes-map-type: atomic
               notifiers:
                 description: 'Notifiers prometheus alertmanager endpoints. Required
-                  at least one of  notifier or notifiers. e.g. http://127.0.0.1:9093
-                  If specified both notifier and notifiers, notifier will be added
-                  as last element to notifiers. only one of notifier options could
-                  be chosen: notifierConfigRef or notifiers +  notifier'
+                  at least one of notifier or notifiers when there are alerting rules.
+                  e.g. http://127.0.0.1:9093 If specified both notifier and notifiers,
+                  notifier will be added as last element to notifiers. only one of
+                  notifier options could be chosen: notifierConfigRef or notifiers
+                  +  notifier'
                 items:
                   description: VMAlertNotifierSpec defines the notifier url for sending
                     information about alerts

diff --git a/controllers/factory/vmalert.go b/controllers/factory/vmalert.go
@@ -170,9 +170,7 @@ func CreateOrUpdateVMAlert(ctx context.Context, cr *victoriametricsv1beta1.VMAle
 		l.Info("additional notifiers with sd selector", "len", len(additionalNotifiers))
 	}
 	cr.Spec.Notifiers = append(cr.Spec.Notifiers, additionalNotifiers...)
-	if len(cr.Spec.Notifiers) == 0 && cr.Spec.NotifierConfigRef == nil {
-		return fmt.Errorf("cannot create or update vmalert: %s, cannot find any notifiers. At cr.spec.Notifiers, cr.spec.Notifier, cr.spec.notifierConfigRef and discovered alertmanager are empty", cr.Name)
-	}
+
 	if err := psp.CreateServiceAccountForCRD(ctx, cr, rclient); err != nil {
 		return fmt.Errorf("failed create service account: %w", err)
 	}

diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
@@ -17,6 +17,7 @@ aliases:
 ## Next release
 
 - [vmalertmanagerconfig](./api.html#vmalertmanagerconfig): add fields `entity`, `actions` and `update_alerts` for opsgenie_configs according to https://prometheus.io/docs/alerting/latest/configuration/#opsgenie_config.
+- [vmoperator](./README.md): remove vmalert notifier null check, since `-notifier.url` is optional and is needed only if there are alerting rules.
 
 <a name="v0.39.3"></a>
 ## [v0.39.3](https://github.com/VictoriaMetrics/operator/releases/tag/v0.39.3) - 16 Nov 2023