Skip to content

Commit 4f12b98

Browse files
authored
[chore] advance architecture refactor (#864)
Signed-off-by: Pavel Okhlopkov <pavel.okhlopkov@flant.com>
1 parent a8dc73a commit 4f12b98

11 files changed

Lines changed: 939 additions & 226 deletions

pkg/app/app.go

Lines changed: 28 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -28,108 +28,86 @@ var (
2828
var PrometheusMetricsPrefix = "shell_operator_"
2929

3030
type FlagInfo struct {
31-
Name string
32-
Help string
33-
Envar string
34-
Define bool
31+
Name string
32+
Help string
33+
Envar string
3534
}
3635

3736
var CommonFlagsInfo = map[string]FlagInfo{
3837
"hooks-dir": {
3938
"hooks-dir",
4039
"A path to a hooks file structure. Can be set with $SHELL_OPERATOR_HOOKS_DIR.",
4140
"SHELL_OPERATOR_HOOKS_DIR",
42-
true,
4341
},
4442
"tmp-dir": {
4543
"tmp-dir",
4644
"A path to store temporary files with data for hooks. Can be set with $SHELL_OPERATOR_TMP_DIR.",
4745
"SHELL_OPERATOR_TMP_DIR",
48-
true,
4946
},
5047
"listen-address": {
5148
"listen-address",
5249
"Address to use to serve metrics to Prometheus. Can be set with $SHELL_OPERATOR_LISTEN_ADDRESS.",
5350
"SHELL_OPERATOR_LISTEN_ADDRESS",
54-
true,
5551
},
5652
"listen-port": {
5753
"listen-port",
5854
"Port to use to serve metrics to Prometheus. Can be set with $SHELL_OPERATOR_LISTEN_PORT.",
5955
"SHELL_OPERATOR_LISTEN_PORT",
60-
true,
6156
},
6257
"prometheus-metrics-prefix": {
6358
"prometheus-metrics-prefix",
6459
"Prefix for Prometheus metrics. Can be set with $SHELL_OPERATOR_PROMETHEUS_METRICS_PREFIX.",
6560
"SHELL_OPERATOR_PROMETHEUS_METRICS_PREFIX",
66-
true,
6761
},
6862
"hook-metrics-listen-port": {
6963
"hook-metrics-listen-port",
7064
"Port to use to serve hooks’ custom metrics to Prometheus. Can be set with $SHELL_OPERATOR_HOOK_METRICS_LISTEN_PORT. Equal to listen-port if empty.",
7165
"SHELL_OPERATOR_HOOK_METRICS_LISTEN_PORT",
72-
true,
7366
},
7467
"namespace": {
7568
"namespace",
7669
"A namespace of a shell-operator. Used to setup validating webhooks. Can be set with $SHELL_OPERATOR_NAMESPACE.",
7770
"SHELL_OPERATOR_NAMESPACE",
78-
true,
7971
},
8072
}
8173

8274
// DefineStartCommandFlags set shell-operator flags for cmd
8375
func DefineStartCommandFlags(kpApp *kingpin.Application, cmd *kingpin.CmdClause) {
84-
var flag FlagInfo
85-
86-
flag = CommonFlagsInfo["hooks-dir"]
87-
if flag.Define {
88-
cmd.Flag(flag.Name, flag.Help).
89-
Envar(flag.Envar).
90-
Default(HooksDir).
91-
StringVar(&HooksDir)
92-
}
76+
flag := CommonFlagsInfo["hooks-dir"]
77+
cmd.Flag(flag.Name, flag.Help).
78+
Envar(flag.Envar).
79+
Default(HooksDir).
80+
StringVar(&HooksDir)
9381

9482
flag = CommonFlagsInfo["tmp-dir"]
95-
if flag.Define {
96-
cmd.Flag(flag.Name, flag.Help).
97-
Envar(flag.Envar).
98-
Default(TempDir).
99-
StringVar(&TempDir)
100-
}
83+
cmd.Flag(flag.Name, flag.Help).
84+
Envar(flag.Envar).
85+
Default(TempDir).
86+
StringVar(&TempDir)
10187

10288
flag = CommonFlagsInfo["listen-address"]
103-
if flag.Define {
104-
cmd.Flag(flag.Name, flag.Help).
105-
Envar(flag.Envar).
106-
Default(ListenAddress).
107-
StringVar(&ListenAddress)
108-
}
89+
cmd.Flag(flag.Name, flag.Help).
90+
Envar(flag.Envar).
91+
Default(ListenAddress).
92+
StringVar(&ListenAddress)
10993

11094
flag = CommonFlagsInfo["listen-port"]
111-
if flag.Define {
112-
cmd.Flag(flag.Name, flag.Help).
113-
Envar(flag.Envar).
114-
Default(ListenPort).
115-
StringVar(&ListenPort)
116-
}
95+
cmd.Flag(flag.Name, flag.Help).
96+
Envar(flag.Envar).
97+
Default(ListenPort).
98+
StringVar(&ListenPort)
11799

118100
flag = CommonFlagsInfo["prometheus-metrics-prefix"]
119-
if flag.Define {
120-
cmd.Flag(flag.Name, flag.Help).
121-
Envar(flag.Envar).
122-
Default(PrometheusMetricsPrefix).
123-
StringVar(&PrometheusMetricsPrefix)
124-
}
101+
cmd.Flag(flag.Name, flag.Help).
102+
Envar(flag.Envar).
103+
Default(PrometheusMetricsPrefix).
104+
StringVar(&PrometheusMetricsPrefix)
125105

126106
flag = CommonFlagsInfo["namespace"]
127-
if flag.Define {
128-
cmd.Flag(flag.Name, flag.Help).
129-
Envar(flag.Envar).
130-
Default(Namespace).
131-
StringVar(&Namespace)
132-
}
107+
cmd.Flag(flag.Name, flag.Help).
108+
Envar(flag.Envar).
109+
Default(Namespace).
110+
StringVar(&Namespace)
133111

134112
DefineKubeClientFlags(cmd)
135113
DefineValidatingWebhookFlags(cmd)

pkg/hook/hook.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,6 @@ const (
2929
serviceName = "hook"
3030
)
3131

32-
type CommonHook interface {
33-
Name() string
34-
}
35-
3632
type Result struct {
3733
Usage *executor.CmdUsage
3834
Metrics []operation.MetricOperation

pkg/hook/hook_discovery_test.go

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
package hook
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"testing"
7+
8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
10+
)
11+
12+
// TestFileSystemHookDiscovery_Discover_emptyDir returns no paths for an empty directory.
13+
func TestFileSystemHookDiscovery_Discover_emptyDir(t *testing.T) {
14+
dir := t.TempDir()
15+
d := FileSystemHookDiscovery{}
16+
paths, err := d.Discover(dir)
17+
require.NoError(t, err)
18+
assert.Empty(t, paths)
19+
}
20+
21+
// TestFileSystemHookDiscovery_Discover_findsExecutables returns executable files.
22+
func TestFileSystemHookDiscovery_Discover_findsExecutables(t *testing.T) {
23+
dir := t.TempDir()
24+
25+
// Create an executable file.
26+
execPath := filepath.Join(dir, "my-hook.sh")
27+
err := os.WriteFile(execPath, []byte("#!/bin/sh\necho ok\n"), 0o755)
28+
require.NoError(t, err)
29+
30+
d := FileSystemHookDiscovery{}
31+
paths, err := d.Discover(dir)
32+
require.NoError(t, err)
33+
assert.Contains(t, paths, execPath)
34+
}
35+
36+
// TestFileSystemHookDiscovery_Discover_ignoresNonExecutables skips regular files.
37+
func TestFileSystemHookDiscovery_Discover_ignoresNonExecutables(t *testing.T) {
38+
dir := t.TempDir()
39+
40+
// Non-executable file.
41+
nonExecPath := filepath.Join(dir, "README.md")
42+
err := os.WriteFile(nonExecPath, []byte("docs"), 0o644)
43+
require.NoError(t, err)
44+
45+
d := FileSystemHookDiscovery{}
46+
paths, err := d.Discover(dir)
47+
require.NoError(t, err)
48+
assert.NotContains(t, paths, nonExecPath)
49+
}
50+
51+
// TestFileSystemHookDiscovery_Discover_recursive finds executables in subdirectories.
52+
func TestFileSystemHookDiscovery_Discover_recursive(t *testing.T) {
53+
dir := t.TempDir()
54+
subdir := filepath.Join(dir, "subdir")
55+
require.NoError(t, os.MkdirAll(subdir, 0o755))
56+
57+
hookPath := filepath.Join(subdir, "hook.sh")
58+
require.NoError(t, os.WriteFile(hookPath, []byte("#!/bin/sh\n"), 0o755))
59+
60+
d := FileSystemHookDiscovery{}
61+
paths, err := d.Discover(dir)
62+
require.NoError(t, err)
63+
assert.Contains(t, paths, hookPath)
64+
}
65+
66+
// TestNewHookManager_defaultDiscovery verifies that a nil HookDiscovery in
67+
// ManagerConfig defaults to FileSystemHookDiscovery inside the Manager.
68+
func TestNewHookManager_defaultDiscovery(t *testing.T) {
69+
hm := newHookManager(t, t.TempDir())
70+
hm.hookDiscovery = nil // reset to exercise the nil-default path via NewHookManager
71+
cfg := &ManagerConfig{
72+
WorkingDir: t.TempDir(),
73+
TempDir: t.TempDir(),
74+
HookDiscovery: nil, // explicitly nil → should default
75+
AdmissionWebhookManager: hm.admissionWebhookManager,
76+
ConversionWebhookManager: hm.conversionWebhookManager,
77+
Logger: hm.logger,
78+
}
79+
hm2 := NewHookManager(cfg)
80+
require.NotNil(t, hm2)
81+
assert.IsType(t, FileSystemHookDiscovery{}, hm2.hookDiscovery)
82+
}
83+
84+
// TestNewHookManager_injectedDiscovery verifies that a custom HookDiscovery is
85+
// stored as-is in the Manager.
86+
func TestNewHookManager_injectedDiscovery(t *testing.T) {
87+
stub := &stubDiscovery{}
88+
hm := newHookManager(t, t.TempDir())
89+
cfg := &ManagerConfig{
90+
WorkingDir: t.TempDir(),
91+
TempDir: t.TempDir(),
92+
HookDiscovery: stub,
93+
AdmissionWebhookManager: hm.admissionWebhookManager,
94+
ConversionWebhookManager: hm.conversionWebhookManager,
95+
Logger: hm.logger,
96+
}
97+
hm2 := NewHookManager(cfg)
98+
assert.Equal(t, stub, hm2.hookDiscovery)
99+
}
100+
101+
// TestManager_Init_usesInjectedDiscovery verifies that Manager.Init calls
102+
// HookDiscovery.Discover rather than the filesystem when a custom discovery is
103+
// injected. An empty result means Init succeeds with zero hooks loaded.
104+
func TestManager_Init_usesInjectedDiscovery(t *testing.T) {
105+
stub := &stubDiscovery{paths: []string{}} // returns nothing
106+
hm := newHookManager(t, t.TempDir())
107+
hm.hookDiscovery = stub
108+
109+
err := hm.Init()
110+
require.NoError(t, err)
111+
assert.True(t, stub.called, "Discover should have been called")
112+
assert.Equal(t, []string{}, hm.GetHookNames())
113+
}
114+
115+
// TestManager_Init_discoveryError propagates discovery errors.
116+
func TestManager_Init_discoveryError(t *testing.T) {
117+
stub := &stubDiscovery{err: errStubDiscovery}
118+
hm := newHookManager(t, t.TempDir())
119+
hm.hookDiscovery = stub
120+
121+
err := hm.Init()
122+
require.Error(t, err)
123+
assert.ErrorIs(t, err, errStubDiscovery)
124+
}
125+
126+
// ---- helpers ----
127+
128+
var errStubDiscovery = &testError{"stub discovery error"}
129+
130+
type testError struct{ msg string }
131+
132+
func (e *testError) Error() string { return e.msg }
133+
134+
type stubDiscovery struct {
135+
paths []string
136+
err error
137+
called bool
138+
}
139+
140+
func (s *stubDiscovery) Discover(_ string) ([]string, error) {
141+
s.called = true
142+
return s.paths, s.err
143+
}

pkg/hook/hook_manager.go

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ type Manager struct {
3636
conversionWebhookManager *conversion.WebhookManager
3737
admissionWebhookManager *admission.WebhookManager
3838

39+
// hookDiscovery resolves the set of hook executables to load.
40+
// Defaults to FileSystemHookDiscovery; tests may inject a stub.
41+
hookDiscovery HookDiscovery
42+
3943
// hook execution options
4044
keepTemporaryHookFiles bool
4145
logProxyHookJSON bool
@@ -64,6 +68,10 @@ type ManagerConfig struct {
6468
AdmissionWebhookManager *admission.WebhookManager
6569
ConversionWebhookManager *conversion.WebhookManager
6670

71+
// HookDiscovery overrides the default filesystem-based hook discovery.
72+
// When nil, FileSystemHookDiscovery is used.
73+
HookDiscovery HookDiscovery
74+
6775
KeepTemporaryHookFiles bool
6876
LogProxyHookJSON bool
6977
LogProxyHookJSONKey string
@@ -72,6 +80,10 @@ type ManagerConfig struct {
7280
}
7381

7482
func NewHookManager(config *ManagerConfig) *Manager {
83+
disc := config.HookDiscovery
84+
if disc == nil {
85+
disc = FileSystemHookDiscovery{}
86+
}
7587
return &Manager{
7688
hooksByName: make(map[string]*Hook),
7789
hookNamesInOrder: make([]string, 0),
@@ -84,6 +96,7 @@ func NewHookManager(config *ManagerConfig) *Manager {
8496
scheduleManager: config.ScheduleManager,
8597
admissionWebhookManager: config.AdmissionWebhookManager,
8698
conversionWebhookManager: config.ConversionWebhookManager,
99+
hookDiscovery: disc,
87100

88101
keepTemporaryHookFiles: config.KeepTemporaryHookFiles,
89102
logProxyHookJSON: config.LogProxyHookJSON,
@@ -114,16 +127,16 @@ func (hm *Manager) Init() error {
114127
log.Err(err))
115128
}
116129

117-
hooksRelativePaths, err := utils_file.RecursiveGetExecutablePaths(hm.workingDir)
130+
hookPaths, err := hm.hookDiscovery.Discover(hm.workingDir)
118131
if err != nil {
119132
return err
120133
}
121134

122135
// sort hooks by path
123-
sort.Strings(hooksRelativePaths)
124-
hm.logger.Debug("Search hooks in paths", slog.Any(pkg.LogKeyPaths, hooksRelativePaths))
136+
sort.Strings(hookPaths)
137+
hm.logger.Debug("Search hooks in paths", slog.Any(pkg.LogKeyPaths, hookPaths))
125138

126-
for _, hookPath := range hooksRelativePaths {
139+
for _, hookPath := range hookPaths {
127140
hook, err := hm.loadHook(hookPath)
128141
if err != nil {
129142
return err
@@ -449,3 +462,37 @@ func (hm *Manager) UpdateConversionChains() error {
449462
func (hm *Manager) FindConversionChain(crdName string, rule conversion.Rule) []conversion.Rule {
450463
return hm.conversionChains.FindConversionChain(crdName, rule)
451464
}
465+
466+
// HookManager is the interface for the hook manager used by the operator.
467+
// It allows substituting test doubles in unit tests.
468+
type HookManager interface {
469+
Init() error
470+
GetHook(name string) *Hook
471+
GetHookNames() []string
472+
GetHooksInOrder(bindingType htypes.BindingType) ([]string, error)
473+
CreateTasksFromKubeEvent(kubeEvent kemtypes.KubeEvent, createTaskFn func(*Hook, controller.BindingExecutionInfo) task.Task) []task.Task
474+
HandleCreateTasksFromScheduleEvent(crontab string, createTaskFn func(*Hook, controller.BindingExecutionInfo) task.Task) []task.Task
475+
HandleAdmissionEvent(ctx context.Context, event admission.Event, createTaskFn func(*Hook, controller.BindingExecutionInfo))
476+
DetectAdmissionEventType(event admission.Event) htypes.BindingType
477+
HandleConversionEvent(ctx context.Context, crdName string, request *v1.ConversionRequest, rule conversion.Rule, createTaskFn func(*Hook, controller.BindingExecutionInfo))
478+
FindConversionChain(crdName string, rule conversion.Rule) []conversion.Rule
479+
}
480+
481+
// HookDiscovery discovers hook executables to be loaded by the Manager.
482+
// The default implementation scans the filesystem; tests and alternative
483+
// runtimes can supply their own.
484+
type HookDiscovery interface {
485+
// Discover returns a sorted list of absolute paths to hook executables.
486+
Discover(workingDir string) ([]string, error)
487+
}
488+
489+
// FileSystemHookDiscovery discovers hooks by recursively scanning workingDir
490+
// for executable files.
491+
type FileSystemHookDiscovery struct{}
492+
493+
func (FileSystemHookDiscovery) Discover(workingDir string) ([]string, error) {
494+
return utils_file.RecursiveGetExecutablePaths(workingDir)
495+
}
496+
497+
// compile-time assertion
498+
var _ HookManager = (*Manager)(nil)

0 commit comments

Comments
 (0)