diff --git a/docs/content/docs/advanced/tls.md b/docs/content/docs/advanced/tls.md index 8c80cc1..422ab38 100644 --- a/docs/content/docs/advanced/tls.md +++ b/docs/content/docs/advanced/tls.md @@ -95,13 +95,13 @@ spec: ## Certificate Modes -### Projected Volumes (Default) +### Secret Volume (Default) When `useCSIDriver: false` (default): -- The operator creates a cert-manager `Certificate` CR per pod -- cert-manager creates a `Secret` per pod -- Secrets are mounted via Kubernetes projected volumes +- The operator creates one cert-manager `Certificate` CR per cluster +- cert-manager creates the matching `Secret` +- The Secret is mounted into every pod as a plain Secret volume **Advantages:** - Works with any cert-manager installation @@ -132,21 +132,32 @@ spec: ### Certificate Creation -For a 3-replica cluster named `my-cluster`: +For a cluster named `my-cluster`, regardless of its replica count: ``` -Certificate: gnmic-my-cluster-0-tls - → Secret: gnmic-my-cluster-0-tls - → CN: gnmic-my-cluster-0 +Certificate: gnmic-my-cluster-api-tls + → Secret: gnmic-my-cluster-api-tls + → CN: gnmic-my-cluster → DNS SANs: - - gnmic-my-cluster-0 - - gnmic-my-cluster-0.gnmic-my-cluster.default.svc + - *.gnmic-my-cluster.default.svc.cluster.local (every pod, via the headless Service) + - *.gnmic-my-cluster.default.svc, *.gnmic-my-cluster.default, *.gnmic-my-cluster + - gnmic-my-cluster.default.svc.cluster.local (the headless Service itself) + - gnmic-my-cluster.default.svc, gnmic-my-cluster.default, gnmic-my-cluster +``` + +The wildcard covers every pod, so scaling the cluster neither issues new +certificates nor changes the pod template; existing pods keep running. A +tunnel-TLS cluster gets a second certificate, `gnmic-my-cluster-tunnel-tls`, +built the same way plus the names of the tunnel Service. -Certificate: gnmic-my-cluster-1-tls - ... +Clusters created by operator versions before 0.6.0 had one certificate per pod +(`gnmic-my-cluster-0-tls`, ...). On upgrade the operator issues the cluster-wide +certificate, rolls the pods onto it, and deletes the per-pod Certificates once +every pod runs the new template. Their Secrets are left in place, because the +operator never writes Secrets; remove them once the rollout is complete: -Certificate: gnmic-my-cluster-2-tls - ... +```bash +kubectl delete secrets -l operator.gnmic.dev/cluster-name=my-cluster,operator.gnmic.dev/pod-name ``` ### Controller CA Distribution @@ -209,7 +220,7 @@ When scaling a TLS-enabled cluster: kubectl get certificates -l operator.gnmic.dev/cluster-name=my-cluster # Check certificate details -kubectl describe certificate gnmic-my-cluster-0-tls +kubectl describe certificate gnmic-my-cluster-api-tls ``` ### Check Secrets @@ -219,7 +230,7 @@ kubectl describe certificate gnmic-my-cluster-0-tls kubectl get secrets -l operator.gnmic.dev/cluster-name=my-cluster # Inspect certificate content -kubectl get secret gnmic-my-cluster-0-tls -o jsonpath='{.data.tls\.crt}' | base64 -d | openssl x509 -text -noout +kubectl get secret gnmic-my-cluster-api-tls -o jsonpath='{.data.tls\.crt}' | base64 -d | openssl x509 -text -noout ``` ### Check Controller CA ConfigMap @@ -237,7 +248,7 @@ kubectl get configmap gnmic-my-cluster-controller-ca -o yaml kubectl logs -n cert-manager deploy/cert-manager # Check certificate conditions -kubectl get certificate gnmic-my-cluster-0-tls -o jsonpath='{.status.conditions}' +kubectl get certificate gnmic-my-cluster-api-tls -o jsonpath='{.status.conditions}' ``` **Issuer not found:** diff --git a/docs/content/docs/user-guide/cluster.md b/docs/content/docs/user-guide/cluster.md index 09e9487..3c45720 100644 --- a/docs/content/docs/user-guide/cluster.md +++ b/docs/content/docs/user-guide/cluster.md @@ -210,7 +210,7 @@ spec: | Resource | Name Pattern | Purpose | |----------|--------------|---------| | Service | `gnmic-{cluster}-tunnel` | Exposes tunnel port to external devices | -| Certificate | `gnmic-{cluster}-{index}-tunnel-tls` | Per-pod tunnel TLS certificate (if TLS enabled) | +| Certificate | `gnmic-{cluster}-tunnel-tls` | Tunnel server certificate shared by the pods (if TLS enabled) | ### Using Tunnel Targets @@ -252,8 +252,8 @@ spec: When TLS is enabled: -1. **Per-Pod Certificates**: The operator creates a cert-manager `Certificate` for each pod -2. **Automatic Mounting**: Certificates are mounted at `/etc/certs/api/` +1. **One Certificate per Cluster**: The operator creates a single cert-manager `Certificate` whose names cover every pod (`*.gnmic-{cluster}.{namespace}.svc`) and the headless Service. Scaling the cluster therefore never changes the pod template or restarts existing pods. +2. **Automatic Mounting**: The certificate Secret is mounted at `/etc/gnmic/tls/` 3. **mTLS**: The operator authenticates to pods using client certificates 4. **CA Sync**: The operator's CA is synced to the cluster namespace as a ConfigMap @@ -316,8 +316,8 @@ When TLS is enabled, additional resources are created: | Resource | Name Pattern | Purpose | |----------|--------------|---------| -| Certificate | `gnmic-{cluster}-{index}-tls` | Per-pod TLS certificate | -| Secret | `gnmic-{cluster}-{index}-tls` | Certificate and key (created by cert-manager) | +| Certificate | `gnmic-{cluster}-api-tls` | Server certificate shared by the pods (wildcard + headless Service names) | +| Secret | `gnmic-{cluster}-api-tls` | Certificate and key (created by cert-manager) | | ConfigMap | `gnmic-{cluster}-controller-ca` | Controller's CA for mTLS verification | ## gNMI Client TLS (Target Connections) diff --git a/internal/controller/api_cert_san_test.go b/internal/controller/api_cert_san_test.go deleted file mode 100644 index 165de5d..0000000 --- a/internal/controller/api_cert_san_test.go +++ /dev/null @@ -1,42 +0,0 @@ -package controller - -import ( - "slices" - "testing" - - gnmicv1alpha1 "github.com/gnmic/operator/api/v1alpha1" -) - -// The certificate a pod presents on its REST and gNMI ports must cover the -// headless Service name as well as the pod's own names: the headless name -// resolves to an arbitrary pod, and a verifying client dialing it was rejected. -func TestAPICertificateCoversHeadlessServiceName(t *testing.T) { - cluster := &gnmicv1alpha1.Cluster{} - cluster.Name, cluster.Namespace = "c1", "telemetry" - cluster.Spec.API = &gnmicv1alpha1.APIConfig{TLS: &gnmicv1alpha1.ClusterTLSConfig{IssuerRef: "ca"}} - r := NewClusterReconcilerForTest() - - cert := r.buildCertificate(cluster, "gnmic-c1-0-tls", "gnmic-c1-0", "gnmic-c1") - for _, want := range []string{ - "gnmic-c1-0", "gnmic-c1-0.gnmic-c1.telemetry.svc", // per-pod, as before - "gnmic-c1", "gnmic-c1.telemetry", "gnmic-c1.telemetry.svc", // headless Service - } { - if !slices.Contains(cert.Spec.DNSNames, want) { - t.Errorf("DNSNames lack %q: %v", want, cert.Spec.DNSNames) - } - } - if cert.Spec.CommonName != "gnmic-c1-0" { - t.Errorf("CommonName = %q, want the pod name", cert.Spec.CommonName) - } - - // A certificate issued before this change differs in DNSNames, so the - // reconciler re-issues it on upgrade rather than leaving the old SAN set. - old := cert.DeepCopy() - old.Spec.DNSNames = old.Spec.DNSNames[:5] - if !r.certificateNeedsUpdate(old, cert) { - t.Error("a certificate without the headless names should be updated") - } - if r.certificateNeedsUpdate(cert, cert.DeepCopy()) { - t.Error("an identical certificate should not be updated") - } -} diff --git a/internal/controller/cluster_certs_test.go b/internal/controller/cluster_certs_test.go new file mode 100644 index 0000000..b2c0d08 --- /dev/null +++ b/internal/controller/cluster_certs_test.go @@ -0,0 +1,207 @@ +package controller + +import ( + "context" + "reflect" + "slices" + "testing" + + certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + gnmicv1alpha1 "github.com/gnmic/operator/api/v1alpha1" + "github.com/gnmic/operator/internal/gnmic" +) + +func tlsCluster(replicas int32) *gnmicv1alpha1.Cluster { + c := &gnmicv1alpha1.Cluster{} + c.Name, c.Namespace = "c1", "telemetry" + c.Spec.Replicas = ptr.To(replicas) + c.Spec.Image = "gnmic:test" + c.Spec.API = &gnmicv1alpha1.APIConfig{RestPort: 7890, TLS: &gnmicv1alpha1.ClusterTLSConfig{IssuerRef: "ca"}} + c.Spec.GRPCTunnel = &gnmicv1alpha1.GRPCTunnelConfig{Port: 57401, TLS: &gnmicv1alpha1.ClusterTLSConfig{IssuerRef: "ca"}} + return c +} + +// Server certificates are cluster-wide: one object, a wildcard for the pods, +// the Service names, and no pod-specific label that would tie it to an ordinal. +func TestServerCertificatesAreClusterScoped(t *testing.T) { + r := NewClusterReconcilerForTest() + cluster := tlsCluster(3) + + api := r.buildCertificate(cluster) + if api.Name != "gnmic-c1-api-tls" || api.Spec.SecretName != "gnmic-c1-api-tls" || api.Spec.CommonName != "gnmic-c1" { + t.Errorf("api certificate identity: name=%s secret=%s cn=%s", api.Name, api.Spec.SecretName, api.Spec.CommonName) + } + for _, want := range []string{ + "*.gnmic-c1.telemetry.svc." + gnmic.ClusterDomain(), // every pod + "gnmic-c1.telemetry.svc", // the headless Service + } { + if !slices.Contains(api.Spec.DNSNames, want) { + t.Errorf("api DNSNames lack %q: %v", want, api.Spec.DNSNames) + } + } + if api.Labels[LabelCertType] != LabelValueCertTypeAPI { + t.Errorf("api certificate lacks cert-type label: %v", api.Labels) + } + if _, perPod := api.Spec.SecretTemplate.Labels[LabelPodName]; perPod { + t.Errorf("api Secret template carries a pod label: %v", api.Spec.SecretTemplate.Labels) + } + + tunnel := r.buildTunnelCertificate(cluster) + if tunnel.Name != "gnmic-c1-tunnel-tls" || tunnel.Spec.CommonName != "gnmic-c1" { + t.Errorf("tunnel certificate identity: name=%s cn=%s", tunnel.Name, tunnel.Spec.CommonName) + } + for _, want := range []string{"*.gnmic-c1.telemetry.svc." + gnmic.ClusterDomain(), "gnmic-c1-grpc-tunnel.telemetry.svc"} { + if !slices.Contains(tunnel.Spec.DNSNames, want) { + t.Errorf("tunnel DNSNames lack %q: %v", want, tunnel.Spec.DNSNames) + } + } + if tunnel.Labels[LabelCertType] != LabelValueCertTypeTunnel { + t.Errorf("tunnel certificate lacks cert-type label: %v", tunnel.Labels) + } + + // Neither name parses as a legacy per-pod certificate, so the legacy + // cleanup can never mistake the new objects for old ones. + if r.extractOrdinalFromCertName(api.Name, "gnmic-c1") >= 0 || r.extractOrdinalFromTunnelCertName(tunnel.Name, "gnmic-c1") >= 0 || + r.extractOrdinalFromCertName(tunnel.Name, "gnmic-c1") >= 0 { + t.Error("cluster-wide certificate names must not look like per-pod ones") + } +} + +// The reason for the change: the pod template must not depend on the replica +// count, or every scale operation rolls every pod. +func TestStatefulSetTemplateIndependentOfReplicas(t *testing.T) { + r := NewClusterReconcilerForTest() + one, _, err := r.buildStatefulSet(tlsCluster(1)) + if err != nil { + t.Fatal(err) + } + three, _, err := r.buildStatefulSet(tlsCluster(3)) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(one.Spec.Template.Spec, three.Spec.Template.Spec) { + t.Errorf("pod template differs between 1 and 3 replicas:\n1: %+v\n3: %+v", one.Spec.Template.Spec.Volumes, three.Spec.Template.Spec.Volumes) + } + // And the volumes are plain Secret mounts of the cluster-wide certificates. + var apiVol, tunnelVol *corev1.Volume + for i := range one.Spec.Template.Spec.Volumes { + switch one.Spec.Template.Spec.Volumes[i].Name { + case "tls-certs": + apiVol = &one.Spec.Template.Spec.Volumes[i] + case "tunnel-tls-certs": + tunnelVol = &one.Spec.Template.Spec.Volumes[i] + } + } + if apiVol == nil || apiVol.Secret == nil || apiVol.Secret.SecretName != "gnmic-c1-api-tls" { + t.Errorf("tls-certs volume = %+v, want Secret gnmic-c1-api-tls", apiVol) + } + if tunnelVol == nil || tunnelVol.Secret == nil || tunnelVol.Secret.SecretName != "gnmic-c1-tunnel-tls" { + t.Errorf("tunnel-tls-certs volume = %+v, want Secret gnmic-c1-tunnel-tls", tunnelVol) + } + for _, m := range one.Spec.Template.Spec.Containers[0].VolumeMounts { + if (m.Name == "tls-certs" || m.Name == "tunnel-tls-certs") && m.SubPathExpr != "" { + t.Errorf("mount %s still uses subPathExpr %q", m.Name, m.SubPathExpr) + } + } +} + +func TestStatefulSetRolledOut(t *testing.T) { + sts := func(gen, obs int64, replicas, ready int32, cur, upd string) *appsv1.StatefulSet { + s := &appsv1.StatefulSet{} + s.Generation = gen + s.Spec.Replicas = ptr.To(int32(2)) + s.Status = appsv1.StatefulSetStatus{ObservedGeneration: obs, Replicas: replicas, ReadyReplicas: ready, CurrentRevision: cur, UpdateRevision: upd} + return s + } + cases := map[string]struct { + sts *appsv1.StatefulSet + want bool + }{ + "converged": {sts(3, 3, 2, 2, "r2", "r2"), true}, + "rollout in flight": {sts(3, 3, 2, 2, "r1", "r2"), false}, + "not all ready": {sts(3, 3, 2, 1, "r2", "r2"), false}, + "old generation": {sts(4, 3, 2, 2, "r2", "r2"), false}, + "fresh, no status yet": {sts(1, 0, 0, 0, "", ""), false}, + "nil": {nil, false}, + } + for name, c := range cases { + if got := statefulSetRolledOut(c.sts); got != c.want { + t.Errorf("%s: got %v, want %v", name, got, c.want) + } + } +} + +// Upgrading from per-pod certificates: the old Certificates are removed and +// nothing else is touched -- not the cluster-wide objects, not another +// cluster's, and not the legacy Secrets (Secrets are read-only for the +// operator; they are left for manual removal). +func TestLegacyPodCertificatesRemoved(t *testing.T) { + scheme := runtime.NewScheme() + for _, add := range []func(*runtime.Scheme) error{clientgoscheme.AddToScheme, gnmicv1alpha1.AddToScheme, certmanagerv1.AddToScheme} { + if err := add(scheme); err != nil { + t.Fatal(err) + } + } + cluster := tlsCluster(1) + clusterLabels := map[string]string{LabelClusterName: "c1"} + cert := func(name string, labels map[string]string) *certmanagerv1.Certificate { + return &certmanagerv1.Certificate{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "telemetry", Labels: labels}} + } + secret := func(name string, labels map[string]string) *corev1.Secret { + return &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "telemetry", Labels: labels}} + } + legacyLabels := map[string]string{LabelClusterName: "c1", LabelPodName: "gnmic-c1-0"} + tunnelLabels := map[string]string{LabelClusterName: "c1", LabelCertType: LabelValueCertTypeTunnel} + apiLabels := map[string]string{LabelClusterName: "c1", LabelCertType: LabelValueCertTypeAPI} + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects( + // legacy per-pod objects: go + cert("gnmic-c1-0-tls", clusterLabels), secret("gnmic-c1-0-tls", legacyLabels), + cert("gnmic-c1-1-tunnel-tls", tunnelLabels), secret("gnmic-c1-1-tunnel-tls", map[string]string{LabelClusterName: "c1", LabelPodName: "gnmic-c1-1", LabelCertType: LabelValueCertTypeTunnel}), + // current objects: stay + cert("gnmic-c1-api-tls", apiLabels), secret("gnmic-c1-api-tls", apiLabels), + cert("gnmic-c1-tunnel-tls", tunnelLabels), secret("gnmic-c1-tunnel-tls", tunnelLabels), + cert("gnmic-c1-client-tls", map[string]string{LabelClusterName: "c1", LabelCertType: LabelValueCertTypeClient}), + // another cluster's legacy object: not ours + cert("gnmic-c2-0-tls", map[string]string{LabelClusterName: "c2"}), + ).Build() + r := &ClusterReconciler{Client: cl, Scheme: scheme} + + if err := r.cleanupLegacyPodCertificates(context.Background(), cluster); err != nil { + t.Fatal(err) + } + gone := func(obj interface{ GetName() string }, get func() error) { + t.Helper() + if err := get(); !apierrors.IsNotFound(err) { + t.Errorf("%s should be deleted, got err=%v", obj.GetName(), err) + } + } + kept := func(name string, get func() error) { + t.Helper() + if err := get(); err != nil { + t.Errorf("%s should survive, got %v", name, err) + } + } + key := func(name string) types.NamespacedName { + return types.NamespacedName{Namespace: "telemetry", Name: name} + } + var c certmanagerv1.Certificate + var s corev1.Secret + gone(cert("gnmic-c1-0-tls", nil), func() error { return cl.Get(context.Background(), key("gnmic-c1-0-tls"), &c) }) + gone(cert("gnmic-c1-1-tunnel-tls", nil), func() error { return cl.Get(context.Background(), key("gnmic-c1-1-tunnel-tls"), &c) }) + for _, name := range []string{"gnmic-c1-api-tls", "gnmic-c1-tunnel-tls", "gnmic-c1-client-tls", "gnmic-c2-0-tls"} { + kept(name, func() error { return cl.Get(context.Background(), key(name), &c) }) + } + for _, name := range []string{"gnmic-c1-api-tls", "gnmic-c1-tunnel-tls", "gnmic-c1-0-tls", "gnmic-c1-1-tunnel-tls"} { + kept(name, func() error { return cl.Get(context.Background(), key(name), &s) }) + } +} diff --git a/internal/controller/cluster_controller.go b/internal/controller/cluster_controller.go index 695a34e..1ae1407 100644 --- a/internal/controller/cluster_controller.go +++ b/internal/controller/cluster_controller.go @@ -165,6 +165,16 @@ func (r *ClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } logger.Info("reconciled cluster statefulset", "replicas", ptr.Deref(statefulSet.Spec.Replicas, 0), "image", statefulSet.Spec.Template.Spec.Containers[0].Image) + // Per-pod certificates from an earlier operator are removed only once every + // pod runs the template that mounts the cluster-wide ones; an old pod still + // projects the old Secrets. Best effort: a failure here is logged and + // retried on the next pass, it must not hold up the apply. + if (certManagerIssued(apiTLS(cluster)) || certManagerIssued(tunnelTLS(cluster))) && statefulSetRolledOut(statefulSet) { + if err := r.cleanupLegacyPodCertificates(ctx, cluster); err != nil { + logger.Error(err, "failed to clean up legacy per-pod certificates") + } + } + // resolve the enabled pipelines referencing this cluster into an apply plan pipelines, err := r.listPipelinesForCluster(ctx, cluster) if err != nil { diff --git a/internal/controller/cluster_helpers.go b/internal/controller/cluster_helpers.go index b7d2f1f..bd6ce9a 100644 --- a/internal/controller/cluster_helpers.go +++ b/internal/controller/cluster_helpers.go @@ -6,7 +6,6 @@ import ( "fmt" "strings" - corev1 "k8s.io/api/core/v1" "k8s.io/utils/ptr" gnmicv1alpha1 "github.com/gnmic/operator/api/v1alpha1" @@ -44,29 +43,3 @@ func PrometheusServiceName(cluster, pipeline, output string) string { head := strings.TrimRight(name[:maxServiceNameLen-len(suffix)-1], "-") return head + "-" + suffix } - -// podNameEnv is the downward-API variable the TLS volume mounts key on. -var podNameEnv = corev1.EnvVar{ - Name: "POD_NAME", - ValueFrom: &corev1.EnvVarSource{ - FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"}, - }, -} - -// ensurePodNameEnv returns env with exactly one POD_NAME, set from the -// downward API. -// -// Both TLS paths need it for their subPathExpr, and the user may also have put -// one in spec.env. The API TLS path used to append unconditionally, so a -// user-supplied POD_NAME produced a duplicate entry. It is replaced rather than -// kept: the volume mount only works when POD_NAME is the pod's own name, so -// when TLS is on the operator owns the variable. -func ensurePodNameEnv(env []corev1.EnvVar) []corev1.EnvVar { - for i := range env { - if env[i].Name == podNameEnv.Name { - env[i] = podNameEnv - return env - } - } - return append(env, podNameEnv) -} diff --git a/internal/controller/cluster_statefulset.go b/internal/controller/cluster_statefulset.go index 93cb208..4b8c15b 100644 --- a/internal/controller/cluster_statefulset.go +++ b/internal/controller/cluster_statefulset.go @@ -372,49 +372,22 @@ func (r *ClusterReconciler) buildStatefulSet(cluster *gnmicv1alpha1.Cluster) (*a }, }) } else { - // use projected volume with subPathExpr to mount the correct certificate per pod - // each pod has a certificate secret named --tls - // organize by pod name so subPathExpr can select the right one - - certSources := []corev1.VolumeProjection{} - for i := int32(0); i < desiredReplicas(cluster); i++ { - podName := fmt.Sprintf("%s-%d", stsName, i) - secretName := fmt.Sprintf("%s-tls", podName) - certSources = append(certSources, corev1.VolumeProjection{ - Secret: &corev1.SecretProjection{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: secretName, - }, - Items: []corev1.KeyToPath{ - {Key: "tls.crt", Path: fmt.Sprintf("%s/tls.crt", podName)}, - {Key: "tls.key", Path: fmt.Sprintf("%s/tls.key", podName)}, - {Key: "ca.crt", Path: fmt.Sprintf("%s/ca.crt", podName)}, - }, - Optional: ptr.To(true), - }, - }) - } - + // One Secret for the whole cluster, mounted as is. A projection of + // one Secret per ordinal here put the replica count into the pod + // template and rolled every pod on every scale. volumes = append(volumes, corev1.Volume{ Name: "tls-certs", VolumeSource: corev1.VolumeSource{ - Projected: &corev1.ProjectedVolumeSource{ - Sources: certSources, - }, + Secret: &corev1.SecretVolumeSource{SecretName: apiCertificateName(cluster)}, }, }) } - // add TLS volume mount to main container using subPathExpr to select the pod's certificate volumeMounts = append(volumeMounts, corev1.VolumeMount{ - Name: "tls-certs", - MountPath: gnmic.CertFilesBasePath, - SubPathExpr: "$(POD_NAME)", // Selects the subdirectory matching the pod name - ReadOnly: true, + Name: "tls-certs", + MountPath: gnmic.CertFilesBasePath, + ReadOnly: true, }) - - // POD_NAME drives the subPathExpr above. - envVars = ensurePodNameEnv(envVars) } // add CA bundle volume if specified (for verifying target certificates) @@ -478,46 +451,20 @@ func (r *ClusterReconciler) buildStatefulSet(cluster *gnmicv1alpha1.Cluster) (*a }, }) } else { - // use projected volume with subPathExpr to mount the correct certificate per pod - tunnelCertSources := []corev1.VolumeProjection{} - for i := int32(0); i < desiredReplicas(cluster); i++ { - podName := fmt.Sprintf("%s-%d", stsName, i) - secretName := fmt.Sprintf("%s-tunnel-tls", podName) - tunnelCertSources = append(tunnelCertSources, corev1.VolumeProjection{ - Secret: &corev1.SecretProjection{ - LocalObjectReference: corev1.LocalObjectReference{ - Name: secretName, - }, - Items: []corev1.KeyToPath{ - {Key: "tls.crt", Path: fmt.Sprintf("%s/tls.crt", podName)}, - {Key: "tls.key", Path: fmt.Sprintf("%s/tls.key", podName)}, - {Key: "ca.crt", Path: fmt.Sprintf("%s/ca.crt", podName)}, - }, - Optional: ptr.To(true), - }, - }) - } - + // one Secret for the whole cluster; see the API TLS volume above volumes = append(volumes, corev1.Volume{ Name: "tunnel-tls-certs", VolumeSource: corev1.VolumeSource{ - Projected: &corev1.ProjectedVolumeSource{ - Sources: tunnelCertSources, - }, + Secret: &corev1.SecretVolumeSource{SecretName: tunnelCertificateName(cluster)}, }, }) } - // add tunnel TLS volume mount volumeMounts = append(volumeMounts, corev1.VolumeMount{ - Name: "tunnel-tls-certs", - MountPath: gnmic.TunnelCertFilesBasePath, - SubPathExpr: "$(POD_NAME)", - ReadOnly: true, + Name: "tunnel-tls-certs", + MountPath: gnmic.TunnelCertFilesBasePath, + ReadOnly: true, }) - - // POD_NAME may already be there from the API TLS path. - envVars = ensurePodNameEnv(envVars) } // add tunnel CA bundle volume if bundleRef is configured for client certificate verification diff --git a/internal/controller/cluster_tls.go b/internal/controller/cluster_tls.go index c994054..983a4e4 100644 --- a/internal/controller/cluster_tls.go +++ b/internal/controller/cluster_tls.go @@ -25,9 +25,11 @@ import ( certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + appsv1 "k8s.io/api/apps/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -36,128 +38,118 @@ import ( "github.com/gnmic/operator/internal/gnmic" ) -// reconcileCertificates creates/updates cert-manager Certificate resources for each pod -// returns true if all certificates are ready, false otherwise +// reconcileCertificates creates/updates the cert-manager Certificate the +// cluster's pods present on the REST and gNMI ports, and reports whether it is +// ready. +// +// One certificate per cluster, with wildcard DNS names, rather than one per +// pod. Per-pod certificates put the replica count into the pod template -- one +// Secret projection per ordinal -- so every scale operation rolled every pod, +// and a new ordinal had to wait for its own issuance before it could start. A +// server certificate gains nothing from a per-pod key: the operator verifies +// the pod's hostname, and the wildcard matches every pod under the headless +// Service. Client TLS already worked this way. func (r *ClusterReconciler) reconcileCertificates(ctx context.Context, cluster *gnmicv1alpha1.Cluster) (bool, error) { - logger := log.FromContext(ctx) - if cluster.Spec.API == nil || cluster.Spec.API.TLS == nil || cluster.Spec.API.TLS.IssuerRef == "" { return true, nil // TLS not configured, skip } + return r.ensureCertificate(ctx, cluster, r.buildCertificate(cluster)) +} - stsName := fmt.Sprintf("%s%s", resourcePrefix, cluster.Name) - replicas := desiredReplicas(cluster) - - allReady := true - - // create/update certificates for each replica - for i := int32(0); i < replicas; i++ { - podName := fmt.Sprintf("%s-%d", stsName, i) - certName := fmt.Sprintf("%s-tls", podName) - - cert := r.buildCertificate(cluster, certName, podName, stsName) - - if err := controllerutil.SetControllerReference(cluster, cert, r.Scheme); err != nil { - return false, err - } +// ensureCertificate creates or updates one Certificate and reports whether it +// is Ready. +func (r *ClusterReconciler) ensureCertificate(ctx context.Context, cluster *gnmicv1alpha1.Cluster, cert *certmanagerv1.Certificate) (bool, error) { + logger := log.FromContext(ctx) - var current certmanagerv1.Certificate - err := r.Get(ctx, types.NamespacedName{Name: certName, Namespace: cluster.Namespace}, ¤t) - if apierrors.IsNotFound(err) { - logger.Info("creating certificate", "certificate", certName) - if err := r.Create(ctx, cert); err != nil { - return false, err - } - allReady = false - continue - } - if err != nil { + if err := controllerutil.SetControllerReference(cluster, cert, r.Scheme); err != nil { + return false, err + } + var current certmanagerv1.Certificate + err := r.Get(ctx, client.ObjectKeyFromObject(cert), ¤t) + if apierrors.IsNotFound(err) { + logger.Info("creating certificate", "certificate", cert.Name) + if err := r.Create(ctx, cert); err != nil { return false, err } - - // check if certificate needs update - if r.certificateNeedsUpdate(¤t, cert) { - current.Spec = cert.Spec - if err := r.Update(ctx, ¤t); err != nil { - return false, err - } - } - - // check if certificate is ready - if !r.isCertificateReady(¤t) { - logger.Info("certificate not ready", "certificate", certName) - allReady = false - } + return false, nil } - - // clean up certificates for replicas that no longer exist (scale down) - var certList certmanagerv1.CertificateList - if err := r.List(ctx, &certList, client.InNamespace(cluster.Namespace), client.MatchingLabels{ - LabelClusterName: cluster.Name, - }); err != nil { + if err != nil { return false, err } - - for _, cert := range certList.Items { - // extract the ordinal from the certificate name (for example: "gnmic-cluster1-2-tls" -> 2) - ordinal := r.extractOrdinalFromCertName(cert.Name, stsName) - if ordinal >= int(replicas) { - logger.Info("deleting certificate for scaled-down replica", "certificate", cert.Name) - if err := r.Delete(ctx, &cert); err != nil && !apierrors.IsNotFound(err) { - return false, err - } + if r.certificateNeedsUpdate(¤t, cert) { + current.Spec = cert.Spec + if err := r.Update(ctx, ¤t); err != nil { + return false, err } } + if !r.isCertificateReady(¤t) { + logger.Info("certificate not ready", "certificate", cert.Name) + return false, nil + } + return true, nil +} + +// apiCertificateName is the Certificate (and Secret) for the REST/gNMI server +// certificate. Distinct from the per-pod names an earlier operator used +// (--tls), which cleanupLegacyPodCertificates removes. +func apiCertificateName(cluster *gnmicv1alpha1.Cluster) string { + return resourcePrefix + cluster.Name + "-api-tls" +} - return allReady, nil +// tunnelCertificateName is the Certificate (and Secret) for the tunnel server. +func tunnelCertificateName(cluster *gnmicv1alpha1.Cluster) string { + return resourcePrefix + cluster.Name + "-tunnel-tls" } -// buildCertificate creates a cert-manager Certificate spec for a pod -func (r *ClusterReconciler) buildCertificate(cluster *gnmicv1alpha1.Cluster, certName, podName, stsName string) *certmanagerv1.Certificate { - // build DNS names for the certificate - // pod DNS: ...svc. - dnsNames := []string{ - podName, - fmt.Sprintf("%s.%s", podName, stsName), - fmt.Sprintf("%s.%s.%s", podName, stsName, cluster.Namespace), - fmt.Sprintf("%s.%s.%s.svc", podName, stsName, cluster.Namespace), - fmt.Sprintf("%s.%s.%s.svc.%s", podName, stsName, cluster.Namespace, gnmic.ClusterDomain()), - // The headless Service name resolves to every pod, so a client that - // dials it lands on an arbitrary one. The certificate this pod presents - // on the REST and gNMI ports must cover that name too, or a verifying - // client can only ever use the per-pod names. The tunnel certificate - // has carried its Service name from the start; this brings the API - // certificate in line. +// serverDNSNames are the names a cluster-wide server certificate must cover: +// every pod under the headless Service, through a wildcard, and the Service +// name itself, which resolves to an arbitrary pod. Bare pod hostnames are not +// included -- a wildcard cannot express them, and the operator dials the fully +// qualified form. +func serverDNSNames(stsName, namespace string) []string { + domain := gnmic.ClusterDomain() + return []string{ + "*." + stsName, + fmt.Sprintf("*.%s.%s", stsName, namespace), + fmt.Sprintf("*.%s.%s.svc", stsName, namespace), + fmt.Sprintf("*.%s.%s.svc.%s", stsName, namespace, domain), stsName, - fmt.Sprintf("%s.%s", stsName, cluster.Namespace), - fmt.Sprintf("%s.%s.svc", stsName, cluster.Namespace), - fmt.Sprintf("%s.%s.svc.%s", stsName, cluster.Namespace, gnmic.ClusterDomain()), + fmt.Sprintf("%s.%s", stsName, namespace), + fmt.Sprintf("%s.%s.svc", stsName, namespace), + fmt.Sprintf("%s.%s.svc.%s", stsName, namespace, domain), } +} +// buildCertificate is the cert-manager Certificate spec for the cluster's +// REST/gNMI server certificate. +func (r *ClusterReconciler) buildCertificate(cluster *gnmicv1alpha1.Cluster) *certmanagerv1.Certificate { + stsName := resourcePrefix + cluster.Name + name := apiCertificateName(cluster) return &certmanagerv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ - Name: certName, + Name: name, Namespace: cluster.Namespace, Labels: map[string]string{ - "app.kubernetes.io/name": "gnmic", - "app.kubernetes.io/managed-by": "gnmic-operator", + "app.kubernetes.io/name": LabelValueName, + "app.kubernetes.io/managed-by": LabelValueManagedBy, LabelClusterName: cluster.Name, + LabelCertType: LabelValueCertTypeAPI, }, }, Spec: certmanagerv1.CertificateSpec{ - SecretName: certName, + SecretName: name, SecretTemplate: &certmanagerv1.CertificateSecretTemplate{ Labels: map[string]string{ LabelClusterName: cluster.Name, - LabelPodName: podName, + LabelCertType: LabelValueCertTypeAPI, }, }, IssuerRef: cmmeta.IssuerReference{ Name: cluster.Spec.API.TLS.IssuerRef, Kind: "Issuer", // defaults to Issuer. TODO: configurable to ClusterIssuer ? }, - CommonName: podName, - DNSNames: dnsNames, + CommonName: stsName, + DNSNames: serverDNSNames(stsName, cluster.Namespace), Usages: []certmanagerv1.KeyUsage{ certmanagerv1.UsageServerAuth, certmanagerv1.UsageClientAuth, @@ -195,7 +187,8 @@ func (r *ClusterReconciler) isCertificateReady(cert *certmanagerv1.Certificate) return false } -// extractOrdinalFromCertName extracts the StatefulSet ordinal from a certificate name +// extractOrdinalFromCertName extracts the StatefulSet ordinal from a legacy per-pod +// certificate name; -1 for anything else (including the cluster-wide certificates). // for example: "gnmic-cluster1-2-tls" with stsName "gnmic-cluster1" returns 2 func (r *ClusterReconciler) extractOrdinalFromCertName(certName, stsName string) int { // remove the "-tls" suffix and the stsName prefix @@ -229,108 +222,33 @@ func (r *ClusterReconciler) cleanupCertificates(ctx context.Context, cluster *gn return nil } -// reconcileTunnelCertificates creates/updates cert-manager Certificate resources for tunnel TLS -// returns true if all certificates are ready, false otherwise +// reconcileTunnelCertificates creates/updates the cert-manager Certificate the +// cluster's pods present on the gRPC tunnel port, and reports whether it is +// ready. One certificate per cluster; see reconcileCertificates. func (r *ClusterReconciler) reconcileTunnelCertificates(ctx context.Context, cluster *gnmicv1alpha1.Cluster) (bool, error) { - logger := log.FromContext(ctx) - if cluster.Spec.GRPCTunnel == nil || cluster.Spec.GRPCTunnel.TLS == nil || cluster.Spec.GRPCTunnel.TLS.IssuerRef == "" { return true, nil // tunnel TLS not configured, skip } - - stsName := fmt.Sprintf("%s%s", resourcePrefix, cluster.Name) - replicas := desiredReplicas(cluster) - - allReady := true - - // create/update certificates for each replica - for i := int32(0); i < replicas; i++ { - podName := fmt.Sprintf("%s-%d", stsName, i) - certName := fmt.Sprintf("%s-tunnel-tls", podName) - - cert := r.buildTunnelCertificate(cluster, certName, podName, stsName) - - if err := controllerutil.SetControllerReference(cluster, cert, r.Scheme); err != nil { - return false, err - } - - var current certmanagerv1.Certificate - err := r.Get(ctx, types.NamespacedName{Name: certName, Namespace: cluster.Namespace}, ¤t) - if apierrors.IsNotFound(err) { - logger.Info("creating tunnel certificate", "certificate", certName) - if err := r.Create(ctx, cert); err != nil { - return false, err - } - allReady = false - continue - } - if err != nil { - return false, err - } - - // check if certificate needs update - if r.certificateNeedsUpdate(¤t, cert) { - current.Spec = cert.Spec - if err := r.Update(ctx, ¤t); err != nil { - return false, err - } - } - - // check if certificate is ready - if !r.isCertificateReady(¤t) { - logger.Info("tunnel certificate not ready", "certificate", certName) - allReady = false - } - } - - // clean up certificates for replicas that no longer exist (scale down) - var certList certmanagerv1.CertificateList - if err := r.List(ctx, &certList, client.InNamespace(cluster.Namespace), client.MatchingLabels{ - LabelClusterName: cluster.Name, - LabelCertType: LabelValueCertTypeTunnel, - }); err != nil { - return false, err - } - - for _, cert := range certList.Items { - // extract the ordinal from the certificate name (for example: "gnmic-cluster1-2-tunnel-tls" -> 2) - ordinal := r.extractOrdinalFromTunnelCertName(cert.Name, stsName) - if ordinal >= int(replicas) { - logger.Info("deleting tunnel certificate for scaled-down replica", "certificate", cert.Name) - if err := r.Delete(ctx, &cert); err != nil && !apierrors.IsNotFound(err) { - return false, err - } - } - } - - return allReady, nil + return r.ensureCertificate(ctx, cluster, r.buildTunnelCertificate(cluster)) } -// buildTunnelCertificate creates a cert-manager Certificate spec for tunnel TLS -func (r *ClusterReconciler) buildTunnelCertificate(cluster *gnmicv1alpha1.Cluster, certName, podName, stsName string) *certmanagerv1.Certificate { - // build DNS names for the certificate - dnsNames := []string{ - podName, - fmt.Sprintf("%s.%s", podName, stsName), - fmt.Sprintf("%s.%s.%s", podName, stsName, cluster.Namespace), - fmt.Sprintf("%s.%s.%s.svc", podName, stsName, cluster.Namespace), - fmt.Sprintf("%s.%s.%s.svc.%s", podName, stsName, cluster.Namespace, gnmic.ClusterDomain()), - } - - // also add the tunnel service DNS names if service is configured - if cluster.Spec.GRPCTunnel.Service != nil { - tunnelServiceName := fmt.Sprintf("%s%s-grpc-tunnel", resourcePrefix, cluster.Name) - dnsNames = append(dnsNames, - tunnelServiceName, - fmt.Sprintf("%s.%s", tunnelServiceName, cluster.Namespace), - fmt.Sprintf("%s.%s.svc", tunnelServiceName, cluster.Namespace), - fmt.Sprintf("%s.%s.svc.%s", tunnelServiceName, cluster.Namespace, gnmic.ClusterDomain()), - ) - } +// buildTunnelCertificate is the cert-manager Certificate spec for the cluster's +// tunnel server certificate. Devices dial the tunnel Service, so its names are +// included alongside the pod wildcard. +func (r *ClusterReconciler) buildTunnelCertificate(cluster *gnmicv1alpha1.Cluster) *certmanagerv1.Certificate { + stsName := resourcePrefix + cluster.Name + name := tunnelCertificateName(cluster) + tunnelServiceName := fmt.Sprintf("%s%s-grpc-tunnel", resourcePrefix, cluster.Name) + dnsNames := append(serverDNSNames(stsName, cluster.Namespace), + tunnelServiceName, + fmt.Sprintf("%s.%s", tunnelServiceName, cluster.Namespace), + fmt.Sprintf("%s.%s.svc", tunnelServiceName, cluster.Namespace), + fmt.Sprintf("%s.%s.svc.%s", tunnelServiceName, cluster.Namespace, gnmic.ClusterDomain()), + ) return &certmanagerv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ - Name: certName, + Name: name, Namespace: cluster.Namespace, Labels: map[string]string{ "app.kubernetes.io/name": LabelValueName, @@ -340,11 +258,10 @@ func (r *ClusterReconciler) buildTunnelCertificate(cluster *gnmicv1alpha1.Cluste }, }, Spec: certmanagerv1.CertificateSpec{ - SecretName: certName, + SecretName: name, SecretTemplate: &certmanagerv1.CertificateSecretTemplate{ Labels: map[string]string{ LabelClusterName: cluster.Name, - LabelPodName: podName, LabelCertType: LabelValueCertTypeTunnel, }, }, @@ -352,7 +269,7 @@ func (r *ClusterReconciler) buildTunnelCertificate(cluster *gnmicv1alpha1.Cluste Name: cluster.Spec.GRPCTunnel.TLS.IssuerRef, Kind: "Issuer", }, - CommonName: podName, + CommonName: stsName, DNSNames: dnsNames, Usages: []certmanagerv1.KeyUsage{ certmanagerv1.UsageServerAuth, @@ -509,3 +426,50 @@ func (r *ClusterReconciler) cleanupClientTLSCertificates(ctx context.Context, cl return nil } + +// statefulSetRolledOut reports whether every pod of the StatefulSet exists, is +// Ready, and runs the current template revision -- i.e. no rollout is in +// flight or pending. +func statefulSetRolledOut(sts *appsv1.StatefulSet) bool { + if sts == nil { + return false + } + want := ptr.Deref(sts.Spec.Replicas, 1) + st := sts.Status + return st.ObservedGeneration >= sts.Generation && + st.Replicas == want && st.ReadyReplicas == want && + st.CurrentRevision != "" && st.CurrentRevision == st.UpdateRevision +} + +// cleanupLegacyPodCertificates removes the per-pod Certificates an earlier +// operator issued (--tls, --tunnel-tls), once +// every pod runs a template that no longer mounts them. Before that, an old +// pod still projects their Secrets and cert-manager would stop renewing files +// that pod depends on. The Certificates are told apart from the cluster-wide +// ones by their ordinal names. +// +// cert-manager does not delete a Certificate's Secret with it, and Secrets are +// read-only for this operator (see the RBAC markers), so the legacy Secrets +// stay behind, labelled with the pod name they served. The documentation gives +// the one-line command to remove them. +func (r *ClusterReconciler) cleanupLegacyPodCertificates(ctx context.Context, cluster *gnmicv1alpha1.Cluster) error { + logger := log.FromContext(ctx) + stsName := resourcePrefix + cluster.Name + + var certs certmanagerv1.CertificateList + if err := r.List(ctx, &certs, client.InNamespace(cluster.Namespace), client.MatchingLabels{LabelClusterName: cluster.Name}); err != nil { + return err + } + for i := range certs.Items { + c := &certs.Items[i] + if r.extractOrdinalFromCertName(c.Name, stsName) < 0 && r.extractOrdinalFromTunnelCertName(c.Name, stsName) < 0 { + continue + } + logger.Info("deleting legacy per-pod certificate; its secret is left for manual removal", + "certificate", c.Name, "secret", c.Spec.SecretName) + if err := r.Delete(ctx, c); err != nil && !apierrors.IsNotFound(err) { + return err + } + } + return nil +} diff --git a/internal/controller/const.go b/internal/controller/const.go index b5196b8..3852840 100644 --- a/internal/controller/const.go +++ b/internal/controller/const.go @@ -21,6 +21,7 @@ const ( LabelCertType = "operator.gnmic.dev/cert-type" LabelValueCertTypeClient = "client" LabelValueCertTypeTunnel = "tunnel" + LabelValueCertTypeAPI = "api" ) const ( diff --git a/internal/controller/low_severity_test.go b/internal/controller/low_severity_test.go index 432c2a8..453ad2c 100644 --- a/internal/controller/low_severity_test.go +++ b/internal/controller/low_severity_test.go @@ -94,34 +94,6 @@ func TestPrometheusServiceNameFitsTheServiceLimit(t *testing.T) { } } -// #36: a user-supplied POD_NAME plus the API TLS path used to yield two entries. -func TestEnsurePodNameEnvIsSingleAndDownwardAPI(t *testing.T) { - user := []corev1.EnvVar{{Name: "FOO", Value: "bar"}, {Name: "POD_NAME", Value: "wrong"}} - got := ensurePodNameEnv(user) - got = ensurePodNameEnv(got) // the tunnel path calls it again - - count := 0 - for _, e := range got { - if e.Name != "POD_NAME" { - continue - } - count++ - if e.Value != "" || e.ValueFrom == nil || e.ValueFrom.FieldRef == nil || e.ValueFrom.FieldRef.FieldPath != "metadata.name" { - t.Fatalf("POD_NAME must come from the downward API, got %+v", e) - } - } - if count != 1 { - t.Fatalf("POD_NAME appears %d times: %+v", count, got) - } - if got[0].Name != "FOO" || len(got) != 2 { - t.Fatalf("other variables disturbed: %+v", got) - } - - if got := ensurePodNameEnv(nil); len(got) != 1 || got[0].Name != "POD_NAME" { - t.Fatalf("absent POD_NAME not added: %+v", got) - } -} - // #37: a Pipeline with an empty clusterRef enqueued a request for the bare // "gnmic-" prefix, driving the not-found cleanup path for a Cluster that never // existed. diff --git a/test/integration/harness/names.go b/test/integration/harness/names.go index 25e576a..a22e193 100644 --- a/test/integration/harness/names.go +++ b/test/integration/harness/names.go @@ -41,9 +41,8 @@ func HeadlessServiceName(cluster string) string { return resourcePrefix + cluste func ConfigMapName(cluster string) string { return resourcePrefix + cluster + "-config" } func TunnelServiceName(cluster string) string { return resourcePrefix + cluster + "-grpc-tunnel" } func ClientCertName(cluster string) string { return resourcePrefix + cluster + "-client-tls" } -func APICertName(cluster string, ordinal int) string { - return fmt.Sprintf("%s%s-%d-tls", resourcePrefix, cluster, ordinal) -} +func APICertName(cluster string) string { return resourcePrefix + cluster + "-api-tls" } +func TunnelCertName(cluster string) string { return resourcePrefix + cluster + "-tunnel-tls" } func ControllerCAConfigMap(cluster string) string { return resourcePrefix + cluster + "-controller-ca" } diff --git a/test/integration/suite/007-tunnel/fixtures/issuers.yaml b/test/integration/suite/007-tunnel/fixtures/issuers.yaml index 3e554d1..9a395ce 100644 --- a/test/integration/suite/007-tunnel/fixtures/issuers.yaml +++ b/test/integration/suite/007-tunnel/fixtures/issuers.yaml @@ -1,5 +1,5 @@ # Suite-local cert-manager issuer chain. selfsigned-issuer bootstraps the CA; -# suite-ca-issuer signs the Cluster's per-pod tunnel server certificates +# suite-ca-issuer signs the Cluster's tunnel server certificate # (spec.grpcTunnel.tls.issuerRef). Applied before gnmi-gen so the Issuer is # Ready when the baseline Cluster asks for certificates. apiVersion: cert-manager.io/v1 diff --git a/test/integration/suite/007-tunnel/tunnel_test.go b/test/integration/suite/007-tunnel/tunnel_test.go index a59143d..86642fb 100644 --- a/test/integration/suite/007-tunnel/tunnel_test.go +++ b/test/integration/suite/007-tunnel/tunnel_test.go @@ -475,11 +475,11 @@ func TestTunnel006_EveryPodReceivesMatches(t *testing.T) { s.K8s.Patch(t, s.K8s.Cluster(t, cluster), `{"spec":{"replicas":1}}`) s.K8s.WaitPodGone(t, harness.PodName(cluster, 1)) }) - // With grpcTunnel.tls the pod template enumerates one certificate source - // per ordinal, so a replica change also rolls the existing pod -- after the - // new ordinal is Ready. Wait for the whole rollout, or a forward opened to - // old pod-0 dies seconds later (seen in CI). The forward from before the - // scale is stale for the same reason. + // Wait for the StatefulSet to settle at two Ready replicas on one revision + // before forwarding: a forward opened to a pod that is about to be rolled + // dies seconds later (seen in CI, back when the tunnel certificate was + // projected per ordinal and a scale rolled pod-0 too). The forward from + // before the scale is re-opened for the same reason. s.K8s.WaitStatefulSetRolledOut(t, cluster, 2) harness.WaitConfigApplied(t, s.K8s, cluster) get0 = collectorAPI(t, 0) diff --git a/test/integration/suite/010-tls/tls_test.go b/test/integration/suite/010-tls/tls_test.go index d3499c7..b5f1661 100644 --- a/test/integration/suite/010-tls/tls_test.go +++ b/test/integration/suite/010-tls/tls_test.go @@ -5,12 +5,14 @@ // // Test -> ID: // -// TestTLS001_ClientCertsMounted -> 010-1 -// TestTLS002_MissingIssuerGatesReady -> 010-2 -// TestTLS003_SkipVerifyCollectsTLS -> 010-3 -// TestTLS004_CAVerification -> 010-4 -// TestTLS005_APITLSKeepsConfigPath -> 010-5 -// TestTLS006_ClientCertRotation -> 010-6 +// TestTLS001_ClientCertsMounted -> 010-1 +// TestTLS002_MissingIssuerGatesReady -> 010-2 +// TestTLS003_SkipVerifyCollectsTLS -> 010-3 +// TestTLS004_CAVerification -> 010-4 +// TestTLS005_APITLSKeepsConfigPath -> 010-5 +// TestTLS006_ClientCertRotation -> 010-6 +// TestTLS007_ScaleDoesNotRollPods -> 010-7 +// TestTLS008_LegacyPerPodCertificateRemoved -> 010-8 // // Run: // @@ -27,6 +29,7 @@ import ( certmanagerv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -215,7 +218,7 @@ func patchAPITLS(t *testing.T, issuerRef string) { t.Helper() s.K8s.Patch(t, s.K8s.Cluster(t, cluster), fmt.Sprintf( `{"spec":{"api":{"restPort":7890,"tls":{"issuerRef":%q}}}}`, issuerRef)) - waitCertReady(t, harness.APICertName(cluster, 0)) + waitCertReady(t, harness.APICertName(cluster)) harness.WaitClusterCondition(t, s.K8s, cluster, harness.CondCertificatesReady, metav1.ConditionTrue, harness.Long) waitClusterReady(t, cluster) waitPodFile(t, cluster, "/etc/gnmic/tls/tls.crt") @@ -637,3 +640,106 @@ func keysOf(m map[string][]byte) []string { } return out } + +// TestTLS007_ScaleDoesNotRollPods: with api.tls, scaling used to roll every +// existing pod, because the pod template enumerated one certificate Secret per +// ordinal. One cluster-wide certificate keeps the template independent of the +// replica count, so pod-0 survives a scale to two untouched. +func TestTLS007_ScaleDoesNotRollPods(t *testing.T) { + waitIdle(t) + const name = "scale" + applyCluster(t, name, map[string]any{"APIIssuer": issuer}) + waitCertReady(t, harness.APICertName(name)) + waitClusterReady(t, name) + pod0 := s.K8s.WaitReadyPods(t, name, 1, harness.Long)[0] + before := s.K8s.StatefulSet(t, harness.StatefulSetName(name)) + + s.K8s.Patch(t, s.K8s.Cluster(t, name), `{"spec":{"replicas":2}}`) + s.K8s.WaitStatefulSetRolledOut(t, name, 2) + s.K8s.WaitReadyPods(t, name, 2, harness.Long) + + after := s.K8s.StatefulSet(t, harness.StatefulSetName(name)) + if after.Status.UpdateRevision != before.Status.UpdateRevision { + t.Errorf("pod template revision changed on scale: %s -> %s", before.Status.UpdateRevision, after.Status.UpdateRevision) + } + var p corev1.Pod + if err := s.K8s.Client.Get(s.Ctx, types.NamespacedName{Namespace: s.Namespace, Name: pod0.Name}, &p); err != nil { + t.Fatalf("pod-0 after scale: %v", err) + } + if p.UID != pod0.UID { + t.Errorf("pod-0 was replaced by the scale (uid %s -> %s)", pod0.UID, p.UID) + } + + // One server certificate for the cluster, none per pod. + var certs certmanagerv1.CertificateList + if err := s.K8s.Client.List(s.Ctx, &certs, client.InNamespace(s.Namespace), + client.MatchingLabels{harness.LabelClusterName: name}); err != nil { + t.Fatal(err) + } + var names []string + for _, c := range certs.Items { + names = append(names, c.Name) + } + if len(certs.Items) != 1 || certs.Items[0].Name != harness.APICertName(name) { + t.Errorf("certificates for %s = %v, want only %s", name, names, harness.APICertName(name)) + } +} + +// TestTLS008_LegacyPerPodCertificateRemoved: operators before 0.6.0 issued one +// Certificate per pod (--tls). After the upgrade the cluster gets +// its wildcard certificate and, once the pods run the template that mounts it, +// the legacy Certificates are deleted. Their Secrets stay behind: the operator +// never writes Secrets (the TLS guide gives the command to remove them). +func TestTLS008_LegacyPerPodCertificateRemoved(t *testing.T) { + waitIdle(t) + const name = "legacy" + legacyName := harness.StatefulSetName(name) + "-0-tls" + // What the old operator left behind: a per-pod Certificate carrying the + // cluster label, and the Secret cert-manager issued for it. + s.K8s.ApplyYAML(t, ` +apiVersion: cert-manager.io/v1 +kind: Certificate +metadata: + name: {{ .Name }} + labels: + operator.gnmic.dev/cluster: {{ .Cluster }} +spec: + secretName: {{ .Name }} + secretTemplate: + labels: + operator.gnmic.dev/cluster: {{ .Cluster }} + operator.gnmic.dev/pod-name: {{ .Pod }} + issuerRef: + name: {{ .Issuer }} + kind: Issuer + commonName: {{ .Pod }} + dnsNames: [{{ .Pod }}] +`, map[string]any{"Name": legacyName, "Cluster": name, "Pod": harness.PodName(name, 0), "Issuer": issuer}) + waitCertReady(t, legacyName) + t.Cleanup(func() { + _ = s.K8s.Client.Delete(s.Ctx, &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Namespace: s.Namespace, Name: legacyName}}) + }) + + applyCluster(t, name, map[string]any{"APIIssuer": issuer}) + waitCertReady(t, harness.APICertName(name)) + waitClusterReady(t, name) + + harness.Wait(t, harness.Long, "legacy per-pod Certificate deleted", func() (bool, string) { + var c certmanagerv1.Certificate + err := s.K8s.Client.Get(s.Ctx, types.NamespacedName{Namespace: s.Namespace, Name: legacyName}, &c) + if apierrors.IsNotFound(err) { + return true, "" + } + if err != nil { + return false, err.Error() + } + return false, "still present" + }) + var sec corev1.Secret + if err := s.K8s.Client.Get(s.Ctx, types.NamespacedName{Namespace: s.Namespace, Name: legacyName}, &sec); err != nil { + t.Errorf("legacy Secret %s should be left in place: %v", legacyName, err) + } + if !conditionTrue(s.K8s.Cluster(t, name), harness.CondReady) { + t.Error("cluster lost Ready during the legacy cleanup") + } +}