POC: purego plugins (not for merge)

[titpetric]

User description

PR uses purego to replace dlopen, which has libc couplings. Attempt a static build with plugin functionality.

• forks plugins package
• replaces dlopen/dlysim with purego
• clean up go tags and build with CGO_ENABLED=0 and -cflags to produce a static build
• verify static build, verify plugin functionality

Status: Blocked, more info.

time="Sep 16 18:13:01" level=error msg="Recovered from panic while loading Go-plugin" error="purego: unsupported kind interface" mwPath=../test/goplugins/goplugins.so mwSymbolName=MyPluginApplyingPolicy

The build does compile to a static binary, so there is some success (as expected).

# file tyk
tyk: ELF 64-bit LSB executable, x86-64, version 1 (GNU/Linux), statically linked, BuildID[sha1]=bc6e060184c844092c2d1304e4c15b2633cda067, for GNU/Linux 3.2.0, with debug_info, not stripped

The main issue is that just stubbing out dlopen doesn't give us full plugin compatibility. Restrictions around purego make it look like they would only be able to consume C plugins, but some type conversions are hinted at: https://pkg.go.dev/github.com/ebitengine/purego#RegisterFunc ; Things here get blurry, but at least a few things are omitted from the internal plugin fork:

• realpath, apparently it's a libc function and doesn't have an api
• internal/runtime has extensive plugin functionality (not included in fork)
• crippled init functionality for plugins
• no listing symbols of plugins to populate an internal map
• panic based lookups

Folow up with:

• can we use the uintptr directly and unsafe.Pointer it into a func(w http.ResponseWriter, r *httpRequest)? It doesn't seem possible because there's still purego limiting us here. Depends on plugin internals.
• alternative plugin interfaces, DataDog/go-libddwaf looks excellent
• purego could likely load libtailscale, a C plugin written in go https://github.com/tailscale/libtailscale (which loses a lot of the go syntax candy...)
• fork more internals related to plugin stdlib implementation, cover init, and other concerns
• zig cc

---

PR Type

enhancement, tests

---

Description

• Replaced dlopen and dlsym with purego for plugin loading and symbol resolution.
• Introduced a new internal package plugin2 to handle plugin operations.
• Added tests to ensure the plugin package can be imported and executed.
• Updated go.mod and go.sum to include the purego dependency.

---

Changes walkthrough 📝

	Relevant files
