Skip to content

Commit a7e3073

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 2f3c84b commit a7e3073

File tree

11 files changed

+836
-572
lines changed

11 files changed

+836
-572
lines changed

‎Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -493,6 +493,7 @@ run-blackbox-ci: check-blackbox-prerequisites binary binary-minimal cli
493493
run-blackbox-cloud-ci: check-blackbox-prerequisites check-awslocal binary $(BATS)
494494
echo running cloud CI bats tests; \
495495
$(BATS) $(BATS_FLAGS) test/blackbox/cloud_only.bats
496+
$(BATS) $(BATS_FLAGS) test/blackbox/sync_cloud.bats
496497

497498
.PHONY: run-blackbox-dedupe-nightly
498499
run-blackbox-dedupe-nightly: check-blackbox-prerequisites check-awslocal binary binary-minimal

‎pkg/cli/server/extensions_test.go

Lines changed: 204 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1653,3 +1653,207 @@ func TestOverlappingSyncRetentionConfig(t *testing.T) {
16531653
So(string(data), ShouldContainSubstring, "overlapping sync content\":{\"Prefix\":\"prod/*")
16541654
})
16551655
}
1656+
1657+
func TestSyncWithRemoteStorageConfig(t *testing.T) {
1658+
oldArgs := os.Args
1659+
1660+
defer func() { os.Args = oldArgs }()
1661+
1662+
Convey("Test verify sync with remote storage works if sync.tmpdir is provided", t, func(c C) {
1663+
tmpfile, err := os.CreateTemp("", "zot-test*.json")
1664+
So(err, ShouldBeNil)
1665+
defer os.Remove(tmpfile.Name()) // clean up
1666+
1667+
content := `{
1668+
"distSpecVersion": "1.1.0-dev",
1669+
"storage": {
1670+
"rootDirectory": "%s",
1671+
"dedupe": false,
1672+
"remoteCache": false,
1673+
"storageDriver": {
1674+
"name": "s3",
1675+
"rootdirectory": "/zot",
1676+
"region": "us-east-2",
1677+
"regionendpoint": "localhost:4566",
1678+
"bucket": "zot-storage",
1679+
"secure": false,
1680+
"skipverify": false
1681+
}
1682+
},
1683+
"http": {
1684+
"address": "0.0.0.0",
1685+
"port": "%s"
1686+
},
1687+
"log": {
1688+
"level": "debug",
1689+
"output": "%s"
1690+
},
1691+
"extensions": {
1692+
"sync": {
1693+
"syncDir": "/tmp/sync",
1694+
"registries": [
1695+
{
1696+
"urls": [
1697+
"http://localhost:9000"
1698+
],
1699+
"onDemand": true,
1700+
"tlsVerify": false,
1701+
"content": [
1702+
{
1703+
"prefix": "**"
1704+
}
1705+
]
1706+
}
1707+
]
1708+
}
1709+
}
1710+
}`
1711+
1712+
logPath, err := runCLIWithConfig(t.TempDir(), content)
1713+
So(err, ShouldBeNil)
1714+
1715+
data, err := os.ReadFile(logPath)
1716+
So(err, ShouldBeNil)
1717+
defer os.Remove(logPath) // clean up
1718+
So(string(data), ShouldNotContainSubstring,
1719+
"using both sync and remote storage features needs config.Extensions.Sync.SyncDir to be specified")
1720+
})
1721+
1722+
Convey("Test verify sync with remote storage panics if sync.tmpdir is not provided", t, func(c C) {
1723+
port := GetFreePort()
1724+
logFile, err := os.CreateTemp("", "zot-log*.txt")
1725+
So(err, ShouldBeNil)
1726+
defer os.Remove(logFile.Name()) // clean up
1727+
1728+
tmpfile, err := os.CreateTemp("", "zot-test*.json")
1729+
So(err, ShouldBeNil)
1730+
defer os.Remove(tmpfile.Name()) // clean up
1731+
content := fmt.Sprintf(`{
1732+
"distSpecVersion": "1.1.0-dev",
1733+
"storage": {
1734+
"rootDirectory": "%s",
1735+
"dedupe": false,
1736+
"remoteCache": false,
1737+
"storageDriver": {
1738+
"name": "s3",
1739+
"rootdirectory": "/zot",
1740+
"region": "us-east-2",
1741+
"regionendpoint": "localhost:4566",
1742+
"bucket": "zot-storage",
1743+
"secure": false,
1744+
"skipverify": false
1745+
}
1746+
},
1747+
"http": {
1748+
"address": "0.0.0.0",
1749+
"port": "%s"
1750+
},
1751+
"log": {
1752+
"level": "debug",
1753+
"output": "%s"
1754+
},
1755+
"extensions": {
1756+
"sync": {
1757+
"registries": [
1758+
{
1759+
"urls": [
1760+
"http://localhost:9000"
1761+
],
1762+
"onDemand": true,
1763+
"tlsVerify": false,
1764+
"content": [
1765+
{
1766+
"prefix": "**"
1767+
}
1768+
]
1769+
}
1770+
]
1771+
}
1772+
}
1773+
}`, t.TempDir(), port, logFile.Name())
1774+
1775+
err = os.WriteFile(tmpfile.Name(), []byte(content), 0o0600)
1776+
So(err, ShouldBeNil)
1777+
1778+
os.Args = []string{"cli_test", "serve", tmpfile.Name()}
1779+
err = cli.NewServerRootCmd().Execute()
1780+
So(err, ShouldNotBeNil)
1781+
1782+
data, err := os.ReadFile(logFile.Name())
1783+
So(err, ShouldBeNil)
1784+
defer os.Remove(logFile.Name()) // clean up
1785+
So(string(data), ShouldContainSubstring,
1786+
"using both sync and remote storage features needs config.Extensions.Sync.SyncDir to be specified")
1787+
})
1788+
1789+
Convey("Test verify sync with remote storage on subpath panics if sync.tmpdir is not provided", t, func(c C) {
1790+
port := GetFreePort()
1791+
logFile, err := os.CreateTemp("", "zot-log*.txt")
1792+
So(err, ShouldBeNil)
1793+
defer os.Remove(logFile.Name()) // clean up
1794+
1795+
tmpfile, err := os.CreateTemp("", "zot-test*.json")
1796+
So(err, ShouldBeNil)
1797+
defer os.Remove(tmpfile.Name()) // clean up
1798+
content := fmt.Sprintf(`{
1799+
"distSpecVersion": "1.1.0-dev",
1800+
"storage": {
1801+
"rootDirectory": "%s",
1802+
"subPaths":{
1803+
"/a": {
1804+
"rootDirectory": "%s",
1805+
"dedupe": false,
1806+
"remoteCache": false,
1807+
"storageDriver":{
1808+
"name":"s3",
1809+
"rootdirectory":"/zot-a",
1810+
"region":"us-east-2",
1811+
"bucket":"zot-storage",
1812+
"secure":true,
1813+
"skipverify":true
1814+
}
1815+
}
1816+
}
1817+
},
1818+
"http": {
1819+
"address": "0.0.0.0",
1820+
"port": "%s"
1821+
},
1822+
"log": {
1823+
"level": "debug",
1824+
"output": "%s"
1825+
},
1826+
"extensions": {
1827+
"sync": {
1828+
"registries": [
1829+
{
1830+
"urls": [
1831+
"http://localhost:9000"
1832+
],
1833+
"onDemand": true,
1834+
"tlsVerify": false,
1835+
"content": [
1836+
{
1837+
"prefix": "**"
1838+
}
1839+
]
1840+
}
1841+
]
1842+
}
1843+
}
1844+
}`, t.TempDir(), t.TempDir(), port, logFile.Name())
1845+
1846+
err = os.WriteFile(tmpfile.Name(), []byte(content), 0o0600)
1847+
So(err, ShouldBeNil)
1848+
1849+
os.Args = []string{"cli_test", "serve", tmpfile.Name()}
1850+
err = cli.NewServerRootCmd().Execute()
1851+
So(err, ShouldNotBeNil)
1852+
1853+
data, err := os.ReadFile(logFile.Name())
1854+
So(err, ShouldBeNil)
1855+
defer os.Remove(logFile.Name()) // clean up
1856+
So(string(data), ShouldContainSubstring,
1857+
"using both sync and remote storage features needs config.Extensions.Sync.SyncDir to be specified")
1858+
})
1859+
}

‎pkg/cli/server/root.go

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

395+
// enforce tmpDir in case sync + s3
396+
if config.Extensions != nil && config.Extensions.Sync != nil && config.Extensions.Sync.SyncDir == "" {
397+
log.Error().Err(zerr.ErrBadConfig).
398+
Msg("using both sync and remote storage features needs config.Extensions.Sync.SyncDir to be specified")
399+
400+
return zerr.ErrBadConfig
401+
}
395402
}
396403

397404
// enforce s3 driver on subpaths in case of using storage driver
@@ -407,6 +414,14 @@ func validateConfiguration(config *config.Config, log zlog.Logger) error {
407414

408415
return zerr.ErrBadConfig
409416
}
417+
418+
// enforce tmpDir in case sync + s3
419+
if config.Extensions != nil && config.Extensions.Sync != nil && config.Extensions.Sync.SyncDir == "" {
420+
log.Error().Err(zerr.ErrBadConfig).
421+
Msg("using both sync and remote storage features needs config.Extensions.Sync.SyncDir to be specified")
422+
423+
return zerr.ErrBadConfig
424+
}
410425
}
411426
}
412427
}

‎pkg/extensions/config/sync/config.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,11 @@ type Credentials struct {
1515
type Config struct {
1616
Enable *bool
1717
CredentialsFile string
18-
TmpDir string
19-
Registries []RegistryConfig
18+
/* SyncDir is needed only in case of using cloud based storages
19+
it uses regclient to first copy images into this dir (as oci layout)
20+
and then move them into storage. */
21+
SyncDir string
22+
Registries []RegistryConfig
2023
}
2124

2225
type RegistryConfig struct {

‎pkg/extensions/extension_sync.go

Lines changed: 3 additions & 8 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"
@@ -46,14 +45,10 @@ func EnableSyncExtension(config *config.Config, metaDB mTypes.MetaDB,
4645
continue
4746
}
4847

49-
tmpDir := config.Extensions.Sync.TmpDir
50-
if tmpDir == "" {
51-
// use an os tmpdir as tmpdir if not set
52-
tmpDir = os.TempDir()
53-
}
48+
tmpDir := config.Extensions.Sync.SyncDir
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() {
@@ -353,7 +359,8 @@ func (service *BaseService) SyncRepo(ctx context.Context, repo string) error {
353359
return nil
354360
}
355361

356-
func (service *BaseService) syncTag(ctx context.Context, destinationRepo, remoteRepo, tag string) (digest.Digest, error) {
362+
func (service *BaseService) syncTag(ctx context.Context, destinationRepo, remoteRepo, tag string,
363+
) (digest.Digest, error) {
357364
copyOptions := getCopyOptions(service.remote.GetContext(), service.destination.GetContext())
358365

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

0 commit comments

Comments
 (0)