Skip to content

Commit 2005089

Browse files
eusebiu-constantin-petu-dbkeusebiu-constantin-petu-dbk
committed
fix(sync): various fixes for s3+remote storage feature
Signed-off-by: Petu Eusebiu <[email protected]>
1 parent 5242f18 commit 2005089

File tree

8 files changed

+796
-57
lines changed

8 files changed

+796
-57
lines changed

‎pkg/cli/server/extensions_test.go

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1637,3 +1637,135 @@ func TestOverlappingSyncRetentionConfig(t *testing.T) {
16371637
So(string(data), ShouldContainSubstring, "overlapping sync content\":{\"Prefix\":\"prod/*")
16381638
})
16391639
}
1640+
1641+
func TestSyncWithRemoteStorageConfig(t *testing.T) {
1642+
oldArgs := os.Args
1643+
1644+
defer func() { os.Args = oldArgs }()
1645+
1646+
Convey("Test verify sync with remote storage works if sync.tmpdir is provided", t, func(c C) {
1647+
tmpfile, err := os.CreateTemp("", "zot-test*.json")
1648+
So(err, ShouldBeNil)
1649+
defer os.Remove(tmpfile.Name()) // clean up
1650+
1651+
content := `{
1652+
"distSpecVersion": "1.1.0-dev",
1653+
"storage": {
1654+
"rootDirectory": "%s",
1655+
"dedupe": false,
1656+
"remoteCache": false,
1657+
"storageDriver": {
1658+
"name": "s3",
1659+
"rootdirectory": "/zot",
1660+
"region": "us-east-2",
1661+
"regionendpoint": "localhost:4566",
1662+
"bucket": "zot-storage",
1663+
"secure": false,
1664+
"skipverify": false
1665+
}
1666+
},
1667+
"http": {
1668+
"address": "0.0.0.0",
1669+
"port": "%s"
1670+
},
1671+
"log": {
1672+
"level": "debug",
1673+
"output": "%s"
1674+
},
1675+
"extensions": {
1676+
"sync": {
1677+
"tmpDir": "/tmp/sync",
1678+
"registries": [
1679+
{
1680+
"urls": [
1681+
"http://localhost:9000"
1682+
],
1683+
"onDemand": true,
1684+
"tlsVerify": false,
1685+
"content": [
1686+
{
1687+
"prefix": "**"
1688+
}
1689+
]
1690+
}
1691+
]
1692+
}
1693+
}
1694+
}`
1695+
1696+
logPath, err := runCLIWithConfig(t.TempDir(), content)
1697+
So(err, ShouldBeNil)
1698+
1699+
data, err := os.ReadFile(logPath)
1700+
So(err, ShouldBeNil)
1701+
defer os.Remove(logPath) // clean up
1702+
So(string(data), ShouldNotContainSubstring,
1703+
"using both sync and remote storage features needs config.Extensions.Sync.TmpDir to be specified")
1704+
})
1705+
1706+
Convey("Test verify sync with remote storage panics if sync.tmpdir is not provided", t, func(c C) {
1707+
port := GetFreePort()
1708+
logFile, err := os.CreateTemp("", "zot-log*.txt")
1709+
So(err, ShouldBeNil)
1710+
defer os.Remove(logFile.Name()) // clean up
1711+
1712+
tmpfile, err := os.CreateTemp("", "zot-test*.json")
1713+
So(err, ShouldBeNil)
1714+
defer os.Remove(tmpfile.Name()) // clean up
1715+
content := fmt.Sprintf(`{
1716+
"distSpecVersion": "1.1.0-dev",
1717+
"storage": {
1718+
"rootDirectory": "%s",
1719+
"dedupe": false,
1720+
"remoteCache": false,
1721+
"storageDriver": {
1722+
"name": "s3",
1723+
"rootdirectory": "/zot",
1724+
"region": "us-east-2",
1725+
"regionendpoint": "localhost:4566",
1726+
"bucket": "zot-storage",
1727+
"secure": false,
1728+
"skipverify": false
1729+
}
1730+
},
1731+
"http": {
1732+
"address": "0.0.0.0",
1733+
"port": "%s"
1734+
},
1735+
"log": {
1736+
"level": "debug",
1737+
"output": "%s"
1738+
},
1739+
"extensions": {
1740+
"sync": {
1741+
"registries": [
1742+
{
1743+
"urls": [
1744+
"http://localhost:9000"
1745+
],
1746+
"onDemand": true,
1747+
"tlsVerify": false,
1748+
"content": [
1749+
{
1750+
"prefix": "**"
1751+
}
1752+
]
1753+
}
1754+
]
1755+
}
1756+
}
1757+
}`, t.TempDir(), port, logFile.Name())
1758+
1759+
err = os.WriteFile(tmpfile.Name(), []byte(content), 0o0600)
1760+
So(err, ShouldBeNil)
1761+
1762+
os.Args = []string{"cli_test", "serve", tmpfile.Name()}
1763+
So(func() { _ = cli.NewServerRootCmd().Execute() }, ShouldPanic)
1764+
1765+
data, err := os.ReadFile(logFile.Name())
1766+
So(err, ShouldBeNil)
1767+
defer os.Remove(logFile.Name()) // clean up
1768+
So(string(data), ShouldContainSubstring,
1769+
"using both sync and remote storage features needs config.Extensions.Sync.TmpDir to be specified")
1770+
})
1771+
}