Enhancement	analyticsplugin.go Replace plugin.Open with plugin2.Open in analyticsplugin  goplugin/analyticsplugin.go Replaced plugin.Open with plugin2.Open. Updated import path to use plugin2. +2/-2      goplugin.go Replace plugin.Open with plugin2.Open in goplugin                goplugin/goplugin.go Replaced plugin.Open with plugin2.Open. Updated import path to use plugin2. +3/-2      plugin.go Add new plugin package with Open and Lookup functions        internal/plugins2/plugin.go Added new plugin package for loading and symbol resolution. Implemented Open and Lookup functions. Defined Plugin and Symbol types. +120/-0  plugin_purego.go Implement plugin loading using purego                                        internal/plugins2/plugin_purego.go Added implementation for plugin loading using purego. Utilized purego.Dlopen and purego.Dlsym. Managed plugin symbol resolution and initialization. +157/-0
Tests	plugin_test.go Add basic test for plugin package import                                  internal/plugins2/plugin_test.go Added a basic test for plugin package import. Ensured executable runs with plugin package. +15/-0
Dependencies	go.mod Add purego dependency to go.mod                                                    go.mod Added dependency on github.com/ebitengine/purego. +1/-0      go.sum Update go.sum with purego checksums                                            go.sum Updated to include checksums for purego dependency. +2/-0

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[github-actions]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review Possible Bug The function open in plugin_purego.go uses purego.Dlopen and purego.Dlsym for dynamic linking, which contradicts the initial goal of removing dlopen dependencies. This could lead to confusion about the actual dependencies and behavior of the plugin system. Code Smell The plugin.go file contains a large block of warnings and recommendations against using plugins in the manner that is being implemented. This suggests that the approach taken may not be best practice and could lead to maintenance or compatibility issues.

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Use defer to ensure mutex unlock in all paths to prevent deadlocks To avoid potential deadlocks or race conditions, ensure that pluginsMu.Unlock() is called before returning from the function in all error handling paths. internal/plugins2/plugin_purego.go [57-64] pluginsMu.Lock() +defer pluginsMu.Unlock() if p := plugins[filepath]; p != nil { - pluginsMu.Unlock() if p.err != "" { return nil, errors.New(`plugin.Open("` + name + `"): ` + p.err + ` (previous failure)`) } <-p.loaded return p, nil } Suggestion importance[1-10]: 10 Why: Using defer to ensure mutex unlock is a best practice that prevents deadlocks and race conditions, making the code more reliable and easier to maintain. This suggestion addresses a critical concurrency issue.	10
Possible bug	Add a nil check for loadedPlugin to enhance error handling and prevent runtime errors Consider checking if loadedPlugin is nil after loading the plugin to prevent potential runtime panics when accessing methods or properties of a nil pointer. goplugin/analyticsplugin.go [15-18] loadedPlugin, err := plugin2.Open(path) if err != nil { return nil, err } +if loadedPlugin == nil { + return nil, errors.New("failed to load plugin, got nil pointer") +} Suggestion importance[1-10]: 9 Why: Adding a nil check for loadedPlugin is a crucial improvement to prevent runtime panics, especially in cases where the plugin fails to load correctly but does not return an error. This enhances the robustness of the function.	9
	Ensure loadedPlugin is not nil after loading to prevent potential nil pointer dereference Add a nil check for loadedPlugin after attempting to open the plugin to ensure it is not nil before proceeding, enhancing the robustness of the function. goplugin/goplugin.go [15-18] loadedPlugin, err := plugin2.Open(modulePath) if err != nil { return nil, err } +if loadedPlugin == nil { + return nil, errors.New("failed to load plugin, got nil pointer") +} Suggestion importance[1-10]: 9 Why: This suggestion is important for preventing potential nil pointer dereference errors, which can lead to runtime panics. It ensures that the function handles all error cases gracefully.	9
	Add return statement in pluginLookup when the result is NULL to prevent passing nil pointers to Go Consider handling the case where pluginLookup returns a nil pointer in pluginLookup function to prevent runtime panics due to nil pointer dereference. internal/plugins2/plugin_purego.go [26-31] void* r = dlsym((void*)h, name); if (r == NULL) { *err = (char*)dlerror(); + return NULL; } return r; Suggestion importance[1-10]: 8 Why: Adding a return statement when pluginLookup returns a nil pointer is a good practice to prevent passing nil pointers to Go, which could cause runtime errors. This improves the safety of the function.	8

[github-actions]

API Changes

--- prev.txt	2024-09-16 16:23:18.253582937 +0000
+++ current.txt	2024-09-16 16:23:15.153562093 +0000
@@ -10836,15 +10836,18 @@
 
 FUNCTIONS
 
-func GetAnalyticsHandler(path string, symbol string) (func(record *analytics.AnalyticsRecord), error)
-func GetHandler(path string, symbol string) (http.HandlerFunc, error)
+func GetAnalyticsHandler(name string, symbol string) (func(record *analytics.AnalyticsRecord), error)
+func GetHandler(modulePath string, symbol string) (http.HandlerFunc, error)
 func GetPluginFileNameToLoad(pluginStorage storage, pluginPath string) (string, error)
     GetPluginFileNameToLoad check which file to load based on name, tyk version,
     os and architecture but it also takes care of returning the name of the file
     that exists
 
-func GetResponseHandler(path string, symbol string) (func(rw http.ResponseWriter, res *http.Response, req *http.Request), error)
+func GetResponseHandler(modulePath string, symbol string) (func(rw http.ResponseWriter, res *http.Response, req *http.Request), error)
 func GetSymbol(modulePath string, symbol string) (interface{}, error)
+    GetSymbol only tests plugin loading. Used in tyk plugin cli. Don't encourage
+    internal use.
+
 
 TYPES
 

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud