Allow local override of operational configs #5314
Conversation
mjungsbluth
changed the title
plugins/logs/plugin.go
Outdated
| err := d.Decode(&configStruct) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("discovery only allows manual configuration of the decision_logs plugin for the reporting.* configuration items") |
There was a problem hiding this comment.
This is a bit odd. The code makes me think that there might be many ways to fail d.Decode() and yet we pretend that it's always due to extra config items.
I suppose one alternative would be something along the lines of
m := map[string]interface{}{}
if err := json.NewDecoder(...).Decode(&m); err != nil {
return err
}
_, ok := m["reporting"]
if ok && len(m) == 1 {
return nil
}
return fmt.Errorf("discovery only allows manual configuration of the decision_logs plugin for the reporting.* configuration items")There was a problem hiding this comment.
yeah, this definitely needs some work. I originally wanted to only allow a subset of reporting, i.e. the buffer sizes and client rates. And agreed that there are other error scenarios, I was about to decode plainly and with disallowing unknown fields and then comparing if plain decoding works without error to narrow the root cause.
If we agree to make the entire runtime tree overridable, your suggestion is of course much simpler. I am not sure about the Trigger config inside runtime, wdyt?
| "plugins": { | ||
| "test_plugin": {"a": "b"} | ||
| }, | ||
| "decision_logs": {"partition_name": "bar"} |
There was a problem hiding this comment.
Would it be worth testing that in the case that discovery also sets the value, the local override wins?
There was a problem hiding this comment.
Yes, also we have to agree on order of the override, which is why I posted this as draft. The current version lets the local version win in case both discovery and local for example contain decision_logs.runtime.* configs. We can also reverse this. This still allows to generally configure operational attributes locally if they are not forced centrally.
| factory := result.factories[name] | ||
| var pluginConfig json.RawMessage | ||
| if name == "decision_logs" { | ||
| if err := logs.AllowConfigOverride(manager.Config.DecisionLogs); err != nil { |
There was a problem hiding this comment.
🤔 so decision logs doesn't live in manager.Config.Plugins["decision_logs"]? Unfortunate 😳
There was a problem hiding this comment.
yeah, that is actually how I started 😄 . All the "initial" plugins also do not implement the Factory interface so I guess this is all structures from the early days. It also leads to quite a bit of boilerplate further down...
Co-authored-by: Stephan Renatus <stephan@styra.com>
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This pull request has been automatically marked as stale because it has not had any activity in the last 30 days. |
|
🟢 Let's circle back on this if your objective still isn't met in any ways. Cleaning up the PR for now. 🧹 |
A draft PR to address #5070
It makes specific attributes on the OPA configuration overridable if discovery is used
decision_logs.runtime.*attributescachingconfiguration to be overriddenIt also introduces an interface for plugin factories to make their config partially overridable.
This PR is still a draft because at least one negative test and a test for the plugin factories is missing