Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 0dc411f

Browse files
committedOct 10, 2022
Introduce artifact max size limit of 50MiB
Add a controller flag named "--artifact-max-size=<bytes>" with the default value of 50MiB. To disable the limit, the value can be set to "--artifact-max-size=-1". The flag enforces a max size limit for the artifact contents produced by source-controller, to avoid out-of-memory crashes of consumers such as kustomize-controller. Signed-off-by: Stefan Prodan <[email protected]>
1 parent 34f127b commit 0dc411f

File tree

6 files changed

+119
-19
lines changed

6 files changed

+119
-19
lines changed
 

‎controllers/helmchart_controller_test.go

+3-3
Original file line numberDiff line numberDiff line change
@@ -421,7 +421,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
421421

422422
tmpDir := t.TempDir()
423423

424-
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
424+
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords, 0)
425425
g.Expect(err).ToNot(HaveOccurred())
426426

427427
gitArtifact := &sourcev1.Artifact{
@@ -901,7 +901,7 @@ func TestHelmChartReconciler_buildFromOCIHelmRepository(t *testing.T) {
901901
metadata, err := loadTestChartToOCI(chartData, chartPath, testRegistryServer)
902902
g.Expect(err).NotTo(HaveOccurred())
903903

904-
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
904+
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords, 0)
905905
g.Expect(err).ToNot(HaveOccurred())
906906

907907
cachedArtifact := &sourcev1.Artifact{
@@ -1118,7 +1118,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
11181118

11191119
tmpDir := t.TempDir()
11201120

1121-
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
1121+
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords, 0)
11221122
g.Expect(err).ToNot(HaveOccurred())
11231123

11241124
chartsArtifact := &sourcev1.Artifact{

‎controllers/storage.go

+10-1
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,13 @@ type Storage struct {
6161
// ArtifactRetentionRecords is the maximum number of artifacts to be kept in
6262
// storage after a garbage collection.
6363
ArtifactRetentionRecords int `json:"artifactRetentionRecords"`
64+
65+
// ArtifactMaxSize sets the max size in bytes for an artifact contents.
66+
ArtifactMaxSize int64 `json:"artifactMaxSize"`
6467
}
6568

6669
// NewStorage creates the storage helper for a given path and hostname.
67-
func NewStorage(basePath string, hostname string, artifactRetentionTTL time.Duration, artifactRetentionRecords int) (*Storage, error) {
70+
func NewStorage(basePath string, hostname string, artifactRetentionTTL time.Duration, artifactRetentionRecords int, artifactMaxSize int64) (*Storage, error) {
6871
if f, err := os.Stat(basePath); os.IsNotExist(err) || !f.IsDir() {
6972
return nil, fmt.Errorf("invalid dir path: %s", basePath)
7073
}
@@ -73,6 +76,7 @@ func NewStorage(basePath string, hostname string, artifactRetentionTTL time.Dura
7376
Hostname: hostname,
7477
ArtifactRetentionTTL: artifactRetentionTTL,
7578
ArtifactRetentionRecords: artifactRetentionRecords,
79+
ArtifactMaxSize: artifactMaxSize,
7680
}, nil
7781
}
7882

@@ -432,6 +436,11 @@ func (s *Storage) Archive(artifact *sourcev1.Artifact, dir string, filter Archiv
432436
return err
433437
}
434438

439+
if s.ArtifactMaxSize > 0 && sz.written > s.ArtifactMaxSize {
440+
return fmt.Errorf("artifact size %d exceeds the max limit of %d bytes, use .sourceignore to exclude files from the artifact",
441+
sz.written, s.ArtifactMaxSize)
442+
}
443+
435444
if err := os.Chmod(tmpName, 0o600); err != nil {
436445
return err
437446
}

‎controllers/storage_test.go

+97-10
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ import (
3838
func TestStorageConstructor(t *testing.T) {
3939
dir := t.TempDir()
4040

41-
if _, err := NewStorage("/nonexistent", "hostname", time.Minute, 2); err == nil {
41+
if _, err := NewStorage("/nonexistent", "hostname", time.Minute, 2, 0); err == nil {
4242
t.Fatal("nonexistent path was allowable in storage constructor")
4343
}
4444

@@ -48,13 +48,13 @@ func TestStorageConstructor(t *testing.T) {
4848
}
4949
f.Close()
5050

51-
if _, err := NewStorage(f.Name(), "hostname", time.Minute, 2); err == nil {
51+
if _, err := NewStorage(f.Name(), "hostname", time.Minute, 2, 0); err == nil {
5252
os.Remove(f.Name())
5353
t.Fatal("file path was accepted as basedir")
5454
}
5555
os.Remove(f.Name())
5656

57-
if _, err := NewStorage(dir, "hostname", time.Minute, 2); err != nil {
57+
if _, err := NewStorage(dir, "hostname", time.Minute, 2, 0); err != nil {
5858
t.Fatalf("Valid path did not successfully return: %v", err)
5959
}
6060
}
@@ -103,7 +103,7 @@ func walkTar(tarFile string, match string, dir bool) (int64, bool, error) {
103103
func TestStorage_Archive(t *testing.T) {
104104
dir := t.TempDir()
105105

106-
storage, err := NewStorage(dir, "hostname", time.Minute, 2)
106+
storage, err := NewStorage(dir, "hostname", time.Minute, 2, 0)
107107
if err != nil {
108108
t.Fatalf("error while bootstrapping storage: %v", err)
109109
}
@@ -263,7 +263,7 @@ func TestStorageRemoveAllButCurrent(t *testing.T) {
263263
t.Run("bad directory in archive", func(t *testing.T) {
264264
dir := t.TempDir()
265265

266-
s, err := NewStorage(dir, "hostname", time.Minute, 2)
266+
s, err := NewStorage(dir, "hostname", time.Minute, 2, 0)
267267
if err != nil {
268268
t.Fatalf("Valid path did not successfully return: %v", err)
269269
}
@@ -277,7 +277,7 @@ func TestStorageRemoveAllButCurrent(t *testing.T) {
277277
g := NewWithT(t)
278278
dir := t.TempDir()
279279

280-
s, err := NewStorage(dir, "hostname", time.Minute, 2)
280+
s, err := NewStorage(dir, "hostname", time.Minute, 2, 0)
281281
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
282282

283283
artifact := sourcev1.Artifact{
@@ -338,7 +338,7 @@ func TestStorageRemoveAll(t *testing.T) {
338338
g := NewWithT(t)
339339
dir := t.TempDir()
340340

341-
s, err := NewStorage(dir, "hostname", time.Minute, 2)
341+
s, err := NewStorage(dir, "hostname", time.Minute, 2, 0)
342342
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
343343

344344
artifact := sourcev1.Artifact{
@@ -364,7 +364,7 @@ func TestStorageCopyFromPath(t *testing.T) {
364364

365365
dir := t.TempDir()
366366

367-
storage, err := NewStorage(dir, "hostname", time.Minute, 2)
367+
storage, err := NewStorage(dir, "hostname", time.Minute, 2, 0)
368368
if err != nil {
369369
t.Fatalf("error while bootstrapping storage: %v", err)
370370
}
@@ -542,7 +542,7 @@ func TestStorage_getGarbageFiles(t *testing.T) {
542542
g := NewWithT(t)
543543
dir := t.TempDir()
544544

545-
s, err := NewStorage(dir, "hostname", tt.ttl, tt.maxItemsToBeRetained)
545+
s, err := NewStorage(dir, "hostname", tt.ttl, tt.maxItemsToBeRetained, 0)
546546
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
547547

548548
artifact := sourcev1.Artifact{
@@ -616,7 +616,7 @@ func TestStorage_GarbageCollect(t *testing.T) {
616616
g := NewWithT(t)
617617
dir := t.TempDir()
618618

619-
s, err := NewStorage(dir, "hostname", time.Second*2, 2)
619+
s, err := NewStorage(dir, "hostname", time.Second*2, 2, 0)
620620
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
621621

622622
artifact := sourcev1.Artifact{
@@ -658,3 +658,90 @@ func TestStorage_GarbageCollect(t *testing.T) {
658658
})
659659
}
660660
}
661+
662+
func TestStorage_MaxSize(t *testing.T) {
663+
createFiles := func(files map[string][]byte) (dir string, err error) {
664+
dir = t.TempDir()
665+
for name, b := range files {
666+
absPath := filepath.Join(dir, name)
667+
if err = os.MkdirAll(filepath.Dir(absPath), 0o750); err != nil {
668+
return
669+
}
670+
f, err := os.Create(absPath)
671+
if err != nil {
672+
return "", fmt.Errorf("could not create file %q: %w", absPath, err)
673+
}
674+
if n, err := f.Write(b); err != nil {
675+
f.Close()
676+
return "", fmt.Errorf("could not write %d bytes to file %q: %w", n, f.Name(), err)
677+
}
678+
f.Close()
679+
}
680+
return
681+
}
682+
683+
tests := []struct {
684+
name string
685+
files map[string][]byte
686+
maxSize int64
687+
wantErrMatch string
688+
}{
689+
{
690+
name: "creates artifact without size limit",
691+
files: map[string][]byte{
692+
"test.txt": []byte(`contents`),
693+
"test.yaml": []byte(`a: b`),
694+
},
695+
maxSize: -1,
696+
wantErrMatch: "",
697+
},
698+
{
699+
name: "fails to create artifact due to size limit",
700+
files: map[string][]byte{
701+
"test.txt": []byte(`contents`),
702+
"test.yaml": []byte(`a: b`),
703+
},
704+
maxSize: 200,
705+
wantErrMatch: "exceeds the max limit",
706+
},
707+
{
708+
name: "creates artifact in the size limit range",
709+
files: map[string][]byte{
710+
"test.txt": []byte(`contents`),
711+
"test.yaml": []byte(`a: b`),
712+
},
713+
maxSize: 300,
714+
},
715+
}
716+
717+
for _, tt := range tests {
718+
t.Run(tt.name, func(t *testing.T) {
719+
g := NewWithT(t)
720+
721+
dir, err := createFiles(tt.files)
722+
if err != nil {
723+
t.Error(err)
724+
return
725+
}
726+
defer os.RemoveAll(dir)
727+
728+
artifact := sourcev1.Artifact{
729+
Path: filepath.Join(randStringRunes(10), randStringRunes(10), randStringRunes(10)+".tar.gz"),
730+
}
731+
732+
s, err := NewStorage(dir, "hostname", time.Second*2, 2, tt.maxSize)
733+
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
734+
735+
if err := s.MkdirAll(artifact); err != nil {
736+
t.Fatalf("artifact directory creation failed: %v", err)
737+
}
738+
739+
err = s.Archive(&artifact, dir, SourceIgnoreFilter(nil, nil))
740+
if tt.wantErrMatch == "" {
741+
g.Expect(err).ToNot(HaveOccurred())
742+
} else {
743+
g.Expect(err.Error()).To(ContainSubstring(tt.wantErrMatch))
744+
}
745+
})
746+
}
747+
}

‎controllers/suite_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -354,7 +354,7 @@ func initTestTLS() {
354354
}
355355

356356
func newTestStorage(s *testserver.HTTPServer) (*Storage, error) {
357-
storage, err := NewStorage(s.Root(), s.URL(), retentionTTL, retentionRecords)
357+
storage, err := NewStorage(s.Root(), s.URL(), retentionTTL, retentionRecords, 0)
358358
if err != nil {
359359
return nil, err
360360
}

‎main.go

+7-3
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ import (
5454
)
5555

5656
const controllerName = "source-controller"
57+
const artifactMaxSizeDefault int64 = 50 << 20
5758

5859
var (
5960
scheme = runtime.NewScheme()
@@ -101,6 +102,7 @@ func main() {
101102
helmCachePurgeInterval string
102103
artifactRetentionTTL time.Duration
103104
artifactRetentionRecords int
105+
artifactMaxSize int64
104106
)
105107

106108
flag.StringVar(&metricsAddr, "metrics-addr", envOrDefault("METRICS_ADDR", ":8080"),
@@ -139,6 +141,8 @@ func main() {
139141
"The duration of time that artifacts will be kept in storage before being garbage collected.")
140142
flag.IntVar(&artifactRetentionRecords, "artifact-retention-records", 2,
141143
"The maximum number of artifacts to be kept in storage after a garbage collection.")
144+
flag.Int64Var(&artifactMaxSize, "artifact-max-size", artifactMaxSizeDefault,
145+
"The max allowed size in bytes of an artifact contents produced from sources. The limit can be disabled by setting the value to -1.")
142146

143147
clientOptions.BindFlags(flag.CommandLine)
144148
logOptions.BindFlags(flag.CommandLine)
@@ -202,7 +206,7 @@ func main() {
202206
if storageAdvAddr == "" {
203207
storageAdvAddr = determineAdvStorageAddr(storageAddr, setupLog)
204208
}
205-
storage := mustInitStorage(storagePath, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, setupLog)
209+
storage := mustInitStorage(storagePath, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, artifactMaxSize, setupLog)
206210

207211
if err = managed.InitManagedTransport(); err != nil {
208212
// Log the error, but don't exit so as to not block reconcilers that are healthy.
@@ -350,14 +354,14 @@ func startFileServer(path string, address string, l logr.Logger) {
350354
}
351355
}
352356

353-
func mustInitStorage(path string, storageAdvAddr string, artifactRetentionTTL time.Duration, artifactRetentionRecords int, l logr.Logger) *controllers.Storage {
357+
func mustInitStorage(path string, storageAdvAddr string, artifactRetentionTTL time.Duration, artifactRetentionRecords int, artifactNaxSize int64, l logr.Logger) *controllers.Storage {
354358
if path == "" {
355359
p, _ := os.Getwd()
356360
path = filepath.Join(p, "bin")
357361
os.MkdirAll(path, 0o700)
358362
}
359363

360-
storage, err := controllers.NewStorage(path, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords)
364+
storage, err := controllers.NewStorage(path, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, artifactNaxSize)
361365
if err != nil {
362366
l.Error(err, "unable to initialise storage")
363367
os.Exit(1)

‎tests/fuzz/gitrepository_fuzzer.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ func startEnvServer(setupReconcilers func(manager.Manager)) *envtest.Environment
174174
panic(err)
175175
}
176176
defer os.RemoveAll(tmpStoragePath)
177-
storage, err = controllers.NewStorage(tmpStoragePath, "localhost:5050", time.Minute*1, 2)
177+
storage, err = controllers.NewStorage(tmpStoragePath, "localhost:5050", time.Minute*1, 2, 0)
178178
if err != nil {
179179
panic(err)
180180
}

0 commit comments

Comments
 (0)
Please sign in to comment.