-
Notifications
You must be signed in to change notification settings - Fork 629
Validate and use pgbouncer logfile config #4252
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.
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
Conversation
internal/util/pod_security.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I think I'd prefer the caller to pass in fsgroup = 0 when they know its OpenShift, but a single place with this knowledge is good, too.
🤔 🤔 PgBouncer is deployed from a PostgresCluster spec which has an isOpenShift override.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on PodSecurityContext, fsgroup is a pointer (with omitempty) so we don't want to set it at all, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I was picturing L33 as if fsgroup > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, updated to try it out -- is that clearer/easier to use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Where does the file go if there is no directory here? What if there's a relative directory?
96d286f to
0d8c35c
Compare
pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ can this be on the PGBouncerPodSpec godoc?
internal/util/pod_security.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I was picturing L33 as if fsgroup > 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Is this any more legible? I have no idea.
self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/") || self.?volumes.additional.optMap(a, a.exists(v, f.startsWith("/volumes/" + v.name))).orValue(true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 a is only referenced once, so maybe ditching an optional is better.
-additional.optMap(a, a.exists(v, +additional.orValue([]).exists(v,
self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/") || self.?volumes.additional.orValue([]).exists(v, f.startsWith("/volumes/" + v.name))).orValue(true)
...ypes.go Co-authored-by: Chris Bandy <bandy.chris@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
With OTEL and the new additional volume settings, we want to allow users to set their pgbouncer
log config without breaking OTEL.
(This version does not have a lot of guardrails guiding the user to find a safe setting. We may want to
consider that or punt this idea to a new ticket.)
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
Users can set any logfile config for pgbouncer, which might interfere with our OTEL
What is the new behavior (if this is a feature change)?
We now restrict users to only set a logfile with the
.logsuffix; and we get the config to handle the OTEL-related logicTODO: Restrict the logfile to only certain locations?
Other Information:
Issues: [PGO-2565]