diff --git a/pkg/tls/certificatemanagement/keypair.go b/pkg/tls/certificatemanagement/keypair.go index 891869b8f9..b42a15e0be 100644 --- a/pkg/tls/certificatemanagement/keypair.go +++ b/pkg/tls/certificatemanagement/keypair.go @@ -277,15 +277,22 @@ func GetKeyCertPEM(secret *corev1.Secret) ([]byte, []byte) { legacySecretCertName5 = "management-cluster.crt" ) data := secret.Data - for keyField, certField := range map[string]string{ - corev1.TLSPrivateKeyKey: corev1.TLSCertKey, - legacySecretKeyName: legacySecretCertName, - legacySecretKeyName2: legacySecretCertName2, - legacySecretKeyName3: legacySecretCertName3, - legacySecretKeyName4: legacySecretCertName4, - legacySecretKeyName5: legacySecretCertName5, + // Check the recognised key/cert field-name pairs in a fixed priority order, standard + // tls.crt/tls.key first, then the legacy names. Iterating a map here would be a bug: Go + // randomizes map iteration order, so a secret that contains more than one recognised pair + // (e.g. the standard tls.crt/tls.key alongside a legacy cert/key that is intentionally kept + // during a certificate-rotation overlap) would return a non-deterministic cert. That flips + // the KeyPair's hash annotation between reconciles, which in turn triggers spurious rolling + // restarts of consumers such as Voltron/tigera-manager (dropping managed-cluster tunnels). + for _, pair := range []struct{ keyField, certField string }{ + {corev1.TLSPrivateKeyKey, corev1.TLSCertKey}, + {legacySecretKeyName, legacySecretCertName}, + {legacySecretKeyName2, legacySecretCertName2}, + {legacySecretKeyName3, legacySecretCertName3}, + {legacySecretKeyName4, legacySecretCertName4}, + {legacySecretKeyName5, legacySecretCertName5}, } { - key, cert := data[keyField], data[certField] + key, cert := data[pair.keyField], data[pair.certField] if len(cert) > 0 { return key, cert } diff --git a/pkg/tls/certificatemanagement/keypair_test.go b/pkg/tls/certificatemanagement/keypair_test.go index f948515faa..9d3e27c283 100644 --- a/pkg/tls/certificatemanagement/keypair_test.go +++ b/pkg/tls/certificatemanagement/keypair_test.go @@ -28,6 +28,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + "github.com/tigera/operator/pkg/tls/certificatemanagement" ) @@ -202,4 +204,52 @@ var _ = Describe("TLS secret metadata", func() { Expect(secret.Labels["certificates.operator.tigera.io/signer"]).To(Equal("unknown")) }) }) + + Describe("GetKeyCertPEM()", func() { + It("should read the standard tls.crt/tls.key pair", func() { + secret := &corev1.Secret{Data: map[string][]byte{ + "tls.key": []byte("standard-key"), + "tls.crt": []byte("standard-cert"), + }} + key, cert := certificatemanagement.GetKeyCertPEM(secret) + Expect(string(key)).To(Equal("standard-key")) + Expect(string(cert)).To(Equal("standard-cert")) + }) + + It("should fall back to legacy cert/key names when the standard pair is absent", func() { + secret := &corev1.Secret{Data: map[string][]byte{ + "key": []byte("legacy-key"), + "cert": []byte("legacy-cert"), + }} + key, cert := certificatemanagement.GetKeyCertPEM(secret) + Expect(string(key)).To(Equal("legacy-key")) + Expect(string(cert)).To(Equal("legacy-cert")) + }) + + // Regression test: a secret may legitimately hold both the standard tls.crt/tls.key pair + // and a legacy cert/key pair at the same time (e.g. an expired legacy cert kept during a + // rotation overlap). Selection must be deterministic and always prefer the standard pair; + // otherwise the KeyPair hash annotation flaps and rolls consumers like tigera-manager. + It("should deterministically prefer tls.crt/tls.key when both standard and legacy pairs exist", func() { + secret := &corev1.Secret{Data: map[string][]byte{ + "tls.key": []byte("standard-key"), + "tls.crt": []byte("standard-cert"), + "key": []byte("legacy-key"), + "cert": []byte("legacy-cert"), + }} + // Run many times to defeat Go's randomized map iteration order. + for i := 0; i < 100; i++ { + key, cert := certificatemanagement.GetKeyCertPEM(secret) + Expect(string(key)).To(Equal("standard-key")) + Expect(string(cert)).To(Equal("standard-cert")) + } + }) + + It("should return nil when no recognised pair is present", func() { + secret := &corev1.Secret{Data: map[string][]byte{"unrelated": []byte("x")}} + key, cert := certificatemanagement.GetKeyCertPEM(secret) + Expect(key).To(BeNil()) + Expect(cert).To(BeNil()) + }) + }) })