‎pkg/cli/server/root.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,13 @@ func validateConfiguration(config *config.Config, log zlog.Logger) error {
381381
return zerr.ErrBadConfig
382382
}
383383

384+
// enforce tmpDir in case sync + s3
385+
if config.Extensions != nil && config.Extensions.Sync != nil && config.Extensions.Sync.TmpDir == "" {
386+
log.Error().Err(zerr.ErrBadConfig).
387+
Msg("using both sync and remote storage features needs config.Extensions.Sync.TmpDir to be specified")
388+
389+
return zerr.ErrBadConfig
390+
}
384391
}
385392

386393
// enforce s3 driver on subpaths in case of using storage driver
@@ -396,6 +403,14 @@ func validateConfiguration(config *config.Config, log zlog.Logger) error {
396403

397404
return zerr.ErrBadConfig
398405
}
406+
407+
// enforce tmpDir in case sync + s3
408+
if config.Extensions != nil && config.Extensions.Sync != nil && config.Extensions.Sync.TmpDir == "" {
409+
log.Error().Err(zerr.ErrBadConfig).
410+
Msg("using both sync and remote storage features needs config.Extensions.Sync.TmpDir to be specified")
411+
412+
return zerr.ErrBadConfig
413+
}
399414
}
400415
}
401416
}

