Plugin installation improvements #5624

SchrodingersGat · 2023-09-27T07:13:05Z

Working on fixing some issues with plugin installation via the API.

Reference: inventree/inventree-brother-plugin#25

Tasks

netlify · 2023-09-27T07:13:29Z

✅ Deploy Preview for inventree canceled.

Name	Link
🔨 Latest commit	`502c4a3`
🔍 Latest deploy log	https://app.netlify.com/sites/inventree/deploys/651abc68f1b1b80007754a08

- Refactor code - Improved logging

…onment

- Once uninstalled, we do not need the configuration options any longer

…abled - requires a strange hack as the plugin instance is not instantiated

matmair

Interesting refactor / extension. Definitely breaking in multiple instances and changing our threat model.

InvenTree/plugin/installer.py

matmair · 2023-09-27T23:35:25Z

InvenTree/plugin/models.py

-            # Ignore if the plugin is not active
-            if not self.active:
-                return None
-


Comment: This means any discovered plugin gets initiated - also deactivated once. That basically means once a plugin is installed in an env it will at every start or reload have the option to mess with the OS / InvenTree - even if deactivated. Is that intended?

Hmm that's a fair point.

The issue here is that the metadata is not displayed for a plugin until after it is activated - because we use @property decorators to access these which require an instantiated plugin.

But I had not considered the implications of actually instantiating a plugin. I'll look into whether these is a different way around this. Perhaps implement @classmethod?

Anything that imports the class will have the same problem, to load the class the module file is opened. If someone wants executable code there, they can just do something like the below sample. This is the reason I structured the code like it is currently. Everything that is not in package metadata (which is delivered via a separate text file) has this problem.

import os from plugin import InvenTreePlugin class MyPlugins(InvenTreePlugin): """Some sample code""" print('I can put everything I want here') os.popen(['sudo rm -R /'])

@matmair all of the metadata gathering has been refactored into plugin/meta.py - I think it should be a lot safer against this sort of thing

I just tested this in my prod-clone. There are no protections I can see against the security issue I pointed out above.

Yes after your previous comments I went back and looked at that, and did some more research into whether there is any way to introspect a python file without running it. Doesn't look like there is.

InvenTree/plugin/installer.py

InvenTree/InvenTree/settings.py

SchrodingersGat · 2023-09-28T01:28:15Z

Interesting refactor / extension. Definitely breaking in multiple instances and changing our threat model.

AFAIK the only "breaking" change here would be the ability to uninstall plugins. Everything else is either a bug fix or usability improvement.

I think I will leave off the "uninstall" feature for a new / separate PR - I would like this one to focus on fixes and improvements.

At the core of this is trying to work out why the "install via web" feature claims to work but does not actually install the plugin in some instances (see the linked issue for the brother plugin)

… installed (based on required features) - Does not require the plugin class to be instantiated

- Run completely outside plugin class - Does not allow plugins to run any code

…data

- Allows the plugin to be visible right away, even without restarting server

- Name clash

- Store a hash of the plugin registry in the database - Check if the hash matches, if not, reload the plugins - Addresses problem of background and foreground workers being out of sync - Any "external" code that calls plugin functionality will trigger a check

- Only if app mixin is enabled

…nstallation # Conflicts: # InvenTree/plugin/registry.py

- Handle version spec better - Don't use os.path

- Re-throw a ValidationError when saving a setting

- Reduce log output - Fix some typos - Loading plugins updates plugin paths correctly - Use class name as default value for plugin NAME

# Conflicts: # InvenTree/plugin/test_plugin.py

- Use a mutex lock in the plugin registry for cleaner loading / prevent recursion - Cleanup / refactor plugin reload mechanism - Update various unit tests - Assorted cleanups

- More cleanup of typos and logging output - Address more issues with unit tests - Fix some bugs which have existed for a while

- class name attribute lookup is better

matmair · 2023-10-02T05:58:52Z

InvenTree/plugin/registry.py

-        blocked_plugin = None
-        retry_counter = settings.PLUGIN_RETRY
+        blocked_plugins = []
+        retry_counter = settings.PLUGIN_RETRY if not settings.TESTING else 2


This might be too low - we have already 2 broken samples. There is a reason the default is 5.

I was trying to track down a different issue here, I'll change it back :)

matmair · 2023-10-02T05:59:57Z

InvenTree/plugin/registry.py

-                if settings.PLUGIN_TESTING:
-                    print(f'[PLUGIN] Above error occurred during testing - {retry_counter}/{settings.PLUGIN_RETRY} retries left')


Why is this removed? Debugging plugin testing will become increasingly hard if reloads are not obvious

matmair · 2023-10-02T06:00:50Z

InvenTree/plugin/registry.py

-        # ensure plugins_loaded is True
-        self.plugins_loaded = True
+        # Cleanup old plugin configs
+        self.cleanup_old_plugin_configs()


