From c6289ec5bae8c301026092a484e5d5243ba3515b Mon Sep 17 00:00:00 2001 From: Andrei Aaron Date: Wed, 8 Apr 2026 00:10:54 +0300 Subject: [PATCH] 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 * fix: data race in GetPort() See https://github.com/project-zot/zot/actions/runs/24045271222/job/70126983674?pr=3942 Signed-off-by: Andrei Aaron * fix(test): reuse ReadLogFileAndSearchString for auto-port log; throttle poll loop Signed-off-by: Andrei Aaron --------- Signed-off-by: Andrei Aaron --- pkg/api/constants/consts_test.go | 6 +-- pkg/api/controller.go | 30 +++++++-------- pkg/api/controller_test.go | 64 +++++++++++++++----------------- pkg/meta/hooks.go | 3 ++ pkg/test/common/fs.go | 48 +++++++++++++++++------- pkg/test/common/fs_test.go | 19 ++++++++++ 6 files changed, 102 insertions(+), 68 deletions(-) diff --git a/pkg/api/constants/consts_test.go b/pkg/api/constants/consts_test.go index 579c1f21..8183a77a 100644 --- a/pkg/api/constants/consts_test.go +++ b/pkg/api/constants/consts_test.go @@ -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) } diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 637581e1..ec2cdd07 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -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() diff --git a/pkg/api/controller_test.go b/pkg/api/controller_test.go index c105fd65..35ba892d 100644 --- a/pkg/api/controller_test.go +++ b/pkg/api/controller_test.go @@ -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", "") diff --git a/pkg/meta/hooks.go b/pkg/meta/hooks.go index c4fc6a95..aaca6fe7 100644 --- a/pkg/meta/hooks.go +++ b/pkg/meta/hooks.go @@ -186,6 +186,9 @@ func OnUpdateManifestDigestTags(ctx context.Context, repo string, tags []string, priorTagManifests, err := priorTagManifestsFromMetaDB(ctx, metaDB, repo, tags) if err != nil { + log.Error().Err(err).Str("repository", repo). + Msg("multi-tag digest push: failed to snapshot prior tag state from metadb") + return err } diff --git a/pkg/test/common/fs.go b/pkg/test/common/fs.go index 4de81ad9..9ce75347 100644 --- a/pkg/test/common/fs.go +++ b/pkg/test/common/fs.go @@ -179,19 +179,29 @@ func ReadLogFileAndSearchString(logPath string, stringToMatch string, timeout ti ctx, cancelFunc := context.WithTimeout(context.Background(), timeout) defer cancelFunc() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { select { case <-ctx.Done(): return false, nil default: - content, err := os.ReadFile(logPath) - if err != nil { - return false, err - } + } - if strings.Contains(string(content), stringToMatch) { - return true, nil - } + content, err := os.ReadFile(logPath) + if err != nil { + return false, err + } + + if strings.Contains(string(content), stringToMatch) { + return true, nil + } + + select { + case <-ctx.Done(): + return false, nil + case <-ticker.C: } } } @@ -202,19 +212,29 @@ func ReadLogFileAndCountStringOccurence(logPath string, stringToMatch string, ctx, cancelFunc := context.WithTimeout(context.Background(), timeout) defer cancelFunc() + ticker := time.NewTicker(10 * time.Millisecond) + defer ticker.Stop() + for { select { case <-ctx.Done(): return false, nil default: - content, err := os.ReadFile(logPath) - if err != nil { - return false, err - } + } - if strings.Count(string(content), stringToMatch) >= count { - return true, nil - } + content, err := os.ReadFile(logPath) + if err != nil { + return false, err + } + + if strings.Count(string(content), stringToMatch) >= count { + return true, nil + } + + select { + case <-ctx.Done(): + return false, nil + case <-ticker.C: } } } diff --git a/pkg/test/common/fs_test.go b/pkg/test/common/fs_test.go index 57d2ac99..82c5f7f7 100644 --- a/pkg/test/common/fs_test.go +++ b/pkg/test/common/fs_test.go @@ -145,6 +145,25 @@ func TestReadLogFileAndSearchString(t *testing.T) { So(err, ShouldBeNil) So(ok, ShouldBeFalse) }) + + Convey("substring appears after append", t, func() { + logPath := path.Join(t.TempDir(), "log.txt") + So(os.WriteFile(logPath, []byte("header\n"), 0o600), ShouldBeNil) + + go func() { + time.Sleep(40 * time.Millisecond) + f, err := os.OpenFile(logPath, os.O_APPEND|os.O_WRONLY, 0o600) + if err != nil { + return + } + defer f.Close() + _, _ = f.WriteString("line with port is unspecified\n") + }() + + ok, err := tcommon.ReadLogFileAndSearchString(logPath, "port is unspecified", 500*time.Millisecond) + So(err, ShouldBeNil) + So(ok, ShouldBeTrue) + }) } func TestReadLogFileAndCountStringOccurence(t *testing.T) {