Skip to content

fix: Reject zero and empty values admission used to let through #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
as51340 merged 1 commit into from
Oct 13, 2026
Merged

fix: Reject zero and empty values admission used to let through #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions api/v1alpha1/memgraphcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Expand Down Expand Up @@ -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"`
Expand Down Expand Up @@ -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"`
Expand Down Expand Up @@ -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"`

Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion charts/memgraph-operator/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
21 changes: 19 additions & 2 deletions charts/memgraph-operator/crds/memgraph.com_memgraphclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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: |-
Expand Down Expand Up @@ -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: |-
Expand Down
21 changes: 19 additions & 2 deletions config/crd/bases/memgraph.com_memgraphclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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: |-
Expand Down Expand Up @@ -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: |-
Expand Down
100 changes: 82 additions & 18 deletions internal/controller/memgraphcluster_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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()) })

Expand Down Expand Up @@ -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{},
},
},
Expand All @@ -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"},
},
},
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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")
Expand All @@ -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{
Expand Down
Loading