From a2ff4b16f52f3acd92784d67c4f0846bd0bb9298 Mon Sep 17 00:00:00 2001 From: Nikolay Date: Tue, 30 Jul 2024 09:38:55 +0200 Subject: [PATCH] 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. https://github.com/VictoriaMetrics/operator/issues/1047 Signed-off-by: f41gh7 --- api/operator/v1beta1/vmuser_types.go | 19 +- api/operator/v1beta1/vmuser_webhook.go | 20 +- config/crd/overlay/crd.yaml | 22 +- docs/CHANGELOG.md | 3 + docs/api.md | 6 +- .../operator/factory/k8stools/test_helpers.go | 2 +- .../operator/factory/vmauth/vmusers_config.go | 357 +++++++++++------ .../factory/vmauth/vmusers_config_test.go | 374 +++++++++++++++--- 8 files changed, 598 insertions(+), 205 deletions(-) diff --git a/api/operator/v1beta1/vmuser_types.go b/api/operator/v1beta1/vmuser_types.go index c3dc1b04..ec453bad 100644 --- 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 index 69f326f1..27755750 100644 --- 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 index 79214a0c..e6dff301 100644 --- 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 index f6e626bf..a9d43c03 100644 --- 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 index 5813278d..6a46a981 100644 --- a/docs/api.md +++ b/docs/api.md @@ -293,7 +293,7 @@ _Appears in:_ | Field | Description | Scheme | Required | | --- | --- | --- | --- | -| `kind` | Kind one of:
VMAgent VMAlert VMCluster VMSingle or VMAlertManager | _string_ | true | +| `kind` | Kind one of:
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
mutually exclusive with CertSecretRef | _string_ | true | | `cert_secret_ref` | Cert defines reference for secret with CA content under given key
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
https://golang.org/pkg/crypto/tls/#pkg-constants | _string array_ | true | | `client_auth_type` | ClientAuthType defines server policy for client authentication
If you want to enable client authentication (aka mTLS), you need to use RequireAndVerifyClientCert
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
mutually exclusive with ClientCASecretRef | _string_ | true | | `client_ca_secret_ref` | ClientCA defines reference for secret with CA content under given key
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.
https://golang.org/pkg/crypto/tls/#CurveID | _string array_ | true | | `key_file` | KeyFile defines path to the pre-mounted file with certificate key
mutually exclusive with KeySecretRef | _string_ | true | | `key_secret_ref` | Key defines reference for secret with certificate key content under given key
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 index 3c3b8d04..33b1f9e1 100644 --- 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 } diff --git a/internal/controller/operator/factory/vmauth/vmusers_config.go b/internal/controller/operator/factory/vmauth/vmusers_config.go index ff8bc124..5fe290f6 100644 --- a/internal/controller/operator/factory/vmauth/vmusers_config.go +++ b/internal/controller/operator/factory/vmauth/vmusers_config.go @@ -10,6 +10,7 @@ import ( "path" "sort" "strings" + "time" vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1" "github.com/VictoriaMetrics/operator/internal/controller/operator/factory/build" @@ -24,6 +25,65 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +// allows to skip users based on external conditions +// implementation should has as less implementation details of vmusers as possible +// potentially it could be re-used later for scrape objects/vmalert rules. +type skipableVMUsers struct { + stopIter bool + users []*vmv1beta1.VMUser + brokenVMUsers []*vmv1beta1.VMUser +} + +// visitAll visits all users objects +func (sus *skipableVMUsers) visitAll(filter func(user *vmv1beta1.VMUser) bool) { + var cnt int + // filter in-place + for _, user := range sus.users { + if sus.stopIter { + return + } + if !filter(user) { + sus.brokenVMUsers = append(sus.brokenVMUsers, user) + continue + } + sus.users[cnt] = user + cnt++ + } + sus.users = sus.users[:cnt] +} + +func (sus *skipableVMUsers) deduplicateBy(cb func(user *vmv1beta1.VMUser) (string, time.Time)) { + // later map[key]index could be re-place with map[key]tuple(index,timestamp) + uniqByIndex := make(map[string]int, len(sus.users)) + var cnt int + for idx, user := range sus.users { + key, createdAt := cb(user) + prevIdx, ok := uniqByIndex[key] + if !ok { + // fast path + uniqByIndex[key] = idx + sus.users[cnt] = user + cnt++ + continue + } + prevUser := sus.users[prevIdx] + if createdAt.After(prevUser.CreationTimestamp.Time) { + prevUser, user = user, prevUser + } + sus.users[prevIdx] = user + prevUser.Status.CurrentSyncError = fmt.Sprintf("user has duplicate token/password with vmuser=%s-%s", user.Namespace, user.Name) + sus.brokenVMUsers = append(sus.brokenVMUsers, prevUser) + } + sus.users = sus.users[:cnt] +} + +func (sus *skipableVMUsers) sort() { + // sort for consistency. + sort.Slice(sus.users, func(i, j int) bool { + return sus.users[i].Name < sus.users[j].Name + }) +} + // builds vmauth config. func buildVMAuthConfig(ctx context.Context, rclient client.Client, vmauth *vmv1beta1.VMAuth, tlsAssets map[string]string) ([]byte, error) { // fetch exist users for vmauth. @@ -31,31 +91,25 @@ func buildVMAuthConfig(ctx context.Context, rclient client.Client, vmauth *vmv1b if err != nil { return nil, err } - // sort for consistency. - sort.Slice(users, func(i, j int) bool { - return users[i].Name < users[j].Name - }) - // check config for dups. - if dup := isUsersUniq(users); len(dup) > 0 { - return nil, fmt.Errorf("duplicate user name detected at VMAuth config: %q", strings.Join(dup, ",")) - } + sus := &skipableVMUsers{users: users} // loads info about exist operator object kind for crdRef. - crdCache, err := FetchCRDRefURLs(ctx, rclient, users) + crdCache, err := fetchCRDRefURLs(ctx, rclient, sus) if err != nil { return nil, err } - toCreateSecrets, toUpdate, err := addAuthCredentialsBuildSecrets(ctx, rclient, users) + toCreateSecrets, toUpdate, err := addAuthCredentialsBuildSecrets(ctx, rclient, sus) if err != nil { return nil, err } - // inject data from exist secrets into vmuser.spec if needed. - // toUpdate := injectAuthSettings(existSecrets, users) + // check config for dups. + filterNonUniqUsers(sus) + sus.sort() // generate yaml config for vmauth. - cfg, err := generateVMAuthConfig(vmauth, users, crdCache, tlsAssets, rclient) + cfg, err := generateVMAuthConfig(vmauth, sus, crdCache, tlsAssets, rclient) if err != nil { return nil, err } @@ -65,14 +119,39 @@ func buildVMAuthConfig(ctx context.Context, rclient client.Client, vmauth *vmv1b return nil, err } // update secrets. - // todo, probably, its better to reconcile it with finalizers merge and etc. for i := range toUpdate { secret := toUpdate[i] if err := rclient.Update(ctx, secret); err != nil { return nil, err } } + for _, user := range sus.users { + // restore status back to normal + if user.Status.LastSyncError != "" { + pt := client.RawPatch(types.MergePatchType, + []byte(`{"status": {"lastSyncError": "" } }`)) + if err := rclient.Status().Patch(ctx, user, pt); err != nil { + return nil, fmt.Errorf("failed to patch status of vmuser=%q: %w", user.Name, err) + } + } + } + var errContexts []string + for _, brokenUser := range sus.brokenVMUsers { + if brokenUser.Status.CurrentSyncError == brokenUser.Status.LastSyncError { + errContexts = append(errContexts, fmt.Sprintf("namespace=%q,name=%q,err=%q", brokenUser.Namespace, brokenUser.Name, brokenUser.Status.CurrentSyncError)) + continue + } + // patch update status + pt := client.RawPatch(types.MergePatchType, + []byte(fmt.Sprintf(`{"status": {"lastSyncError": %q } }`, brokenUser.Status.CurrentSyncError))) + if err := rclient.Status().Patch(ctx, brokenUser, pt); err != nil { + return nil, fmt.Errorf("failed to patch status of broken vmuser=%q: %w", brokenUser.Name, err) + } + } + if len(errContexts) > 0 { + logger.WithContext(ctx).Error(fmt.Errorf("vmauth has broken vmuser configurations"), strings.Join(errContexts, ",")) + } return cfg, nil } @@ -86,26 +165,22 @@ func createVMUserSecrets(ctx context.Context, rclient client.Client, secrets []* return nil } -func isUsersUniq(users []*vmv1beta1.VMUser) []string { - uniq := make(map[string]struct{}, len(users)) - var dupUsers []string - for i := range users { - user := users[i] - userName := user.Name - if user.Spec.Name != nil { - userName = *user.Spec.Name +// duplicates logic from vmauth auth_config +// parseAuthConfigUsers +func filterNonUniqUsers(sus *skipableVMUsers) { + sus.deduplicateBy(func(user *vmv1beta1.VMUser) (string, time.Time) { + var at string + if user.Spec.UserName != nil { + at = "basicAuth:" + *user.Spec.UserName } - // its ok to override userName, in this case it must be nil. - if user.Spec.BearerToken != nil { - userName = *user.Spec.BearerToken + if user.Spec.Password != nil { + at += ":" + *user.Spec.Password } - if _, ok := uniq[userName]; ok { - dupUsers = append(dupUsers, userName) - continue + if user.Spec.BearerToken != nil { + at = "bearerToken:" + *user.Spec.BearerToken } - uniq[userName] = struct{}{} - } - return dupUsers + return at, user.CreationTimestamp.Time + }) } type objectWithURL interface { @@ -113,31 +188,46 @@ type objectWithURL interface { AsURL() string } -func getAsURLObject(ctx context.Context, rclient client.Client, obj objectWithURL) (string, error) { +func getAsURLObject(ctx context.Context, rclient client.Client, objT objectWithURL) (string, error) { + obj := objT.(client.Object) + // dirty hack to restore original type of vmcluster + // since cluster type erased by wrapping it into clusterWithURL + // we must restore it original type by unwrapping type + uw, ok := objT.(unwrapObject) + if ok { + obj = uw.origin() + } if err := rclient.Get(ctx, types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}, obj); err != nil { if errors.IsNotFound(err) { - return "", nil + return "", fmt.Errorf("cannot find object by the given ref,namespace=%q,name=%q: %w", obj.GetNamespace(), obj.GetName(), err) } - return "", err + return "", fmt.Errorf("cannot get object by given ref namespace=%q,name=%q: %w", obj.GetNamespace(), obj.GetName(), err) } - return obj.AsURL(), nil + return objT.AsURL(), nil } -func addAuthCredentialsBuildSecrets(ctx context.Context, rclient client.Client, users []*vmv1beta1.VMUser) (needToCreateSecrets []*corev1.Secret, needToUpdateSecrets []*corev1.Secret, err error) { +func addAuthCredentialsBuildSecrets(ctx context.Context, rclient client.Client, sus *skipableVMUsers) (needToCreateSecrets []*corev1.Secret, needToUpdateSecrets []*corev1.Secret, resultErr error) { dst := make(map[string]*corev1.Secret) - for i := range users { - user := users[i] + + sus.visitAll(func(user *vmv1beta1.VMUser) bool { switch { case user.Spec.PasswordRef != nil: v, err := k8stools.GetCredFromSecret(ctx, rclient, user.Namespace, user.Spec.PasswordRef, fmt.Sprintf("%s/%s", user.Namespace, user.Spec.PasswordRef.Name), dst) if err != nil { - return needToCreateSecrets, needToUpdateSecrets, err + if !errors.IsNotFound(err) { + resultErr = fmt.Errorf("cannot get cred from secret=%w", err) + sus.stopIter = true + return true + } + user.Status.CurrentSyncError = fmt.Sprintf("cannot get cred from secret for passwordRef: %q", err) + return false } user.Spec.Password = ptr.To(v) case user.Spec.TokenRef != nil: v, err := k8stools.GetCredFromSecret(ctx, rclient, user.Namespace, user.Spec.TokenRef, fmt.Sprintf("%s/%s", user.Namespace, user.Spec.TokenRef.Name), dst) if err != nil { - return needToCreateSecrets, needToUpdateSecrets, err + user.Status.CurrentSyncError = fmt.Sprintf("cannot get cred from secret for tokenRef: %q", err) + return false } user.Spec.BearerToken = ptr.To(v) } @@ -145,15 +235,18 @@ func addAuthCredentialsBuildSecrets(ctx context.Context, rclient client.Client, if !user.Spec.DisableSecretCreation { var vmus corev1.Secret if err := rclient.Get(ctx, types.NamespacedName{Namespace: user.Namespace, Name: user.SecretName()}, &vmus); err != nil { - if errors.IsNotFound(err) { - userSecret, err := buildVMUserSecret(user) - if err != nil { - return needToCreateSecrets, needToUpdateSecrets, err - } - needToCreateSecrets = append(needToCreateSecrets, userSecret) - } else { - return needToCreateSecrets, needToUpdateSecrets, fmt.Errorf("cannot query kubernetes api for vmuser secrets: %w", err) + if !errors.IsNotFound(err) { + resultErr = fmt.Errorf("cannot get secret from API=%w", err) + sus.stopIter = true + return true } + userSecret, err := buildVMUserSecret(user) + if err != nil { + user.Status.CurrentSyncError = fmt.Sprintf("cannot build user secret with password: %q", err) + return false + } + needToCreateSecrets = append(needToCreateSecrets, userSecret) + } else { // secret exists, check it's state if injectAuthSettings(&vmus, user) { @@ -162,10 +255,18 @@ func addAuthCredentialsBuildSecrets(ctx context.Context, rclient client.Client, } } if err := injectBackendAuthHeader(ctx, rclient, user, dst); err != nil { - return needToCreateSecrets, needToUpdateSecrets, fmt.Errorf("cannot inject auth backend header into vmuser=%q: %w", user.Name, err) + if !errors.IsNotFound(err) { + resultErr = fmt.Errorf("cannot inject backend auth header=%w", err) + sus.stopIter = true + return true + } + user.Status.CurrentSyncError = fmt.Sprintf("cannot inject auth backend header : %q", err) + return false } - } + return true + }) + return } @@ -186,7 +287,7 @@ func injectBackendAuthHeader(ctx context.Context, rclient client.Client, user *v token := bac.Username + ":" + bac.Password token64 := base64.StdEncoding.EncodeToString([]byte(token)) Header := "Authorization: Basic " + token64 - ref.URLMapCommon.RequestHeaders = append(ref.URLMapCommon.RequestHeaders, Header) + ref.RequestHeaders = append(ref.RequestHeaders, Header) } } return nil @@ -237,11 +338,54 @@ func injectAuthSettings(secret *corev1.Secret, vmuser *vmv1beta1.VMUser) bool { return needUpdate } -// FetchCRDRefURLs performs a fetch for CRD objects for vmauth users and returns an url by crd ref key name -func FetchCRDRefURLs(ctx context.Context, rclient client.Client, users []*vmv1beta1.VMUser) (map[string]string, error) { +var crdNameToObject = map[string]objectWithURL{ + "VMAgent": &vmv1beta1.VMAgent{}, + "VMAlert": &vmv1beta1.VMAlert{}, + "VMSingle": &vmv1beta1.VMSingle{}, + "VMAlertmanager": &vmv1beta1.VMAlertmanager{}, + "VMCluster/vmselect": newClusterWithURL("vmselect"), + "VMCluster/vminsert": newClusterWithURL("vminsert"), + "VMCluster/vmstorage": newClusterWithURL("vmstorage"), +} + +// helper interface to restore VMCluster type +type unwrapObject interface { + origin() client.Object +} + +type clusterWithURL struct { + client.Object + vmc *vmv1beta1.VMCluster + component string +} + +func newClusterWithURL(component string) *clusterWithURL { + vmc := &vmv1beta1.VMCluster{} + return &clusterWithURL{vmc, vmc, component} +} + +func (c *clusterWithURL) origin() client.Object { + return c.vmc +} + +func (c *clusterWithURL) AsURL() string { + switch c.component { + case "vmselect": + return c.vmc.VMSelectURL() + case "vmstorage": + return c.vmc.VMStorageURL() + case "vminsert": + return c.vmc.VMInsertURL() + default: + panic(fmt.Sprintf("BUG: not expected component=%q for clusterWithURL object", c.component)) + } +} + +// fetchCRDRefURLs performs a fetch for CRD objects for vmauth users and returns an url by crd ref key name +func fetchCRDRefURLs(ctx context.Context, rclient client.Client, sus *skipableVMUsers) (map[string]string, error) { crdCacheURLCache := make(map[string]string) - for i := range users { - user := users[i] + var resultErr error + sus.visitAll(func(user *vmv1beta1.VMUser) bool { for j := range user.Spec.TargetRefs { ref := user.Spec.TargetRefs[j] if ref.CRD == nil { @@ -250,75 +394,31 @@ func FetchCRDRefURLs(ctx context.Context, rclient client.Client, users []*vmv1be if _, ok := crdCacheURLCache[ref.CRD.AsKey()]; ok { continue } - switch name := ref.CRD.Kind; name { - case "VMAgent": - var crd vmv1beta1.VMAgent - ref.CRD.AddRefToObj(&crd) - url, err := getAsURLObject(ctx, rclient, &crd) - if err != nil { - return nil, err - } - crdCacheURLCache[ref.CRD.AsKey()] = url - case "VMAlert": - var crd vmv1beta1.VMAlert - ref.CRD.AddRefToObj(&crd) - url, err := getAsURLObject(ctx, rclient, &crd) - if err != nil { - return nil, err - } - crdCacheURLCache[ref.CRD.AsKey()] = url - - case "VMSingle": - var crd vmv1beta1.VMSingle - ref.CRD.AddRefToObj(&crd) - url, err := getAsURLObject(ctx, rclient, &crd) - if err != nil { - return nil, err - } - crdCacheURLCache[ref.CRD.AsKey()] = url - case "VMAlertmanager": - var crd vmv1beta1.VMAlertmanager - ref.CRD.AddRefToObj(&crd) - url, err := getAsURLObject(ctx, rclient, &crd) - if err != nil { - return nil, err - } - crdCacheURLCache[ref.CRD.AsKey()] = url - - case "VMCluster/vmselect", "VMCluster/vminsert", "VMCluster/vmstorage": - var crd vmv1beta1.VMCluster - ref.CRD.AddRefToObj(&crd) - url, err := getAsURLObject(ctx, rclient, &crd) - if err != nil { - return nil, err - } - if url == "" { - continue - } - var targetURL string - switch { - case strings.HasSuffix(name, "vmselect"): - targetURL = crd.VMSelectURL() - case strings.HasSuffix(name, "vminsert"): - targetURL = crd.VMInsertURL() - case strings.HasSuffix(name, "vmstorage"): - targetURL = crd.VMStorageURL() - default: - logger.WithContext(ctx).Error(fmt.Errorf("unsupported kind for VMCluster: %s", name), "cannot select crd ref") - continue + crdObj, ok := crdNameToObject[ref.CRD.Kind] + if !ok { + user.Status.CurrentSyncError = fmt.Sprintf("unsupported kind for ref: %q at idx=%d", ref.CRD.Kind, j) + return false + } + ref.CRD.AddRefToObj(crdObj.(client.Object)) + url, err := getAsURLObject(ctx, rclient, crdObj) + if err != nil { + if !errors.IsNotFound(err) { + resultErr = fmt.Errorf("cannot get object as url: %w", err) + sus.stopIter = true + return true } - crdCacheURLCache[ref.CRD.AsKey()] = targetURL - default: - logger.WithContext(ctx).Error(fmt.Errorf("unsupported kind: %s", name), "cannot select crd ref") - continue + user.Status.CurrentSyncError = fmt.Sprintf("cannot fined CRD link for kind=%q at ref idx=%d: %q", ref.CRD.Kind, j, err) + return false } + crdCacheURLCache[ref.CRD.AsKey()] = url } - } - return crdCacheURLCache, nil + return true + }) + return crdCacheURLCache, resultErr } // generateVMAuthConfig create VMAuth cfg for given Users. -func generateVMAuthConfig(cr *vmv1beta1.VMAuth, users []*vmv1beta1.VMUser, crdCache map[string]string, tlsAssets map[string]string, rclient client.Client) ([]byte, error) { +func generateVMAuthConfig(cr *vmv1beta1.VMAuth, sus *skipableVMUsers, crdCache map[string]string, tlsAssets map[string]string, rclient client.Client) ([]byte, error) { var cfg yaml.MapSlice secretCache := make(map[string]*corev1.Secret) @@ -332,16 +432,18 @@ func generateVMAuthConfig(cr *vmv1beta1.VMAuth, users []*vmv1beta1.VMUser, crdCa ConfigmapCache: configmapCache, TLSAssets: tlsAssets, } - var cfgUsers []yaml.MapSlice - for i := range users { - user := users[i] + + sus.visitAll(func(user *vmv1beta1.VMUser) bool { userCfg, err := genUserCfg(user, crdCache, cb) if err != nil { - return nil, err + user.Status.CurrentSyncError = err.Error() + return false } cfgUsers = append(cfgUsers, userCfg) - } + return true + }) + if len(cfgUsers) > 0 { cfg = yaml.MapSlice{ { @@ -376,7 +478,11 @@ func generateVMAuthConfig(cr *vmv1beta1.VMAuth, users []*vmv1beta1.VMUser, crdCa if len(unAuthorizedAccessValue) > 0 { cfg = append(cfg, yaml.MapItem{Key: "unauthorized_user", Value: unAuthorizedAccessValue}) } - return yaml.Marshal(cfg) + ac, err := yaml.Marshal(cfg) + if err != nil { + return nil, fmt.Errorf("failed to serialize configuration to yaml: %w", err) + } + return ac, nil } func appendIfNotNull(src []string, key string, origin yaml.MapSlice) yaml.MapSlice { @@ -653,6 +759,9 @@ func genURLMaps(userName string, refs []vmv1beta1.TargetRef, result yaml.MapSlic } urlMaps = append(urlMaps, urlMap) } + if len(urlMaps) == 0 { + return nil, fmt.Errorf("user must has at least 1 url target") + } result = append(result, yaml.MapItem{Key: "url_map", Value: urlMaps}) return result, nil } diff --git a/internal/controller/operator/factory/vmauth/vmusers_config_test.go b/internal/controller/operator/factory/vmauth/vmusers_config_test.go index ff967e3d..59a601b9 100644 --- a/internal/controller/operator/factory/vmauth/vmusers_config_test.go +++ b/internal/controller/operator/factory/vmauth/vmusers_config_test.go @@ -4,6 +4,7 @@ import ( "context" "strings" "testing" + "time" vmv1beta1 "github.com/VictoriaMetrics/operator/api/operator/v1beta1" "github.com/VictoriaMetrics/operator/internal/controller/operator/factory/build" @@ -436,10 +437,16 @@ password: pass "foo": "bar", "buz": "qux", }, + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{URL: "http://localhost:8435"}, + }, + }, }, }, }, - want: `url_map: [] + want: `url_prefix: +- http://localhost:8435 name: user1 metric_labels: buz: qux @@ -495,7 +502,7 @@ func Test_genPassword(t *testing.T) { func Test_selectVMUserSecrets(t *testing.T) { type args struct { - vmUsers []*vmv1beta1.VMUser + vmUsers *skipableVMUsers } tests := []struct { name string @@ -508,20 +515,22 @@ func Test_selectVMUserSecrets(t *testing.T) { { name: "want 1 updateSecret", args: args{ - vmUsers: []*vmv1beta1.VMUser{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "exist", - Namespace: "default", + vmUsers: &skipableVMUsers{ + users: []*vmv1beta1.VMUser{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "exist", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, }, - Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "not-exist", - Namespace: "default", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "not-exist", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, }, - Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, }, }, }, @@ -536,30 +545,32 @@ func Test_selectVMUserSecrets(t *testing.T) { { name: "want 1 updateSecret", args: args{ - vmUsers: []*vmv1beta1.VMUser{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "must-not-exist", - Namespace: "default", - }, - Spec: vmv1beta1.VMUserSpec{ - BearerToken: ptr.To("some-bearer"), - DisableSecretCreation: true, + vmUsers: &skipableVMUsers{ + users: []*vmv1beta1.VMUser{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "must-not-exist", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + BearerToken: ptr.To("some-bearer"), + DisableSecretCreation: true, + }, }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "not-exists-must-create", - Namespace: "default", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "not-exists-must-create", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, }, - Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "exists", - Namespace: "default", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "exists", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, }, - Spec: vmv1beta1.VMUserSpec{BearerToken: ptr.To("some-bearer")}, }, }, }, @@ -574,22 +585,24 @@ func Test_selectVMUserSecrets(t *testing.T) { { name: "want nothing", args: args{ - vmUsers: []*vmv1beta1.VMUser{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "exist-with-generated", - Namespace: "default", - }, - Spec: vmv1beta1.VMUserSpec{ - GeneratePassword: true, + vmUsers: &skipableVMUsers{ + users: []*vmv1beta1.VMUser{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "exist-with-generated", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + GeneratePassword: true, + }, }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "exist-hardcoded", - Namespace: "default", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "exist-hardcoded", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{}, }, - Spec: vmv1beta1.VMUserSpec{}, }, }, }, @@ -609,22 +622,24 @@ func Test_selectVMUserSecrets(t *testing.T) { { name: "update secret value", args: args{ - vmUsers: []*vmv1beta1.VMUser{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "exist-with-generated", - Namespace: "default", - }, - Spec: vmv1beta1.VMUserSpec{ - GeneratePassword: true, + vmUsers: &skipableVMUsers{ + users: []*vmv1beta1.VMUser{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "exist-with-generated", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + GeneratePassword: true, + }, }, - }, - { - ObjectMeta: metav1.ObjectMeta{ - Name: "exist-to-update", - Namespace: "default", + { + ObjectMeta: metav1.ObjectMeta{ + Name: "exist-to-update", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{Password: ptr.To("some-new-password"), UserName: ptr.To("some-user")}, }, - Spec: vmv1beta1.VMUserSpec{Password: ptr.To("some-new-password"), UserName: ptr.To("some-user")}, }, }, }, @@ -699,7 +714,7 @@ func Test_buildVMAuthConfig(t *testing.T) { predefinedObjects []runtime.Object }{ { - name: "default cfg", + name: "simple cfg", args: args{ vmauth: &vmv1beta1.VMAuth{ ObjectMeta: metav1.ObjectMeta{ @@ -764,6 +779,91 @@ func Test_buildVMAuthConfig(t *testing.T) { bearer_token: bearer-token-2 `, }, + { + name: "simple cfg with duplicated users", + args: args{ + vmauth: &vmv1beta1.VMAuth{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vmauth", + Namespace: "default", + }, + Spec: vmv1beta1.VMAuthSpec{ + SelectAllByDefault: true, + }, + }, + }, + predefinedObjects: []runtime.Object{ + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-1", + Namespace: "default", + CreationTimestamp: metav1.Time{Time: time.Unix(123, 0)}, + }, + Spec: vmv1beta1.VMUserSpec{ + Name: ptr.To("user1"), + BearerToken: ptr.To("bearer"), + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{URL: "http://some-static"}, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-1-duplicate", + Namespace: "default", + CreationTimestamp: metav1.Time{Time: time.Unix(150, 0)}, + }, + Spec: vmv1beta1.VMUserSpec{ + Name: ptr.To("user1"), + BearerToken: ptr.To("bearer"), + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{URL: "http://some-static"}, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-2", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + BearerToken: ptr.To("bearer-token-2"), + TargetRefs: []vmv1beta1.TargetRef{ + { + CRD: &vmv1beta1.CRDRef{ + Kind: "VMAgent", + Name: "test", + Namespace: "default", + }, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMAgent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + }, + }, + want: `users: +- url_prefix: + - http://some-static + name: user1 + bearer_token: bearer +- url_prefix: + - http://vmagent-test.default.svc:8429 + bearer_token: bearer-token-2 +`, + }, + { name: "with targetRef basicauth secret refs and headers", args: args{ @@ -1712,6 +1812,152 @@ unauthorized_user: url_prefix: - http://route-1 - http://route-2 +`, + }, + { + name: "with duplicated users and broken links", + args: args{ + vmauth: &vmv1beta1.VMAuth{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-vmauth", + Namespace: "default", + }, + Spec: vmv1beta1.VMAuthSpec{ + SelectAllByDefault: true, + }, + }, + }, + predefinedObjects: []runtime.Object{ + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-1", + Namespace: "default", + CreationTimestamp: metav1.Time{Time: time.Unix(123, 0)}, + }, + Spec: vmv1beta1.VMUserSpec{ + Name: ptr.To("user1"), + BearerToken: ptr.To("bearer"), + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{URL: "http://some-static"}, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-5-duplicate", + Namespace: "default", + CreationTimestamp: metav1.Time{Time: time.Unix(150, 0)}, + }, + Spec: vmv1beta1.VMUserSpec{ + Name: ptr.To("user1"), + BearerToken: ptr.To("bearer"), + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{URL: "http://some-static"}, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-6-duplicate", + Namespace: "default", + CreationTimestamp: metav1.Time{Time: time.Unix(135, 0)}, + }, + Spec: vmv1beta1.VMUserSpec{ + Name: ptr.To("user1"), + BearerToken: ptr.To("bearer"), + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{URL: "http://some-static"}, + Paths: []string{"/"}, + }, + }, + }, + }, + + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-2", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + BearerToken: ptr.To("bearer-token-2"), + TargetRefs: []vmv1beta1.TargetRef{ + { + CRD: &vmv1beta1.CRDRef{ + Kind: "VMAgent", + Name: "test", + Namespace: "default", + }, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMAgent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-3-broken-crd-link", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + BearerToken: ptr.To("bearer-token-17"), + TargetRefs: []vmv1beta1.TargetRef{ + { + CRD: &vmv1beta1.CRDRef{ + Kind: "VMAgent", + Name: "test-not-found", + Namespace: "default", + }, + Paths: []string{"/"}, + }, + }, + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-3-missing-urls", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + BearerToken: ptr.To("bearer-token-15"), + }, + }, + &vmv1beta1.VMUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "user-10-non-exist-secret-ref", + Namespace: "default", + }, + Spec: vmv1beta1.VMUserSpec{ + TokenRef: &corev1.SecretKeySelector{}, + TargetRefs: []vmv1beta1.TargetRef{ + { + Static: &vmv1beta1.StaticRef{ + URL: "http://some", + }, + }, + }, + }, + }, + }, + want: `users: +- url_prefix: + - http://some-static + name: user1 + bearer_token: bearer +- url_prefix: + - http://vmagent-test.default.svc:8429 + bearer_token: bearer-token-2 `, }, }