This means a temporary dependency clash that throws an error deletes a plugin config - i.e. all linked settings. Is that intended?

Fair point - what if we only run cleanup if plugins loaded without any errors?

At least that - this is a breaking change in how plugins/database settings behave and can break a lot of auto-scaling deployments as any container might delete a bunch of settings before the environment setup is completed.

matmair · 2023-10-02T06:03:12Z

InvenTree/plugin/registry.py

-        logger.info('Collected %s plugins', len(collected_plugins))
+        logger.debug('Collected %s plugins', len(collected_plugins))


Why is this changed? This means a sysadmin is needed to debug plugins.

matmair · 2023-10-02T22:02:25Z

InvenTree/plugin/base/integration/mixins.py

        if not name:
-            name = self.human_name
+            name = plugin.meta.get_plugin_classname(self)


Question: Why is this changed?

matmair · 2023-10-02T22:08:29Z

InvenTree/plugin/base/label/test_label_mixin.py

-        # Get all label plugins
-        plugins = registry.with_mixin('labels')
-        self.assertEqual(len(plugins), 2)

-        # But, it is not 'active'
+        self.do_activate_plugin()
        plugins = registry.with_mixin('labels')
-        self.assertEqual(len(plugins), 1)
+        self.assertEqual(len(plugins), 2)


So activation is not tested anymore?

There are plenty of other tests for plugin activation, either via the API or directly called via the registry.

we had problems with this not working for label plugins in the past but fine for me if you are confident the other test catch that

This refactor has uncovered a few areas where the plugins were not behaving as "expected" - I will definitely ensure that the unit tests are up to scratch :)

At this point this is a full refactor, I will have to retest and rewrite everything in my repos anyway.

matmair · 2023-10-02T22:15:47Z

This diff is getting so large and spread around different areas of the plugin system, I am not confident simple reviews will be enough to get this merged safely. We will have to test this thoughtfully in the real world, nearly all important surfaces/mechanisms and tests have been changed at this point.

SchrodingersGat · 2023-10-02T22:30:52Z

This diff is getting so large and spread around different areas of the plugin system, I am not confident simple reviews will be enough to get this merged safely. We will have to test this thoughtfully in the real world, nearly all important surfaces/mechanisms and tests have been changed at this point.

@matmair I fear you are correct here. I started this to fix a bug, and have got deeper and deeper into the weeds. I think what I will have to do is re-submit as smaller piecemeal changes - otherwise it is unreasonable to expect it can be adequately reviewed or tested.

What I have learned in my journey here is that there a few areas where the plugin system needs some work :)

I'll make some smaller target PRs and tag you for review

matmair · 2023-10-02T22:34:47Z

A lot of decisions in the system are learned in weeks of testing, so I would be careful with large changes. While installations fail in the current implementation in specific cases it is stable and proven - after building an image and deploying it, I am not sure if this PR will be any time soon.

SchrodingersGat · 2023-10-02T22:56:19Z

Closing this, and re-implementing as a series of smaller PRs - #5645

SchrodingersGat added 2 commits September 27, 2023 16:35

Improve description for plugin settings

c8eb662

Adding framework for improved plugin installer (WIP)

3440ebe

SchrodingersGat added enhancement This is an suggested enhancement or new feature plugin Plugin ecosystem labels Sep 27, 2023

SchrodingersGat requested a review from matmair as a code owner September 27, 2023 07:13

SchrodingersGat marked this pull request as draft September 27, 2023 07:13

SchrodingersGat mentioned this pull request Sep 27, 2023

plugin not installing? inventree/inventree-brother-plugin#25

Open

SchrodingersGat added 6 commits September 27, 2023 22:23

Capture pip output and re-throw as ValidationError

d2d3d1e

Display the actual path where plugin was installed

44a9e0c

Refactor existing code

6c6771f

Improve method for installing plugins file

8f9c27b

- Refactor code - Improved logging

Remove dead code

90e7687

Cleanup plugin table

7697a9b

SchrodingersGat added the api Relates to the API label Sep 27, 2023

SchrodingersGat added 3 commits September 28, 2023 00:16

Just log a warning if InvenTree is not running within a virtual envir…

c9fbac1

…onment

Cleanup PluginConfig objects for old plugins

3474709

- Once uninstalled, we do not need the configuration options any longer

Enable metadata extraction for plugins which are installed but not en…

1893d82

…abled - requires a strange hack as the plugin instance is not instantiated

matmair reviewed Sep 27, 2023

View reviewed changes

SchrodingersGat added 10 commits September 28, 2023 11:34

Improve log messages

2c00a9c

Revert behaviour for sample plugins

466314f

When attempting to activate a plugin, determine if is "allowed" to be…

e85b661

… installed (based on required features) - Does not require the plugin class to be instantiated

Refactor metadata gathering for plugins

38c0572

