fix: address code review comments (#3942)

* fix: address code review comments in https://github.com/project-zot/zot/pull/3885#pullrequestreview-4045836197 Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com> * fix: data race in GetPort() See https://github.com/project-zot/zot/actions/runs/24045271222/job/70126983674?pr=3942 Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com> * fix(test): reuse ReadLogFileAndSearchString for auto-port log; throttle poll loop Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com> --------- Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>
2026-06-17 12:58:02 +08:00 · 2026-04-08 00:10:54 +03:00
parent 78c6e915dd
commit c6289ec5ba
6 changed files with 102 additions and 68 deletions
@@ -3,6 +3,8 @@ package constants_test
 import (
 	"testing"

+	"github.com/stretchr/testify/assert"
+
 	"zotregistry.dev/zot/v2/pkg/api/constants"
 	zreg "zotregistry.dev/zot/v2/pkg/regexp"
 )
@@ -12,7 +14,5 @@ func TestMaxManifestDigestQueryTagsDerived(t *testing.T) {

 	want := (8192 - 2048) / (len("tag=") + zreg.TagMaxLen + 1)

-	if constants.MaxManifestDigestQueryTags != want {
-		t.Fatalf("MaxManifestDigestQueryTags = %d, want %d", constants.MaxManifestDigestQueryTags, want)
-	}
+	assert.Equal(t, want, constants.MaxManifestDigestQueryTags)
 }
@@ -57,8 +57,8 @@ type Controller struct {
 	LDAPClient      *LDAPClient
 	taskScheduler   *scheduler.Scheduler
 	Healthz         *common.Healthz
-	// runtime params
-	chosenPort int // kernel-chosen port
+	// runtime params (atomic: Run may set the port concurrently with GetPort readers, e.g. tests)
+	chosenPort atomic.Int64
 	// TLS certificate management
 	TlsWatcher atomic.Pointer[TlsConfigWatcher]
 }
@@ -127,7 +127,7 @@ func NewController(appConfig *config.Config) *Controller {
 }

 func (c *Controller) GetPort() int {
-	return c.chosenPort
+	return int(c.chosenPort.Load())
 }

 func (c *Controller) Run() error {
@@ -186,23 +186,21 @@ func (c *Controller) Run() error {
 		return err
 	}

+	// Always derive the bound port from the listener so GetPort matches the actual socket (numeric
+	// config, kernel-chosen :0, or service names like "http" resolved by net.Listen).
+	chosenAddr, ok := listener.Addr().(*net.TCPAddr)
+	if !ok {
+		c.Log.Error().Str("port", port).Msg("invalid addr type")
+
+		return errors.ErrBadType
+	}
+
+	c.chosenPort.Store(int64(chosenAddr.Port))
+
 	if port == "0" || port == "" {
-		chosenAddr, ok := listener.Addr().(*net.TCPAddr)
-		if !ok {
-			c.Log.Error().Str("port", port).Msg("invalid addr type")
-
-			return errors.ErrBadType
-		}
-
-		c.chosenPort = chosenAddr.Port
-
 		c.Log.Info().Int("port", chosenAddr.Port).IPAddr("address", chosenAddr.IP).Msg(
 			"port is unspecified, listening on kernel chosen port",
 		)
-	} else {
-		chosenPort, _ := strconv.ParseInt(port, 10, 32)
-
-		c.chosenPort = int(chosenPort)
 	}

 	tlsConfig := c.Config.CopyTLSConfig()
@@ -89,6 +89,9 @@ var (
 	LDAPBindDN       = "cn=reader," + LDAPBaseDN //nolint: gochecknoglobals
 	LDAPBindPassword = "ldappass"                //nolint: gochecknoglobals
 	LDAPUserAttr     = "uid"                     //nolint: gochecknoglobals
+
+	errTimedOutWaitingForControllerPort = goerrors.New("timed out waiting for controller port") //nolint: gochecknoglobals
+	errGetCertificateFailed             = goerrors.New("GetCertificate failed")                 //nolint: gochecknoglobals
 )

 // setupBearerAuthServerCerts generates CA and server certificates for bearer auth server testing
@@ -580,43 +583,41 @@ func TestAutoPortSelection(t *testing.T) {

 		cm := test.NewControllerManager(ctlr)
 		cm.StartServer()
-		time.Sleep(1000 * time.Millisecond)
+		So(waitForControllerPort(ctlr, 30*time.Second), ShouldBeNil)
+		cm.WaitServerToBeReady(strconv.Itoa(ctlr.GetPort()))

 		defer cm.StopServer()

-		scanner := bufio.NewScanner(logFile)
+		found, err := test.ReadLogFileAndSearchString(logFile.Name(), "port is unspecified", 30*time.Second)
+		So(err, ShouldBeNil)
+		So(found, ShouldBeTrue)

-		var contents bytes.Buffer
-
-		start := time.Now()
-
-		for scanner.Scan() {
-			if time.Since(start) < time.Second*30 {
-				t.Logf("Exhausted: Controller did not print the expected log within 30 seconds")
-			}
-
-			text := scanner.Text()
-			contents.WriteString(text)
-
-			if strings.Contains(text, "Port unspecified") {
-				break
-			}
-
-			t.Logf("%s", scanner.Text())
-		}
-
-		So(scanner.Err(), ShouldBeNil)
-		So(contents.String(), ShouldContainSubstring,
+		contents, err := os.ReadFile(logFile.Name())
+		So(err, ShouldBeNil)
+		So(string(contents), ShouldContainSubstring,
 			"port is unspecified, listening on kernel chosen port",
 		)
-		So(contents.String(), ShouldContainSubstring, "\"address\":\"127.0.0.1\"")
-		So(contents.String(), ShouldContainSubstring, "\"port\":")
+		So(string(contents), ShouldContainSubstring, "\"address\":\"127.0.0.1\"")
+		So(string(contents), ShouldContainSubstring, "\"port\":")

 		So(ctlr.GetPort(), ShouldBeGreaterThan, 0)
 		So(ctlr.GetPort(), ShouldBeLessThan, 65536)
 	})
 }

+func waitForControllerPort(ctlr interface{ GetPort() int }, timeout time.Duration) error {
+	deadline := time.Now().Add(timeout)
+
+	for time.Now().Before(deadline) {
+		if p := ctlr.GetPort(); p > 0 {
+			return nil
+		}
+		time.Sleep(50 * time.Millisecond)
+	}
+
+	return errTimedOutWaitingForControllerPort
+}
+
 func TestObjectStorageController(t *testing.T) {
 	tskip.SkipS3(t)
 	tskip.SkipDynamo(t)
@@ -7600,9 +7601,7 @@ func TestManifestValidation(t *testing.T) {
 		dir := t.TempDir()
 		ctlr := makeController(conf, dir)
 		cm := test.NewControllerManager(ctlr)
-		// this blocks
-		cm.StartServer()
-		time.Sleep(1000 * time.Millisecond)
+		cm.StartAndWait(port)

 		defer cm.StopServer()

@@ -7820,8 +7819,7 @@ func TestManifestDigestQueryTags(t *testing.T) {
 		dir := t.TempDir()
 		ctlr := makeController(conf, dir)
 		cm := test.NewControllerManager(ctlr)
-		cm.StartServer()
-		time.Sleep(1000 * time.Millisecond)
+		cm.StartAndWait(port)

 		defer cm.StopServer()

@@ -7957,9 +7955,7 @@ func TestArtifactReferences(t *testing.T) {
 		dir := t.TempDir()
 		ctlr := makeController(conf, dir)
 		cm := test.NewControllerManager(ctlr)
-		// this blocks
-		cm.StartServer()
-		time.Sleep(1000 * time.Millisecond)
+		cm.StartAndWait(port)

 		defer cm.StopServer()

@@ -14049,8 +14045,6 @@ func readTagsFromStorage(rootDir, repoName string, digest godigest.Digest) ([]st
 	return result, nil
 }

-var errGetCertificateFailed = goerrors.New("GetCertificate failed")
-
 func TestDynamicTLSCertificateReloading(t *testing.T) {
 	Convey("Test dynamic TLS certificate reloading", t, func() {
 		logger := log.NewLogger("debug", "")