api/vmuser: adds status.lastSyncError (#1049)

Changes behaviour of VMAuth config generation: * now it skips misconfigured VMUser objects and continues config generation without it * misconfigured VMUsers marked with error at lastSyncError field * configuration checks better for misconfiguration (properly deduplicate user tokens, checks static url links and result url_map) Performs refactoring for CRDRef fetching. Use generic interface and reduce copy-paste. Introduce new status field for VMUser - `lastSyncError`. Adds column print for it. #1047 Signed-off-by: f41gh7 <[email protected]>
VictoriaMetrics · Jul 30, 2024 · a2ff4b1 · a2ff4b1
1 parent 3b927af
commit a2ff4b1
Show file tree

Hide file tree

Showing 8 changed files with 598 additions and 205 deletions.
diff --git a/api/operator/v1beta1/vmuser_types.go b/api/operator/v1beta1/vmuser_types.go
@@ -88,7 +88,8 @@ type VMUserIPFilters struct {
 // CRDRef describe CRD target reference.
 type CRDRef struct {
 	// Kind one of:
-	// VMAgent VMAlert VMCluster VMSingle or VMAlertManager
+	// VMAgent,VMAlert, VMSingle, VMCluster/vmselect, VMCluster/vmstorage,VMCluster/vminsert  or VMAlertManager
+	// +kubebuilder:validation:Enum=VMAgent;VMAlert;VMSingle;VMAlertManager;VMCluster/vmselect;VMCluster/vmstorage;VMCluster/vminsert
 	Kind string `json:"kind"`
 	// Name target CRD object name
 	Name string `json:"name"`
@@ -129,11 +130,18 @@ type TargetRefBasicAuth struct {
 }
 
 // VMUserStatus defines the observed state of VMUser
-type VMUserStatus struct{}
+type VMUserStatus struct {
+	// LastSyncError contains error message for unsuccessful config generation
+	// for given user
+	LastSyncError string `json:"lastSyncError,omitempty"`
+	// CurrentSyncError holds an error occured during reconcile loop
+	CurrentSyncError string `json:"-"`
+}
 
 // VMUser is the Schema for the vmusers API
 // +kubebuilder:object:root=true
 // +kubebuilder:subresource:status
+// +kubebuilder:printcolumn:name="Sync Error",type="string",JSONPath=".status.lastSyncError"
 // +genclient
 type VMUser struct {
 	metav1.TypeMeta   `json:",inline"`
@@ -182,7 +190,7 @@ func (cr *VMUser) AsOwner() []metav1.OwnerReference {
 
 func (cr VMUser) AnnotationsFiltered() map[string]string {
 	annotations := make(map[string]string)
-	for annotation, value := range cr.ObjectMeta.Annotations {
+	for annotation, value := range cr.Annotations {
 		if !strings.HasPrefix(annotation, "kubectl.kubernetes.io/") {
 			annotations[annotation] = value
 		}
@@ -199,10 +207,11 @@ func (cr VMUser) SelectorLabels() map[string]string {
 	}
 }
 
+// AllLabels returns combined labels for VMUser
 func (cr VMUser) AllLabels() map[string]string {
 	labels := cr.SelectorLabels()
-	if cr.ObjectMeta.Labels != nil {
-		for label, value := range cr.ObjectMeta.Labels {
+	if cr.Labels != nil {
+		for label, value := range cr.Labels {
 			if _, ok := labels[label]; ok {
 				// forbid changes for selector labels
 				continue

diff --git a/api/operator/v1beta1/vmuser_webhook.go b/api/operator/v1beta1/vmuser_webhook.go
@@ -48,7 +48,10 @@ func (r *VMUser) sanityCheck() error {
 	if r.Spec.PasswordRef != nil && r.Spec.Password != nil {
 		return fmt.Errorf("one of spec.password or spec.passwordRef must be used for user, got both")
 	}
-	isRetryCodesSet := len(r.Spec.UserConfigOption.RetryStatusCodes) > 0
+	if len(r.Spec.TargetRefs) == 0 {
+		return fmt.Errorf("at least 1 TargetRef must be provided for spec.targetRefs")
+	}
+	isRetryCodesSet := len(r.Spec.RetryStatusCodes) > 0
 	for i := range r.Spec.TargetRefs {
 		targetRef := r.Spec.TargetRefs[i]
 		if targetRef.CRD != nil && targetRef.Static != nil {
@@ -57,6 +60,11 @@ func (r *VMUser) sanityCheck() error {
 		if targetRef.CRD == nil && targetRef.Static == nil {
 			return fmt.Errorf("targetRef validation failed, one of `crd` or `static` must be configured, got none")
 		}
+		if targetRef.Static != nil {
+			if targetRef.Static.URL == "" && len(targetRef.Static.URLs) == 0 {
+				return fmt.Errorf("for targetRef.static url or urls option must be set at idx=%d", i)
+			}
+		}
 		if targetRef.CRD != nil {
 			switch targetRef.CRD.Kind {
 			case "VMAgent", "VMAlert", "VMAlertmanager", "VMSingle", "VMCluster/vmselect", "VMCluster/vminsert", "VMCluster/vmstorage":
@@ -67,20 +75,20 @@ func (r *VMUser) sanityCheck() error {
 				return fmt.Errorf("crd.name and crd.namespace cannot be empty")
 			}
 		}
-		if err := parseHeaders(targetRef.URLMapCommon.ResponseHeaders); err != nil {
+		if err := parseHeaders(targetRef.ResponseHeaders); err != nil {
 			return fmt.Errorf("failed to parse targetRef response headers :%w", err)
 		}
-		if err := parseHeaders(targetRef.URLMapCommon.RequestHeaders); err != nil {
+		if err := parseHeaders(targetRef.RequestHeaders); err != nil {
 			return fmt.Errorf("failed to parse targetRef headers :%w", err)
 		}
-		if isRetryCodesSet && len(targetRef.URLMapCommon.RetryStatusCodes) > 0 {
+		if isRetryCodesSet && len(targetRef.RetryStatusCodes) > 0 {
 			return fmt.Errorf("retry_status_codes already set at VMUser.spec level")
 		}
 	}
-	if err := parseHeaders(r.Spec.UserConfigOption.Headers); err != nil {
+	if err := parseHeaders(r.Spec.Headers); err != nil {
 		return fmt.Errorf("failed to parse vmuser headers: %w", err)
 	}
-	if err := parseHeaders(r.Spec.UserConfigOption.ResponseHeaders); err != nil {
+	if err := parseHeaders(r.Spec.ResponseHeaders); err != nil {
 		return fmt.Errorf("failed to parse vmuser response headers: %w", err)
 	}
 	return nil

diff --git a/config/crd/overlay/crd.yaml b/config/crd/overlay/crd.yaml
@@ -27698,7 +27698,11 @@ spec:
     singular: vmuser
   scope: Namespaced
   versions:
-  - name: v1beta1
+  - additionalPrinterColumns:
+    - jsonPath: .status.lastSyncError
+      name: Sync Error
+      type: string
+    name: v1beta1
     schema:
       openAPIV3Schema:
         description: VMUser is the Schema for the vmusers API
@@ -27863,7 +27867,15 @@ spec:
                         kind:
                           description: |-
                             Kind one of:
-                            VMAgent VMAlert VMCluster VMSingle or VMAlertManager
+                            VMAgent,VMAlert, VMSingle, VMCluster/vmselect, VMCluster/vmstorage,VMCluster/vminsert  or VMAlertManager
+                          enum:
+                          - VMAgent
+                          - VMAlert
+                          - VMSingle
+                          - VMAlertManager
+                          - VMCluster/vmselect
+                          - VMCluster/vmstorage
+                          - VMCluster/vminsert
                           type: string
                         name:
                           description: Name target CRD object name
@@ -28226,6 +28238,12 @@ spec:
             type: object
           status:
             description: VMUserStatus defines the observed state of VMUser
+            properties:
+              lastSyncError:
+                description: |-
+                  LastSyncError contains error message for unsuccessful config generation
+                  for given user
+                type: string
             type: object
         type: object
     served: true

diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
@@ -11,6 +11,9 @@ aliases:
   - /operator/changelog/
   - /operator/changelog/index.html
 ---
+
+- [vmuser](https://docs.victoriametrics.com/operator/resources/vmuser/):  adds `status.lastSyncError` field, adds server-side validation for `spec.targetRefs.crd.kind`. Adds small refactoring.
+- [vmuser](https://docs.victoriametrics.com/operator/resources/vmuser/): allows to skip `VMUser` from `VMAuth` config generation if it has misconfigured fields. Such as references to non-exist `CRD` objects or missing fields. It's highly recommended to enable `Validation` webhook for `VMUsers`, it should reduce surface of potential misconfiguration. See this [issue](https://github.com/VictoriaMetrics/operator/issues/1047) for details.
 - [operator](./README.md): properly release `PodDisruptionBudget` object finalizer. Previously it could be kept due to typo. See this [issue](https://github.com/VictoriaMetrics/operator/issues/1036) for details.
 - [operator](./README.md): refactors finalizers usage. Simplifies finalizer manipulation with helper functions
 - [vmalertmanager](./api.md#vmalertmanager): adds `webConfig` that simplifies tls configuration for alertmanager and allows to properly build probes and access urls for alertmanager. See this [issue](https://github.com/VictoriaMetrics/operator/issues/994) for details.

diff --git a/docs/api.md b/docs/api.md
@@ -293,7 +293,7 @@ _Appears in:_
 
 | Field | Description | Scheme | Required |
 | --- | --- | --- | --- |
-| `kind` | Kind one of:<br />VMAgent VMAlert VMCluster VMSingle or VMAlertManager | _string_ | true |
+| `kind` | Kind one of:<br />VMAgent,VMAlert, VMSingle, VMCluster/vmselect, VMCluster/vmstorage,VMCluster/vminsert  or VMAlertManager | _string_ | true |
 | `name` | Name target CRD object name | _string_ | true |
 | `namespace` | Namespace target CRD object namespace. | _string_ | true |
 
@@ -3736,11 +3736,11 @@ _Appears in:_
 | --- | --- | --- | --- |
 | `cert_file` | CertFile defines path to the pre-mounted file with certificate<br />mutually exclusive with CertSecretRef | _string_ | true |
 | `cert_secret_ref` | Cert defines reference for secret with CA content under given key<br />mutually exclusive with CertFile | _[SecretKeySelector](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#secretkeyselector-v1-core)_ | true |
-| `cipher_suites` | CipherSuites defines list of supported cipher suites for TLS versions up to TLS 1.2 | _string array_ | true |
+| `cipher_suites` | CipherSuites defines list of supported cipher suites for TLS versions up to TLS 1.2<br />https://golang.org/pkg/crypto/tls/#pkg-constants | _string array_ | true |
 | `client_auth_type` | ClientAuthType defines server policy for client authentication<br />If you want to enable client authentication (aka mTLS), you need to use RequireAndVerifyClientCert<br />Note, mTLS is supported only at enterprise version of VictoriaMetrics components | _string_ | true |
 | `client_ca_file` | ClientCAFile defines path to the pre-mounted file with CA<br />mutually exclusive with ClientCASecretRef | _string_ | true |
 | `client_ca_secret_ref` | ClientCA defines reference for secret with CA content under given key<br />mutually exclusive with ClientCAFile | _[SecretKeySelector](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#secretkeyselector-v1-core)_ | true |
-| `curve_preferences` | CurvePreferences defines elliptic curves that will be used in an ECDHE handshake, in preference order. | _string array_ | true |
+| `curve_preferences` | CurvePreferences defines elliptic curves that will be used in an ECDHE handshake, in preference order.<br />https://golang.org/pkg/crypto/tls/#CurveID | _string array_ | true |
 | `key_file` | KeyFile defines path to the pre-mounted file with certificate key<br />mutually exclusive with KeySecretRef | _string_ | true |
 | `key_secret_ref` | Key defines reference for secret with certificate key content under given key<br />mutually exclusive with KeyFile | _[SecretKeySelector](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.30/#secretkeyselector-v1-core)_ | true |
 | `max_version` | MaxVersion maximum TLS version that is acceptable. | _string_ | true |

diff --git a/internal/controller/operator/factory/k8stools/test_helpers.go b/internal/controller/operator/factory/k8stools/test_helpers.go
@@ -59,7 +59,7 @@ func GetTestClientWithObjects(predefinedObjects []runtime.Object) client.Client
 	for _, o := range predefinedObjects {
 		obj = append(obj, o.(client.Object))
 	}
-	fclient := fake.NewClientBuilder().WithScheme(testGetScheme()).WithObjects(obj...).Build()
+	fclient := fake.NewClientBuilder().WithScheme(testGetScheme()).WithStatusSubresource(&vmv1beta1.VMUser{}).WithObjects(obj...).Build()
 	return fclient
 }