SDK extensibility: add hooks (plugins, discovery, sdk) #6053
Conversation
srenatus
force-pushed
the
sr/hooks
branch
2 times, most recently
from
e214af0
to
eca1663
Compare
srenatus
force-pushed
the
sr/hooks
branch
4 times, most recently
from
e816b76
to
fd32a41
Compare
internal/errors/join_go1.20.go
Outdated
There was a problem hiding this comment.
[comment]: The combination of this file + internal/errors/join.go (with it's "anything not Go 1.20" build tag) is a clever workaround for not having errors.Join in older Golang stdlib versions. 🙂
[question]: Is there a way to specify a "less than " constraint in the build tag for internal/errors/join.go, instead of the !go1.20 constraint?
There was a problem hiding this comment.
AFAIK go1.20 means 1.20 and onwards; !go1.20 means anything before. Don't have a good reference at hand right now, though.
| m := &Manager{ | ||
| Store: store, | ||
| Config: parsedConfig, | ||
| ID: id, | ||
| keys: keys, | ||
| pluginStatus: map[string]*Status{}, | ||
| pluginStatusListeners: map[string]StatusListener{}, | ||
| maxErrors: -1, | ||
| interQueryBuiltinCacheConfig: interQueryBuiltinCacheConfig, | ||
| serverInitialized: make(chan struct{}), | ||
| bootstrapConfigLabels: parsedConfig.Labels, | ||
| Store: store, | ||
| Config: parsedConfig, | ||
| ID: id, | ||
| pluginStatus: map[string]*Status{}, | ||
| pluginStatusListeners: map[string]StatusListener{}, | ||
| maxErrors: -1, | ||
| serverInitialized: make(chan struct{}), | ||
| bootstrapConfigLabels: parsedConfig.Labels, |
There was a problem hiding this comment.
[comment]: This change makes sense in light of what happens a few lines further down in this function; we're delaying populating some of the Manager's fields until after all the overrides have happened.
| d, err := discovery.New(manager, discovery.Factories(opa.plugins)) | ||
| d, err := discovery.New(manager, | ||
| discovery.Factories(opa.plugins), | ||
| discovery.Hooks(opa.hooks), |
There was a problem hiding this comment.
[comment]: "Where the rubber hits the road" for the PR: the hooks are finally wired in next to the places they're used. This is pleasantly clean to look at.
plugins/hooks.go
Outdated
| return func(m *Manager) { | ||
| m.hooks = hs | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: What's the reason to create a new file for this v/s adding it in plugins/plugins.go?
| } | ||
|
|
||
| // ConfigHook allows inspecting or rewriting the configuration when the plugin | ||
| // manager is processing it. |
There was a problem hiding this comment.
This hook is expected to run only when a new manager is created not on reconfigure.
There was a problem hiding this comment.
Good point. Maybe it should. I'll look into it.
There was a problem hiding this comment.
Added a note to the comment.
hooks/hooks.go
Outdated
| OnConfig(context.Context, *config.Config) (*config.Config, error) | ||
| } | ||
|
|
||
| // ConfigHook allows inspecting or rewriting the configuration when the discovery |
There was a problem hiding this comment.
Nit: It's probably helpful to make it explicit that we're referring to the discovered config here. Also like the ConfigHook hook this isn't applicable on reconfigure.
This adds a lightweight extensibility mechanism to OPA: hooks. Loosely modelled on what franz-go supports (see refs below). We're starting with a configuration hook. It allows us to inspect or alter the configuration of OPA after... 1. the config is read and parsed: OnConfig 2. a discovery bundle is processed: OnConfigDiscovery References: - franz-go: https://pkg.go.dev/github.com/twmb/franz-go/pkg/kgo#Hook To follow: - more hooks where they are useful - runtime support for hooks, wiring them into the proper other places Signed-off-by: Stephan Renatus <stephan@styra.com>
Signed-off-by: Stephan Renatus <stephan@styra.com>
Signed-off-by: Stephan Renatus <stephan@styra.com>
Why the changes in this PR are needed?
This adds a lightweight extensibility mechanism to OPA: hooks. Loosely modelled on what franz-go supports (see refs below).
We're starting with a configuration hook. It allows us to inspect or alter the configuration of OPA after...
References:
To follow: