feat(plugin): plugin-loader #325
Conversation
|
|
||
| package com.netflix.spinnaker.kork.plugins.spring; | ||
|
|
||
| public class PluginLoaderException extends RuntimeException { |
There was a problem hiding this comment.
We should be using the base exceptions defined in kork-exceptions. For instance, I see this exception being used for both exceptional cases where the integration is at fault, as well as cases where the system itself is at fault. It would be good to differentiate.
There was a problem hiding this comment.
Do you recommend using those exceptions or extending them? And when you say the integration is at fault, do you mean throw an IntegrationException in the case of malformed config file or throw an IntegrationException in the case of a plugin not having required config values?
There was a problem hiding this comment.
The path forward is that we use these classes as base classes, extending them for the specific throw. For example, TitusException extends IntegrationException, and there's other exceptions that extend TitusException for specific throw scenarios.
There are 4 base exception classes; System, Integration, User and ConstraintViolation. For your question, probably one that is MalformedConfigurationException extends IntegrationException and another MissingRequiredConfigurationException extends IntegrationException or something. Does this make sense?
There was a problem hiding this comment.
Yup, added two new exception classes that inherit from IntegrationException, I named them slightly differently to reflect that they're plugin configuration exceptions.
kork-plugins/src/main/java/com/netflix/spinnaker/kork/plugins/spring/PluginProperties.java
Outdated
Show resolved
Hide resolved
kork-plugins/src/main/java/com/netflix/spinnaker/kork/plugins/spring/PluginProperties.java
Outdated
Show resolved
Hide resolved
| @Data | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| public static class PluginConfiguration { |
There was a problem hiding this comment.
What's your thoughts on plugin-specific configurations? Where do these go?
There was a problem hiding this comment.
Or rather... how do they get loaded? Is this up to the responsibility of something else?
There was a problem hiding this comment.
Plugin specific configurations will be laid down with the rest of the service configs (eg via halyard) and are accessible during context initialization
Eg. in orca.yml, you can have
...
plugins:
myStagePlugin:
version: x.y.z
configs:
foo: bar
cat: dog
and then reference plugins.myStagePlugin.configs.foo in an init method (have tested this with the @PostConstruct annotation)
There was a problem hiding this comment.
Interesting. A couple things:
myStagePluginshould probably be namespaced. Something likearmory/myStagePlugin, so that Netflix could also have amyStagePluginbut not conflict.versionbeing an embedded property seems... weird? Why is this part of an operator configuration? Shouldn't that information come from the Plugin manifest (e.g. compiled directly into the plugin)?
There was a problem hiding this comment.
For the first item, I do agree, plugins should be namespaced. Are you suggesting we add validation for this? eg. enforce a / character in the plugin name?
Agree on 2, that will be part of the plugin manifest
There was a problem hiding this comment.
Sorry, I missed this comment! Yes. Let's validate & enforce that convention.
| import org.springframework.boot.builder.SpringApplicationBuilder; | ||
| import org.springframework.boot.web.servlet.support.SpringBootServletInitializer; | ||
|
|
||
| public class SpinnakerApplication extends SpringBootServletInitializer { |
There was a problem hiding this comment.
Should have a SpinnakerApplicationSystemTest for this. Not only for validating that this works as desired, but also to show as an example of how this gets integrated into services.
There was a problem hiding this comment.
This SystemTest should also have a really simple plugin that we can load and assert its outputs.
There was a problem hiding this comment.
I'm planning on making a simple plugin jar and adding it to the repo, is that OK to do? Or are there alternatives you guys can think of?
There was a problem hiding this comment.
That seems fine. I've never tried something like this before, we can always adjust in the future if we discover a better way of doing it.
There was a problem hiding this comment.
@robzienert I've updated with some more tests, mind taking a look? While it doesn't actually load a jar, it shows the classpath being updated after the call to initialize and shows an example of how it's used. I've been attempting to write a test for this that actually loads a jar, however its proven to be difficult.
There was a problem hiding this comment.
That's fine for now! Thanks for putting those tests together - there's bigger fish to fry. Perhaps just plop a TODO comment on the class so we might be reminded to revisit someday. :)
There was a problem hiding this comment.
Added this to the test class itself
| throw new PluginLoaderException(e); | ||
| } | ||
| addJarsToClassPath(source); | ||
| } |
There was a problem hiding this comment.
loadPlugins should definitely return a result object that outlines what plugins were loaded and what weren't (and for what reason). I'm sure there will be other pieces of metadata we'll want as well.
This information will be handy for outputting into startup logs and probably other things.
There was a problem hiding this comment.
A couple ideas:
- I can get a list of all plugins listed and then compare it to the list produced by the getEnabledJarURLs call (would convert to compare strings to strings), but it wouldn't include information about why a plugin wasn't loaded.
- In getEnabledJarURLs, I could create a Map<String, String> that would be passed to getUrlFromPath and updated in case of an exception. I like this idea more since it would the exception message.
In either case, I'd like to fail the deployment, but I do agree, it would be good to have more info as to why it failed. This module is only concerned with updating the classpath, so we don't have access to what happens during the context initialization.
There was a problem hiding this comment.
since we're just adding all enabled plugins to the classpath, I can just output the plugin properties info.
There was a problem hiding this comment.
Not just in failure, but also in the successful path. When loading a plugin, it should have a manifest such that we know the name of the plugin, maintainer, version, and whether the plugin is enabled or not (along with the loaded configuration). This stuff will be fairly critical for debugging when someone comes into Slack or somewhere else reporting errors.
| .orElse(PluginLoader.DEFAULT_PLUGIN_CONFIG_PATH)); | ||
| } | ||
|
|
||
| public void loadPlugins(Class source) { |
There was a problem hiding this comment.
How do we handle loading the right plugin if different versions exist in the source directory?
There was a problem hiding this comment.
The plugins.yml is the absolute path of the jarfile(s) to load, not just a path to a directory. The plugins.yml file will contain only plugins relevant to the service.
There was a problem hiding this comment.
As per the Plugins RFC, Plugin environment sandboxing is a non-goal. Please see: https://docs.google.com/document/d/1-n5wGif28Glao_NLkSGlv5LzAeC_i1clntjMhJVfIQI/edit#heading=h.pk2yupt9bm2v
| } | ||
|
|
||
| private void addJarsToClassPath(Class source) { | ||
| Thread.currentThread() |
There was a problem hiding this comment.
Wouldn't this lead to conflicts if a class in the plugin happen to have a fully qualified name as an existing class?
There was a problem hiding this comment.
Vice versa if the defined plugin isn't found, the search would be delegated to the parent class loader (in this case the app class loader) which could reload the conflicting classes. (Perhaps this isn't a problem)
There was a problem hiding this comment.
We are working on creating a simple interface for plugins that plugin developers would have to implement, these plugins would be would need to be part of the com.netflix.spinnaker.plugin package. This reduces, but doesn't eliminate the possibility of problems, as two plugins could have the same fully qualified name. That being said, I don't see this as a problem right now.
| import org.springframework.boot.builder.SpringApplicationBuilder; | ||
| import org.springframework.boot.web.servlet.support.SpringBootServletInitializer; | ||
|
|
||
| public class SpinnakerApplication extends SpringBootServletInitializer { |
link108
force-pushed
the
plugin-loader
branch
2 times, most recently
from
40e652d
to
c0ad27d
Compare
| public void loadPlugins(Class source) { | ||
| URL[] urls; | ||
| if (checkFileExists() == false) { | ||
| log.info("No plugin configuration file found, skipping loading plugins"); |
There was a problem hiding this comment.
This reads a little weird.
"Not loading plugins: No plugin configuration file found: %s" where %s is the path searched.
| /** @return true if a plugin is enabled */ | ||
| private Predicate<PluginProperties.PluginConfiguration> isEnabled() { | ||
| return p -> p.enabled == true; | ||
| } |
There was a problem hiding this comment.
Seems odd to have broken a one-liner predicate out into a method which is only called once. 🤔
| * @param pluginConfigurations | ||
| */ | ||
| private void logPlugins(List<PluginProperties.PluginConfiguration> pluginConfigurations) { | ||
| if (pluginConfigurations.size() > 0) { |
There was a problem hiding this comment.
!pluginConfigurations.isEmpty() is the preferential way to do this.
| private void logPlugins(List<PluginProperties.PluginConfiguration> pluginConfigurations) { | ||
| if (pluginConfigurations.size() > 0) { | ||
| for (PluginProperties.PluginConfiguration pluginConfiguration : pluginConfigurations) { | ||
| log.info("Loading " + pluginConfiguration.toString()); |
There was a problem hiding this comment.
You can also do log.info("Loading {}", pluginConfiguration).
| @Data | ||
| @AllArgsConstructor | ||
| @NoArgsConstructor | ||
| public static class PluginConfiguration { |
There was a problem hiding this comment.
Sorry, I missed this comment! Yes. Let's validate & enforce that convention.
| import org.springframework.boot.builder.SpringApplicationBuilder; | ||
| import org.springframework.boot.web.servlet.support.SpringBootServletInitializer; | ||
|
|
||
| public class SpinnakerApplication extends SpringBootServletInitializer { |
There was a problem hiding this comment.
That's fine for now! Thanks for putting those tests together - there's bigger fish to fry. Perhaps just plop a TODO comment on the class so we might be reminded to revisit someday. :)
| * @param args The application arguments (usually passed from a Java main method) | ||
| */ | ||
| public static void initialize(Map<String, Object> defaultProps, Class source, String... args) { | ||
| PluginLoader pluginLoader = new PluginLoader(); |
There was a problem hiding this comment.
Comment only: Herein lies scope creep - so it's just for conversation.
I'd like to allow overriding the PluginLoader; would be handy to create this class by name and pass in via env. My use case here is that some company may want to customize how jars are loaded (perhaps via an object store instead of local filesystem?). No need to build it until it's needed though, but wondering if this seems like a reasonable use case?
There was a problem hiding this comment.
is that to say have PluginLoader be an interface? I have added a method to help with testing, where you can pass in a PluginLoader, but I believe you're talking about an interface, not a testing method. I think this is a reasonable use case, I'm going to have a PR up after this one is merged that enables spinnaker services to download the plugin jars, so that should help in the meantime.
There was a problem hiding this comment.
Yes, to have it be an interface. We don't need to deal with it for now. :)
|
Does this require a file at |
This PR is related to spinnaker/spinnaker#4181.
Spinnaker services can extend
SpinnakerApplicationand call its initialize() method to load plugins into the classpath prior to spring context initialization. When the plugins are initialized, they have access to spring configuration, so plugin configuration will be stored alongside the service configs.I've captured some of my thoughts around the plugin loader here: https://docs.google.com/document/d/1KFDCIIC_abJMVczmcTqeb3XenNibFet5acN4bq9KD3k
Plugin RFC: https://docs.google.com/document/d/1-n5wGif28Glao_NLkSGlv5LzAeC_i1clntjMhJVfIQI