‎pkg/extensions/extension_sync.go

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ package extensions
66
import (
77
"net"
88
"net/url"
9-
"os"
109
"strings"
1110

1211
zerr "zotregistry.io/zot/errors"
@@ -47,13 +46,9 @@ func EnableSyncExtension(config *config.Config, metaDB mTypes.MetaDB,
4746
}
4847

4948
tmpDir := config.Extensions.Sync.TmpDir
50-
if tmpDir == "" {
51-
// use an os tmpdir as tmpdir if not set
52-
tmpDir = os.TempDir()
53-
}
49+
credsPath := config.Extensions.Sync.CredentialsFile
5450

55-
service, err := sync.New(registryConfig, config.Extensions.Sync.CredentialsFile, tmpDir,
56-
storeController, metaDB, log)
51+
service, err := sync.New(registryConfig, credsPath, tmpDir, storeController, metaDB, log)
5752
if err != nil {
5853
return nil, err
5954
}

‎pkg/extensions/sync/destination.go

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,22 +37,18 @@ type DestinationRegistry struct {
3737
}
3838

3939
func NewDestinationRegistry(
40-
storeController storage.StoreController,
41-
tmpStorage OciLayoutStorage,
40+
storeController storage.StoreController, // local store controller
41+
tempStoreController storage.StoreController, // temp store controller
4242
metaDB mTypes.MetaDB,
4343
log log.Logger,
4444
) Destination {
45-
if tmpStorage == nil {
46-
// to allow passing nil we can do this, noting that it will only work for a local StoreController
47-
tmpStorage = NewOciLayoutStorage(storeController)
48-
}
4945
return &DestinationRegistry{
5046
storeController: storeController,
47+
tempStorage: NewOciLayoutStorage(tempStoreController),
5148
metaDB: metaDB,
5249
// first we sync from remote (using containers/image copy from docker:// to oci:) to a temp imageStore
5350
// then we copy the image from tempStorage to zot's storage using ImageStore APIs
54-
tempStorage: tmpStorage,
55-
log: log,
51+
log: log,
5652
}
5753
}
5854

@@ -288,9 +284,11 @@ func getImageStoreFromImageReference(imageReference types.ImageReference, repo,
288284
tempRootDir = strings.ReplaceAll(imageReference.StringWithinTransport(), fmt.Sprintf("%s:", repo), "")
289285
}
290286

291-
metrics := monitoring.NewMetricsServer(false, log.Logger{})
287+
return getImageStore(tempRootDir)
288+
}
292289

293-
tempImageStore := local.NewImageStore(tempRootDir, false, false, log.Logger{}, metrics, nil, nil)
290+
func getImageStore(rootDir string) storageTypes.ImageStore {
291+
metrics := monitoring.NewMetricsServer(false, log.Logger{})
294292

295-
return tempImageStore
293+
return local.NewImageStore(rootDir, false, false, log.Logger{}, metrics, nil, nil)
296294
}

‎pkg/extensions/sync/service.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"zotregistry.io/zot/pkg/log"
2121
mTypes "zotregistry.io/zot/pkg/meta/types"
2222
"zotregistry.io/zot/pkg/storage"
23-
"zotregistry.io/zot/pkg/storage/local"
2423
)
2524

