Skip to content

Commit ebafe3b

Browse files
neiswneisw
authored
Revert "UPSTREAM: <drop>: Clear cache on startup, use tempDir for unpacking (#1669)"
This reverts commit 4740843.
1 parent 3d533fc commit ebafe3b

File tree

7 files changed

+140
-155
lines changed

7 files changed

+140
-155
lines changed

‎catalogd/cmd/catalogd/main.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ 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"
6766
)
6867

6968
var (
@@ -258,8 +257,8 @@ func main() {
258257
systemNamespace = podNamespace()
259258
}
260259

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

‎catalogd/internal/source/containers_image.go‎

Lines changed: 68 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ 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"
3432
)
3533

3634
const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
@@ -72,11 +70,12 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
7270
//
7371
//////////////////////////////////////////////////////
7472
unpackPath := i.unpackPath(catalog.Name, canonicalRef.Digest())
75-
if isUnpacked, unpackTime, err := source.IsImageUnpacked(unpackPath); isUnpacked && err == nil {
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+
}
7677
l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
77-
return successResult(unpackPath, canonicalRef, unpackTime), nil
78-
} else if err != nil {
79-
return nil, fmt.Errorf("error checking image already unpacked: %w", err)
78+
return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil
8079
}
8180

8281
//////////////////////////////////////////////////////
@@ -149,7 +148,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
149148
//
150149
//////////////////////////////////////////////////////
151150
if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil {
152-
if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
151+
if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil {
153152
err = errors.Join(err, cleanupErr)
154153
}
155154
return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -190,7 +189,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa
190189
}
191190

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

292-
if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
293-
return fmt.Errorf("error ensuring empty unpack directory: %w", err)
291+
if err := os.MkdirAll(unpackPath, 0700); err != nil {
292+
return fmt.Errorf("error creating unpack directory: %w", err)
294293
}
295294
l := log.FromContext(ctx)
296295
l.Info("unpacking image", "path", unpackPath)
@@ -308,10 +307,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
308307
l.Info("applied layer", "layer", i)
309308
return nil
310309
}(); err != nil {
311-
return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath))
310+
return errors.Join(err, deleteRecursive(unpackPath))
312311
}
313312
}
314-
if err := source.SetReadOnlyRecursive(unpackPath); err != nil {
313+
if err := setReadOnlyRecursive(unpackPath); err != nil {
315314
return fmt.Errorf("error making unpack directory read-only: %w", err)
316315
}
317316
return nil
@@ -355,13 +354,69 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo
355354
continue
356355
}
357356
imgDirPath := filepath.Join(catalogPath, imgDir.Name())
358-
if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil {
357+
if err := deleteRecursive(imgDirPath); err != nil {
359358
return fmt.Errorf("error removing image directory: %w", err)
360359
}
361360
}
362361
return nil
363362
}
364363

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+
365420
func wrapTerminal(err error, isTerminal bool) error {
366421
if !isTerminal {
367422
return err

‎cmd/operator-controller/main.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,6 @@ 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"
7271
"github.com/operator-framework/operator-controller/internal/version"
7372
)
7473

@@ -298,11 +297,6 @@ func main() {
298297
}
299298
}
300299

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-
306300
unpacker := &source.ContainersImageRegistry{
307301
BaseCachePath: filepath.Join(cachePath, "unpack"),
308302
SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) {
@@ -367,7 +361,7 @@ func main() {
367361
crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
368362
}
369363

370-
helmApplier := &applier.Helm{
364+
applier := &applier.Helm{
371365
ActionClientGetter: acg,
372366
Preflights: preflights,
373367
}
@@ -387,7 +381,7 @@ func main() {
387381
Client: cl,
388382
Resolver: resolver,
389383
Unpacker: unpacker,
390-
Applier: helmApplier,
384+
Applier: applier,
391385
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
392386
Finalizers: clusterExtensionFinalizers,
393387
Manager: cm,

‎internal/rukpak/source/containers_image.go‎

Lines changed: 67 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,6 @@ 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"
2826
)
2927

3028
type ContainersImageRegistry struct {
@@ -64,11 +62,12 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
6462
//
6563
//////////////////////////////////////////////////////
6664
unpackPath := i.unpackPath(bundle.Name, canonicalRef.Digest())
67-
if isUnpacked, _, err := IsImageUnpacked(unpackPath); isUnpacked && err == nil {
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+
}
6869
l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
6970
return successResult(bundle.Name, unpackPath, canonicalRef), nil
70-
} else if err != nil {
71-
return nil, fmt.Errorf("error checking bundle already unpacked: %w", err)
7271
}
7372

7473
//////////////////////////////////////////////////////
@@ -141,7 +140,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
141140
//
142141
//////////////////////////////////////////////////////
143142
if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil {
144-
if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
143+
if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil {
145144
err = errors.Join(err, cleanupErr)
146145
}
147146
return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -169,7 +168,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic
169168
}
170169

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

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

255-
if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
256-
return fmt.Errorf("error ensuring empty unpack directory: %w", err)
254+
if err := os.MkdirAll(unpackPath, 0700); err != nil {
255+
return fmt.Errorf("error creating unpack directory: %w", err)
257256
}
258257
l := log.FromContext(ctx)
259258
l.Info("unpacking image", "path", unpackPath)
@@ -271,10 +270,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
271270
l.Info("applied layer", "layer", i)
272271
return nil
273272
}(); err != nil {
274-
return errors.Join(err, DeleteReadOnlyRecursive(unpackPath))
273+
return errors.Join(err, deleteRecursive(unpackPath))
275274
}
276275
}
277-
if err := SetReadOnlyRecursive(unpackPath); err != nil {
276+
if err := setReadOnlyRecursive(unpackPath); err != nil {
278277
return fmt.Errorf("error making unpack directory read-only: %w", err)
279278
}
280279
return nil
@@ -311,9 +310,65 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK
311310
continue
312311
}
313312
imgDirPath := filepath.Join(bundlePath, imgDir.Name())
314-
if err := DeleteReadOnlyRecursive(imgDirPath); err != nil {
313+
if err := deleteRecursive(imgDirPath); err != nil {
315314
return fmt.Errorf("error removing image directory: %w", err)
316315
}
317316
}
318317
return nil
319318
}
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: 1 addition & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -277,16 +277,7 @@ func TestUnpackUnexpectedFile(t *testing.T) {
277277
require.NoError(t, os.WriteFile(unpackPath, []byte{}, 0600))
278278

279279
// Attempt to pull and unpack the image
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))
280+
assert.Panics(t, func() { _, _ = unpacker.Unpack(context.Background(), bundleSource) })
290281
}
291282

292283
func TestUnpackCopySucceedsMountFails(t *testing.T) {

0 commit comments

Comments
 (0)