From a57cf74726775049ff67c873f943c205abe251ca Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Fri, 4 Sep 2026 01:42:02 +0200 Subject: [PATCH 1/2] fix(api): let javaArgs be empty so the heap can follow the memory limit resolveJavaArgs sizes the JVM heap at 90 percent of the pod's memory limit when javaArgs is unset. The CRD default made that unreachable: the field was never empty, the explicit branch always won, and every Server ran with -Xms512m -Xmx1024m no matter how much memory it was given. A Server with 8Gi used 1Gi of it. Third instance of one mistake after #550 and #576. A nested or plain default removes the empty state a fallback depends on, so the default belongs in the chart where it is visible, not in the CRD where it disables code. The existing unit tests did not catch this and could not: they call resolveJavaArgs directly and therefore pass whether or not the field can ever be empty in a real cluster. Added the round-trip through the API server instead, which fails against the old CRD with the materialised value in the message. Restores the auto-tuning statement in the README and the feature list, which I had corrected to match the broken behaviour rather than the intended one. Closes #592 --- README.md | 2 +- api/v1alpha1/javaargs_default_test.go | 36 +++++++++++++++++++ api/v1alpha1/server_types.go | 6 +++- .../crds/openvox.voxpupuli.org_servers.yaml | 9 +++-- .../bases/openvox.voxpupuli.org_servers.yaml | 9 +++-- docs/_snippets/features.md | 2 +- docs/reference/server.md | 2 +- 7 files changed, 58 insertions(+), 8 deletions(-) create mode 100644 api/v1alpha1/javaargs_default_test.go diff --git a/README.md b/README.md index be8ed47a..9ca9d581 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ A Kubernetes Operator that maps [OpenVox Server](https://github.com/OpenVoxProje - 🔄 **Multi-Version Deployments** - Run different server versions side by side - canary deployments, rolling upgrades - 🔒 **Rootless & OpenShift Ready** - Random UID compatible, no root, no ezbake, no privilege escalation - ðŸŠķ **Minimal Image** - UBI9-based, no agent Ruby, no ezbake packaging - smaller footprint, fewer updates -- 🧠 **JVM sizing** - Set `javaArgs` per Server or Database; see [Server](docs/reference/server.md) for the current default +- 🧠 **Auto-tuned JVM** - Heap derived from the pod memory limit (90%) unless `javaArgs` is set - ðŸ“Ķ **OCI Image Volumes** - Package Puppet code as OCI images, deploy immutably with automatic rollout (K8s 1.35+) - 🌐 **Gateway API** - SNI-based TLSRoute support - share a single LoadBalancer across environments via TLS passthrough - 🗄ïļ **Managed OpenVox DB** - Deploy OpenVox DB (PuppetDB) with external PostgreSQL - TLS, config, and credentials managed by the operator diff --git a/api/v1alpha1/javaargs_default_test.go b/api/v1alpha1/javaargs_default_test.go new file mode 100644 index 00000000..796e0e4f --- /dev/null +++ b/api/v1alpha1/javaargs_default_test.go @@ -0,0 +1,36 @@ +package v1alpha1 + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// TestServerJavaArgsHasNoDefault is the test that was missing while the bug +// existed. The unit tests around resolveJavaArgs all call the function +// directly and therefore pass whether or not the field can ever be empty; only +// a round-trip through the API server shows that. +// +// A default here is not cosmetic: the controller derives the heap from the +// pod's memory limit exactly when javaArgs is empty, so a default silently +// pins every Server to the same heap. +func TestServerJavaArgsHasNoDefault(t *testing.T) { + ctx := context.Background() + + server := &Server{ + ObjectMeta: metav1.ObjectMeta{GenerateName: "test-server-", Namespace: "default"}, + Spec: ServerSpec{ + ConfigRef: "production", + CertificateRef: "production-cert", + }, + } + if err := k8sClient.Create(ctx, server); err != nil { + t.Fatalf("creating Server: %v", err) + } + t.Cleanup(func() { _ = k8sClient.Delete(ctx, server) }) + + if server.Spec.JavaArgs != "" { + t.Errorf("javaArgs must come back empty so the heap can be derived, got %q", server.Spec.JavaArgs) + } +} diff --git a/api/v1alpha1/server_types.go b/api/v1alpha1/server_types.go index 45d9bb68..805128b9 100644 --- a/api/v1alpha1/server_types.go +++ b/api/v1alpha1/server_types.go @@ -82,7 +82,11 @@ type ServerSpec struct { Resources corev1.ResourceRequirements `json:"resources,omitempty"` // JavaArgs defines the JVM arguments. - // +kubebuilder:default="-Xms512m -Xmx1024m" + // + // There is deliberately no default. A defaulted field is never empty, and + // the controller uses emptiness to decide whether to derive the heap from + // the pod's memory limit. With a default in place that derivation is dead + // code and every Server runs on the same heap regardless of its limit. // +optional JavaArgs string `json:"javaArgs,omitempty"` diff --git a/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml b/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml index 9712cfb0..7de5904f 100644 --- a/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml +++ b/charts/openvox-operator/crds/openvox.voxpupuli.org_servers.yaml @@ -3428,8 +3428,13 @@ spec: type: string type: object javaArgs: - default: -Xms512m -Xmx1024m - description: JavaArgs defines the JVM arguments. + description: |- + JavaArgs defines the JVM arguments. + + There is deliberately no default. A defaulted field is never empty, and + the controller uses emptiness to decide whether to derive the heap from + the pod's memory limit. With a default in place that derivation is dead + code and every Server runs on the same heap regardless of its limit. type: string maxActiveInstances: default: 1 diff --git a/config/crd/bases/openvox.voxpupuli.org_servers.yaml b/config/crd/bases/openvox.voxpupuli.org_servers.yaml index 9712cfb0..7de5904f 100644 --- a/config/crd/bases/openvox.voxpupuli.org_servers.yaml +++ b/config/crd/bases/openvox.voxpupuli.org_servers.yaml @@ -3428,8 +3428,13 @@ spec: type: string type: object javaArgs: - default: -Xms512m -Xmx1024m - description: JavaArgs defines the JVM arguments. + description: |- + JavaArgs defines the JVM arguments. + + There is deliberately no default. A defaulted field is never empty, and + the controller uses emptiness to decide whether to derive the heap from + the pod's memory limit. With a default in place that derivation is dead + code and every Server runs on the same heap regardless of its limit. type: string maxActiveInstances: default: 1 diff --git a/docs/_snippets/features.md b/docs/_snippets/features.md index 68aa6cc0..c7a61bba 100644 --- a/docs/_snippets/features.md +++ b/docs/_snippets/features.md @@ -6,7 +6,7 @@ - 🔄 **Multi-Version Deployments** - Run different server versions side by side - canary deployments, rolling upgrades - 🔒 **Rootless & OpenShift Ready** - Random UID compatible, no root, no ezbake, no privilege escalation - ðŸŠķ **Minimal Image** - UBI9-based, no agent Ruby, no ezbake packaging - smaller footprint, fewer updates -- 🧠 **JVM sizing** - Set `javaArgs` per Server or Database; see [Server](reference/server.md) for the current default +- 🧠 **Auto-tuned JVM** - Heap derived from the pod memory limit (90%) unless `javaArgs` is set - ðŸ“Ķ **OCI Image Volumes** - Package Puppet code as OCI images, deploy immutably with automatic rollout (K8s 1.35+) - 🌐 **Gateway API** - SNI-based TLSRoute support - share a single LoadBalancer across environments via TLS passthrough - 🗄ïļ **Managed OpenVox DB** - Deploy OpenVox DB (PuppetDB) with external PostgreSQL - TLS, config, and credentials managed by the operator diff --git a/docs/reference/server.md b/docs/reference/server.md index 0995066c..f58641ee 100644 --- a/docs/reference/server.md +++ b/docs/reference/server.md @@ -33,7 +33,7 @@ spec: | `replicas` | int32 | `1` | Number of pod replicas | | `autoscaling` | [AutoscalingSpec](#autoscalingspec) | - | HPA configuration | | `resources` | ResourceRequirements | - | CPU/memory requests and limits | -| `javaArgs` | string | `-Xms512m -Xmx1024m` | JVM arguments. The controller can derive the heap from the memory limit, but the CRD default makes that path unreachable today - see [#592](https://github.com/slauger/openvox-operator/issues/592). Set this explicitly to size the heap | +| `javaArgs` | string | *(derived)* | JVM arguments. Unset derives the heap from the pod's memory limit (90%), falling back to `-Xms512m -Xmx1024m` when no limit is set | | `maxActiveInstances` | int32 | `1` | Number of JRuby instances per pod | | `code` | [[]CodeSpec](index.md#codespec) | - | Override the Config's code sources (replace, not merge). A list; see [CodeSpec](index.md#codespec) | | `topologySpreadConstraints` | []TopologySpreadConstraint | - | Pod spread constraints across topology domains | From 8d827856a68fd08803f1af4c94f2e74d0ab5a8b5 Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Fri, 4 Sep 2026 01:45:48 +0200 Subject: [PATCH 2/2] fix(ca): apply signing policy changes without restarting the CA Every SigningPolicy edit rolled the CA pod. The policy Secret was mounted with SubPath, which the kubelet never refreshes, so the operator compensated with a hash annotation on the pod template. Because the CA Deployment uses the Recreate strategy, that meant a short outage - no signing, no CRL - for a change that alters no running state. On a VM-based Puppet, editing autosign configuration has no serving impact at all. Nothing downstream required the restart: openvox-autosign is executed per CSR and reads the file each time. Only the mount did. Mount the Secret as a directory instead, the way the CRL Secret already is for the same reason, and pass the file to the binary with --config. The hash annotation is gone, so a policy edit no longer touches the pod template. The trade-off is timing: a change now takes effect within the kubelet sync period, up to about a minute, rather than immediately after a restart. For a rule deciding which CSRs get signed, that is the better end of the trade. The former test asserted the annotation exists; it now asserts the opposite and that the mount carries no SubPath, since a SubPath would silently reintroduce the whole problem. The rendered puppet.conf is checked for the config path too - a wrong one denies every CSR and would otherwise only surface in an end-to-end run. Closes #588 --- docs/concepts/config-rollout.md | 17 ++++++++-- internal/controller/config_autosign.go | 9 ++++++ internal/controller/config_controller_test.go | 6 ++++ internal/controller/config_rendering.go | 12 +++---- internal/controller/server_controller_test.go | 31 ++++++++++++++++--- internal/controller/server_deployment.go | 17 +++++----- 6 files changed, 69 insertions(+), 23 deletions(-) diff --git a/docs/concepts/config-rollout.md b/docs/concepts/config-rollout.md index 9f36c0f9..ec60e256 100644 --- a/docs/concepts/config-rollout.md +++ b/docs/concepts/config-rollout.md @@ -15,7 +15,6 @@ Tracked annotations: | `openvox.voxpupuli.org/ca-secret-hash` | CA Secret (`{ca}-ca`) | Yes | | `openvox.voxpupuli.org/enc-secret-hash` | ENC Secret (`{config}-enc`) | Yes | | `openvox.voxpupuli.org/report-webhook-secret-hash` | Report webhook Secret (`{config}-report-webhook`) | Yes | -| `openvox.voxpupuli.org/autosign-policy-secret-hash` | Autosign policy Secret (`{ca}-autosign-policy`, CA pods only) | Yes | | `openvox.voxpupuli.org/code-image` | Code OCI image reference | Yes | ## What Triggers a Restart @@ -88,8 +87,20 @@ Creating or updating a SigningPolicy: 1. Config controller is triggered via SigningPolicy watcher 2. Autosign policy Secret (`{ca}-autosign-policy`) is updated -3. Server controller detects the updated `autosign-policy-secret-hash` annotation -4. CA pod is recreated so `openvox-autosign` reads the new policy on the next CSR (no manual restart) +3. The kubelet syncs the updated Secret into the CA pod, which mounts it as a + directory rather than through `subPath` +4. `openvox-autosign` reads the file on the next CSR + +**No restart is involved.** The policy Secret is deliberately the one Secret +that does not roll its pod. A `subPath` mount is never refreshed by the +kubelet, which is why this used to need a restart - and because the CA +Deployment uses the `Recreate` strategy, that restart meant a short outage with +no signing and no CRL, for a change that alters no running state. + +The trade-off is timing: a policy edit takes effect within the kubelet sync +period, up to about a minute, instead of immediately after a restart. For a +rule that governs which CSRs get signed, waiting a minute is preferable to +dropping the CA. ### Changing ENC Configuration diff --git a/internal/controller/config_autosign.go b/internal/controller/config_autosign.go index 841aa4a5..8aeb190f 100644 --- a/internal/controller/config_autosign.go +++ b/internal/controller/config_autosign.go @@ -20,6 +20,15 @@ import ( const autosignBinaryPath = "/usr/local/bin/openvox-autosign" +// autosignPolicyDir is where the rendered policy Secret is mounted. It is a +// directory so the kubelet keeps it in sync; see the mount in +// server_deployment.go. +const autosignPolicyDir = "/etc/puppetlabs/puppet/autosign-policy" + +// autosignPolicyPath is the file inside that directory, passed to the binary +// with --config. +const autosignPolicyPath = autosignPolicyDir + "/autosign-policy.yaml" + // findSigningPolicies returns all SigningPolicies referencing the given CA. // // A list error is returned rather than swallowed: an empty policy set renders diff --git a/internal/controller/config_controller_test.go b/internal/controller/config_controller_test.go index 74d9ca93..e0c04e06 100644 --- a/internal/controller/config_controller_test.go +++ b/internal/controller/config_controller_test.go @@ -212,6 +212,12 @@ func TestConfigReconcile_PuppetConfWithCA(t *testing.T) { if !strings.Contains(puppetConf, "autosign = ") { t.Errorf("puppet.conf missing autosign\n---\n%s", puppetConf) } + // The policy lives in a directory mount, so the binary has to be told where + // to look. A wrong path here denies every CSR and would otherwise surface + // only in an end-to-end run. + if !strings.Contains(puppetConf, "--config "+autosignPolicyPath) { + t.Errorf("puppet.conf must point the binary at %s\n---\n%s", autosignPolicyPath, puppetConf) + } } func TestConfigReconcile_PuppetConfWithENC(t *testing.T) { diff --git a/internal/controller/config_rendering.go b/internal/controller/config_rendering.go index 18e473a8..07cdc502 100644 --- a/internal/controller/config_rendering.go +++ b/internal/controller/config_rendering.go @@ -76,12 +76,12 @@ func (r *ConfigReconciler) renderPuppetConf(ctx context.Context, cfg *openvoxv1a } // Autosign: by default point to the built-in binary, which reads the policy - // Secret (mounted by the server controller) and decides sign/deny. A policy - // change rewrites the Secret and the server controller rolls the CA pod via - // the autosign-policy-secret-hash annotation, so it applies without a manual - // restart. A custom autosignCommand replaces the built-in binary and disables - // the SigningPolicy-driven flow (the policy Secret is not mounted). - autosignCmd := autosignBinaryPath + // Secret (mounted by the server controller as a directory) and decides + // sign/deny. The binary re-reads the file on every CSR and the kubelet keeps + // the mount in sync, so a policy change applies without restarting the CA. + // A custom autosignCommand replaces the built-in binary and disables the + // SigningPolicy-driven flow (the policy Secret is not mounted). + autosignCmd := fmt.Sprintf("%s --config %s", autosignBinaryPath, autosignPolicyPath) if cfg.Spec.Puppet.AutosignCommand != "" { autosignCmd = cfg.Spec.Puppet.AutosignCommand } diff --git a/internal/controller/server_controller_test.go b/internal/controller/server_controller_test.go index d0505b55..acf07784 100644 --- a/internal/controller/server_controller_test.go +++ b/internal/controller/server_controller_test.go @@ -6,6 +6,7 @@ import ( appsv1 "k8s.io/api/apps/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" + corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" policyv1 "k8s.io/api/policy/v1" "k8s.io/apimachinery/pkg/types" @@ -180,7 +181,12 @@ func TestServerReconcile_AnnotationHashes(t *testing.T) { } } -func TestServerReconcile_AutosignPolicyHashAnnotation(t *testing.T) { +// TestServerReconcile_AutosignPolicyIsLiveMounted replaces the former hash +// annotation test. A policy edit must not roll the CA pod: the CA Deployment +// uses the Recreate strategy, so a restart is a short outage with no signing +// and no CRL - for a change that alters no running state. The binary re-reads +// the file on every CSR, so nothing downstream needs the restart either. +func TestServerReconcile_AutosignPolicyIsLiveMounted(t *testing.T) { objs := append(serverPrereqs(), newSecret("production-ca-autosign-policy", map[string][]byte{ "autosign-policy.yaml": []byte("policies:\n"), @@ -199,9 +205,26 @@ func TestServerReconcile_AutosignPolicyHashAnnotation(t *testing.T) { t.Fatalf("Deployment not found: %v", err) } - // The CA pod must carry the autosign-policy hash so a SigningPolicy change rolls it. - if v, ok := deploy.Spec.Template.Annotations["openvox.voxpupuli.org/autosign-policy-secret-hash"]; !ok || v == "" { - t.Error("CA pod should carry the autosign-policy-secret-hash annotation") + if _, ok := deploy.Spec.Template.Annotations["openvox.voxpupuli.org/autosign-policy-secret-hash"]; ok { + t.Error("the policy hash must not be in the pod template any more, it would roll the CA on every edit") + } + + // A SubPath mount is never refreshed by the kubelet, which is what forced + // the restart. The mount has to stay a directory for the sync to happen. + var mount *corev1.VolumeMount + for i := range deploy.Spec.Template.Spec.Containers[0].VolumeMounts { + if deploy.Spec.Template.Spec.Containers[0].VolumeMounts[i].Name == "autosign-policy" { + mount = &deploy.Spec.Template.Spec.Containers[0].VolumeMounts[i] + } + } + if mount == nil { + t.Fatal("the CA pod must mount the autosign policy") + } + if mount.SubPath != "" { + t.Errorf("the policy must not be mounted with SubPath, got %q", mount.SubPath) + } + if mount.MountPath != autosignPolicyDir { + t.Errorf("expected the policy directory %q, got %q", autosignPolicyDir, mount.MountPath) } } diff --git a/internal/controller/server_deployment.go b/internal/controller/server_deployment.go index 558a01e4..3ab8c561 100644 --- a/internal/controller/server_deployment.go +++ b/internal/controller/server_deployment.go @@ -118,14 +118,6 @@ func (r *ServerReconciler) reconcileDeployment(ctx context.Context, server *open // SigningPolicy changes. The policy Secret is subPath-mounted, so kubelet does not // live-sync it; hashing it into the pod template rolls the CA pod when the rendered // policy changes, so a SigningPolicy edit applies without a manual restart. - // Skipped when a custom autosignCommand disables the SigningPolicy-driven flow. - if server.Spec.CA && cfg.Spec.Puppet.AutosignCommand == "" { - autosignSecretName := fmt.Sprintf("%s-autosign-policy", ca.Name) - if autosignHash, err := r.secretHash(ctx, autosignSecretName, server.Namespace); err == nil { - annotations["openvox.voxpupuli.org/autosign-policy-secret-hash"] = autosignHash - } - } - deploy := &appsv1.Deployment{} err = r.Get(ctx, types.NamespacedName{Name: deployName, Namespace: server.Namespace}, deploy) if errors.IsNotFound(err) { @@ -312,10 +304,15 @@ func (r *ServerReconciler) buildPodSpec(server *openvoxv1alpha1.Server, cfg *ope // flow: the command is then responsible for its own signing decision. if cfg.Spec.Puppet.AutosignCommand == "" { autosignSecretName := fmt.Sprintf("%s-autosign-policy", ca.Name) + // Mounted as a directory rather than with SubPath: a SubPath mount is + // never refreshed by the kubelet, which is why this used to need a + // pod restart on every policy change. The CA Deployment uses the + // Recreate strategy, so that restart was a short CA outage - no + // signing, no CRL - for an edit that changes no running state. The + // CRL Secret is mounted the same way for the same reason. volumeMounts = append(volumeMounts, corev1.VolumeMount{ Name: "autosign-policy", - MountPath: "/etc/puppetlabs/puppet/autosign-policy.yaml", - SubPath: "autosign-policy.yaml", + MountPath: autosignPolicyDir, ReadOnly: true, }) volumes = append(volumes, corev1.Volume{