2625
type BaseService struct {
@@ -67,13 +66,20 @@ func New(
6766

6867
service.contentManager = NewContentManager(opts.Content, log)
6968

70-
tmpImageStore := local.NewImageStore(tmpDir,
71-
false, false, log, nil, nil, nil,
72-
)
73-
74-
tmpStorage := NewOciLayoutStorage(storage.StoreController{DefaultStore: tmpImageStore})
75-
76-
service.destination = NewDestinationRegistry(storeController, tmpStorage, metadb, log)
69+
if len(tmpDir) == 0 {
70+
// first it will sync in tmpDir then it will move everything into local ImageStore
71+
service.destination = NewDestinationRegistry(storeController, storeController, metadb, log)
72+
} else {
73+
// first it will sync under /rootDir/reponame/.sync/ then it will move everything into local ImageStore
74+
service.destination = NewDestinationRegistry(
75+
storeController,
76+
storage.StoreController{
77+
DefaultStore: getImageStore(tmpDir),
78+
},
79+
metadb,
80+
log,
81+
)
82+
}
7783

7884
retryOptions := &retry.RetryOptions{}
7985

@@ -140,7 +146,7 @@ func (service *BaseService) SetNextAvailableClient() error {
140146
if err != nil {
141147
service.log.Error().Err(err).Str("url", url).Msg("sync: failed to initialize http client")
142148

143-
continue
149+
return err
144150
}
145151

146152
if !service.client.Ping() {
@@ -355,7 +361,8 @@ func (service *BaseService) SyncRepo(ctx context.Context, repo string) error {
355361
return nil
356362
}
357363

358-
func (service *BaseService) syncTag(ctx context.Context, destinationRepo, remoteRepo, tag string) (digest.Digest, error) {
364+
func (service *BaseService) syncTag(ctx context.Context, destinationRepo, remoteRepo, tag string,
365+
) (digest.Digest, error) {
359366
copyOptions := getCopyOptions(service.remote.GetContext(), service.destination.GetContext())
360367

361368
policyContext, err := getPolicyContext(service.log)

‎pkg/extensions/sync/sync_internal_test.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,8 @@ func TestDestinationRegistry(t *testing.T) {
185185
syncImgStore := local.NewImageStore(dir, true, true, log, metrics, nil, cacheDriver)
186186
repoName := "repo"
187187

188-
registry := NewDestinationRegistry(storage.StoreController{DefaultStore: syncImgStore}, nil, nil, log)
188+
storeController := storage.StoreController{DefaultStore: syncImgStore}
189+
registry := NewDestinationRegistry(storeController, storeController, nil, log)
189190
imageReference, err := registry.GetImageReference(repoName, "1.0")
190191
So(err, ShouldBeNil)
191192
So(imageReference, ShouldNotBeNil)
@@ -302,7 +303,8 @@ func TestDestinationRegistry(t *testing.T) {
302303
syncImgStore := local.NewImageStore(dir, true, true, log, metrics, linter, cacheDriver)
303304
repoName := "repo"
304305

305-
registry := NewDestinationRegistry(storage.StoreController{DefaultStore: syncImgStore}, nil, nil, log)
306+
storeController := storage.StoreController{DefaultStore: syncImgStore}
307+
registry := NewDestinationRegistry(storeController, storeController, nil, log)
306308

307309
err = registry.CommitImage(imageReference, repoName, "1.0")
308310
So(err, ShouldBeNil)
@@ -336,7 +338,8 @@ func TestDestinationRegistry(t *testing.T) {
336338
})
337339

338340
Convey("trigger metaDB error on index manifest in CommitImage()", func() {
339-
registry := NewDestinationRegistry(storage.StoreController{DefaultStore: syncImgStore}, nil, mocks.MetaDBMock{
341+
storeController := storage.StoreController{DefaultStore: syncImgStore}
342+
registry := NewDestinationRegistry(storeController, storeController, mocks.MetaDBMock{
340343
SetRepoReferenceFn: func(ctx context.Context, repo string, reference string, imageMeta mTypes.ImageMeta) error {
341344
if reference == "1.0" {
342345
return zerr.ErrRepoMetaNotFound
@@ -351,7 +354,8 @@ func TestDestinationRegistry(t *testing.T) {
351354
})
352355

353356
Convey("trigger metaDB error on image manifest in CommitImage()", func() {
354-
registry := NewDestinationRegistry(storage.StoreController{DefaultStore: syncImgStore}, nil, mocks.MetaDBMock{
357+
storeController := storage.StoreController{DefaultStore: syncImgStore}
358+
registry := NewDestinationRegistry(storeController, storeController, mocks.MetaDBMock{
355359
SetRepoReferenceFn: func(ctx context.Context, repo, reference string, imageMeta mTypes.ImageMeta) error {
356360
return zerr.ErrRepoMetaNotFound
357361
},

‎test/blackbox/ci.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ PATH=$PATH:${SCRIPTPATH}/../../hack/tools/bin
99

1010
tests=("pushpull" "pushpull_authn" "delete_images" "referrers" "metadata" "anonymous_policy"
1111
"annotations" "detect_manifest_collision" "cve" "sync" "sync_docker" "sync_replica_cluster"
12-
"scrub" "garbage_collect" "metrics" "metrics_minimal")
12+
"sync_remote" "scrub" "garbage_collect" "metrics" "metrics_minimal")
1313

1414
for test in ${tests[*]}; do
1515
${BATS} ${BATS_FLAGS} ${SCRIPTPATH}/${test}.bats > ${test}.log & pids+=($!)

0 commit comments

Comments
 (0)