fix(api): make pullSecrets and the Server pull policy take effect - #576
Merged
Merged
Conversation
Three fields were declared and never reached a pod. ImageSpec.PullSecrets was read nowhere. The only imagePullSecrets on any pod came from code image entries, so a private registry for the server, database or CA setup image could not work at all. All three pod specs now carry them, deduplicated against the code image secrets. Server.image.pullPolicy was ignored: both container specs read cfg.Spec.Image.PullPolicy unconditionally. Fixing that required removing the nested kubebuilder default, for the same reason #549 removed it from repository and tag: a nested default is materialised into every Server, so the field was never empty and could not express inherit. The fallback now lives in code - Server, then Config, then IfNotPresent. ConditionSSLBootstrapped existed as a constant and as a promise in docs/reference/server.md, but nothing ever set it. It now reports whether the referenced Certificate is usable, which also gives the Server a visible status while it waits: a missing or unsigned Certificate previously left no trace at all, so nothing happens looked the same as nothing is wrong. Documented the resolution rules, including that pullSecrets on a Server replaces the Config's list rather than extending it.
This was referenced Sep 3, 2026
slauger
added a commit
that referenced
this pull request
Sep 3, 2026
README and the feature list promise 'Heap size calculated from memory limits (90%) - no manual -Xmx tuning needed'. The controller does contain that calculation, but it is unreachable: ServerSpec.JavaArgs carries a CRD default, so the field is never empty and the first branch of resolveJavaArgs always wins. Every Server runs with -Xms512m -Xmx1024m no matter how much memory it is given. Replaced the claim with what actually applies, and noted it on the field in the Server reference with a link to #592, where the fix is tracked. Once the default is removed the derivation works and the wording can go back. Third instance of the same pattern after #550 and #576: a CRD default takes away the empty state a fallback depends on.
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
1 participant
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.
Three fields from the second review pass that were declared but never reached a pod. Each test was run against the old code first; eight of the ten fail there.
pullSecrets reached nothing
ImageSpec.PullSecretswas read nowhere. The onlyimagePullSecretson any pod came from code image entries (server_deployment.go), so a private registry for the server, database or CA setup image could not work at all -- the field existed on all three CRDs and did nothing.All three pod specs now carry them, deduplicated against the code image secrets, which can point at a different registry.
Server.image.pullPolicy was ignored
Both container specs read
cfg.Spec.Image.PullPolicyunconditionally, so the field on the Server did nothing.Fixing it required removing the nested kubebuilder default, for the same reason #549 removed it from
repositoryandtag: a nested default is materialised into every Server whether or not the parent object was specified, so the field was never empty and could not express "inherit". The fallback now lives in code:Upgrade note. Existing Servers carry
pullPolicy: IfNotPresentmaterialised from the old default, so they will keep overriding the Config until the field is cleared. Same shape as therepository/tagnote in #540. For chart users ahelm upgradeclears it.SSLBootstrapped was a promise, not a condition
ConditionSSLBootstrappedexisted as a constant and was documented indocs/reference/server.mdas one of the two Server conditions, but nothing ever set it.It now reports whether the referenced Certificate is usable:
CertificateSignedCertificateNotFoundcertificateRefpoints at nothingCertificateNotSignedThat also closes a gap several review perspectives found independently: a Server waiting for a dependency left no trace in its status, so "nothing happens" looked the same as "nothing is wrong".
Also
Documented the resolution rules in the Config and Server references, including that
pullSecretson a Server replaces the Config's list rather than extending it -- deliberate, so a Server pulling from another registry does not drag the Config's credentials along.make test,make manifestsand golangci-lint 2.13.2 are clean.Not included
The cluster-wide unfiltered Secret cache from the same review is a separate change (informer scoping plus an uncached reader for user-supplied Secrets) and gets its own issue.