Add validation for pgbouncer logfile#4280
Merged
benjaminjb merged 7 commits intoCrunchyData:mainfrom Sep 15, 2025
Merged
Conversation
benjaminjb
commented
Sep 11, 2025
| 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) |
Contributor
Author
There was a problem hiding this comment.
Missed this in an earlier PR re: OTEL handling user-set logfiles.
benjaminjb
commented
Sep 11, 2025
Comment on lines
+31
to
+32
| // +kubebuilder:validation:XValidation:rule=`!has(self.logfile) || self.logfile.endsWith('.log')`,message=`logfile config must end with '.log'` | ||
| // +kubebuilder:validation:MaxProperties=50 |
Contributor
Author
There was a problem hiding this comment.
How do we feel about adding these two validations/exclusions here to v1beta1?
Member
There was a problem hiding this comment.
👍🏻 These new rules/criteria seem fine to me.
dsessler7
reviewed
Sep 11, 2025
Comment on lines
+145
to
+148
| assert.NilError(t, features.SetFromMap(map[string]bool{ | ||
| feature.OpenTelemetryLogs: true, | ||
| feature.OpenTelemetryMetrics: true, | ||
| })) |
Collaborator
There was a problem hiding this comment.
👍 to setting it here since nothing happens until instrumentation is in the spec
cbandy
requested changes
Sep 11, 2025
Comment on lines
+31
to
+32
| // +kubebuilder:validation:XValidation:rule=`!has(self.logfile) || self.logfile.endsWith('.log')`,message=`logfile config must end with '.log'` | ||
| // +kubebuilder:validation:MaxProperties=50 |
Member
There was a problem hiding this comment.
👍🏻 These new rules/criteria seem fine to me.
10 tasks
This PR adds validation for v1 pgBouncer so that the logfile must either be in /tmp or one of the attached additional volumes. This PR also adds validation for v1beta1 that restricts the number of global config settings and requires a '.log' suffix on the logfile. (This may be reverted in the future as we are learning towards validation in v1 and documentation in v1beta1.) This PR also fixes an error in the OTEL setup around a custom logfile. Issues: [PGO-2565]
k8s 1.28 cannot handle the .? validation we wanted to use in CEL, so this PR uses a less complex version.
5.8+ has k8s 1.30 for its floor; OCP is 4.14 (k8s 1.27) but 4.16 (k8s 1.29) works with this CEL.
…ypes.go Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
benjaminjb
force-pushed
the
benjb/pgbouncer-log-validation
branch
from
2c08ff5 to
746c906
Compare
benjaminjb
commented
Sep 12, 2025
| // 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'` |
Contributor
Author
There was a problem hiding this comment.
Suggested change
| // +kubebuilder:validation:XValidation:rule=`!has(self.logfile) || self.logfile.endsWith('.log')`,message=`logfile config must end with '.log'` | |
| // +kubebuilder:validation:XValidation:rule=`self.?logfile.endsWith('.log')orValue(true)`,message=`logfile config must end with '.log'` |
Is something like this more legible?
Contributor
Author
There was a problem hiding this comment.
Ah, that doesn't work:
compilation failed: ERROR: <input>:1:23: found no matching overload for 'endsWith' applied to 'optional_type(string).(string)'")
But this works:
self.?logfile.orValue("").endsWith('.log')
Contributor
Author
There was a problem hiding this comment.
meh, for just one step, I'll do without the "?"
This path is just one step, so use the 'has' macro
cbandy
approved these changes
Sep 15, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds validation for v1 pgBouncer so that the logfile must either be in /tmp or one of the attached additional volumes.
This PR also adds validation for v1beta1 that restricts the number of global config settings and requires a '.log' suffix on the logfile. (This may be reverted in the future as we are learning towards validation in v1 and documentation in v1beta1.)
This PR also fixes an error in the OTEL setup around a custom logfile.
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
What is the new behavior (if this is a feature change)?
BREAKING: A cluster with more than 50 configs on pgbouncer or a logfile that didn't end in .log would be rejected by the API now.
Other Information:
Issues: [PGO-2565]