From c84d8a6d887d716cda9e9ccca77cc93550741f09 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 1 Feb 2026 23:01:02 +0000 Subject: [PATCH] Address PR review comments: fix test description and add BATS test clarification Co-authored-by: rchincha <45800463+rchincha@users.noreply.github.com> --- pkg/api/tlscert_test.go | 25 ++++++++++++------------- test/blackbox/tls_cert_reload.bats | 5 +++++ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/pkg/api/tlscert_test.go b/pkg/api/tlscert_test.go index a4367d28..7762647b 100644 --- a/pkg/api/tlscert_test.go +++ b/pkg/api/tlscert_test.go @@ -198,7 +198,7 @@ func TestCertReloaderDirectly(t *testing.T) { So(cert, ShouldNotBeNil) }) - Convey("GetCertificateFunc should handle only cert file modification", func() { + Convey("GetCertificateFunc should handle cert file modification", func() { reloader, err := api.NewCertReloader(certPath, keyPath, log.NewTestLogger()) So(err, ShouldBeNil) defer reloader.Close() @@ -211,29 +211,28 @@ func TestCertReloaderDirectly(t *testing.T) { // Wait to ensure modification time will be different time.Sleep(2 * time.Second) - // Modify only the cert file (touch it to update mtime) - // Generate new cert with same key + // Generate new certificate (both cert and key files will be modified) newServerOpts := &tlsutils.CertificateOptions{ Hostname: "127.0.0.1", - CommonName: "Updated Cert Only", + CommonName: "Updated Cert", NotAfter: time.Now().AddDate(1, 0, 0), } - // Read the existing key - keyData, err := os.ReadFile(keyPath) - So(err, ShouldBeNil) - - // Generate new cert using the existing key err = tlsutils.GenerateServerCertToFile(caCertPEM, caKeyPEM, certPath, keyPath, newServerOpts) So(err, ShouldBeNil) - // Restore original key - err = os.WriteFile(keyPath, keyData, 0o600) - So(err, ShouldBeNil) - // Get certificate again - should reload updatedCert, err := getCert(nil) So(err, ShouldBeNil) So(updatedCert, ShouldNotBeNil) + + // Verify certificates are different + initialLeaf, err := x509.ParseCertificate(initialCert.Certificate[0]) + So(err, ShouldBeNil) + updatedLeaf, err := x509.ParseCertificate(updatedCert.Certificate[0]) + So(err, ShouldBeNil) + + So(initialLeaf.Subject.CommonName, ShouldEqual, "Initial Cert") + So(updatedLeaf.Subject.CommonName, ShouldEqual, "Updated Cert") }) Convey("GetCertificateFunc should handle concurrent access", func() { diff --git a/test/blackbox/tls_cert_reload.bats b/test/blackbox/tls_cert_reload.bats index 7d55d5bd..a1b270ea 100644 --- a/test/blackbox/tls_cert_reload.bats +++ b/test/blackbox/tls_cert_reload.bats @@ -194,9 +194,14 @@ function teardown_file() { openssl x509 -noout -subject 2>/dev/null) # Temporarily remove certificate files (will cause reload to fail) + # Note: Moving the file won't trigger fsnotify (only Write/Create events are monitored), + # so this test relies on the maybeReload() fallback mechanism being triggered during + # the TLS handshake when curl connects below. This verifies the server continues + # serving with the old certificate when reload fails. mv ${cert_dir}/server.cert ${cert_dir}/server.cert.backup # Wait and try to connect - should still work with old certificate + # The maybeReload() mechanism will detect the missing file but won't fail the handshake sleep 2 run curl --cacert ${cert_dir}/ca.crt https://127.0.0.1:${zot_port}/v2/ [ "$status" -eq 0 ]