api/vmauth: introduce unauthorizedUserAccessSpec field at vmauth.spec

Previously for unauthorized_user config section of vmauth. Operator used two configuration fields: `unauthorizedAccessConfig` and inlined fields from `VMUserOptions`. This behaviour doesn't aligh with configuration file supported by vmauth. It also incorrectly exposed fields from `VMUserOptions` at `spec`. Which could mislead users, since `spec.default_urls` could be treated as global config option for vmauth, but in fact, it can only be used at `unauthorized_user` section. This commit replaces both fields with the new field `unauthorizedUserAccess`. It combines both config options - `url_map` and `VMUserOptions`. Replaced fields marked as deprecated and will be removed at `v1.0` operator API release. Also `VMauth` now properly validates `unauthorized_user` related configuration and returns proper error to the user, instead of crashing `vmauth` container in runtime. Related issues: - #1168 - #1169 --------- Signed-off-by: f41gh7 <[email protected]> Co-authored-by: Hui Wang <[email protected]>
VictoriaMetrics · Dec 12, 2024 · 76a1c94 · 76a1c94
1 parent 2a083e4
commit 76a1c94
Show file tree

Hide file tree

Showing 16 changed files with 1,103 additions and 569 deletions.
diff --git a/api/operator/v1beta1/vmauth_types.go b/api/operator/v1beta1/vmauth_types.go
diff --git a/api/operator/v1beta1/vmauth_webhook.go b/api/operator/v1beta1/vmauth_webhook.go
@@ -18,13 +18,16 @@ package v1beta1
 
 import (
 	"fmt"
+	"regexp"
 
 	"k8s.io/apimachinery/pkg/runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
+var labelNameRegexp = regexp.MustCompile("^[a-zA-Z_:.][a-zA-Z0-9_:.]*$")
+
 // SetupWebhookWithManager will setup the manager to manage the webhooks
 func (r *VMAuth) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
@@ -59,6 +62,26 @@ func (r *VMAuth) sanityCheck() error {
 				r.Spec.ExternalConfig.SecretRef.Name, r.Spec.ExternalConfig.SecretRef.Key)
 		}
 	}
+	if len(r.Spec.UnauthorizedAccessConfig) > 0 && r.Spec.UnauthorizedUserAccessSpec != nil {
+		return fmt.Errorf("at most one option can be used `spec.unauthorizedAccessConfig` or `spec.unauthorizedUserAccessSpec`, got both")
+	}
+	if len(r.Spec.UnauthorizedAccessConfig) > 0 {
+		for _, urlMap := range r.Spec.UnauthorizedAccessConfig {
+			if err := urlMap.Validate(); err != nil {
+				return fmt.Errorf("incorrect r.spec.UnauthorizedAccessConfig: %w", err)
+			}
+		}
+		if err := r.Spec.VMUserConfigOptions.Validate(); err != nil {
+			return fmt.Errorf("incorrect r.spec UnauthorizedAccessConfig options: %w", err)
+		}
+	}
+
+	if r.Spec.UnauthorizedUserAccessSpec != nil {
+		if err := r.Spec.UnauthorizedUserAccessSpec.Validate(); err != nil {
+			return fmt.Errorf("incorrect r.spec.UnauthorizedUserAccess syntax: %w", err)
+		}
+	}
+
 	return nil
 }
 

diff --git a/api/operator/v1beta1/vmauth_webhook_test.go b/api/operator/v1beta1/vmauth_webhook_test.go
@@ -1,57 +1,108 @@
 package v1beta1
 
 import (
-	"testing"
+	"encoding/json"
 
-	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	"gopkg.in/yaml.v2"
 )
 
