From 7be177a2c4c721c8f36adda14fd4ff92f4b83dba Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 04:12:42 +0000 Subject: [PATCH 1/4] feat: enable cert rotation for allocation API service (fixes #422) Add CertificateWatcher that polls TLS cert files for changes and reloads them dynamically. Refactor AllocationApiServer to use GetCertificate/GetConfigForClient callbacks instead of static cert bytes. Mount tls-secret as volume for file-based cert access. Closes #422 Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com> Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/3528b7ff-34c9-4621-a98f-b9b8427da515 --- docs/quickstart/installing-thundernetes.md | 4 +- .../config/default/kustomization.yaml | 4 + .../default/manager_alloc_tls_patch.yaml | 20 ++ pkg/operator/config/manager/manager.yaml | 2 + .../controllers/allocation_api_server.go | 35 +- .../controllers/allocation_api_server_test.go | 14 +- pkg/operator/controllers/cert_watcher.go | 152 +++++++++ pkg/operator/controllers/cert_watcher_test.go | 311 ++++++++++++++++++ pkg/operator/controllers/config.go | 1 + pkg/operator/controllers/suite_test.go | 2 +- pkg/operator/main.go | 55 ++-- 11 files changed, 535 insertions(+), 65 deletions(-) create mode 100644 pkg/operator/config/default/manager_alloc_tls_patch.yaml create mode 100644 pkg/operator/controllers/cert_watcher.go create mode 100644 pkg/operator/controllers/cert_watcher_test.go diff --git a/docs/quickstart/installing-thundernetes.md b/docs/quickstart/installing-thundernetes.md index 8f40faa7..7ceba964 100644 --- a/docs/quickstart/installing-thundernetes.md +++ b/docs/quickstart/installing-thundernetes.md @@ -80,7 +80,7 @@ There are two ways to generate a certificate. #### Using cert-manager to generate certificates -Since cert-manager is already installed in the cluster, it can be used to generate a certificate for mTLS authentication. This is the recommended approach. +Since cert-manager is already installed in the cluster, it can be used to generate a certificate for mTLS authentication. This is the recommended approach. When using cert-manager, certificate rotation is handled automatically — Thundernetes watches the mounted certificate files and reloads them when they change, so there is no need to restart the controller pod when certificates are renewed. First of all, you need to create the namespace `thundernetes-system`: @@ -155,6 +155,8 @@ kubectl apply --server-side -f https://raw.githubusercontent.com/PlayFab/thunder **Note:** The two installation files (operator.yaml and operator_with_security.yaml) are identical except for the API_SERVICE_SECURITY environment variable that is passed into the controller container. +> **Certificate rotation:** When TLS is enabled, Thundernetes automatically monitors the mounted TLS secret for changes and reloads the certificate without requiring a pod restart. This works with cert-manager automatic renewal or manual secret updates. The controller polls for certificate file changes every 30 seconds, so renewed certificates are picked up shortly after kubelet syncs the updated secret to the pod (typically within ~60 seconds total). + ### Next steps Check the [.NET sample](sample-dotnet.md) document to learn how to test your installation by using our fake .NET game server sample. \ No newline at end of file diff --git a/pkg/operator/config/default/kustomization.yaml b/pkg/operator/config/default/kustomization.yaml index f8ac8dea..9c43b11a 100644 --- a/pkg/operator/config/default/kustomization.yaml +++ b/pkg/operator/config/default/kustomization.yaml @@ -38,6 +38,10 @@ patchesStrategicMerge: # crd/kustomization.yaml - manager_webhook_patch.yaml +# [TLS] Mount the tls-secret for the allocation API service cert rotation +# The secret is optional so the pod starts even without TLS configured +- manager_alloc_tls_patch.yaml + # [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'. # Uncomment 'CERTMANAGER' sections in crd/kustomization.yaml to enable the CA injection in the admission webhooks. # 'CERTMANAGER' needs to be enabled to use ca injection diff --git a/pkg/operator/config/default/manager_alloc_tls_patch.yaml b/pkg/operator/config/default/manager_alloc_tls_patch.yaml new file mode 100644 index 00000000..d93e43c4 --- /dev/null +++ b/pkg/operator/config/default/manager_alloc_tls_patch.yaml @@ -0,0 +1,20 @@ +apiVersion: apps/v1 +kind: Deployment +metadata: + name: controller-manager + namespace: system +spec: + template: + spec: + containers: + - name: manager + volumeMounts: + - mountPath: /tmp/alloc-api-serving-certs + name: alloc-api-cert + readOnly: true + volumes: + - name: alloc-api-cert + secret: + defaultMode: 420 + secretName: tls-secret + optional: true diff --git a/pkg/operator/config/manager/manager.yaml b/pkg/operator/config/manager/manager.yaml index bedd11e1..34bb860c 100644 --- a/pkg/operator/config/manager/manager.yaml +++ b/pkg/operator/config/manager/manager.yaml @@ -54,6 +54,8 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace + - name: TLS_CERT_DIR + value: "/tmp/alloc-api-serving-certs" name: manager securityContext: allowPrivilegeEscalation: false diff --git a/pkg/operator/controllers/allocation_api_server.go b/pkg/operator/controllers/allocation_api_server.go index 97ea638d..3e8c045b 100644 --- a/pkg/operator/controllers/allocation_api_server.go +++ b/pkg/operator/controllers/allocation_api_server.go @@ -3,7 +3,6 @@ package controllers import ( "context" "crypto/tls" - "crypto/x509" "encoding/json" "errors" "fmt" @@ -38,10 +37,9 @@ const ( // so it can be added to our Manager type AllocationApiServer struct { Client client.Client - // CrtBytes is the PEM-encoded certificate - CrtBytes []byte - // KeyBytes is the PEM-encoded key - KeyBytes []byte + // certWatcher watches and reloads TLS certificates from disk for dynamic cert rotation. + // If nil, the server runs without TLS. + certWatcher *CertificateWatcher // gameServerQueue is a map of priority queues for game servers gameServerQueue *GameServersQueue // events is a buffered channel of GenericEvent @@ -51,10 +49,9 @@ type AllocationApiServer struct { listeningPort int32 } -func NewAllocationApiServer(crt, key []byte, cl client.Client, port int32) *AllocationApiServer { +func NewAllocationApiServer(certWatcher *CertificateWatcher, cl client.Client, port int32) *AllocationApiServer { return &AllocationApiServer{ - CrtBytes: crt, - KeyBytes: key, + certWatcher: certWatcher, Client: cl, events: make(chan event.GenericEvent, 100), logger: log.Log.WithName("allocation-api"), @@ -103,25 +100,19 @@ func (s *AllocationApiServer) Start(ctx context.Context) error { close(done) }() - if s.CrtBytes != nil && s.KeyBytes != nil { - s.logger.Info("starting TLS enabled allocation API service") - // Generate a key pair from your pem-encoded cert and key ([]byte). - cert, err := tls.X509KeyPair(s.CrtBytes, s.KeyBytes) - if err != nil { - return nil - } - caCertPool := x509.NewCertPool() - caCertPool.AppendCertsFromPEM(s.CrtBytes) - // Construct a tls.config + if s.certWatcher != nil { + s.logger.Info("starting TLS enabled allocation API service with dynamic certificate rotation") + // Use dynamic TLS configuration via CertificateWatcher callbacks + // This enables automatic certificate rotation without server restart tlsConfig := &tls.Config{ - Certificates: []tls.Certificate{cert}, - ClientCAs: caCertPool, - ClientAuth: tls.RequireAndVerifyClientCert, + GetCertificate: s.certWatcher.GetCertificate, + GetConfigForClient: s.certWatcher.GetConfigForClient, + ClientAuth: tls.RequireAndVerifyClientCert, } // Build a server: srv.TLSConfig = tlsConfig - // Finally: serve. + // Finally: serve. Empty strings because certs are provided via GetCertificate callback. if err := srv.ListenAndServeTLS("", ""); err != nil && err != http.ErrServerClosed { return err } diff --git a/pkg/operator/controllers/allocation_api_server_test.go b/pkg/operator/controllers/allocation_api_server_test.go index 47483fac..11177ed8 100644 --- a/pkg/operator/controllers/allocation_api_server_test.go +++ b/pkg/operator/controllers/allocation_api_server_test.go @@ -27,7 +27,7 @@ var _ = Describe("allocation API service input validation tests", func() { It("empty body should return error", func() { req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", nil) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, nil, allocationApiSvcPort) + h := NewAllocationApiServer(nil, nil, allocationApiSvcPort) h.handleAllocationRequest(w, req) res := w.Result() defer res.Body.Close() @@ -38,7 +38,7 @@ var _ = Describe("allocation API service input validation tests", func() { It("GET method should return error", func() { req := httptest.NewRequest(http.MethodGet, "/api/v1/allocate", nil) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, nil, allocationApiSvcPort) + h := NewAllocationApiServer(nil, nil, allocationApiSvcPort) h.handleAllocationRequest(w, req) res := w.Result() defer res.Body.Close() @@ -49,7 +49,7 @@ var _ = Describe("allocation API service input validation tests", func() { It("bad body should return error", func() { req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", bytes.NewBufferString("{\"foo\":\"bar\"}")) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, nil, allocationApiSvcPort) + h := NewAllocationApiServer(nil, nil, allocationApiSvcPort) h.handleAllocationRequest(w, req) res := w.Result() defer res.Body.Close() @@ -60,7 +60,7 @@ var _ = Describe("allocation API service input validation tests", func() { It("buildID should be a GUID", func() { req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", bytes.NewBufferString("{\"buildID\":\"NOT_A_GUID\",\"sessionID\":\"9bb3bbb2-5031-42fd-8982-5a3f76ef2c8a\"}")) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, nil, allocationApiSvcPort) + h := NewAllocationApiServer(nil, nil, allocationApiSvcPort) h.handleAllocationRequest(w, req) res := w.Result() defer res.Body.Close() @@ -71,7 +71,7 @@ var _ = Describe("allocation API service input validation tests", func() { It("should return NotFound on an empty list", func() { req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", bytes.NewBufferString("{\"sessionID\":\"9bb3bbb2-5031-42fd-8982-5a3f76ef2c8a\",\"buildID\":\"9bb3bbb2-5031-42fd-8982-5a3f76ef2c8a\"}")) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, testNewSimpleK8sClient(), allocationApiSvcPort) + h := NewAllocationApiServer(nil, testNewSimpleK8sClient(), allocationApiSvcPort) h.handleAllocationRequest(w, req) res := w.Result() defer res.Body.Close() @@ -85,7 +85,7 @@ var _ = Describe("allocation API service input validation tests", func() { Expect(err).ToNot(HaveOccurred()) req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", bytes.NewBufferString(fmt.Sprintf("{\"sessionID\":\"%s\",\"buildID\":\"%s\"}", sessionID1, buildID1))) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, client, allocationApiSvcPort) + h := NewAllocationApiServer(nil, client, allocationApiSvcPort) h.handleAllocationRequest(w, req) res := w.Result() defer res.Body.Close() @@ -105,7 +105,7 @@ var _ = Describe("allocation API service input validation tests", func() { Expect(err).ToNot(HaveOccurred()) req := httptest.NewRequest(http.MethodPost, "/api/v1/allocate", bytes.NewBufferString(fmt.Sprintf("{\"sessionID\":\"%s\",\"buildID\":\"%s\"}", sessionID1, buildID1))) w := httptest.NewRecorder() - h := NewAllocationApiServer(nil, nil, client, allocationApiSvcPort) + h := NewAllocationApiServer(nil, client, allocationApiSvcPort) h.gameServerQueue = NewGameServersQueue() h.gameServerQueue.PushToQueue(&GameServerForQueue{ Name: gsName, diff --git a/pkg/operator/controllers/cert_watcher.go b/pkg/operator/controllers/cert_watcher.go new file mode 100644 index 00000000..b7a23a9e --- /dev/null +++ b/pkg/operator/controllers/cert_watcher.go @@ -0,0 +1,152 @@ +package controllers + +import ( + "context" + "crypto/tls" + "crypto/x509" + "fmt" + "os" + "sync" + "time" + + "github.com/go-logr/logr" + "sigs.k8s.io/controller-runtime/pkg/log" +) + +const ( + // defaultCertPollInterval is how often the CertificateWatcher checks for cert file changes + defaultCertPollInterval = 30 * time.Second +) + +// CertificateWatcher watches certificate files on disk and reloads them when they change. +// It is designed to work with Kubernetes Secret volume mounts, where kubelet atomically +// rotates the files via symlink swaps. +// CertificateWatcher implements the manager.Runnable interface so it can be added to the controller manager. +type CertificateWatcher struct { + mu sync.RWMutex + currentCert *tls.Certificate + caCertPool *x509.CertPool + certPath string + keyPath string + pollInterval time.Duration + logger logr.Logger +} + +// NewCertificateWatcher creates a new CertificateWatcher that monitors the given cert and key files. +func NewCertificateWatcher(certPath, keyPath string) *CertificateWatcher { + return &CertificateWatcher{ + certPath: certPath, + keyPath: keyPath, + pollInterval: defaultCertPollInterval, + logger: log.Log.WithName("cert-watcher"), + } +} + +// SetPollInterval sets the polling interval for file change detection. +// Must be called before Start(). +func (cw *CertificateWatcher) SetPollInterval(d time.Duration) { + cw.pollInterval = d +} + +// LoadCertificate reads and parses the certificate and key from disk. +// It is safe to call concurrently. +func (cw *CertificateWatcher) LoadCertificate() error { + certPEM, err := os.ReadFile(cw.certPath) + if err != nil { + return fmt.Errorf("reading certificate file %s: %w", cw.certPath, err) + } + keyPEM, err := os.ReadFile(cw.keyPath) + if err != nil { + return fmt.Errorf("reading key file %s: %w", cw.keyPath, err) + } + + cert, err := tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return fmt.Errorf("parsing TLS key pair: %w", err) + } + + caCertPool := x509.NewCertPool() + caCertPool.AppendCertsFromPEM(certPEM) + + cw.mu.Lock() + cw.currentCert = &cert + cw.caCertPool = caCertPool + cw.mu.Unlock() + + cw.logger.Info("loaded TLS certificate", "certPath", cw.certPath, "keyPath", cw.keyPath) + return nil +} + +// GetCertificate returns the current TLS certificate. +// It is intended to be used as the tls.Config.GetCertificate callback. +func (cw *CertificateWatcher) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error) { + cw.mu.RLock() + defer cw.mu.RUnlock() + if cw.currentCert == nil { + return nil, fmt.Errorf("no certificate loaded") + } + return cw.currentCert, nil +} + +// GetConfigForClient returns a tls.Config with the current CA cert pool for client certificate verification. +// It is intended to be used as the tls.Config.GetConfigForClient callback. +func (cw *CertificateWatcher) GetConfigForClient(_ *tls.ClientHelloInfo) (*tls.Config, error) { + cw.mu.RLock() + defer cw.mu.RUnlock() + if cw.caCertPool == nil { + return nil, fmt.Errorf("no CA certificate pool loaded") + } + return &tls.Config{ + ClientCAs: cw.caCertPool, + ClientAuth: tls.RequireAndVerifyClientCert, + GetCertificate: cw.GetCertificate, + GetConfigForClient: nil, // avoid recursive call + }, nil +} + +// Start implements the manager.Runnable interface. +// It polls the cert files for changes and reloads them when modified. +func (cw *CertificateWatcher) Start(ctx context.Context) error { + cw.logger.Info("starting certificate watcher", "certPath", cw.certPath, "keyPath", cw.keyPath, "pollInterval", cw.pollInterval) + + ticker := time.NewTicker(cw.pollInterval) + defer ticker.Stop() + + var lastCertModTime, lastKeyModTime time.Time + + // Initialize modification times + if info, err := os.Stat(cw.certPath); err == nil { + lastCertModTime = info.ModTime() + } + if info, err := os.Stat(cw.keyPath); err == nil { + lastKeyModTime = info.ModTime() + } + + for { + select { + case <-ctx.Done(): + cw.logger.Info("stopping certificate watcher") + return nil + case <-ticker.C: + changed := false + if info, err := os.Stat(cw.certPath); err == nil { + if !info.ModTime().Equal(lastCertModTime) { + lastCertModTime = info.ModTime() + changed = true + } + } + if info, err := os.Stat(cw.keyPath); err == nil { + if !info.ModTime().Equal(lastKeyModTime) { + lastKeyModTime = info.ModTime() + changed = true + } + } + if changed { + cw.logger.Info("certificate file change detected, reloading") + if err := cw.LoadCertificate(); err != nil { + cw.logger.Error(err, "failed to reload certificate, keeping previous certificate") + } + } + } + } +} diff --git a/pkg/operator/controllers/cert_watcher_test.go b/pkg/operator/controllers/cert_watcher_test.go new file mode 100644 index 00000000..c7fa049d --- /dev/null +++ b/pkg/operator/controllers/cert_watcher_test.go @@ -0,0 +1,311 @@ +package controllers + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// generateTestCert creates a self-signed certificate and returns PEM-encoded cert and key bytes. +func generateTestCert(t *testing.T, cn string) ([]byte, []byte) { + t.Helper() + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + template := x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: cn}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, + IsCA: true, + BasicConstraintsValid: true, + } + + certDER, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + require.NoError(t, err) + + certPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}) + + keyDER, err := x509.MarshalECPrivateKey(key) + require.NoError(t, err) + keyPEM := pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) + + return certPEM, keyPEM +} + +// writeCertFiles writes cert and key PEM bytes to the specified directory. +func writeCertFiles(t *testing.T, dir string, certPEM, keyPEM []byte) { + t.Helper() + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.crt"), certPEM, 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.key"), keyPEM, 0600)) +} + +func TestCertificateWatcher_InitialLoad(t *testing.T) { + dir := t.TempDir() + certPEM, keyPEM := generateTestCert(t, "initial-load-test") + writeCertFiles(t, dir, certPEM, keyPEM) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), + ) + + err := cw.LoadCertificate() + require.NoError(t, err) + + // Verify GetCertificate returns a valid cert + cert, err := cw.GetCertificate(nil) + require.NoError(t, err) + assert.NotNil(t, cert) + + // Verify GetConfigForClient returns a valid config + cfg, err := cw.GetConfigForClient(nil) + require.NoError(t, err) + assert.NotNil(t, cfg) + assert.Equal(t, tls.RequireAndVerifyClientCert, cfg.ClientAuth) + assert.NotNil(t, cfg.ClientCAs) +} + +func TestCertificateWatcher_GetCertificateBeforeLoad(t *testing.T) { + cw := NewCertificateWatcher("/nonexistent/tls.crt", "/nonexistent/tls.key") + + // GetCertificate should return an error if no cert has been loaded + cert, err := cw.GetCertificate(nil) + assert.Error(t, err) + assert.Nil(t, cert) + + // GetConfigForClient should also return an error + cfg, err := cw.GetConfigForClient(nil) + assert.Error(t, err) + assert.Nil(t, cfg) +} + +func TestCertificateWatcher_MissingFiles(t *testing.T) { + cw := NewCertificateWatcher("/nonexistent/tls.crt", "/nonexistent/tls.key") + + err := cw.LoadCertificate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "reading certificate file") +} + +func TestCertificateWatcher_MissingKeyFile(t *testing.T) { + dir := t.TempDir() + certPEM, _ := generateTestCert(t, "missing-key-test") + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.crt"), certPEM, 0600)) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), // does not exist + ) + + err := cw.LoadCertificate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "reading key file") +} + +func TestCertificateWatcher_InvalidCertContent(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.crt"), []byte("not a cert"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.key"), []byte("not a key"), 0600)) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), + ) + + err := cw.LoadCertificate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "parsing TLS key pair") +} + +func TestCertificateWatcher_Reload(t *testing.T) { + dir := t.TempDir() + + // Write initial cert + certPEM1, keyPEM1 := generateTestCert(t, "cert-v1") + writeCertFiles(t, dir, certPEM1, keyPEM1) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), + ) + + err := cw.LoadCertificate() + require.NoError(t, err) + + cert1, err := cw.GetCertificate(nil) + require.NoError(t, err) + + // Write a new cert (different CN) + certPEM2, keyPEM2 := generateTestCert(t, "cert-v2") + writeCertFiles(t, dir, certPEM2, keyPEM2) + + // Reload + err = cw.LoadCertificate() + require.NoError(t, err) + + cert2, err := cw.GetCertificate(nil) + require.NoError(t, err) + + // The two certs should be different + assert.NotEqual(t, cert1.Certificate, cert2.Certificate) +} + +func TestCertificateWatcher_StartDetectsFileChange(t *testing.T) { + dir := t.TempDir() + + // Write initial cert + certPEM1, keyPEM1 := generateTestCert(t, "start-test-v1") + writeCertFiles(t, dir, certPEM1, keyPEM1) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), + ) + cw.SetPollInterval(50 * time.Millisecond) // fast polling for tests + + err := cw.LoadCertificate() + require.NoError(t, err) + + cert1, err := cw.GetCertificate(nil) + require.NoError(t, err) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // Start the watcher in a goroutine + go func() { + _ = cw.Start(ctx) + }() + + // Wait a bit, then write a new cert + time.Sleep(100 * time.Millisecond) + certPEM2, keyPEM2 := generateTestCert(t, "start-test-v2") + writeCertFiles(t, dir, certPEM2, keyPEM2) + + // Wait for the watcher to detect the change + assert.Eventually(t, func() bool { + cert2, err := cw.GetCertificate(nil) + if err != nil { + return false + } + return !certsEqual(cert1, cert2) + }, 2*time.Second, 50*time.Millisecond, "watcher should detect cert file change and reload") + + cancel() +} + +func TestCertificateWatcher_ConcurrentAccess(t *testing.T) { + dir := t.TempDir() + certPEM, keyPEM := generateTestCert(t, "concurrent-test") + writeCertFiles(t, dir, certPEM, keyPEM) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), + ) + + err := cw.LoadCertificate() + require.NoError(t, err) + + var wg sync.WaitGroup + const concurrency = 50 + + // Multiple readers + for i := 0; i < concurrency; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 100; j++ { + cert, err := cw.GetCertificate(nil) + assert.NoError(t, err) + assert.NotNil(t, cert) + + cfg, err := cw.GetConfigForClient(nil) + assert.NoError(t, err) + assert.NotNil(t, cfg) + } + }() + } + + // Concurrent reloaders + for i := 0; i < 5; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 20; j++ { + err := cw.LoadCertificate() + assert.NoError(t, err) + } + }() + } + + wg.Wait() +} + +func TestCertificateWatcher_ReloadKeepsPreviousOnError(t *testing.T) { + dir := t.TempDir() + + // Write valid cert + certPEM, keyPEM := generateTestCert(t, "keep-previous-test") + writeCertFiles(t, dir, certPEM, keyPEM) + + cw := NewCertificateWatcher( + filepath.Join(dir, "tls.crt"), + filepath.Join(dir, "tls.key"), + ) + + err := cw.LoadCertificate() + require.NoError(t, err) + + cert1, err := cw.GetCertificate(nil) + require.NoError(t, err) + + // Now write invalid cert content + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.crt"), []byte("invalid"), 0600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "tls.key"), []byte("invalid"), 0600)) + + // LoadCertificate should fail + err = cw.LoadCertificate() + assert.Error(t, err) + + // Previous cert should still be available + cert2, err := cw.GetCertificate(nil) + require.NoError(t, err) + assert.Equal(t, cert1.Certificate, cert2.Certificate) +} + +// certsEqual checks if two TLS certificates have the same raw certificate data. +func certsEqual(a, b *tls.Certificate) bool { + if len(a.Certificate) != len(b.Certificate) { + return false + } + for i := range a.Certificate { + if len(a.Certificate[i]) != len(b.Certificate[i]) { + return false + } + for j := range a.Certificate[i] { + if a.Certificate[i][j] != b.Certificate[i][j] { + return false + } + } + } + return true +} diff --git a/pkg/operator/controllers/config.go b/pkg/operator/controllers/config.go index 2431e3eb..26842596 100644 --- a/pkg/operator/controllers/config.go +++ b/pkg/operator/controllers/config.go @@ -8,6 +8,7 @@ type Config struct { TlsSecretNamespace string `env:"TLS_SECRET_NAMESPACE" envDefault:"thundernetes-system"` TlsCertificateName string `env:"TLS_CERTIFICATE_FILENAME" envDefault:"tls.crt"` TlsPrivateKeyFilename string `env:"TLS_PRIVATE_KEY_FILENAME" envDefault:"tls.key"` + TlsCertDir string `env:"TLS_CERT_DIR" envDefault:"/tmp/alloc-api-serving-certs"` PortRegistryExclusivelyGameServerNodes bool `env:"PORT_REGISTRY_EXCLUSIVELY_GAME_SERVER_NODES" envDefault:"false"` LogLevel string `env:"LOG_LEVEL" envDefault:"info"` MinPort int32 `env:"MIN_PORT" envDefault:"10000"` diff --git a/pkg/operator/controllers/suite_test.go b/pkg/operator/controllers/suite_test.go index 8f83feb4..3860976e 100644 --- a/pkg/operator/controllers/suite_test.go +++ b/pkg/operator/controllers/suite_test.go @@ -126,7 +126,7 @@ var _ = BeforeSuite(func() { Expect(err).ToNot(HaveOccurred()) // allocation api service is a controller, so add it to the manager - testAllocationApiServer = NewAllocationApiServer(nil, nil, k8sManager.GetClient(), allocationApiSvcPort) + testAllocationApiServer = NewAllocationApiServer(nil, k8sManager.GetClient(), allocationApiSvcPort) err = testAllocationApiServer.SetupWithManager(k8sManager) Expect(err).ToNot(HaveOccurred()) diff --git a/pkg/operator/main.go b/pkg/operator/main.go index 269dee26..0006b8dc 100644 --- a/pkg/operator/main.go +++ b/pkg/operator/main.go @@ -22,6 +22,7 @@ import ( "flag" "log" "os" + "path/filepath" "time" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) @@ -31,7 +32,6 @@ import ( _ "go.uber.org/automaxprocs" "go.uber.org/zap/zapcore" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" _ "k8s.io/client-go/plugin/pkg/client/auth" @@ -105,13 +105,29 @@ func main() { setupLog.Error(err, "unable to start manager") os.Exit(1) } - // initialize a live API client, used for the PortRegistry and fetching the mTLS secret + // initialize a live API client, used for the PortRegistry k8sClient := mgr.GetAPIReader() - // get public and private key, if enabled - crt, key := getCrtKeyIfTlsEnabled(k8sClient, cfg) + + // initialize the certificate watcher for the allocation API service, if TLS is enabled + var certWatcher *controllers.CertificateWatcher + if cfg.ApiServiceSecurity == "usetls" { + certPath := filepath.Join(cfg.TlsCertDir, cfg.TlsCertificateName) + keyPath := filepath.Join(cfg.TlsCertDir, cfg.TlsPrivateKeyFilename) + certWatcher = controllers.NewCertificateWatcher(certPath, keyPath) + // load the initial certificate to fail fast if the cert files are missing or invalid + if err := certWatcher.LoadCertificate(); err != nil { + setupLog.Error(err, "unable to load initial TLS certificate for allocation API") + os.Exit(1) + } + // add the cert watcher as a runnable so it polls for cert changes alongside the manager + if err := mgr.Add(certWatcher); err != nil { + setupLog.Error(err, "unable to add certificate watcher to manager") + os.Exit(1) + } + } // initialize the allocation API service, which is also a controller. So we add it to the manager - aas := controllers.NewAllocationApiServer(crt, key, mgr.GetClient(), int32(allocationApiSvcPort)) + aas := controllers.NewAllocationApiServer(certWatcher, mgr.GetClient(), int32(allocationApiSvcPort)) if err = aas.SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create HTTP allocation API Server", "Allocation API Server", "HTTP Allocation API Server") os.Exit(1) @@ -211,20 +227,6 @@ func initializePortRegistry(k8sClient client.Reader, crClient client.Client, set return portRegistry, nil } -// getTlsSecret returns the TLS secret from the given namespace -// used in the allocation API service -func getTlsSecret(k8sClient client.Reader, cfg *controllers.Config) ([]byte, []byte, error) { - var secret corev1.Secret - err := k8sClient.Get(context.Background(), types.NamespacedName{ - Name: cfg.TlsSecretName, - Namespace: cfg.TlsSecretNamespace, - }, &secret) - if err != nil { - return nil, nil, err - } - return []byte(secret.Data[cfg.TlsCertificateName]), []byte(secret.Data[cfg.TlsPrivateKeyFilename]), nil -} - // validateMinMaxPort validates minimum and maximum ports func validateMinMaxPort(cfg *controllers.Config) (int32, int32, error) { if cfg.MinPort >= cfg.MaxPort { @@ -254,18 +256,3 @@ func getLogLevel(logLevel string) zapcore.LevelEnabler { } } -// getCrtKeyIfTlsEnabled returns public and private key components for securing the allocation API service with mTLS -// for this to happen, user has to set "API_SERVICE_SECURITY" env as "usetls" and set the env "TLS_SECRET_NAMESPACE" with the namespace -// that contains the Kubernetes Secret with the cert -// if any of the mentioned conditions are not set, method returns nil -func getCrtKeyIfTlsEnabled(c client.Reader, cfg *controllers.Config) ([]byte, []byte) { - if cfg.ApiServiceSecurity == "usetls" { - crt, key, err := getTlsSecret(c, cfg) - if err != nil { - setupLog.Error(err, "unable to get TLS secret") - os.Exit(1) - } - return crt, key - } - return nil, nil -} From 78b7ad64a3cbc644ea9dadbd0e7a9a21dbde3128 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 04:17:39 +0000 Subject: [PATCH 2/4] fix: apply go fmt formatting fixes Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com> Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/3528b7ff-34c9-4621-a98f-b9b8427da515 --- pkg/operator/controllers/allocation_api_server.go | 4 ++-- pkg/operator/controllers/cert_watcher_test.go | 14 +++++++------- pkg/operator/main.go | 1 - 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/pkg/operator/controllers/allocation_api_server.go b/pkg/operator/controllers/allocation_api_server.go index 3e8c045b..dada3c3b 100644 --- a/pkg/operator/controllers/allocation_api_server.go +++ b/pkg/operator/controllers/allocation_api_server.go @@ -105,9 +105,9 @@ func (s *AllocationApiServer) Start(ctx context.Context) error { // Use dynamic TLS configuration via CertificateWatcher callbacks // This enables automatic certificate rotation without server restart tlsConfig := &tls.Config{ - GetCertificate: s.certWatcher.GetCertificate, + GetCertificate: s.certWatcher.GetCertificate, GetConfigForClient: s.certWatcher.GetConfigForClient, - ClientAuth: tls.RequireAndVerifyClientCert, + ClientAuth: tls.RequireAndVerifyClientCert, } // Build a server: diff --git a/pkg/operator/controllers/cert_watcher_test.go b/pkg/operator/controllers/cert_watcher_test.go index c7fa049d..13d47d76 100644 --- a/pkg/operator/controllers/cert_watcher_test.go +++ b/pkg/operator/controllers/cert_watcher_test.go @@ -28,13 +28,13 @@ func generateTestCert(t *testing.T, cn string) ([]byte, []byte) { require.NoError(t, err) template := x509.Certificate{ - SerialNumber: big.NewInt(1), - Subject: pkix.Name{CommonName: cn}, - NotBefore: time.Now().Add(-time.Hour), - NotAfter: time.Now().Add(24 * time.Hour), - KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, - ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, - IsCA: true, + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: cn}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, + IsCA: true, BasicConstraintsValid: true, } diff --git a/pkg/operator/main.go b/pkg/operator/main.go index 0006b8dc..b44a3380 100644 --- a/pkg/operator/main.go +++ b/pkg/operator/main.go @@ -255,4 +255,3 @@ func getLogLevel(logLevel string) zapcore.LevelEnabler { return zapcore.InfoLevel } } - From 26e89a75d79e84d1f0e308bec1a8b0caf4d0de47 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 04:18:29 +0000 Subject: [PATCH 3/4] chore: regenerate install files with TLS volume mount patch Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com> Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/3528b7ff-34c9-4621-a98f-b9b8427da515 --- installfiles/operator.yaml | 10 ++++++++++ installfiles/operator_with_monitoring.yaml | 10 ++++++++++ installfiles/operator_with_security.yaml | 10 ++++++++++ .../operator_with_security_and_monitoring.yaml | 10 ++++++++++ 4 files changed, 40 insertions(+) diff --git a/installfiles/operator.yaml b/installfiles/operator.yaml index 03b3e733..924c5c3d 100755 --- a/installfiles/operator.yaml +++ b/installfiles/operator.yaml @@ -17658,6 +17658,8 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace + - name: TLS_CERT_DIR + value: /tmp/alloc-api-serving-certs image: ghcr.io/playfab/thundernetes-operator:0.6.0 imagePullPolicy: IfNotPresent livenessProbe: @@ -17690,6 +17692,9 @@ spec: securityContext: allowPrivilegeEscalation: false volumeMounts: + - mountPath: /tmp/alloc-api-serving-certs + name: alloc-api-cert + readOnly: true - mountPath: /tmp/k8s-webhook-server/serving-certs name: cert readOnly: true @@ -17700,6 +17705,11 @@ spec: serviceAccountName: thundernetes-controller-manager terminationGracePeriodSeconds: 10 volumes: + - name: alloc-api-cert + secret: + defaultMode: 420 + optional: true + secretName: tls-secret - name: cert secret: defaultMode: 420 diff --git a/installfiles/operator_with_monitoring.yaml b/installfiles/operator_with_monitoring.yaml index 1acf1fbd..c578ae17 100644 --- a/installfiles/operator_with_monitoring.yaml +++ b/installfiles/operator_with_monitoring.yaml @@ -17658,6 +17658,8 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace + - name: TLS_CERT_DIR + value: /tmp/alloc-api-serving-certs image: ghcr.io/playfab/thundernetes-operator:0.6.0 imagePullPolicy: IfNotPresent livenessProbe: @@ -17690,6 +17692,9 @@ spec: securityContext: allowPrivilegeEscalation: false volumeMounts: + - mountPath: /tmp/alloc-api-serving-certs + name: alloc-api-cert + readOnly: true - mountPath: /tmp/k8s-webhook-server/serving-certs name: cert readOnly: true @@ -17700,6 +17705,11 @@ spec: serviceAccountName: thundernetes-controller-manager terminationGracePeriodSeconds: 10 volumes: + - name: alloc-api-cert + secret: + defaultMode: 420 + optional: true + secretName: tls-secret - name: cert secret: defaultMode: 420 diff --git a/installfiles/operator_with_security.yaml b/installfiles/operator_with_security.yaml index 5a2a2980..271d5572 100644 --- a/installfiles/operator_with_security.yaml +++ b/installfiles/operator_with_security.yaml @@ -17658,6 +17658,8 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace + - name: TLS_CERT_DIR + value: /tmp/alloc-api-serving-certs image: ghcr.io/playfab/thundernetes-operator:0.6.0 imagePullPolicy: IfNotPresent livenessProbe: @@ -17690,6 +17692,9 @@ spec: securityContext: allowPrivilegeEscalation: false volumeMounts: + - mountPath: /tmp/alloc-api-serving-certs + name: alloc-api-cert + readOnly: true - mountPath: /tmp/k8s-webhook-server/serving-certs name: cert readOnly: true @@ -17700,6 +17705,11 @@ spec: serviceAccountName: thundernetes-controller-manager terminationGracePeriodSeconds: 10 volumes: + - name: alloc-api-cert + secret: + defaultMode: 420 + optional: true + secretName: tls-secret - name: cert secret: defaultMode: 420 diff --git a/installfiles/operator_with_security_and_monitoring.yaml b/installfiles/operator_with_security_and_monitoring.yaml index 5a2a2980..271d5572 100644 --- a/installfiles/operator_with_security_and_monitoring.yaml +++ b/installfiles/operator_with_security_and_monitoring.yaml @@ -17658,6 +17658,8 @@ spec: valueFrom: fieldRef: fieldPath: metadata.namespace + - name: TLS_CERT_DIR + value: /tmp/alloc-api-serving-certs image: ghcr.io/playfab/thundernetes-operator:0.6.0 imagePullPolicy: IfNotPresent livenessProbe: @@ -17690,6 +17692,9 @@ spec: securityContext: allowPrivilegeEscalation: false volumeMounts: + - mountPath: /tmp/alloc-api-serving-certs + name: alloc-api-cert + readOnly: true - mountPath: /tmp/k8s-webhook-server/serving-certs name: cert readOnly: true @@ -17700,6 +17705,11 @@ spec: serviceAccountName: thundernetes-controller-manager terminationGracePeriodSeconds: 10 volumes: + - name: alloc-api-cert + secret: + defaultMode: 420 + optional: true + secretName: tls-secret - name: cert secret: defaultMode: 420 From 3c477a96c461c81173e76730614b5c01f17406de Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 24 Mar 2026 05:12:23 +0000 Subject: [PATCH 4/4] fix: race condition in port registry test - use atomic LoadOrStore for name deduplication The port_registry_test.go had two race conditions in the parallel allocation test: 1. First block (line 434-438): Used Load() to check if a name existed, then Store() to claim it - a TOCTOU race where two goroutines could both see the name as unclaimed and both proceed. 2. Second block (line 497): Had no name deduplication at all, so random name collisions with existing entries caused "ports already assigned for this GameServer" errors. Fix: Use sync.Map.LoadOrStore() in both blocks to atomically claim the generated name, preventing any possibility of duplicate registration. Co-authored-by: dgkanatsios <8256138+dgkanatsios@users.noreply.github.com> Agent-Logs-Url: https://github.com/PlayFab/thundernetes/sessions/677c2bb3-f657-4a37-a811-3b14c993038a --- pkg/operator/controllers/port_registry_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/operator/controllers/port_registry_test.go b/pkg/operator/controllers/port_registry_test.go index 03b762b9..b81bb79a 100644 --- a/pkg/operator/controllers/port_registry_test.go +++ b/pkg/operator/controllers/port_registry_test.go @@ -433,7 +433,8 @@ var _ = Describe("Random port registration on port registry with two thousand po var gameServerName string for { // make sure we don't register the same GameServer twice gameServerName = generateName(prefix) - if _, ok := gameServerNamesAndPorts.Load(gameServerName); !ok { + // use LoadOrStore to atomically claim the name, avoiding TOCTOU races + if _, loaded := gameServerNamesAndPorts.LoadOrStore(gameServerName, int32(0)); !loaded { break } } @@ -494,7 +495,14 @@ var _ = Describe("Random port registration on port registry with two thousand po defer wg.Done() n := rand.Intn(200) + 50 // n will be between 50 and 250 time.Sleep(time.Duration(n) * time.Millisecond) - gameServerName := generateName(prefix) + var gameServerName string + for { // make sure we don't register the same GameServer twice + gameServerName = generateName(prefix) + // use LoadOrStore to atomically claim the name, avoiding TOCTOU races + if _, loaded := gameServerNamesAndPorts.LoadOrStore(gameServerName, int32(0)); !loaded { + break + } + } ports, err := portRegistry.GetNewPorts(testnamespace, gameServerName, 1) Expect(err).ToNot(HaveOccurred()) Expect(len(ports)).To(Equal(1))