Skip to content

Conversation

@araspitzu
Copy link
Contributor

@araspitzu araspitzu commented Feb 10, 2020

This PR rewords the way we load plugins providing a stricter and safer way to load them. We now require plugins to exhibit a MANIFEST file with some well defined entries:
- Main-Class to identify the plugin entry point ( class implementing Plugin)
- X-Eclair-Supported-Versions contains a list of all supported eclair version

The Main-Class is mainly used to load the plugin without scanning using org.clapper.classutils, this allows us to avoid an illegal reflective call during startup. The list of supported version allows us to check if the plugin we're loading supports the current version of eclair, this prevents from running outdated (and incompatible) plugins.

This PR slightly reworks the way we load plugins to avoid doing an illegal reflective call. Instead of using org.clapper.classutils to scan for classes (and perform the illegal reflective call) we now require the plugin to tell us which class is implementing the Plugin interface via MANIFEST file.

@araspitzu araspitzu changed the title Avoid illegal reflective operation when loading a plugin Avoid illegal reflective operation during startup Feb 11, 2020
@araspitzu araspitzu marked this pull request as ready for review February 11, 2020 09:33
@araspitzu araspitzu force-pushed the plugin_loading_rework branch from a0a1340 to 11d3630 Compare February 21, 2020 09:11
@codecov-io
Copy link

codecov-io commented Feb 21, 2020

Codecov Report

Merging #1313 into master will decrease coverage by 0.06%.
The diff coverage is 21.05%.

@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
- Coverage   77.62%   77.56%   -0.07%     
==========================================
  Files         144      144              
  Lines       10141    10152      +11     
  Branches      406      406              
==========================================
+ Hits         7872     7874       +2     
- Misses       2269     2278       +9
Impacted Files Coverage Δ
...r-node/src/main/scala/fr/acinq/eclair/Plugin.scala 21.05% <21.05%> (+21.05%) ⬆️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 90.55% <0%> (-1.12%) ⬇️
...q/eclair/blockchain/electrum/ElectrumWatcher.scala 55.2% <0%> (-0.8%) ⬇️
...src/main/scala/fr/acinq/eclair/router/Router.scala 91.93% <0%> (-0.68%) ⬇️
...in/scala/fr/acinq/eclair/channel/Commitments.scala 90.94% <0%> (+0.34%) ⬆️
...clair/blockchain/electrum/ElectrumClientPool.scala 82.79% <0%> (+4.3%) ⬆️

@pm47 pm47 self-assigned this Feb 24, 2020
@pm47 pm47 self-requested a review February 24, 2020 13:48
 We now require the plugin to supply a manifest entry for the "Main-Class" attribute, this is used to load the plugin without doing illegal reflection operations.
 We also get rid of the dependency org.clapper.classutil.
@araspitzu araspitzu force-pushed the plugin_loading_rework branch from 11d3630 to 97843fa Compare February 27, 2020 10:51
@araspitzu araspitzu changed the title Avoid illegal reflective operation during startup Improve plugin loading Feb 27, 2020
@araspitzu araspitzu force-pushed the plugin_loading_rework branch from 97843fa to 056e269 Compare February 27, 2020 13:14
@araspitzu araspitzu force-pushed the plugin_loading_rework branch from 89202a2 to 6748cfa Compare February 28, 2020 15:27
@araspitzu araspitzu changed the title Improve plugin loading Avoid illegal reflective operation during startup Feb 28, 2020
@araspitzu
Copy link
Contributor Author

The PR has been updated to not apply any change regarding the version checks of the plugins.

@araspitzu araspitzu force-pushed the plugin_loading_rework branch from 6748cfa to 9d48631 Compare February 28, 2020 15:36
@pm47 pm47 removed their assignment Feb 28, 2020
@araspitzu araspitzu merged commit f978cfb into master Feb 28, 2020
@araspitzu araspitzu deleted the plugin_loading_rework branch February 28, 2020 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants