Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 55 additions & 22 deletions v1/download/oci_download.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,33 @@ import (

// NewOCI returns a new Downloader that can be started.
func NewOCI(config Config, client rest.Client, path, storePath string) *OCIDownloader {
localStoreIsTemp := false
if storePath == "" {
var err error
storePath, err = os.MkdirTemp("", "opa-oci-*")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned about orphaned temporary directories, this path can be called when a new bundle or updated bundle comes in. There should be a way to clean up these dirs when the downloader is stopped.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I’ve pushed a follow-up change that cleans up downloader-owned temporary OCI stores when Stop() is called.

Stop() now waits for the periodic download loop and any in-flight manual trigger to finish before removing the temporary store. Explicit store paths, including persistence_directory-backed stores, are preserved. I also made sure the opened OCI bundle file is closed before cleanup so the directory can be removed reliably.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Could you add tests covering the default vs. explicit store paths, the temp-dir cleanup behavior, and the new stopOnce/triggerWG teardown ? The plugin wiring in bundle/discovery for the persistence_directory branch would be worth a quick test too.

if err != nil {
panic(err)
}
localStoreIsTemp = true
}

localstore, err := oci.New(storePath)
if err != nil {
if localStoreIsTemp {
_ = os.RemoveAll(storePath)
}
panic(err)
}
return &OCIDownloader{
config: config,
path: path,
localStorePath: storePath,
client: client,
trigger: make(chan chan struct{}),
stop: make(chan chan struct{}),
logger: client.Logger(),
store: localstore,
config: config,
path: path,
localStorePath: storePath,
localStoreIsTemp: localStoreIsTemp,
client: client,
trigger: make(chan chan struct{}),
stop: make(chan chan struct{}),
logger: client.Logger(),
store: localstore,
}
}

Expand Down Expand Up @@ -99,9 +113,19 @@ func (d *OCIDownloader) SetCache(etag string) {
// Trigger can be used to control when the downloader attempts to download
// a new bundle in manual triggering mode.
func (d *OCIDownloader) Trigger(ctx context.Context) error {
d.stateMtx.Lock()
if d.stopped {
d.stateMtx.Unlock()
return errors.New("downloader stopped")
}
d.triggerWG.Add(1)
d.stateMtx.Unlock()

done := make(chan error)

go func() {
defer d.triggerWG.Done()

err := d.oneShot(ctx)
if err != nil {
d.logger.Error("OCI - Bundle download failed: %v.", err)
Expand Down Expand Up @@ -129,20 +153,29 @@ func (d *OCIDownloader) Start(ctx context.Context) {

// Stop tells the Downloader to stop downloading bundles.
func (d *OCIDownloader) Stop(context.Context) {
if *d.config.Trigger == plugins.TriggerManual {
return
}
d.stopOnce.Do(func() {
d.stateMtx.Lock()
d.stopped = true
d.stateMtx.Unlock()

if *d.config.Trigger == plugins.TriggerPeriodic {
done := make(chan struct{})
d.stop <- done
<-done
}

d.mtx.Lock()
defer d.mtx.Unlock()
d.triggerWG.Wait()
d.cleanupLocalStore()
})
}

if d.stopped {
func (d *OCIDownloader) cleanupLocalStore() {
if !d.localStoreIsTemp {
return
}

done := make(chan struct{})
d.stop <- done
<-done
if err := os.RemoveAll(d.localStorePath); err != nil {
d.logger.Error("OCI - Failed to remove temporary store %q: %v.", d.localStorePath, err)
}
}

func (d *OCIDownloader) doStart(context.Context) {
Expand All @@ -155,7 +188,6 @@ func (d *OCIDownloader) doStart(context.Context) {
done := <-d.stop // blocks until there's something to read
cancel()
d.wg.Wait()
d.stopped = true
close(done)
}

Expand Down Expand Up @@ -261,14 +293,15 @@ func (d *OCIDownloader) download(ctx context.Context, m metrics.Metrics) (*downl
}, nil
}
fileReader, err := os.Open(bundleFilePath)
if err != nil {
return nil, err
}
defer fileReader.Close()

cnt := &count{}
r := io.TeeReader(fileReader, cnt)
tee := io.TeeReader(r, &buf)

if err != nil {
return nil, err
}
loader := bundle.NewTarballLoaderWithBaseURL(tee, d.localStorePath)
reader := bundle.NewCustomReader(loader).
WithMetrics(m).
Expand Down
5 changes: 4 additions & 1 deletion v1/download/oci_downloader.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,18 @@ type OCIDownloader struct {
client rest.Client // HTTP client to use for bundle downloading
path string // path for OCI image as <registry>/<org>/<repo>:<tag>
localStorePath string // path for the local OCI storage
localStoreIsTemp bool // whether localStorePath should be removed when stopping
trigger chan chan struct{} // channel to signal downloads when manual triggering is enabled
stop chan chan struct{} // used to signal plugin to stop running
f func(context.Context, Update) error // callback function invoked when download updates occur
sizeLimitBytes *int64 // max bundle file size in bytes (passed to reader)
bvc *bundle.VerificationConfig
wg sync.WaitGroup
triggerWG sync.WaitGroup
logger logging.Logger
mtx sync.Mutex
stateMtx sync.Mutex
stopped bool
stopOnce sync.Once
persist bool
store *oci.Store
etag string
Expand Down
2 changes: 1 addition & 1 deletion v1/plugins/bundle/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,7 @@ func (p *Plugin) newDownloader(name string, source *Source, bundles map[string]*
return p.oneShot(ctx, name, u)
}
if strings.ToLower(client.Config().Type) == "oci" {
ociStorePath := filepath.Join(os.TempDir(), "opa", "oci") // use temporary folder /tmp/opa/oci
ociStorePath := ""
if cfg := p.manager.GetConfig(); cfg.PersistenceDirectory != nil {
ociStorePath = filepath.Join(*cfg.PersistenceDirectory, "oci")
}
Expand Down
2 changes: 1 addition & 1 deletion v1/plugins/discovery/discovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ func New(manager *plugins.Manager, opts ...func(*Discovery)) (*Discovery, error)
result.config = config
restClient := manager.Client(config.service)
if strings.ToLower(restClient.Config().Type) == "oci" {
ociStorePath := filepath.Join(os.TempDir(), "opa", "oci") // use temporary folder /tmp/opa/oci
ociStorePath := ""
if managerConfig.PersistenceDirectory != nil {
ociStorePath = filepath.Join(*managerConfig.PersistenceDirectory, "oci")
}
Expand Down