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/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/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 | 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{