- Run completely outside plugin class - Does not allow plugins to run any code

Prevent callable methods or property methods from being used for meta…

586f430

…data

Reload plugins after plugin is installed

2c0b93f

- Allows the plugin to be visible right away, even without restarting server

Fix issue with get_plugin_config

705c263

- Name clash

Update comment

4a0e9c4

Auto-run migrations when a plugin is activated

da7ac84

- Only if app mixin is enabled

SchrodingersGat added 2 commits October 1, 2023 12:34

Merge commit '3cd7ee9cd25e264946c69ca677a56c1d0135d2f1' into plugin-i…

cb2114d

…nstallation # Conflicts: # InvenTree/plugin/registry.py

Installer fixes

bf78d04

- Handle version spec better - Don't use os.path

SchrodingersGat mentioned this pull request Oct 1, 2023

Implement method for uninstalling plugins #5637

Closed

Add plugin table filters for 'builtin' and 'sample' attributes

6179da7

SchrodingersGat mentioned this pull request Oct 1, 2023

Backport for plugin bug fix #5638

Merged

SchrodingersGat added 14 commits October 1, 2023 14:37

More unit test fixes

4681c67

- Re-throw a ValidationError when saving a setting

Fixes for registry and unit testing

945fa8e

- Reduce log output - Fix some typos - Loading plugins updates plugin paths correctly - Use class name as default value for plugin NAME

More CI fixes for test_plugin.py

e975851

Cleanup plugin table a bit more

1c9aaed

Further unit test fixes

6948078

Merge remote-tracking branch 'inventree/master' into plugin-installation

2961113

# Conflicts: # InvenTree/plugin/test_plugin.py

Share more detail when a custom panel fails to render

550f06f

Further improvements / fixes / hacks

b999b52

- Use a mutex lock in the plugin registry for cleaner loading / prevent recursion - Cleanup / refactor plugin reload mechanism - Update various unit tests - Assorted cleanups

Further updates

1409f2b

- More cleanup of typos and logging output - Address more issues with unit tests - Fix some bugs which have existed for a while

Split more mixin classes out into separate files

1798ed1

Improve metadata generation

7610a4f

- class name attribute lookup is better

Cleanup tests for action and label mixins

6b504c4

Fixing more tests

3f9c331

Fix API unit tests for plugin registry

502c4a3

matmair reviewed Oct 2, 2023

View reviewed changes

matmair added the breaking Indicates a major update or change which breaks compatibility label Oct 2, 2023

SchrodingersGat mentioned this pull request Oct 2, 2023

[Roadmap] Plugin Improvements #5645

Open

SchrodingersGat closed this Oct 2, 2023

SchrodingersGat deleted the plugin-installation branch October 5, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin installation improvements #5624

Plugin installation improvements #5624

SchrodingersGat commented Sep 27, 2023 •

edited

Loading

netlify bot commented Sep 27, 2023 •

edited

Loading

matmair left a comment

matmair Sep 27, 2023

SchrodingersGat Sep 28, 2023

matmair Sep 28, 2023

SchrodingersGat Sep 29, 2023

matmair Oct 2, 2023

SchrodingersGat Oct 2, 2023

SchrodingersGat commented Sep 28, 2023

matmair Oct 2, 2023

SchrodingersGat Oct 2, 2023

matmair Oct 2, 2023

matmair Oct 2, 2023

SchrodingersGat Oct 2, 2023

matmair Oct 2, 2023

matmair Oct 2, 2023

matmair Oct 2, 2023

matmair Oct 2, 2023

SchrodingersGat Oct 2, 2023

matmair Oct 2, 2023

SchrodingersGat Oct 2, 2023

matmair Oct 2, 2023

matmair commented Oct 2, 2023 •

edited

Loading

SchrodingersGat commented Oct 2, 2023

matmair commented Oct 2, 2023

SchrodingersGat commented Oct 2, 2023

		if settings.PLUGIN_TESTING:
		print(f'[PLUGIN] Above error occurred during testing - {retry_counter}/{settings.PLUGIN_RETRY} retries left')

		logger.info('Collected %s plugins', len(collected_plugins))
		logger.debug('Collected %s plugins', len(collected_plugins))

Plugin installation improvements #5624

Plugin installation improvements #5624

Conversation

SchrodingersGat commented Sep 27, 2023 • edited Loading

Tasks

netlify bot commented Sep 27, 2023 • edited Loading

✅ Deploy Preview for inventree canceled.

matmair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SchrodingersGat commented Sep 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matmair commented Oct 2, 2023 • edited Loading

SchrodingersGat commented Oct 2, 2023

matmair commented Oct 2, 2023

SchrodingersGat commented Oct 2, 2023

SchrodingersGat commented Sep 27, 2023 •

edited

Loading

netlify bot commented Sep 27, 2023 •

edited

Loading

matmair commented Oct 2, 2023 •

edited

Loading