Add adminSecretFileRef as alternative to adminSecretRef for file-based credentials in ClusterConnection - #60
Conversation
…d credentials in ClusterConnection
|
Hi @fredordercloud thanks for the contribution, I'm reviewing this PR right now and will come back to you. |
There was a problem hiding this comment.
Thanks for the PR and for adding this feature and KubernetesServiceTest. The Secret path had no tests before.
We still have some work before we can merge this:
Blocking
- A missing
usernamein the file crashes the connection with aNullPointerException. Ausernameis always required for PostgreSQL. Reject a missing one with a clear error and document it as required. cluster-connection.mdis the reference page. Add the JSON file format there and move the mount instructions there from the dev environment page.- Remove the
gradle.propertieschange and the.gitignoreentries from this PR. They are not related to the feature. Open an issue that describes the Windows build problem, then we will fix it. - Remove "AWS Secrets Manager" from the error messages and the Javadoc. The operator reads a file. Where the file comes from does not matter.
Requested
- JSpecify
@Nullableon the two new spec fields, likeRoleSpec. - A CRD validation test for the new CEL rule, like
DefaultPrivilegeReconcilerTest. - Tests: remove the duplicate and the two overlapping dispatcher tests, use
@ParameterizedTestfor the missing/null cases, use the fabric8 mock server instead of mocking the client chain, add one end-to-end case with a file. - Naming and style:
var, blank lines,FileRef,getAdminCredentials.
On our side
- We will add
volumesandvolumeMountsvalues to the Helm chart in a follow-up PR. Add one sentence to the docs that the chart does not support this yet. - Enable ErrorProne rule
Varifierto throw an error
|
|
||
| #### Mount the credentials file | ||
|
|
||
| The file must be accessible inside the operator pod at the path specified in `adminSecretFileRef.path`. Mount it using a Volume and VolumeMount on the operator Deployment: |
There was a problem hiding this comment.
[follow-up] The operator is installed with the generated Helm chart, but the chart exposes no volumes or volumeMounts values.
So a chart user cannot mount this file without patching the rendered Deployment, and the feature is not usable through the documented install path.
This page also describes the local dev setup, where the operator runs on your machine and there is no pod to patch.
Fix: We will add app.volumes and app.volumeMounts chart values in a follow-up PR on our side, since quarkus-helm cannot add fields that Quarkus does not generate (quarkiverse/quarkus-helm#453).
|
|
||
| # Workaround for Windows: avoid forked process where -D args with {{ }} get mangled by cmd.exe | ||
| systemProp.gradle.quarkus.gradle-worker.no-process=true |
There was a problem hiding this comment.
This property is not related to the feature and it changes the build for everyone, including CI.
Quarkus reads gradle.quarkus.gradle-worker.no-process and then runs its build steps inside the Gradle daemon with classloader isolation instead of a separate process (QuarkusTask.java:58 in Quarkus 3.35.3).
Put it in your own ~/.gradle/gradle.properties, and open an issue that describes the problem with Windows instead (I am using a MacBook due to work), then we will fix it.
| # Workaround for Windows: avoid forked process where -D args with {{ }} get mangled by cmd.exe | |
| systemProp.gradle.quarkus.gradle-worker.no-process=true |
There was a problem hiding this comment.
@ThoSap I will open a new issue then, thanks.
… rename to FileRef, use fabric8 mock server, remove AWS references, updated unit tests and docs
|
Addresses all first-round feedback: FileRef rename, AWS refs removed, username required for fileRef, fabric8 mock server, parameterized tests, docs consolidated. |
There was a problem hiding this comment.
Almost there 🎉
Thanks for the fast turnaround. 15 of the first-round threads are resolved: the FileRef rename, the fabric8 mock server, the parameterized tests, the moved docs, the removed Gradle property and more.
I request changes once more for the points below. Each has an inline comment, and the carried-over ones link to the first-round comment.
Blocking
- The username check covers the file branch only. A Secret without
usernamestill reachesProperties.setPropertyand throws aNullPointerException. Move the check after both branches ingetAdminCredentials. - The first Javadoc line of
FileRefis cut off: "A reference to a file inside". - The docs still say
ResourceFileRefin the spec table and the heading, and the Deployment snippet names the containeroperatorinstead ofpostgresql-operator. - The duplicate test is still there (lines 199 and 248), and the end-to-end test with a file ref is still missing. The two new
AdminSecretExclusivitytests only check the rejection.
Small
- Inject the
ObjectMapper, checkisTextual()or switch toreadValue. The comment has a sketch. - Message wording: "Credentials file" as suggested, lowercase at line 75.
- Blank
pathtest case for the CEL rule. - Heading level and duplicated format text in
cluster-connection.md. - Move
**/out/into the IntelliJ section.
Still open from round one
- If you want and if it is still relevant, please open and describe the issue for the Windows issue, see #60 (comment).
On our side
- The Helm chart values for
volumesandvolumeMountscome in a follow-up PR. - The ErrorProne Varifier rule in a follow-up PR
| # Quinoa | ||
| .quinoa/ | ||
|
|
||
| **/out/ |
There was a problem hiding this comment.
Follow-up to #60 (comment). The pattern is generic now, thanks. Please move it into the "IntelliJ IDEA" section above (lines 13-22) instead of the end of the file.
@fredordercloud why was this needed in the first place, did you not use the Gradle tasks but the IntelliJ builder instead?
… Secret branch, improve tests and docs
|
Addressed PR review: inject ObjectMapper, fix Secret username NPE, use readValue with record, rename to FileRef in docs, fix error messages, add missing tests, restructure docs |
ThoSap
left a comment
There was a problem hiding this comment.
Well done, thanks again for your contribution 🔥
I take the liberty of applying a few small changes myself in a follow-up PR, so you do not have to do another round for them.
I will also open a different follow-up PR, which adds volumes and volumeMounts values to the Helm chart, because without them this feature cannot be used through helm install.
adminSecretFileRef as alternative to adminSecretRef for file-based credentials in ClusterConnection
Two files in #60 were committed with CRLF although .editorconfig asks for LF. Git now normalizes every text file to LF on commit and checks it out as LF on every platform, so the editor setting can no longer be bypassed. gradlew.bat keeps CRLF in the working tree. jar and png files are marked binary. Also fix the linguist-generated pattern: the jOOQ sources live under generated/src/, not src/generated/, so the old pattern matched nothing.
Summary
(e.g. via AWS Secrets Manager CSI driver)
Test plan