enable direct error handling for bundle plugin trigger method #7143
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
torwunder
force-pushed
the
bundle_plugin_error_return_value
branch
2 times, most recently
from
8ae5c93
to
201abe0
Compare
…r errors for bundles in manual trigger mode Signed-off-by: Torsten Wunderlich <[email protected]>
torwunder
force-pushed
the
bundle_plugin_error_return_value
branch
from
201abe0
to
c510621
Compare
| @@ -6522,6 +6522,68 @@ func TestPluginManualTriggerWithTimeout(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestPluginManualTriggerWithServerError(t *testing.T) { | |||
There was a problem hiding this comment.
This test triggers the race condition.
My investigation led to the conclusion that this is triggered by having the bundle with the "periodic" trigger mode in the test.
The downloader.oneShot function will be triggered by the explicitly trigger method and also by the downloader.loop function.
The access to the shared variables (f.ex. etag, client) in Downloader is not synchronized and leads to the race condition.
A possible solution is to only trigger bundle dowloads with "manual" trigger mode in the trigger method.
F.ex. like this (in bundle.Plugin.Trigger)
func (p *Plugin) Trigger(ctx context.Context) error {
[...]
for name, dl := range p.downloaders {
downloaders[name] = dl
p.cfgMtx.RLock()
triggerMode := p.config.Bundles[name].Trigger
p.cfgMtx.RUnlock()
if triggerMode != nil && *triggerMode == plugins.TriggerManual {
downloaders[name] = dl
}
}
[...]
This would also solve the question if Trigger should return errors for bundles with "periodic" trigger mode.
There was a problem hiding this comment.
@ashutosh-narkar
Do you have a recommendation how to proceed with this?
To make the test green, I could remove the configuration for the bundle in "periodic" trigger mode but I think this might be a problem in the bundle plugin that should be addressed.
There was a problem hiding this comment.
I think having only a manual trigger for the test should be fine.
There was a problem hiding this comment.
Thanks for the contribution @torwunder! One high level comment I have is about the structure of the error. If we can model it similar to https://github.com/open-policy-agent/opa/blob/main/plugins/bundle/status.go#L64, it would be more informative I think.
* model Error similar to bundle status Signed-off-by: Torsten Wunderlich <[email protected]>
|
@ashutosh-narkar I modelled the error similar to the status. |
| return fmt.Sprintf("Bundle name: %s, Code: %s, HTTPCode: %d, Message: %s", e.BundleName, errCode, e.HTTPCode, e.Message) | ||
| } | ||
|
|
||
| func (e Error) Unwrap() error { |
There was a problem hiding this comment.
Nit: Is there a reason to define this method?
There was a problem hiding this comment.
it will allow using errors.As for retrieving the wrapped error. Something like this:
errs := Errors{NewBundleError("example", download.HTTPError{StatusCode: 500})}
var httpError download.HTTPError
errors.As(errs, &httpError)
if httpError.StatusCode == 500 { ...
There is a test in errors_test.go (TestUnwrap) that will start to fail if we remove the function.
| // plugin callback will also log the trigger error and include it in the bundle status | ||
| err := d.Trigger(ctx) | ||
| // only return errors for TriggerMode manual as periodic bundles will be retried | ||
| if err != nil && *p.config.Bundles[name].Trigger == plugins.TriggerManual { |
There was a problem hiding this comment.
Use the Config() method on the plugin to retrieve the config.
There was a problem hiding this comment.
@ashutosh-narkar do you think it makes sense to only return the errors for bundles in manual trigger mode?
It felt sensible to me as periodic triggermode will be retried by the bundle plugin but might also just create confusion.
it also creates this additional if statement which will then not be tested.
There was a problem hiding this comment.
Manual trigger mode can currently only be configured via a library integration. So to keep parity with the discovery plugin, returning errors in the manual mode makes sense.
| @@ -6522,6 +6522,68 @@ func TestPluginManualTriggerWithTimeout(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestPluginManualTriggerWithServerError(t *testing.T) { | |||
There was a problem hiding this comment.
I think having only a manual trigger for the test should be fine.
Why the changes in this PR are needed?
When using opa as a library you may want to directly control when and how opa interacts with the bundle registry.
When switching to manual trigger mode and triggering the the bundle plugin directly you may want to react to errors that occurred when downloading the bundles.
The current capability of using the bundle status requires configuring a listener and wait for it to be triggered which differs from the approach taken f.ex. in the discovery plugin.
By returning the downloader errors directly in the bundle plugin trigger method you can f.ex. check for HTTPError and its status code so to determine if a retry can be successful (status code 5xx) or will yield the same result (4xx).
What are the changes in this PR?
Notes to assist PR review:
Further comments: