Skip to content

Commit 4740843

Browse files
oceanc80openshift-merge-bot[bot]
oceanc80
authored and
openshift-merge-bot[bot]
committed
UPSTREAM: <drop>: Clear cache on startup, use tempDir for unpacking (#1669)
Signed-off-by: Per Goncalves da Silva <[email protected]>
1 parent 8f2307c commit 4740843

File tree

7 files changed

+155
-140
lines changed

7 files changed

+155
-140
lines changed

‎catalogd/cmd/catalogd/main.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ import (
6363
"github.com/operator-framework/operator-controller/catalogd/internal/storage"
6464
"github.com/operator-framework/operator-controller/catalogd/internal/version"
6565
"github.com/operator-framework/operator-controller/catalogd/internal/webhook"
66+
"github.com/operator-framework/operator-controller/internal/util"
6667
)
6768

6869
var (
@@ -257,8 +258,8 @@ func main() {
257258
systemNamespace = podNamespace()
258259
}
259260

260-
if err := os.MkdirAll(cacheDir, 0700); err != nil {
261-
setupLog.Error(err, "unable to create cache directory")
261+
if err := util.EnsureEmptyDirectory(cacheDir, 0700); err != nil {
262+
setupLog.Error(err, "unable to ensure empty cache directory")
262263
os.Exit(1)
263264
}
264265

‎catalogd/internal/source/containers_image.go‎

Lines changed: 13 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,8 @@ import (
2929
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3030

3131
catalogdv1 "github.com/operator-framework/operator-controller/catalogd/api/v1"
32+
"github.com/operator-framework/operator-controller/internal/rukpak/source"
33+
"github.com/operator-framework/operator-controller/internal/util"
3234
)
3335

3436
const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
@@ -70,12 +72,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
7072
//
7173
//////////////////////////////////////////////////////
7274
unpackPath := i.unpackPath(catalog.Name, canonicalRef.Digest())
73-
if unpackStat, err := os.Stat(unpackPath); err == nil {
74-
if !unpackStat.IsDir() {
75-
panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath))
76-
}
75+
if isUnpacked, unpackTime, err := source.IsImageUnpacked(unpackPath); isUnpacked && err == nil {
7776
l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
78-
return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil
77+
return successResult(unpackPath, canonicalRef, unpackTime), nil
78+
} else if err != nil {
79+
return nil, fmt.Errorf("error checking image already unpacked: %w", err)
7980
}
8081

8182
//////////////////////////////////////////////////////
@@ -148,7 +149,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
148149
//
149150
//////////////////////////////////////////////////////
150151
if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil {
151-
if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil {
152+
if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
152153
err = errors.Join(err, cleanupErr)
153154
}
154155
return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -189,7 +190,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa
189190
}
190191

191192
func (i *ContainersImageRegistry) Cleanup(_ context.Context, catalog *catalogdv1.ClusterCatalog) error {
192-
if err := deleteRecursive(i.catalogPath(catalog.Name)); err != nil {
193+
if err := source.DeleteReadOnlyRecursive(i.catalogPath(catalog.Name)); err != nil {
193194
return fmt.Errorf("error deleting catalog cache: %w", err)
194195
}
195196
return nil
@@ -288,8 +289,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
288289
return wrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
289290
}
290291

291-
if err := os.MkdirAll(unpackPath, 0700); err != nil {
292-
return fmt.Errorf("error creating unpack directory: %w", err)
292+
if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
293+
return fmt.Errorf("error ensuring empty unpack directory: %w", err)
293294
}
294295
l := log.FromContext(ctx)
295296
l.Info("unpacking image", "path", unpackPath)
@@ -307,10 +308,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
307308
l.Info("applied layer", "layer", i)
308309
return nil
309310
}(); err != nil {
310-
return errors.Join(err, deleteRecursive(unpackPath))
311+
return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath))
311312
}
312313
}
313-
if err := setReadOnlyRecursive(unpackPath); err != nil {
314+
if err := source.SetReadOnlyRecursive(unpackPath); err != nil {
314315
return fmt.Errorf("error making unpack directory read-only: %w", err)
315316
}
316317
return nil
@@ -354,69 +355,13 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo
354355
continue
355356
}
356357
imgDirPath := filepath.Join(catalogPath, imgDir.Name())
357-
if err := deleteRecursive(imgDirPath); err != nil {
358+
if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil {
358359
return fmt.Errorf("error removing image directory: %w", err)
359360
}
360361
}
361362
return nil
362363
}
363364

364-
func setReadOnlyRecursive(root string) error {
365-
if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
366-
if err != nil {
367-
return err
368-
}
369-
370-
fi, err := d.Info()
371-
if err != nil {
372-
return err
373-
}
374-
375-
if err := func() error {
376-
switch typ := fi.Mode().Type(); typ {
377-
case os.ModeSymlink:
378-
// do not follow symlinks
379-
// 1. if they resolve to other locations in the root, we'll find them anyway
380-
// 2. if they resolve to other locations outside the root, we don't want to change their permissions
381-
return nil
382-
case os.ModeDir:
383-
return os.Chmod(path, 0500)
384-
case 0: // regular file
385-
return os.Chmod(path, 0400)
386-
default:
387-
return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
388-
}
389-
}(); err != nil {
390-
return err
391-
}
392-
return nil
393-
}); err != nil {
394-
return fmt.Errorf("error making catalog cache read-only: %w", err)
395-
}
396-
return nil
397-
}
398-
399-
func deleteRecursive(root string) error {
400-
if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
401-
if os.IsNotExist(err) {
402-
return nil
403-
}
404-
if err != nil {
405-
return err
406-
}
407-
if !d.IsDir() {
408-
return nil
409-
}
410-
if err := os.Chmod(path, 0700); err != nil {
411-
return err
412-
}
413-
return nil
414-
}); err != nil {
415-
return fmt.Errorf("error making catalog cache writable for deletion: %w", err)
416-
}
417-
return os.RemoveAll(root)
418-
}
419-
420365
func wrapTerminal(err error, isTerminal bool) error {
421366
if !isTerminal {
422367
return err

‎cmd/operator-controller/main.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ import (
6868
"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
6969
"github.com/operator-framework/operator-controller/internal/rukpak/source"
7070
"github.com/operator-framework/operator-controller/internal/scheme"
71+
"github.com/operator-framework/operator-controller/internal/util"
7172
"github.com/operator-framework/operator-controller/internal/version"
7273
)
7374

@@ -297,6 +298,11 @@ func main() {
297298
}
298299
}
299300

301+
if err := util.EnsureEmptyDirectory(cachePath, 0700); err != nil {
302+
setupLog.Error(err, "unable to ensure empty cache directory")
303+
os.Exit(1)
304+
}
305+
300306
unpacker := &source.ContainersImageRegistry{
301307
BaseCachePath: filepath.Join(cachePath, "unpack"),
302308
SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) {
@@ -361,7 +367,7 @@ func main() {
361367
crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
362368
}
363369

364-
applier := &applier.Helm{
370+
helmApplier := &applier.Helm{
365371
ActionClientGetter: acg,
366372
Preflights: preflights,
367373
}
@@ -381,7 +387,7 @@ func main() {
381387
Client: cl,
382388
Resolver: resolver,
383389
Unpacker: unpacker,
384-
Applier: applier,
390+
Applier: helmApplier,
385391
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
386392
Finalizers: clusterExtensionFinalizers,
387393
Manager: cm,

‎internal/rukpak/source/containers_image.go‎

Lines changed: 12 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ import (
2323
"github.com/opencontainers/go-digest"
2424
"sigs.k8s.io/controller-runtime/pkg/log"
2525
"sigs.k8s.io/controller-runtime/pkg/reconcile"
26+
27+
"github.com/operator-framework/operator-controller/internal/util"
2628
)
2729

2830
type ContainersImageRegistry struct {
@@ -62,12 +64,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
6264
//
6365
//////////////////////////////////////////////////////
6466
unpackPath := i.unpackPath(bundle.Name, canonicalRef.Digest())
65-
if unpackStat, err := os.Stat(unpackPath); err == nil {
66-
if !unpackStat.IsDir() {
67-
panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath))
68-
}
67+
if isUnpacked, _, err := IsImageUnpacked(unpackPath); isUnpacked && err == nil {
6968
l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
7069
return successResult(bundle.Name, unpackPath, canonicalRef), nil
70+
} else if err != nil {
71+
return nil, fmt.Errorf("error checking bundle already unpacked: %w", err)
7172
}
7273

7374
//////////////////////////////////////////////////////
@@ -140,7 +141,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
140141
//
141142
//////////////////////////////////////////////////////
142143
if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil {
143-
if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil {
144+
if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
144145
err = errors.Join(err, cleanupErr)
145146
}
146147
return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -168,7 +169,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic
168169
}
169170

170171
func (i *ContainersImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error {
171-
return deleteRecursive(i.bundlePath(bundle.Name))
172+
return DeleteReadOnlyRecursive(i.bundlePath(bundle.Name))
172173
}
173174

174175
func (i *ContainersImageRegistry) bundlePath(bundleName string) string {
@@ -251,8 +252,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
251252
return fmt.Errorf("error creating image source: %w", err)
252253
}
253254

254-
if err := os.MkdirAll(unpackPath, 0700); err != nil {
255-
return fmt.Errorf("error creating unpack directory: %w", err)
255+
if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
256+
return fmt.Errorf("error ensuring empty unpack directory: %w", err)
256257
}
257258
l := log.FromContext(ctx)
258259
l.Info("unpacking image", "path", unpackPath)
@@ -270,10 +271,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
270271
l.Info("applied layer", "layer", i)
271272
return nil
272273
}(); err != nil {
273-
return errors.Join(err, deleteRecursive(unpackPath))
274+
return errors.Join(err, DeleteReadOnlyRecursive(unpackPath))
274275
}
275276
}
276-
if err := setReadOnlyRecursive(unpackPath); err != nil {
277+
if err := SetReadOnlyRecursive(unpackPath); err != nil {
277278
return fmt.Errorf("error making unpack directory read-only: %w", err)
278279
}
279280
return nil
@@ -310,65 +311,9 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK
310311
continue
311312
}
312313
imgDirPath := filepath.Join(bundlePath, imgDir.Name())
313-
if err := deleteRecursive(imgDirPath); err != nil {
314+
if err := DeleteReadOnlyRecursive(imgDirPath); err != nil {
314315
return fmt.Errorf("error removing image directory: %w", err)
315316
}
316317
}
317318
return nil
318319
}
319-
320-
func setReadOnlyRecursive(root string) error {
321-
if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
322-
if err != nil {
323-
return err
324-
}
325-
326-
fi, err := d.Info()
327-
if err != nil {
328-
return err
329-
}
330-
331-
if err := func() error {
332-
switch typ := fi.Mode().Type(); typ {
333-
case os.ModeSymlink:
334-
// do not follow symlinks
335-
// 1. if they resolve to other locations in the root, we'll find them anyway
336-
// 2. if they resolve to other locations outside the root, we don't want to change their permissions
337-
return nil
338-
case os.ModeDir:
339-
return os.Chmod(path, 0500)
340-
case 0: // regular file
341-
return os.Chmod(path, 0400)
342-
default:
343-
return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
344-
}
345-
}(); err != nil {
346-
return err
347-
}
348-
return nil
349-
}); err != nil {
350-
return fmt.Errorf("error making bundle cache read-only: %w", err)
351-
}
352-
return nil
353-
}
354-
355-
func deleteRecursive(root string) error {
356-
if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
357-
if os.IsNotExist(err) {
358-
return nil
359-
}
360-
if err != nil {
361-
return err
362-
}
363-
if !d.IsDir() {
364-
return nil
365-
}
366-
if err := os.Chmod(path, 0700); err != nil {
367-
return err
368-
}
369-
return nil
370-
}); err != nil {
371-
return fmt.Errorf("error making bundle cache writable for deletion: %w", err)
372-
}
373-
return os.RemoveAll(root)
374-
}

‎internal/rukpak/source/containers_image_test.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,16 @@ func TestUnpackUnexpectedFile(t *testing.T) {
277277
require.NoError(t, os.WriteFile(unpackPath, []byte{}, 0600))
278278

279279
// Attempt to pull and unpack the image
280-
assert.Panics(t, func() { _, _ = unpacker.Unpack(context.Background(), bundleSource) })
280+
_, err := unpacker.Unpack(context.Background(), bundleSource)
281+
require.NoError(t, err)
282+
283+
// Ensure unpack path is now a directory
284+
stat, err := os.Stat(unpackPath)
285+
require.NoError(t, err)
286+
require.True(t, stat.IsDir())
287+
288+
// Unset read-only to allow cleanup
289+
require.NoError(t, source.UnsetReadOnlyRecursive(unpackPath))
281290
}
282291

283292
func TestUnpackCopySucceedsMountFails(t *testing.T) {

0 commit comments

Comments
 (0)