From 1d5dffc5cbad38e844e1ccf5dfa3ed8ab148a75d Mon Sep 17 00:00:00 2001 From: as51340 Date: Tue, 13 Oct 2026 11:13:57 +0200 Subject: [PATCH] fix: Reject zero and empty values admission used to let through Four fields accepted a value that only failed, or silently changed meaning, after admission: - monitoring.vmagentRemote.scrapeInterval matched "" (every unit in its duration pattern is optional). A written "" is stored as is, since a schema default fills only a missing field, and the builder then ran vmagent on its own 15s fallback. MinLength=1. - securityContext.runAsUser allowed 0, but the Memgraph container sets runAsNonRoot, so the kubelet refused to start it. Minimum=1; runAsGroup and fsGroup keep 0, which is a legitimate gid. - coreDumps.uploader.envFromSecrets items had no constraint, so "" or a non-DNS name surfaced as ApplyFailed. Items now carry the Secret name rules every other secretName has. - storage..libPVCSize and coreDumps..size accepted 0 and negative quantities at creation; the existing rules only stop a shrink. Both must now be greater than zero. An existing object holding one of these values fails its next update until it is corrected. The validation test's raw manifests share a rawCluster helper now that there are three. --- api/v1alpha1/memgraphcluster_types.go | 11 +- charts/memgraph-operator/Chart.yaml | 2 +- .../crds/memgraph.com_memgraphclusters.yaml | 21 +++- .../bases/memgraph.com_memgraphclusters.yaml | 21 +++- .../memgraphcluster_validation_test.go | 100 ++++++++++++++---- 5 files changed, 130 insertions(+), 25 deletions(-) diff --git a/api/v1alpha1/memgraphcluster_types.go b/api/v1alpha1/memgraphcluster_types.go index 820a37d..24d9d12 100644 --- a/api/v1alpha1/memgraphcluster_types.go +++ b/api/v1alpha1/memgraphcluster_types.go @@ -557,6 +557,7 @@ type RoleStorageSpec struct { // Memgraph's data directory (snapshots, WAL, and durability metadata). It // can grow on a live cluster and never shrink. With fileLogging on it holds // the role's log files too. + // +kubebuilder:validation:XValidation:rule="quantity(string(self)).isGreaterThan(quantity('0'))",message="libPVCSize must be greater than zero" // +kubebuilder:default="10Gi" // +optional LibPVCSize *resource.Quantity `json:"libPVCSize,omitempty"` @@ -646,6 +647,7 @@ type RoleCoreDumpsSpec struct { // this is per role: a data instance holds the graph, a coordinator holds // Raft state. Size it against the role's memory limit, not its data. It // can grow while the role collects dumps and never shrink. + // +kubebuilder:validation:XValidation:rule="quantity(string(self)).isGreaterThan(quantity('0'))",message="coreDumps size must be greater than zero" // +kubebuilder:default="10Gi" // +optional Size *resource.Quantity `json:"size,omitempty"` @@ -691,6 +693,9 @@ type CoreDumpsUploaderSpec struct { // envFromSecrets names Secrets in the cluster's namespace whose keys become // environment variables of the sidecar. This is how credentials reach it: // by reference, so no secret material ever appears in this resource. + // +kubebuilder:validation:items:MinLength=1 + // +kubebuilder:validation:items:MaxLength=253 + // +kubebuilder:validation:items:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$` // +listType=set // +optional EnvFromSecrets []string `json:"envFromSecrets,omitempty"` @@ -861,8 +866,9 @@ type SysctlInitContainerSpec struct { type PodSecurityContextSpec struct { // runAsUser is the uid every container of the pod runs as. Absent, the // pod names none and the image's user (or the platform's assignment) - // decides. - // +kubebuilder:validation:Minimum=0 + // decides. It cannot be 0: the Memgraph container sets runAsNonRoot, so + // the kubelet would refuse to start it. + // +kubebuilder:validation:Minimum=1 // +optional RunAsUser *int64 `json:"runAsUser,omitempty"` @@ -1813,6 +1819,7 @@ type VMAgentRemoteSpec struct { // scrapeInterval is how often vmagent scrapes every instance, as a // Prometheus duration such as "15s" or "1m". + // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:Pattern=`^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$` // +kubebuilder:default="15s" // +optional diff --git a/charts/memgraph-operator/Chart.yaml b/charts/memgraph-operator/Chart.yaml index 99bfa33..6b8a32d 100644 --- a/charts/memgraph-operator/Chart.yaml +++ b/charts/memgraph-operator/Chart.yaml @@ -4,7 +4,7 @@ description: Kubernetes operator for Memgraph high-availability clusters type: application # version is the chart version, appVersion the operator version it installs. # Both move together with the git tag at release time. -version: 1.0.1-rc.1 +version: 1.0.1-rc.2 appVersion: "1.0.1-rc.1" # The CustomResourceDefinition uses CEL validation rules # (x-kubernetes-validations), available from Kubernetes 1.25. diff --git a/charts/memgraph-operator/crds/memgraph.com_memgraphclusters.yaml b/charts/memgraph-operator/crds/memgraph.com_memgraphclusters.yaml index 359a265..5a92fc5 100644 --- a/charts/memgraph-operator/crds/memgraph.com_memgraphclusters.yaml +++ b/charts/memgraph-operator/crds/memgraph.com_memgraphclusters.yaml @@ -160,6 +160,9 @@ spec: can grow while the role collects dumps and never shrink. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: coreDumps size must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) type: object x-kubernetes-validations: - message: 'coreDumps size cannot shrink while the role collects @@ -186,6 +189,9 @@ spec: can grow while the role collects dumps and never shrink. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: coreDumps size must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) type: object x-kubernetes-validations: - message: 'coreDumps size cannot shrink while the role collects @@ -263,6 +269,9 @@ spec: environment variables of the sidecar. This is how credentials reach it: by reference, so no secret material ever appears in this resource. items: + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string type: array x-kubernetes-list-type: set @@ -1329,6 +1338,7 @@ spec: description: |- scrapeInterval is how often vmagent scrapes every instance, as a Prometheus duration such as "15s" or "1m". + minLength: 1 pattern: ^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$ type: string required: @@ -2809,9 +2819,10 @@ spec: description: |- runAsUser is the uid every container of the pod runs as. Absent, the pod names none and the image's user (or the platform's assignment) - decides. + decides. It cannot be 0: the Memgraph container sets runAsNonRoot, so + the kubelet would refuse to start it. format: int64 - minimum: 0 + minimum: 1 type: integer type: object storage: @@ -2854,6 +2865,9 @@ spec: the role's log files too. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: libPVCSize must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) libStorageAccessMode: default: ReadWriteOnce description: |- @@ -2930,6 +2944,9 @@ spec: the role's log files too. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: libPVCSize must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) libStorageAccessMode: default: ReadWriteOnce description: |- diff --git a/config/crd/bases/memgraph.com_memgraphclusters.yaml b/config/crd/bases/memgraph.com_memgraphclusters.yaml index 75f1112..169216c 100644 --- a/config/crd/bases/memgraph.com_memgraphclusters.yaml +++ b/config/crd/bases/memgraph.com_memgraphclusters.yaml @@ -158,6 +158,9 @@ spec: can grow while the role collects dumps and never shrink. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: coreDumps size must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) type: object x-kubernetes-validations: - message: 'coreDumps size cannot shrink while the role collects @@ -184,6 +187,9 @@ spec: can grow while the role collects dumps and never shrink. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: coreDumps size must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) type: object x-kubernetes-validations: - message: 'coreDumps size cannot shrink while the role collects @@ -261,6 +267,9 @@ spec: environment variables of the sidecar. This is how credentials reach it: by reference, so no secret material ever appears in this resource. items: + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ type: string type: array x-kubernetes-list-type: set @@ -1327,6 +1336,7 @@ spec: description: |- scrapeInterval is how often vmagent scrapes every instance, as a Prometheus duration such as "15s" or "1m". + minLength: 1 pattern: ^(0|(([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?)$ type: string required: @@ -2807,9 +2817,10 @@ spec: description: |- runAsUser is the uid every container of the pod runs as. Absent, the pod names none and the image's user (or the platform's assignment) - decides. + decides. It cannot be 0: the Memgraph container sets runAsNonRoot, so + the kubelet would refuse to start it. format: int64 - minimum: 0 + minimum: 1 type: integer type: object storage: @@ -2852,6 +2863,9 @@ spec: the role's log files too. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: libPVCSize must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) libStorageAccessMode: default: ReadWriteOnce description: |- @@ -2928,6 +2942,9 @@ spec: the role's log files too. pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$ x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: libPVCSize must be greater than zero + rule: quantity(string(self)).isGreaterThan(quantity('0')) libStorageAccessMode: default: ReadWriteOnce description: |- diff --git a/internal/controller/memgraphcluster_validation_test.go b/internal/controller/memgraphcluster_validation_test.go index 42c1740..1cf0462 100644 --- a/internal/controller/memgraphcluster_validation_test.go +++ b/internal/controller/memgraphcluster_validation_test.go @@ -37,6 +37,8 @@ import ( // the key external-dns actually watches is part of what these specs are for. const externalDNSAnnotation = "external-dns.alpha.kubernetes.io/hostname" +const remoteWriteURL = "http://vmsingle:8428/api/v1/write" + // These specs exercise the CRD schema itself — defaults, creation-time // validation and the CEL transition rules that pin the topology counts. They // never reconcile: the API server is the unit under test, which is exactly the @@ -88,6 +90,18 @@ var _ = Describe("MemgraphCluster CRD validation", func() { return stored } + // rawCluster is a cluster as an unstructured manifest, for the specs the + // typed client cannot express: omitempty drops an empty string before it + // reaches the API server, and a user can still write one in YAML. + rawCluster := func(name string, spec map[string]any) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": memgraphcomv1alpha1.SchemeGroupVersion.String(), + "kind": "MemgraphCluster", + "metadata": map[string]any{"name": name, "namespace": resourceNamespace}, + "spec": spec, + }} + } + // expectRejected asserts that admission rejected the spec as invalid and // that the message actually tells the user what to change. expectRejected := func(name string, spec memgraphcomv1alpha1.MemgraphClusterSpec, wantMessage string) { @@ -175,16 +189,11 @@ var _ = Describe("MemgraphCluster CRD validation", func() { // "no dynamic provisioning"; both must survive a round trip through the // API server as distinct values. It("should preserve the difference between an unset and an empty storage class", func() { - raw := &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": memgraphcomv1alpha1.SchemeGroupVersion.String(), - "kind": "MemgraphCluster", - "metadata": map[string]any{"name": "valid-empty-storage-class", "namespace": resourceNamespace}, - "spec": map[string]any{ - "storage": map[string]any{ - "data": map[string]any{"libStorageClassName": ""}, - }, + raw := rawCluster("valid-empty-storage-class", map[string]any{ + "storage": map[string]any{ + "data": map[string]any{"libStorageClassName": ""}, }, - }} + }) Expect(k8sClient.Create(ctx, raw)).To(Succeed()) DeferCleanup(func() { Expect(k8sClient.Delete(ctx, raw)).To(Succeed()) }) @@ -590,7 +599,7 @@ var _ = Describe("MemgraphCluster CRD validation", func() { Monitoring: &memgraphcomv1alpha1.MonitoringSpec{ VMAgentRemote: &memgraphcomv1alpha1.VMAgentRemoteSpec{ RemoteWrite: memgraphcomv1alpha1.RemoteWriteSpec{ - URL: "http://vmsingle:8428/api/v1/write", + URL: remoteWriteURL, BasicAuth: &memgraphcomv1alpha1.BasicAuthSecretSpec{}, }, }, @@ -601,7 +610,7 @@ var _ = Describe("MemgraphCluster CRD validation", func() { memgraphcomv1alpha1.MemgraphClusterSpec{ Monitoring: &memgraphcomv1alpha1.MonitoringSpec{ VMAgentRemote: &memgraphcomv1alpha1.VMAgentRemoteSpec{ - RemoteWrite: memgraphcomv1alpha1.RemoteWriteSpec{URL: "http://vmsingle:8428/api/v1/write"}, + RemoteWrite: memgraphcomv1alpha1.RemoteWriteSpec{URL: remoteWriteURL}, ExternalLabels: map[string]string{"cluster-name": "production"}, }, }, @@ -650,7 +659,50 @@ var _ = Describe("MemgraphCluster CRD validation", func() { memgraphcomv1alpha1.MemgraphClusterSpec{ SecurityContext: &memgraphcomv1alpha1.PodSecurityContextSpec{RunAsUser: ptr.To(int64(-1))}, }, - "should be greater than or equal to 0"), + "should be greater than or equal to 1"), + // The Memgraph container sets runAsNonRoot, so the kubelet would + // refuse to start it as root. + Entry("root as the uid", "invalid-root-uid", + memgraphcomv1alpha1.MemgraphClusterSpec{ + SecurityContext: &memgraphcomv1alpha1.PodSecurityContextSpec{RunAsUser: ptr.To(int64(0))}, + }, + "should be greater than or equal to 1"), + Entry("a zero lib claim", "invalid-zero-lib-size", + memgraphcomv1alpha1.MemgraphClusterSpec{ + Storage: memgraphcomv1alpha1.StorageSpec{ + Data: memgraphcomv1alpha1.RoleStorageSpec{LibPVCSize: ptr.To(resource.MustParse("0"))}, + }, + }, + "libPVCSize must be greater than zero"), + Entry("a negative core dumps claim", "invalid-negative-core-dumps-size", + memgraphcomv1alpha1.MemgraphClusterSpec{ + CoreDumps: memgraphcomv1alpha1.CoreDumpsSpec{ + Data: &memgraphcomv1alpha1.RoleCoreDumpsSpec{Size: ptr.To(resource.MustParse("-1Gi"))}, + }, + }, + "coreDumps size must be greater than zero"), + Entry("an uploader Secret that is not a Secret name", "invalid-uploader-env-from-secret", + memgraphcomv1alpha1.MemgraphClusterSpec{ + CoreDumps: memgraphcomv1alpha1.CoreDumpsSpec{ + Data: &memgraphcomv1alpha1.RoleCoreDumpsSpec{}, + Uploader: &memgraphcomv1alpha1.CoreDumpsUploaderSpec{ + Image: "amazon/aws-cli:2.33.28", + EnvFromSecrets: []string{"Uploader_Credentials"}, + }, + }, + }, + "spec.coreDumps.uploader.envFromSecrets[0]"), + Entry("an empty uploader Secret name", "invalid-uploader-env-from-empty", + memgraphcomv1alpha1.MemgraphClusterSpec{ + CoreDumps: memgraphcomv1alpha1.CoreDumpsSpec{ + Data: &memgraphcomv1alpha1.RoleCoreDumpsSpec{}, + Uploader: &memgraphcomv1alpha1.CoreDumpsUploaderSpec{ + Image: "amazon/aws-cli:2.33.28", + EnvFromSecrets: []string{""}, + }, + }, + }, + "should be at least 1 chars long"), // The chown target has to be named: a platform-assigned uid is not // known when the pod template is built. Entry("the ownership container beside an identity left to the platform", "invalid-fix-ownership-no-uid", @@ -907,12 +959,7 @@ var _ = Describe("MemgraphCluster CRD validation", func() { // YAML are submitted as a raw manifest instead. DescribeTable("should reject a blanked-out field of a hand-written manifest", func(name, block, field string) { - raw := &unstructured.Unstructured{Object: map[string]any{ - "apiVersion": memgraphcomv1alpha1.SchemeGroupVersion.String(), - "kind": "MemgraphCluster", - "metadata": map[string]any{"name": name, "namespace": resourceNamespace}, - "spec": map[string]any{block: map[string]any{field: ""}}, - }} + raw := rawCluster(name, map[string]any{block: map[string]any{field: ""}}) err := k8sClient.Create(ctx, raw) Expect(err).To(HaveOccurred(), "expected admission to reject the manifest") @@ -926,6 +973,23 @@ var _ = Describe("MemgraphCluster CRD validation", func() { Entry("an empty organization key", "invalid-raw-organization-key", "secrets", "organizationKey"), ) + // An empty scrape interval would be stored as written, since a schema + // default fills only a missing field, and the builder would then run + // vmagent on its own fallback the object never shows. + It("should reject a blanked-out vmagent scrape interval", func() { + raw := rawCluster("invalid-raw-scrape-interval", map[string]any{ + "monitoring": map[string]any{"vmagentRemote": map[string]any{ + "remoteWrite": map[string]any{"url": remoteWriteURL}, + "scrapeInterval": "", + }}, + }) + + err := k8sClient.Create(ctx, raw) + Expect(err).To(HaveOccurred(), "expected admission to reject the manifest") + Expect(apierrors.IsInvalid(err)).To(BeTrue(), "expected an Invalid error, got %v", err) + Expect(err.Error()).To(ContainSubstring("should be at least 1 chars long")) + }) + It("should accept external access with decorations on both roles", func() { stored := createAccepted("valid-external-access", memgraphcomv1alpha1.MemgraphClusterSpec{ ExternalAccess: &memgraphcomv1alpha1.ExternalAccessSpec{