-
Notifications
You must be signed in to change notification settings - Fork 0
AB-449 PostgreSQL Operator: Project Setup + ClusterConnection and Role CRD
#1
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
AB-449 PostgreSQL Operator
Based on the research and PoC done in the ticket and PR https://github.com/aboutbits/minio-operator/pull/1 We most likely will use CloudNativePG AB-396 for the DB Setup and this operator to manage the access, e.g. PostgreSQL roles and grants. The scope of the operator is:
|
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.
Pull request overview
This PR introduces a PostgreSQL Operator for Kubernetes, implementing custom resource definitions (CRDs) and reconcilers to manage PostgreSQL cluster connections and database roles. The operator enables declarative management of PostgreSQL resources through Kubernetes manifests.
Key changes:
- Implements
ClusterConnectionandRoleCRDs with their respective reconcilers for managing PostgreSQL connections and database roles - Adds comprehensive test infrastructure with test data builders and integration tests
- Sets up the Quarkus-based operator framework with Helm chart generation, health checks, and configuration management
Reviewed changes
Copilot reviewed 46 out of 71 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/it/aboutbits/postgresql/crd/role/* | Core Role CRD implementation including reconciler, spec, and utility classes for managing PostgreSQL roles |
| src/main/java/it/aboutbits/postgresql/crd/connection/* | ClusterConnection CRD implementation for managing PostgreSQL cluster connections |
| src/main/java/it/aboutbits/postgresql/core/* | Shared core classes including status management, authentication utilities, and base reconciler |
| src/test/java/it/aboutbits/postgresql/crd/*Test.java | Integration tests for Role and ClusterConnection reconcilers |
| src/test/java/it/aboutbits/postgresql/_support/* | Test infrastructure including test data creators and builders |
| src/main/resources/application*.yml | Application configuration for different environments (dev, test, prod) |
| gradle/*, build files | Gradle build configuration with dependencies and plugin setup |
| Docker files, Makefile, README.md | Deployment artifacts and documentation |
Files not reviewed (6)
- .idea/checkstyle-idea.xml: Language not supported
- .idea/codeStyles/Project.xml: Language not supported
- .idea/codeStyles/codeStyleConfig.xml: Language not supported
- .idea/misc.xml: Language not supported
- .idea/sqldialects.xml: Language not supported
- .idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/it/aboutbits/postgresql/_support/testdata/base/TestDataCreator.java
Outdated
Show resolved
Hide resolved
src/main/java/it/aboutbits/postgresql/MinioInstanceReadinessCheck.java
Outdated
Show resolved
Hide resolved
src/main/java/it/aboutbits/postgresql/core/ClusterReference.java
Outdated
Show resolved
Hide resolved
…y default (will also be excluded from stats and diffs)
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.
Pull request overview
Copilot reviewed 49 out of 74 changed files in this pull request and generated 1 comment.
Files not reviewed (6)
- .idea/checkstyle-idea.xml: Language not supported
- .idea/codeStyles/Project.xml: Language not supported
- .idea/codeStyles/codeStyleConfig.xml: Language not supported
- .idea/misc.xml: Language not supported
- .idea/sqldialects.xml: Language not supported
- .idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
alexlanz
left a comment
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.
Really nice work Thomas! I left some comments where I had some questions. Overall I want to discuss with you and Martin the following points:
- You introduced JOOQ. What is the reason for it and are these operations not possible with our existing tools? Did you discuss this already with Martin?
- Do we really need that much flags for the roles? What is the reason why you did not start with just the basic parameters that we need? Do we really need all of these? I personally am a fan of starting simple and add things when we need it. I think we don't need 90% of the flags. But at the end if we have them than we need to implement and maintain them. What are your thoughts here?
| @@ -0,0 +1,15 @@ | |||
| name: Test | |||
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.
Here you used the structure/approach of our project pipelines. But wouldn't it be more appropriate to build this repository similar to a package where we have the publishing workflows?
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.
Yes, this makes sense. I will switch it to a publishing workflow, as we did it for other Java libs.
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 left it for now as is. I will change this workflow in a follow-up PR.
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.
Makes sense.
src/main/resources/application.yml
Outdated
| # Additionally to Prometheus, we could also consume the metrics in JSON format | ||
| # by calling /q/metrics with the HTTP header "Accept: application/json" | ||
| json: | ||
| enabled: true | ||
| datasource: |
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 is this for? Do we need this right now? What benefit does it give us?
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 don't need it, it is only handy if you want to look at the Prometheus metrics in a JSON format instead of the Prometheus format.
I will remove it.
| # NOTE that this option is only considered when *not* in production mode | ||
| # as applying the CRD to a production cluster could be dangerous if done automatically. | ||
| apply: true | ||
| # Whether controllers should only process events if the associated resource generation |
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 please explain this to me?
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.
Starting the Operator when it was built (e.g. running the resulting container image) will not automatically apply the CRDs, but the generated Helm chart would do that.
The reason for this being potentially dangerous is only when you have the operator + CRD already running, and you introduce a breaking change, which could damage an existing PostgreSQL DB roles/grants/databases.
We could create two Helm charts as MinIO did in their Helm charts, one for the Operator itself and one for the CRDs, but I think this is not necessary in our case.
The second option generation-aware only lets the reconciler run if a kubectl apply or a server-side apply PATCH operation increases the metadata.generation number.
| # "io.quarkiverse.operatorsdk": | ||
| # level: DEBUG | ||
|
|
||
| # Config for the generated Helm chart # |
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 please explain to me why we need inside the Java application information about Helm charts? I don't understand this part.
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.
Quarkus Helm generates a fully functioning Helm chart with the generated CRDs for us.
We can publish this Helm chart as a *.tgz file or an OCI image in the GitHub release assets, download it in a GitHub workflow, and helm install it.
We could also publish it here if we want https://operatorhub.io/
| clusterRef.getName() | ||
| )); | ||
|
|
||
| return UpdateControl.patchStatus(resource) |
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.
Uh, this is nice!
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.
It is really cool that the Java Operator SDK allows us to do something like this 🔥
Thanks!
Unfortunately I did not discuss using jOOQ with Martin beforehand, I'm sorry for that :(
SummarizingHibernate / Plain JDBC
jOOQ
|
| datasource: | ||
| devservices: | ||
| enabled: true | ||
| image-name: postgres:17.7 |
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 always need to be careful when we use new features of Postgres, so that we don't break old versions.
It would actually be nice to have integration tests against the Postgres versions that we support. Currently we have 15 and 16 running from what I could quickly see in Notion.
Maybe something we should consider in a second PR. 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.
For the then planned EMVA/Aichner K3s migration in August 2025 Peter Moser already evaluated PostgreSQL 17 for Alex, this is the reason I started with 17.
I did not use 18 as it is too new.
It was already my intention to use the Quarkus Compose Dev Services, where I can configure multiple different PostgreSQL major versions and run the full test suite against them by launching the tests in dedicated version test profiles.
https://quarkus.io/guides/compose-dev-services
I did not find the time yet in this PR as I wanted to let you review first before I finish the whole thing already ^^
I will do it in a follow-up PR.
See https://github.com/aboutbits/minio-operator/pull/1#issuecomment-3648539800

src/main/java/it/aboutbits/postgresql/core/PostgreSQLAuthenticationUtil.java
Outdated
Show resolved
Hide resolved
…ole CR instance is deleted (also in the tests)
… rule so it is immutable
…s the role from the database
|
For an example on how to test it manually without using JUnit Tests see |
| @@ -0,0 +1,15 @@ | |||
| name: Test | |||
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.
Makes sense.
README.md
Outdated
|
|
||
| Afterward, the project can be started in IntelliJ by navigating to `Run` -> `Run '...'`. | ||
|
|
||
| ## Test the CRD on the Dev Services cluster and AI assistance |
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 have some concerns about the AI part of this guide. Several tasks mention something like "Ask AI ...". This is something that for someone performing the steps can be challenging for several reasons:
- The person does not know exactly how to write the prompt so that he/she gets the right result.
- The person does not know if the result is right. He/she has nothing to compare with.
I personally would rather paste example results and maybe only provide a hint how the person can make use of AI to get to the result quicker.
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.
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.
Fixed
mmalfertheiner
left a comment
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.
LGTM. Just added a few non blocking comments.
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.
Just a general question: Do we need all three Dockerfiles? Are they handwritten by you?
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.
No, we don't.
These are automatically generated by https://code.quarkus.io/
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 removed the native Dockerfiles.
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.
Wdyt about renaming the package from connection to clusterConnection?
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.
Yes, I already thought about that, and in the end, it makes sense.
I left it last week, as I only named the CRD ClusterConnection like it is, as in the Java world Connection is already used by java.sql and many others.
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.
Renamed it.
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.
The Java Operator SDK apparently does not like underscores in the CRD package name at all, the tests fail with the underscore.
Fix: 1343797 (#1)
|
|
||
| @NullMarked | ||
| @Slf4j | ||
| public abstract class BaseReconciler<CR extends CustomResource<?, S> & Named, S extends CRStatus> { |
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.
Lets see how this class evolves. I think it could also be a mix of services and utils instead of an abstract class. For now I would leave it as is and if it starts to feel weird you could reconsider it.
….postgresql.crd.cluster_connection
alexlanz
left a comment
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.
Nice job! Thanks for updating the readme.
ClusterConnection and Role CRD
ClusterConnection and Role CRDClusterConnection and Role CRD


Warning
Work in Progress
The Grant and Database CRD will follow in a follow-up PR
https://www.postgresql.org/docs/17/sql-createrole.html
https://www.postgresql.org/docs/17/sql-alterrole.html
https://www.postgresql.org/docs/17/sql-grant.html
I have not yet put in the effort to clean up and refactor the generated
README.mdfrom https://code.quarkus.io/.I will do that in the follow-up PR!
I still decided to use Lombok, as without it, we had a lot of boilerplate code.
I can show you if I delombok the files.
I also did not yet implement the cleanup/deleting callback of the CRD by implementing theCleaner<T>in the*Reconcilerimplementation. I will also do that in the follow-up PR.Update: This is done now in this PR, so the tests are repeatable in the continuous quarkusDev testing mode.