diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index 21d81125d..5fe0db4d0 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml @@ -14284,7 +14284,12 @@ spec: description: |- Settings that apply to the entire PgBouncer process. More info: https://www.pgbouncer.org/config.html + maxProperties: 50 type: object + x-kubernetes-map-type: granular + x-kubernetes-validations: + - message: logfile config must end with '.log' + rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')' users: additionalProperties: type: string @@ -16258,6 +16263,12 @@ spec: x-kubernetes-list-type: map type: object type: object + x-kubernetes-validations: + - message: config.global.logfile destination is restricted to + '/tmp/logs/pgbouncer/' or an existing additional volume + rule: self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/") + || (self.?volumes.additional.hasValue() && self.volumes.additional.exists(v, + f.startsWith("/volumes/" + v.name)))).orValue(true) required: - pgBouncer type: object @@ -33063,7 +33074,12 @@ spec: description: |- Settings that apply to the entire PgBouncer process. More info: https://www.pgbouncer.org/config.html + maxProperties: 50 type: object + x-kubernetes-map-type: granular + x-kubernetes-validations: + - message: logfile config must end with '.log' + rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')' users: additionalProperties: type: string diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index 613f0dbae..304749d3b 100644 --- a/internal/pgbouncer/reconcile.go +++ b/internal/pgbouncer/reconcile.go @@ -223,7 +223,7 @@ func Pod( if collector.OpenTelemetryLogsOrMetricsEnabled(ctx, inCluster) { collector.AddToPod(ctx, inCluster.Spec.Instrumentation, inCluster.Spec.ImagePullPolicy, inConfigMap, template, []corev1.VolumeMount{configVolumeMount}, string(inSecret.Data["pgbouncer-password"]), - []string{naming.PGBouncerLogPath}, true, true) + []string{logPath}, true, true) } } diff --git a/internal/pgbouncer/reconcile_test.go b/internal/pgbouncer/reconcile_test.go index fe0287931..ba964e1c8 100644 --- a/internal/pgbouncer/reconcile_test.go +++ b/internal/pgbouncer/reconcile_test.go @@ -142,6 +142,10 @@ func TestPod(t *testing.T) { t.Parallel() features := feature.NewGate() + assert.NilError(t, features.SetFromMap(map[string]bool{ + feature.OpenTelemetryLogs: true, + feature.OpenTelemetryMetrics: true, + })) ctx := feature.NewContext(context.Background(), features) cluster := new(v1beta1.PostgresCluster) @@ -463,6 +467,59 @@ containers: - mountPath: /etc/pgbouncer name: pgbouncer-config readOnly: true +- command: + - bash + - -ceu + - -- + - "monitor() {\n\nmkdir -p '/tmp/receiver' && { chmod 0775 '/tmp/receiver' || :; + }\nOTEL_PIDFILE=/tmp/otel.pid\n\nstart_otel_collector() {\n\techo \"Starting OTel + Collector\"\n\t/otelcol-contrib --config /etc/otel-collector/config.yaml &\n\techo + $! > $OTEL_PIDFILE\n}\nstart_otel_collector\n\nexec {fd}<> <(:||:)\nwhile read + -r -t 5 -u \"${fd}\" ||:; do\n\tlogrotate -s /tmp/logrotate.status /etc/logrotate.d/logrotate.conf\n\tif + [[ \"${directory}\" -nt \"/proc/self/fd/${fd}\" ]] && kill -HUP $(head -1 ${OTEL_PIDFILE?});\n\tthen\n\t\techo + \"OTel configuration changed...\"\n\t\texec {fd}>&- && exec {fd}<> <(:||:)\n\t\tstat + --format='Loaded configuration dated %y' \"${directory}\"\n\tfi\n\tif [[ ! -e + /proc/$(head -1 ${OTEL_PIDFILE?}) ]] ; then\n\t\tstart_otel_collector\n\tfi\ndone\n}; + export directory=\"$1\"; export -f monitor; exec -a \"$0\" bash -ceu monitor" + - collector + - /etc/otel-collector + env: + - name: K8S_POD_NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + - name: K8S_POD_NAME + valueFrom: + fieldRef: + fieldPath: metadata.name + - name: PGPASSWORD + imagePullPolicy: Always + name: collector + ports: + - containerPort: 9187 + name: otel-metrics + protocol: TCP + resources: {} + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + privileged: false + readOnlyRootFilesystem: true + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + volumeMounts: + - mountPath: /etc/pgbouncer + name: pgbouncer-config + readOnly: true + - mountPath: /etc/otel-collector + name: collector-config + readOnly: true + - mountPath: /etc/logrotate.d + name: logrotate-config + readOnly: true volumes: - name: pgbouncer-config projected: @@ -490,6 +547,20 @@ volumes: items: - key: ca.crt path: ~postgres-operator/backend-ca.crt +- name: logrotate-config + projected: + sources: + - configMap: + items: + - key: logrotate.conf + path: logrotate.conf +- name: collector-config + projected: + sources: + - configMap: + items: + - key: collector.yaml + path: config.yaml `)) }) @@ -498,6 +569,8 @@ volumes: "logfile": "/volumes/required/mylog.log", } logfile = "/volumes/required/mylog.log" + // Reset the instrumentation from the previous test + cluster.Spec.Instrumentation = nil call() diff --git a/internal/testing/validation/pgbouncer_test.go b/internal/testing/validation/pgbouncer_test.go new file mode 100644 index 000000000..3e6345a65 --- /dev/null +++ b/internal/testing/validation/pgbouncer_test.go @@ -0,0 +1,168 @@ +// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package validation + +import ( + "context" + "testing" + + "gotest.tools/v3/assert" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/crunchydata/postgres-operator/internal/testing/require" + v1 "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1" +) + +func TestV1PGBouncerLogging(t *testing.T) { + ctx := context.Background() + cc := require.Kubernetes(t) + t.Parallel() + + namespace := require.Namespace(t, cc) + + base := v1.NewPostgresCluster() + base.Namespace = namespace.Name + base.Name = "pgbouncer-logging" + // required fields + require.UnmarshalInto(t, &base.Spec, `{ + postgresVersion: 16, + instances: [{ + dataVolumeClaimSpec: { + accessModes: [ReadWriteOnce], + resources: { requests: { storage: 1Mi } }, + }, + }], + }`) + + assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll), + "expected this base to be valid") + + t.Run("Can set logging on tmp with .log", func(t *testing.T) { + tmp := base.DeepCopy() + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/logs/pgbouncer/log.log" + } + } + } + }`) + assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll), + "expected this option to be valid") + }) + + t.Run("Cannot set logging on tmp without .log", func(t *testing.T) { + tmp := base.DeepCopy() + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/logs/pgbouncer/log.txt" + } + } + } + }`) + + err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile config must end with '.log'") + }) + + t.Run("Cannot set logging on tmp without correct subdir", func(t *testing.T) { + tmp := base.DeepCopy() + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/logs/log.log" + } + } + } + }`) + + err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/pgbouncer/log.log" + } + } + } + }`) + + err = cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + }) + + t.Run("Cannot set logging on volumes that don't exist", func(t *testing.T) { + vol := base.DeepCopy() + + require.UnmarshalInto(t, &vol.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/volumes/logging/log.log" + } + } + } + }`) + + err := cc.Create(ctx, vol.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + }) + + t.Run("Cannot set logging elsewhere", func(t *testing.T) { + vol := base.DeepCopy() + + require.UnmarshalInto(t, &vol.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/var/log.log" + } + } + } + }`) + + err := cc.Create(ctx, vol.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + }) + + t.Run("Can set logging on volumes that exist", func(t *testing.T) { + vol := base.DeepCopy() + + require.UnmarshalInto(t, &vol.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/volumes/logging/log.log" + } + }, + volumes: { + additional: [ + { + name: logging, + claimName: required-1 + }] + } + } + }`) + + assert.NilError(t, cc.Create(ctx, vol.DeepCopy(), client.DryRunAll), + "expected this option to be valid") + }) +} diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go new file mode 100644 index 000000000..48f71f017 --- /dev/null +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go @@ -0,0 +1,15 @@ +// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package v1 + +import ( + "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" +) + +// PGBouncerPodSpec defines the desired state of a PgBouncer connection pooler. +// +kubebuilder:validation:XValidation:rule=`self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/") || (self.?volumes.additional.hasValue() && self.volumes.additional.exists(v, f.startsWith("/volumes/" + v.name)))).orValue(true)`,message=`config.global.logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume` +type PGBouncerPodSpec struct { + v1beta1.PGBouncerPodSpec `json:",inline"` +} diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go index 6dc58bde0..2c3b9c411 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go @@ -613,7 +613,7 @@ type PostgresInstanceSetStatus struct { type PostgresProxySpec struct { // Defines a PgBouncer proxy and connection pooler. - PGBouncer *v1beta1.PGBouncerPodSpec `json:"pgBouncer"` + PGBouncer *PGBouncerPodSpec `json:"pgBouncer"` } // Default sets the defaults for any proxies that are set. diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go index 4fd20b671..80043ab76 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go @@ -182,6 +182,22 @@ func (in *MonitoringStatus) DeepCopy() *MonitoringStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PGBouncerPodSpec) DeepCopyInto(out *PGBouncerPodSpec) { + *out = *in + in.PGBouncerPodSpec.DeepCopyInto(&out.PGBouncerPodSpec) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PGBouncerPodSpec. +func (in *PGBouncerPodSpec) DeepCopy() *PGBouncerPodSpec { + if in == nil { + return nil + } + out := new(PGBouncerPodSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresCluster) DeepCopyInto(out *PostgresCluster) { *out = *in @@ -623,7 +639,7 @@ func (in *PostgresProxySpec) DeepCopyInto(out *PostgresProxySpec) { *out = *in if in.PGBouncer != nil { in, out := &in.PGBouncer, &out.PGBouncer - *out = new(v1beta1.PGBouncerPodSpec) + *out = new(PGBouncerPodSpec) (*in).DeepCopyInto(*out) } } diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go index 49e713c17..9632ecae1 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go @@ -26,7 +26,14 @@ type PGBouncerConfiguration struct { // Settings that apply to the entire PgBouncer process. // More info: https://www.pgbouncer.org/config.html + // --- + // # Logging + // +kubebuilder:validation:XValidation:rule=`!has(self.logfile) || self.logfile.endsWith('.log')`,message=`logfile config must end with '.log'` + // +kubebuilder:validation:MaxProperties=50 + // See also XValidation rule on v1 PostgresProxySpec + // // +optional + // +mapType=granular Global map[string]string `json:"global,omitempty"` // PgBouncer database definitions. The key is the database requested by a diff --git a/pkg/apis/postgres-operator.crunchydata.com/validation.md b/pkg/apis/postgres-operator.crunchydata.com/validation.md index e1e383931..49a243d4c 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/validation.md +++ b/pkg/apis/postgres-operator.crunchydata.com/validation.md @@ -95,3 +95,17 @@ The `additionalProperties` property indicates that the keys are unknown; these f > When possible, use [OpenAPI properties](#FIXME) rather than CEL rules. > The former do not affect the CRD [validation budget](#FIXME). +## Optional field syntax + +CEL offers a safe traversal/retrieval through the use of `?` as an [optional field marker]. + +As an example, when attempting to retrieve `self.config.global.logfile`, in older (but still supported) +versions of k8s, we may need to incrementally check the path: `has(self.config) && has(self.config.global)...` + +With the optional field marker, we can use that field safely without checking the entire path: +`self.?config.global.logfile` will return a value or no value; and anything using that value will +likewise be considered optional. + +The optional field syntax is only available in K8s 1.29+. + +[optional field marker]: https://pkg.go.dev/github.com/google/cel-go/cel#hdr-Syntax_Changes-OptionalTypes.