From 51bd9b0cdad806c6b210daedde4f6635d1c9cf82 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 27 Nov 2025 12:40:37 -0700 Subject: [PATCH 1/4] Add test for handling registration errors Ensure that graph errors are handled as expected. Signed-off-by: Derek McGowan --- plugin.go | 2 + plugin_test.go | 153 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 154 insertions(+), 1 deletion(-) diff --git a/plugin.go b/plugin.go index 1196e0f..a068cf0 100644 --- a/plugin.go +++ b/plugin.go @@ -39,6 +39,8 @@ var ( ErrPluginNotFound = errors.New("plugin: not found") // ErrPluginMultipleInstances is used when a plugin is expected a single instance but has multiple ErrPluginMultipleInstances = errors.New("plugin: multiple instances") + // ErrPluginCircularDependency is used when the graph detect a circular plugin dependency + ErrPluginCircularDependency = errors.New("plugin: dependency loop detected") // ErrInvalidRequires will be thrown if the requirements for a plugin are // defined in an invalid manner. diff --git a/plugin_test.go b/plugin_test.go index 671149c..c1436ba 100644 --- a/plugin_test.go +++ b/plugin_test.go @@ -44,7 +44,6 @@ const ( ) func testRegistry() Registry { - var register Registry return register.Register(&Registration{ Type: TaskMonitorPlugin, @@ -517,6 +516,158 @@ func testPlugin(t Type, id string, i interface{}, err error) *Plugin { } } +func TestRequiresAll(t *testing.T) { + var register Registry + register = register.Register(&Registration{ + Type: InternalPlugin, + ID: "system", + }).Register(&Registration{ + Type: ServicePlugin, + ID: "introspection", + Requires: []Type{ + "*", + }, + }).Register(&Registration{ + Type: ServicePlugin, + ID: "task", + Requires: []Type{ + InternalPlugin, + }, + }).Register(&Registration{ + Type: ServicePlugin, + ID: "version", + }) + ordered := register.Graph(mockPluginFilter) + expectedURI := []string{ + "io.containerd.internal.v1.system", + "io.containerd.service.v1.task", + "io.containerd.service.v1.version", + "io.containerd.service.v1.introspection", + } + cmpOrdered(t, ordered, expectedURI) +} + +func TestRegisterErrors(t *testing.T) { + + for _, tc := range []struct { + name string + expected error + register func(Registry) Registry + }{ + { + name: "duplicate", + expected: ErrIDRegistered, + register: func(r Registry) Registry { + return r.Register(&Registration{ + Type: TaskMonitorPlugin, + ID: "cgroups", + }).Register(&Registration{ + Type: TaskMonitorPlugin, + ID: "cgroups", + }) + }, + }, + { + name: "circular", + expected: ErrPluginCircularDependency, + register: func(r Registry) Registry { + // Circular dependencies should not loop but order will be based on registration order + return r.Register(&Registration{ + Type: InternalPlugin, + ID: "p1", + Requires: []Type{ + RuntimePlugin, + }, + }).Register(&Registration{ + Type: RuntimePlugin, + ID: "p2", + Requires: []Type{ + InternalPlugin, + }, + }).Register(&Registration{ + Type: InternalPlugin, + ID: "p3", + }) + }, + }, + { + name: "self", + expected: ErrInvalidRequires, + register: func(r Registry) Registry { + // Circular dependencies should not loop but order will be based on registration order + return r.Register(&Registration{ + Type: InternalPlugin, + ID: "p1", + Requires: []Type{ + InternalPlugin, + }, + }) + }, + }, + { + name: "no-type", + expected: ErrNoType, + register: func(r Registry) Registry { + // Circular dependencies should not loop but order will be based on registration order + return r.Register(&Registration{ + Type: "", + ID: "p1", + }) + }, + }, + { + name: "no-ID", + expected: ErrNoPluginID, + register: func(r Registry) Registry { + // Circular dependencies should not loop but order will be based on registration order + return r.Register(&Registration{ + Type: InternalPlugin, + ID: "", + }) + }, + }, + { + name: "bad-requires-all", + expected: ErrInvalidRequires, + register: func(r Registry) Registry { + // Circular dependencies should not loop but order will be based on registration order + return r.Register(&Registration{ + Type: InternalPlugin, + ID: "p1", + Requires: []Type{ + "*", + InternalPlugin, + }, + }) + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + var ( + r Registry + panicAny any + ) + func() { + defer func() { + panicAny = recover() + }() + + tc.register(r).Graph(mockPluginFilter) + }() + if panicAny == nil { + t.Fatalf("expected panic with error %v", tc.expected) + } + err, ok := panicAny.(error) + if !ok { + t.Fatalf("expected panic: %v, expected error %v", panicAny, tc.expected) + } + if !errors.Is(err, tc.expected) { + t.Fatalf("unexpected error type: %v, expected %v", panicAny, tc.expected) + } + }) + } +} + func BenchmarkGraph(b *testing.B) { register := testRegistry() b.ResetTimer() From 85991023b9685bf88cedcd4fd5b1805ead2ebf85 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 27 Nov 2025 09:43:44 -0700 Subject: [PATCH 2/4] Simply handling of disabled and added plugins Signed-off-by: Derek McGowan --- plugin.go | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/plugin.go b/plugin.go index a068cf0..d8439a8 100644 --- a/plugin.go +++ b/plugin.go @@ -112,37 +112,34 @@ type Registry []*Registration // Graph computes the ordered list of registrations based on their dependencies, // filtering out any plugins which match the provided filter. func (registry Registry) Graph(filter DisableFilter) []Registration { - disabled := map[*Registration]bool{} - for _, r := range registry { - if filter(r) { - disabled[r] = true + handled := make(map[*Registration]bool, len(registry)) + if filter != nil { + for _, r := range registry { + if filter(r) { + handled[r] = true + } } } - ordered := make([]Registration, 0, len(registry)-len(disabled)) - added := map[*Registration]bool{} + ordered := make([]Registration, 0, len(registry)-len(handled)) for _, r := range registry { - if disabled[r] { + if handled[r] { continue } - children(r, registry, added, disabled, &ordered) - if !added[r] { - ordered = append(ordered, *r) - added[r] = true - } + handled[r] = true + children(r, registry, handled, &ordered) + ordered = append(ordered, *r) } return ordered } -func children(reg *Registration, registry []*Registration, added, disabled map[*Registration]bool, ordered *[]Registration) { +func children(reg *Registration, registry []*Registration, handled map[*Registration]bool, ordered *[]Registration) { for _, t := range reg.Requires { for _, r := range registry { - if (t == "*" || r.Type == t) && r != reg && !disabled[r] { - children(r, registry, added, disabled, ordered) - if !added[r] { - *ordered = append(*ordered, *r) - added[r] = true - } + if (t == "*" || r.Type == t) && r != reg && !handled[r] { + handled[r] = true + children(r, registry, handled, ordered) + *ordered = append(*ordered, *r) } } } From 01f62ae782f755d23bfe60611ef2c2487afb6873 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 27 Nov 2025 14:45:09 -0700 Subject: [PATCH 3/4] Fix circular dependency detection Ensure circular dependencies are detected a panic with the plugin URI that first experienced the circular dependency. Signed-off-by: Derek McGowan --- plugin.go | 30 ++++++++++++++++++++---------- 1 file changed, 20 insertions(+), 10 deletions(-) diff --git a/plugin.go b/plugin.go index d8439a8..ecce877 100644 --- a/plugin.go +++ b/plugin.go @@ -112,34 +112,44 @@ type Registry []*Registration // Graph computes the ordered list of registrations based on their dependencies, // filtering out any plugins which match the provided filter. func (registry Registry) Graph(filter DisableFilter) []Registration { - handled := make(map[*Registration]bool, len(registry)) + handled := make(map[*Registration]struct{}, len(registry)) if filter != nil { for _, r := range registry { if filter(r) { - handled[r] = true + handled[r] = struct{}{} } } } ordered := make([]Registration, 0, len(registry)-len(handled)) + stack := make([]*Registration, 0, cap(ordered)) for _, r := range registry { - if handled[r] { + if _, ok := handled[r]; ok { continue } - handled[r] = true - children(r, registry, handled, &ordered) + children(append(stack, r), registry, handled, &ordered) + handled[r] = struct{}{} ordered = append(ordered, *r) } return ordered } -func children(reg *Registration, registry []*Registration, handled map[*Registration]bool, ordered *[]Registration) { +func children(stack []*Registration, registry []*Registration, handled map[*Registration]struct{}, ordered *[]Registration) { + reg := stack[len(stack)-1] for _, t := range reg.Requires { for _, r := range registry { - if (t == "*" || r.Type == t) && r != reg && !handled[r] { - handled[r] = true - children(r, registry, handled, ordered) - *ordered = append(*ordered, *r) + if (t == "*" || r.Type == t) && r != reg { + if _, ok := handled[r]; !ok { + // Ensure not in current stack + for _, p := range stack[:len(stack)-1] { + if p == r { + panic(fmt.Errorf("circular plugin dependency at %s: %w", r.URI(), ErrPluginCircularDependency)) + } + } + children(append(stack, r), registry, handled, ordered) + handled[r] = struct{}{} + *ordered = append(*ordered, *r) + } } } } From e8139c6cac3394eed10eef90be924ff14f477da4 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 27 Nov 2025 15:31:53 -0700 Subject: [PATCH 4/4] Add check for plugin requiring itself This should be considered an invalid requirement Signed-off-by: Derek McGowan --- plugin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin.go b/plugin.go index ecce877..e31b221 100644 --- a/plugin.go +++ b/plugin.go @@ -169,7 +169,7 @@ func (registry Registry) Register(r *Registration) Registry { } for _, requires := range r.Requires { - if requires == "*" && len(r.Requires) != 1 { + if (requires == "*" && len(r.Requires) != 1) || requires == r.Type { panic(ErrInvalidRequires) } }