Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Race condition in bundle plugin #6849

Closed
Pushpalanka opened this issue Jul 4, 2024 · 3 comments · Fixed by #6921
Closed

Race condition in bundle plugin #6849

Pushpalanka opened this issue Jul 4, 2024 · 3 comments · Fixed by #6921
Assignees
@ashutosh-narkar
Labels
bug

Comments

@Pushpalanka
Copy link
Contributor

Pushpalanka commented Jul 4, 2024 •
edited
Loading

Short description

We are using OPA v0.65.0 as a library and encountered below race condition while using listeners on bundle plugin status. It seems there is a race between writes from reconfigure method and reads at persistbundle methods on bundle plugin configuration.

Error trace:

WARNING: DATA RACE
Write at 0x00c0000001a0 by goroutine 98:
  github.com/open-policy-agent/opa/plugins/bundle.(*Plugin).Reconfigure()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/bundle/plugin.go:160 +0x147
  github.com/open-policy-agent/opa/plugins/discovery.(*Discovery).reconfigure()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/discovery/discovery.go:405 +0x196
  github.com/open-policy-agent/opa/plugins/discovery.(*Discovery).processUpdate()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/discovery/discovery.go:339 +0x513
  github.com/open-policy-agent/opa/plugins/discovery.(*Discovery).oneShot()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/discovery/discovery.go:308 +0x8f
  github.com/open-policy-agent/opa/plugins/discovery.(*Discovery).oneShot-fm()
      <autogenerated>:1 +0x[75](https://github.com/zalando/skipper/actions/runs/9785181899/job/27017670664?pr=3120#step:6:76)
  github.com/open-policy-agent/opa/download.(*Downloader).oneShot()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:279 +0x506
  github.com/open-policy-agent/opa/download.(*Downloader).loop()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:227 +0x10d
  github.com/open-policy-agent/opa/download.(*Downloader).doStart.gowrap1()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:191 +0x4f

Previous read at 0x00c0000001a0 by goroutine 107:
  github.com/open-policy-agent/opa/plugins/bundle.(*Plugin).persistBundle()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/bundle/plugin.go:646 +0xe18
  github.com/open-policy-agent/opa/plugins/bundle.(*Plugin).process()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/bundle/plugin.go:512 +0xd93
  github.com/open-policy-agent/opa/plugins/bundle.(*Plugin).oneShot()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/bundle/plugin.go:453 +0x124
  github.com/open-policy-agent/opa/plugins/bundle.(*Plugin).newDownloader.func1()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/plugins/bundle/plugin.go:425 +0xa4
  github.com/open-policy-agent/opa/download.(*Downloader).oneShot()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:2[79](https://github.com/zalando/skipper/actions/runs/9785181899/job/27017670664?pr=3120#step:6:80) +0x506
  github.com/open-policy-agent/opa/download.(*Downloader).loop()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:227 +0x10d
  github.com/open-policy-agent/opa/download.(*Downloader).doStart.gowrap1()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:191 +0x4f

Goroutine 98 (running) created at:
  github.com/open-policy-agent/opa/download.(*Downloader).doStart()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:191 +0x113
  github.com/open-policy-agent/opa/download.(*Downloader).Start.gowrap1()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:1[82](https://github.com/zalando/skipper/actions/runs/9785181899/job/27017670664?pr=3120#step:6:83) +0x4f

Goroutine 107 (running) created at:
  github.com/open-policy-agent/opa/download.(*Downloader).doStart()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:191 +0x113
  github.com/open-policy-agent/opa/download.(*Downloader).Start.gowrap1()
      /home/runner/go/pkg/mod/github.com/open-policy-agent/opa@v0.66.0/download/download.go:182 +0x4f
==================

cc: @mjungsbluth
@Pushpalanka Pushpalanka added the bug label Jul 4, 2024
This was referenced Jul 4, 2024
Draft
Closed
@ashutosh-narkar
Copy link
Member

One option would be to use the config mutex in places where the config is accessed.

@mjungsbluth
Copy link
Contributor

@ashutosh-narkar that is possible but will be brittle given the number of code sites and entry points, we might worsen the situation by introducing deadlocks. We could change it to a RW Lock which would make it easier to control. Wdyt?

@ashutosh-narkar
Copy link
Member

Sure. That should be fine.

ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 8, 2024
@ashutosh-narkar
plugins/bundle: Avoid race during bundle reconfiguration and activation
ef3612c 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugins occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 8, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
e611d0e 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugins occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
@ashutosh-narkar ashutosh-narkar mentioned this issue Aug 8, 2024
Merged
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 8, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
6d966f9 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugin occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 8, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
41687ac 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugin occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
@ashutosh-narkar ashutosh-narkar moved this from Backlog to In Progress in Open Policy Agent Aug 8, 2024
@ashutosh-narkar ashutosh-narkar self-assigned this Aug 8, 2024
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 13, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
be9966c 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugin occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 13, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
ba5152f 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugin occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit to ashutosh-narkar/opa that referenced this issue Aug 13, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
ea352f3 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugin occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: open-policy-agent#6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar added a commit that referenced this issue Aug 14, 2024
@ashutosh-narkar @Pushpalanka
plugins/bundle: Avoid race during bundle reconfiguration and activation
5c0f300 
This change attempts to fix a race condition that could occur
when a reconfiguration on the bundle plugin occurs in parallel
with the activation of a downloaded bundle.

One scenario where this could occur is when the discovery plugin
attempts to reconfigure the bundle plugin and concurrently a bundle gets
downloaded and needs to be activated. A reconfig operation will perform
a write on the plugin's config. During a bundle activation, the plugin's
config is read. Currently we hold a lock when the config is being updated.
This change locks the config while reading thereby avoiding the race.

Fixes: #6849

Co-authored-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
@github-project-automation github-project-automation bot moved this from In Progress to Done in Open Policy Agent Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ashutosh-narkar ashutosh-narkar

Labels
bug
Projects
Open Policy Agent
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

plugins/bundle: Avoid race during bundle reconfiguration and activation ashutosh-narkar/opa
3 participants
@Pushpalanka @mjungsbluth @ashutosh-narkar