Plugin reconfiguration support#5166
Conversation
Signed-off-by: Andrew Harding <azdagron@gmail.com>
azdagron
requested review from
MarcosDY,
amartinezfayo,
evan2645 and
rturner3
as code owners
Signed-off-by: Andrew Harding <azdagron@gmail.com>
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
This looks great, @azdagron!
I wanted to leave a couple of suggestions before finishing the review.
|
|
||
| Please see the [built-in plugins](#built-in-plugins) section below for information on plugins that are available out-of-the-box. | ||
|
|
||
| ### Reconfiguring plugins (Posix only) |
There was a problem hiding this comment.
Maybe it's just me, but it's not super clear if the plugin configuration should include plugin_data { or not. Maybe a short example of how a plugin config file looks like would be good?
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-ch: | ||
| reconfigurer.Reconfigure(ctx) |
There was a problem hiding this comment.
If I'm sending the SIGUSR1 signal to the process and I don't see any plugin being reconfigured (because I missed to configure plugin_data_file), I would probably benefit from having a log entry indicating that a SIGUSR1 signal was received.
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
amartinezfayo
left a comment
amartinezfayo
left a comment
There was a problem hiding this comment.
LGTM, thank you @azdagron!
MarcosDY
left a comment
MarcosDY
left a comment
There was a problem hiding this comment.
looks good, what I think it is missed here is add this new field into "full" configurations:
| option2 = 3 | ||
| ``` | ||
|
|
||
| #### External Plugin with Static Configuration |
There was a problem hiding this comment.
If we have a config for external plugins with plugin_data, it is possible that some users may not realize that plugin_data_file can also be used in external plugins. To improve clarity, we should add a minimal description indicating that this feature can be utilized in external plugins as well.
There was a problem hiding this comment.
I'm happy to add another example if that would help, but the section above mentions that all of the plugin options apply for all plugins. What I want to avoid is having an example with every different combination?
There was a problem hiding this comment.
another example may be an overkill a comment maybe is good enough
| expectServiceClient: true, | ||
| }) | ||
| }) | ||
| t.Run("configure from file success", func(t *testing.T) { |
There was a problem hiding this comment.
this test seems to be exercising regular load + a simple reload,
may this title reflect that?
| return PluginConfig{}, err | ||
| } | ||
| if data := buf.String(); data != "" { | ||
| dataSource = FixedData(data) |
There was a problem hiding this comment.
In case buf is empty, this can result in no FixedData or FileData to be provided here, and have a nil DataSource,
or I'm missing something? (if this is expected can you add a unit test that cover this scenario?)
There was a problem hiding this comment.
This is expected, at least from a backcompat perspective.
If you have a plugin that doesn't need configuration, .e.g. memory key manager, we allowed you do define it like so:
KeyManager "memory {
plugin_data = {}
}
We want to treat this as "no configuration", so the catalog doesn't complain that the plugin does not implement the configuration service.
I'll add a test.
Signed-off-by: Andrew Harding <azdagron@gmail.com>
Signed-off-by: Andrew Harding <azdagron@gmail.com>
4 participants
Adds reconfiguration support to the plugin catalog. Triggering reconfiguration is done by sending SIGUSR1 to SPIRE Server/Agent. This support is currently only for posix systems.
Only plugins that use a dynamic data source are reconfigured.
A new dynamic data source option
plugin_data_fileis introduced with this change. This option is mutually exclusive withplugin_data. It supports (re)loading from a file on disk.If there is no change to configuration then the plugin is not configured again.
This PR does not add support for reconfiguring the DataStore.
Resolves #5165