-
Notifications
You must be signed in to change notification settings - Fork 16.4k
pgbouncer exporter version update #19315
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
* update alpine base image to newer version * changes to default version of pgbouncer exporter build script * tag in values.yaml '
| LABEL maintainer="Apache Airflow Community <dev@airflow.apache.org>" | ||
|
|
||
| # Add a non root user | ||
| RUN addgroup -S pgbouncer_exporter \ |
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.
What do you think about support arbitrary user ids? https://docs.openshift.com/container-platform/4.9/openshift_images/create-images.html#images-create-guide-openshift_create-images
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.
we are defining userid's in securityContext: , if the securityContext is left black or id's are in range of openshift we wont have any issues.
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.
How about using user "nobody" (Like in the pgbouncer itself). I think this case is not the same as airflow - in airflow often people want to use different user or use the "arbitrary user id" to avoid some potential "user' clashes between containers running on the same machine and they evn might want to choose a specific ID because they can have volumes mounted with specific users etc. So it makes sense to support the "arbitrary user id". We also created a home directory there because airflow workers might spawn pretty much arbitrary operators (thus any external libraries/binaries) and many of those extarnal binaries/librraries might rely on the ${HOME} directory set and present (and writeable) - for example when they do any caching, they often do that in the ${HOME} dir.
But pgbouncer_exporter is really simple, "standalone" binary tool that is rather simple - it connects to pgbouncer, queries it for stats and exposes to prometheus via non-priviledged port (9127 which is > 1024) - so there is no need for any special priviledges, also it is unlikely it needs HOME directory (we can check that likely).
This is nobody user:
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
WDYT?
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.
If we use nobody, it makes a bit simpler.
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.
yeah both approaches are right and USER nobody option is simpler this service just exposes metrics on http endpoint. Thanks @potiuk
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 you make that change @pgvishnuram so we can merge this PR
|
ping @pgvishnuram Can you rebase the PR on the latest main and address the comment please |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
Problem:
How does this PR fix the problem above:
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.