-func TestVMAuth_sanityCheck(t *testing.T) {
-	type fields struct {
-		TypeMeta   v1.TypeMeta
-		ObjectMeta v1.ObjectMeta
-		Spec       VMAuthSpec
-		Status     VMAuthStatus
-	}
-	tests := []struct {
-		name    string
-		fields  fields
-		wantErr bool
-	}{
-		{
-			name: "invalid ingress",
-			fields: fields{
-				Spec: VMAuthSpec{
-					Ingress: &EmbeddedIngress{
-						TlsHosts: []string{"host-1", "host-2"},
-					},
-				},
+var _ = Describe("VMAuth Webhook", func() {
+	Context("When creating VMAuth under Validating Webhook", func() {
+		DescribeTable("fail validation",
+			func(srcYAML string, wantErrText string) {
+				var amc VMAuth
+				Expect(yaml.Unmarshal([]byte(srcYAML), &amc)).To(Succeed())
+				cfgJSON, err := json.Marshal(amc)
+				Expect(err).ShouldNot(HaveOccurred())
+				Expect(json.Unmarshal(cfgJSON, &amc)).ShouldNot(HaveOccurred())
+				Expect(amc.sanityCheck()).To(MatchError(wantErrText))
 			},
-			wantErr: true,
-		},
-		{
-			name: "valid cfg",
-			fields: fields{
-				Spec: VMAuthSpec{
-					Ingress: &EmbeddedIngress{
-						TlsHosts:      []string{"host1"},
-						TlsSecretName: "secret-1",
-					},
-				},
-			},
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			cr := &VMAuth{
-				TypeMeta:   tt.fields.TypeMeta,
-				ObjectMeta: tt.fields.ObjectMeta,
-				Spec:       tt.fields.Spec,
-				Status:     tt.fields.Status,
-			}
-			if err := cr.sanityCheck(); (err != nil) != tt.wantErr {
-				t.Errorf("sanityCheck() error = %v, wantErr %v", err, tt.wantErr)
-			}
-		})
-	}
-}
+			Entry("invalid ingress", `
+        apiVersion: v1 
+        kind: VMAuth
+        metadata:
+          name: must-fail
+        spec:
+          ingress:
+            tlsHosts: 
+            - host-1
+            - host-2
+        `, `spec.ingress.tlsSecretName cannot be empty with non-empty spec.ingress.tlsHosts`),
+			Entry("both configSecret and external config is defined at the same time", `
+        apiVersion: v1 
+        kind: VMAuth
+        metadata:
+          name: must-fail
+        spec:
+         configSecret: some-value
+         externalConfig:
+           secretRef:
+             key: secret
+             name: access
+        `, `spec.configSecret and spec.externalConfig.secretRef cannot be used at the same time`),
+			Entry("incorrect unauthorized access config, missing backends", `
+        apiVersion: v1 
+        kind: VMAuth
+        metadata:
+          name: must-fail
+        spec:
+         unauthorizedUserAccessSpec:
+            default_url: 
+            - http://url-1
+        `, "incorrect r.spec.UnauthorizedUserAccess syntax: at least one of `url_map` or `url_prefix` must be defined"),
+			Entry("incorrect unauthorized access config, bad metric_labels syntax", `
+        apiVersion: v1 
+        kind: VMAuth
+        metadata:
+          name: must-fail
+        spec:
+         unauthorizedUserAccessSpec:
+            metric_labels:
+                124124asff: 12fsaf
+            url_prefix: http://some-dst
+            default_url: 
+            - http://url-1
+        `, `incorrect r.spec.UnauthorizedUserAccess syntax: incorrect metricLabelName="124124asff", must match pattern="^[a-zA-Z_:.][a-zA-Z0-9_:.]*$"`),
+			Entry("incorrect unauthorized access config url_map", `
+        apiVersion: v1 
+        kind: VMAuth
+        metadata:
+          name: must-fail
+        spec:
+         unauthorizedUserAccessSpec:
+            metric_labels:
+                label: 12fsaf-value
+            url_map:
+            - url_prefix: http://some-url
+              src_paths: ["/path-1"]
+            - url_prefix: http://some-url-2
+            default_url: 
+            - http://url-1
+        `, `incorrect r.spec.UnauthorizedUserAccess syntax: incorrect url_map at idx=1: incorrect url_map config at least of one src_paths,src_hosts,src_query_args or src_headers must be defined`,
+			),
+			Entry("both unauthorizedUserAccessSpec and UnauthorizedUserAccess defined", `
+        apiVersion: v1 
+        kind: VMAuth
+        metadata:
+          name: must-fail
+        spec:
+         unauthorizedAccessConfig: 
+         - url_prefix: http://some-url
+           src_paths: ["/path-1"]
+         - url_prefix: http://some-url-2
+           src_paths: ["/path-1"]
+         unauthorizedUserAccessSpec:
+            metric_labels:
+                label: 12fsaf-value
+            url_map:
+            - url_prefix: http://some-url
+              src_paths: ["/path-1"]
+            default_url: 
+            - http://url-1
+        `, "at most one option can be used `spec.unauthorizedAccessConfig` or `spec.unauthorizedUserAccessSpec`, got both",
+			),
+		)
+	})
+})
diff --git a/api/operator/v1beta1/vmextra_types.go b/api/operator/v1beta1/vmextra_types.go
@@ -1399,11 +1399,11 @@ func buildStatusPatch(currentStatus interface{}) (client.Patch, error) {
 type ExternalConfig struct {
 	// SecretRef defines selector for externally managed secret which contains configuration
 	// +optional
-	SecretRef *v1.SecretKeySelector `json:"secretRef,omitempty"`
+	SecretRef *v1.SecretKeySelector `json:"secretRef,omitempty" yaml:"secretRef,omitempty"`
 	// LocalPath contains static path to a config, which is managed externally for cases
 	// when using secrets is not applicable, e.g.: Vault sidecar.
 	// +optional
-	LocalPath string `json:"localPath,omitempty"`
+	LocalPath string `json:"localPath,omitempty" yaml:"localPath,omitempty"`
 }
 
 // StatusMetadata holds metadata of application update status

diff --git a/api/operator/v1beta1/vmuser_types.go b/api/operator/v1beta1/vmuser_types.go
@@ -38,7 +38,7 @@ type VMUserSpec struct {
 	// TargetRefs - reference to endpoints, which user may access.
 	TargetRefs []TargetRef `json:"targetRefs"`
 
-	UserConfigOption `json:",inline"`
+	VMUserConfigOptions `json:",inline"`
 
 	// MetricLabels - additional labels for metrics exported by vmauth for given user.
 	// +optional

diff --git a/api/operator/v1beta1/vmuser_webhook.go b/api/operator/v1beta1/vmuser_webhook.go
@@ -64,6 +64,16 @@ func (r *VMUser) sanityCheck() error {
 			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.Static.URL != "" {
+				if err := validateURLPrefix(targetRef.Static.URL); err != nil {
+					return fmt.Errorf("incorrect static.url: %w", err)
+				}
+			}
+			for _, staticURL := range targetRef.Static.URLs {
+				if err := validateURLPrefix(staticURL); err != nil {
+					return fmt.Errorf("incorrect value at static.urls: %w", err)
+				}
+			}
 		}
 		if targetRef.CRD != nil {
 			switch targetRef.CRD.Kind {
@@ -75,35 +85,30 @@ func (r *VMUser) sanityCheck() error {
 				return fmt.Errorf("crd.name and crd.namespace cannot be empty")
 			}
 		}
-		if err := parseHeaders(targetRef.ResponseHeaders); err != nil {
+		if err := validateHTTPHeaders(targetRef.ResponseHeaders); err != nil {
 			return fmt.Errorf("failed to parse targetRef response headers :%w", err)
 		}
-		if err := parseHeaders(targetRef.RequestHeaders); err != nil {
+		if err := validateHTTPHeaders(targetRef.RequestHeaders); err != nil {
 			return fmt.Errorf("failed to parse targetRef headers :%w", err)
 		}
 		if isRetryCodesSet && len(targetRef.RetryStatusCodes) > 0 {
 			return fmt.Errorf("retry_status_codes already set at VMUser.spec level")
 		}
 	}
-	if err := parseHeaders(r.Spec.Headers); err != nil {
+	for k := range r.Spec.MetricLabels {
+		if !labelNameRegexp.MatchString(k) {
+			return fmt.Errorf("incorrect metricLabels key=%q, must match pattern=%q", k, labelNameRegexp)
+		}
+	}
+	if err := validateHTTPHeaders(r.Spec.Headers); err != nil {
 		return fmt.Errorf("failed to parse vmuser headers: %w", err)
 	}
-	if err := parseHeaders(r.Spec.ResponseHeaders); err != nil {
+	if err := validateHTTPHeaders(r.Spec.ResponseHeaders); err != nil {
 		return fmt.Errorf("failed to parse vmuser response headers: %w", err)
 	}
 	return nil
 }
 
-func parseHeaders(src []string) error {
-	for idx, s := range src {
-		n := strings.IndexByte(s, ':')
-		if n < 0 {
-			return fmt.Errorf("missing speparator char ':' between Name and Value in the header: %q at idx: %d; expected format - 'Name: Value'", s, idx)
-		}
-	}
-	return nil
-}
-
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *VMUser) ValidateCreate() (admission.Warnings, error) {
 	if mustSkipValidation(r) {