From 3adcc228d6f25180badc9106ba3514930576d978 Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Tue, 4 Aug 2026 15:38:49 +0200 Subject: [PATCH 1/8] add site systemd service enabler --- .../system/nonkube/site_service_enabler.go | 59 +++++ internal/cmd/skupper/system/system.go | 1 + internal/nonkube/bootstrap/install.go | 7 + .../site_service_enabler_installer.go | 126 ++++++++++ .../site_service_enabler_installer_test.go | 222 ++++++++++++++++++ .../site_service_enabler_service.template | 12 + internal/nonkube/bootstrap/uninstall.go | 2 + internal/nonkube/enabler/enabler.go | 136 +++++++++++ internal/nonkube/enabler/enabler_test.go | 217 +++++++++++++++++ 9 files changed, 782 insertions(+) create mode 100644 internal/cmd/skupper/system/nonkube/site_service_enabler.go create mode 100644 internal/nonkube/bootstrap/site_service_enabler_installer.go create mode 100644 internal/nonkube/bootstrap/site_service_enabler_installer_test.go create mode 100644 internal/nonkube/bootstrap/site_service_enabler_service.template create mode 100644 internal/nonkube/enabler/enabler.go create mode 100644 internal/nonkube/enabler/enabler_test.go diff --git a/internal/cmd/skupper/system/nonkube/site_service_enabler.go b/internal/cmd/skupper/system/nonkube/site_service_enabler.go new file mode 100644 index 000000000..f6b8f8b7c --- /dev/null +++ b/internal/cmd/skupper/system/nonkube/site_service_enabler.go @@ -0,0 +1,59 @@ +package nonkube + +import ( + "log/slog" + "os" + "os/signal" + "syscall" + + "github.com/skupperproject/skupper/internal/filesystem" + "github.com/skupperproject/skupper/internal/nonkube/enabler" + "github.com/skupperproject/skupper/internal/version" + "github.com/skupperproject/skupper/pkg/nonkube/api" + "github.com/spf13/cobra" +) + +func NewCmdSiteServiceEnabler() *cobra.Command { + return &cobra.Command{ + Use: "_site-service-enabler", + Hidden: true, + RunE: func(cmd *cobra.Command, args []string) error { + slog.Info("Starting site-service-enabler", slog.String("version", version.Version)) + + stop := make(chan struct{}) + + sigs := make(chan os.Signal, 1) + signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) + + go func() { + <-sigs + slog.Info("Shutting down site-service-enabler") + close(stop) + }() + + return run(stop) + }, + } +} + +func run(stop <-chan struct{}) error { + namespacesDir := api.GetDefaultOutputNamespacesPath() + + watcher, err := filesystem.NewWatcher(slog.String("component", "site-service-enabler")) + if err != nil { + return err + } + + siteServiceEnabler := enabler.NewServiceEnabler() + //this checks that the script directory is created, where the systemd services are going to be stored + nsHandler := &enabler.NamespaceScriptHandler{ + NamespacesDir: namespacesDir, + Watcher: watcher, + Enabler: siteServiceEnabler, + } + watcher.Add(namespacesDir, nsHandler) + watcher.Start(stop) + + <-stop + return nil +} diff --git a/internal/cmd/skupper/system/system.go b/internal/cmd/skupper/system/system.go index c4ef0a0d8..9e7609c51 100644 --- a/internal/cmd/skupper/system/system.go +++ b/internal/cmd/skupper/system/system.go @@ -35,6 +35,7 @@ approach, which is based on the new set of Custom Resource Definitions (CRDs).`, cmd.AddCommand(CmdSystemGenerateBundleFactory(platform)) cmd.AddCommand(CmdSystemApplyFactory(platform)) cmd.AddCommand(CmdSystemDeleteFactory(platform)) + cmd.AddCommand(nonkube.NewCmdSiteServiceEnabler()) return cmd } diff --git a/internal/nonkube/bootstrap/install.go b/internal/nonkube/bootstrap/install.go index 16aeb2e62..dcd8f0dc8 100644 --- a/internal/nonkube/bootstrap/install.go +++ b/internal/nonkube/bootstrap/install.go @@ -144,6 +144,13 @@ func Install(platform string, reloadType string) error { return fmt.Errorf("failed to create system-controller systemd service: %v", err) } + if reloadType == types.SystemReloadTypeAuto { + enabler := newSiteServiceEnablerInstaller() + if err = enabler.Install(); err != nil { + return fmt.Errorf("failed to install skupper site service enabler: %v", err) + } + } + fmt.Printf("Platform %s is now configured for Skupper\n", platform) return nil diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer.go b/internal/nonkube/bootstrap/site_service_enabler_installer.go new file mode 100644 index 000000000..19a9de7a5 --- /dev/null +++ b/internal/nonkube/bootstrap/site_service_enabler_installer.go @@ -0,0 +1,126 @@ +package bootstrap + +import ( + "bytes" + _ "embed" + "fmt" + "os" + "os/exec" + "path" + "text/template" + + "github.com/skupperproject/skupper/pkg/nonkube/api" +) + +//go:embed site_service_enabler_service.template +var siteServiceEnablerServiceTemplate string + +const ( + siteServiceEnablerRootSystemdBasePath = "/etc/systemd/system" + siteServiceEnablerName = "skupper-site-service-enabler" + siteServiceEnablerServiceFile = siteServiceEnablerName + ".service" + siteServiceEnablerWrapperScript = siteServiceEnablerName +) + +type siteServiceEnablerData struct { + ScriptPath string + WantedBy string +} + +type SiteServiceEnablerInstaller struct { + uid int + rootSystemdBasePath string + scriptDir string + command func(string, ...string) *exec.Cmd +} + +func newSiteServiceEnablerInstaller() *SiteServiceEnablerInstaller { + return &SiteServiceEnablerInstaller{ + uid: os.Getuid(), + rootSystemdBasePath: siteServiceEnablerRootSystemdBasePath, + scriptDir: path.Join(api.GetSystemControllerPath(), "bin"), + command: exec.Command, + } +} + +func (s *SiteServiceEnablerInstaller) Install() error { + if err := os.MkdirAll(s.scriptDir, 0755); err != nil { + return fmt.Errorf("unable to create script directory %q: %w", s.scriptDir, err) + } + + scriptPath := path.Join(s.scriptDir, siteServiceEnablerWrapperScript) + script := "#!/bin/sh\nexec skupper system _site-service-enabler\n" + if err := os.WriteFile(scriptPath, []byte(script), 0755); err != nil { + return fmt.Errorf("unable to write wrapper script %q: %w", scriptPath, err) + } + + tmplData := s.templateData(scriptPath) + serviceFile := s.unitPath(siteServiceEnablerServiceFile) + if err := s.renderFile(siteServiceEnablerServiceTemplate, tmplData, serviceFile, 0644); err != nil { + return fmt.Errorf("unable to write site enabler service unit: %w", err) + } + + if err := s.systemctl("daemon-reload"); err != nil { + return fmt.Errorf("daemon-reload failed: %w", err) + } + if err := s.systemctl("enable", siteServiceEnablerServiceFile); err != nil { + return fmt.Errorf("unable to enable %s: %w", siteServiceEnablerServiceFile, err) + } + if err := s.systemctl("start", siteServiceEnablerServiceFile); err != nil { + return fmt.Errorf("unable to start %s: %w", siteServiceEnablerServiceFile, err) + } + + return nil +} + +func (s *SiteServiceEnablerInstaller) Remove() { + _ = s.systemctl("stop", siteServiceEnablerServiceFile) + _ = s.systemctl("disable", siteServiceEnablerServiceFile) + _ = s.systemctl("daemon-reload") + + _ = os.Remove(s.unitPath(siteServiceEnablerServiceFile)) + _ = os.Remove(path.Join(s.scriptDir, siteServiceEnablerWrapperScript)) +} + +func (s *SiteServiceEnablerInstaller) templateData(scriptPath string) siteServiceEnablerData { + wantedBy := "default.target" + if s.uid == 0 { + wantedBy = "multi-user.target" + } + return siteServiceEnablerData{ + ScriptPath: scriptPath, + WantedBy: wantedBy, + } +} + +func (s *SiteServiceEnablerInstaller) unitPath(unit string) string { + return path.Join(s.userSystemdDir(), unit) +} + +func (s *SiteServiceEnablerInstaller) userSystemdDir() string { + if s.uid == 0 { + return s.rootSystemdBasePath + } + return path.Join(api.GetConfigHome(), "systemd", "user") +} + +func (s *SiteServiceEnablerInstaller) systemctl(args ...string) error { + var fullArgs []string + if s.uid != 0 { + fullArgs = append(fullArgs, "--user") + } + fullArgs = append(fullArgs, args...) + return s.command("systemctl", fullArgs...).Run() +} + +func (s *SiteServiceEnablerInstaller) renderFile(tmplText string, data siteServiceEnablerData, dst string, mode os.FileMode) error { + tmpl, err := template.New("").Parse(tmplText) + if err != nil { + return err + } + var buf bytes.Buffer + if err = tmpl.Execute(&buf, data); err != nil { + return err + } + return os.WriteFile(dst, buf.Bytes(), mode) +} diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go new file mode 100644 index 000000000..617822f98 --- /dev/null +++ b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go @@ -0,0 +1,222 @@ +package bootstrap + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "gotest.tools/v3/assert" +) + +func fakeCommand(calls *[][]string) func(string, ...string) *exec.Cmd { + return func(name string, args ...string) *exec.Cmd { + *calls = append(*calls, append([]string{name}, args...)) + return exec.Command("true") + } +} + +func newTestInstaller(t *testing.T, uid int, calls *[][]string) *SiteServiceEnablerInstaller { + t.Helper() + tmp := t.TempDir() + return &SiteServiceEnablerInstaller{ + uid: uid, + rootSystemdBasePath: filepath.Join(tmp, "etc", "systemd", "system"), + scriptDir: filepath.Join(tmp, "bin"), + command: fakeCommand(calls), + } +} + +func TestTemplateData_NonRoot(t *testing.T) { + s := &SiteServiceEnablerInstaller{uid: 1000} + d := s.templateData("/some/path/script") + assert.Equal(t, d.ScriptPath, "/some/path/script") + assert.Equal(t, d.WantedBy, "default.target") +} + +func TestTemplateData_Root(t *testing.T) { + s := &SiteServiceEnablerInstaller{uid: 0} + d := s.templateData("/some/path/script") + assert.Equal(t, d.WantedBy, "multi-user.target") +} + +func TestUserSystemdDir_Root(t *testing.T) { + s := &SiteServiceEnablerInstaller{uid: 0, rootSystemdBasePath: "/etc/systemd/system"} + assert.Equal(t, s.userSystemdDir(), "/etc/systemd/system") +} + +func TestUserSystemdDir_NonRoot(t *testing.T) { + t.Setenv("XDG_CONFIG_HOME", "/fake/config") + s := &SiteServiceEnablerInstaller{uid: 1000} + got := s.userSystemdDir() + assert.Assert(t, strings.HasPrefix(got, "/fake/config"), "expected XDG_CONFIG_HOME prefix, got %s", got) +} + +func TestUnitPath(t *testing.T) { + s := &SiteServiceEnablerInstaller{uid: 0, rootSystemdBasePath: "/etc/systemd/system"} + got := s.unitPath("skupper-site-service-enabler.service") + assert.Equal(t, got, "/etc/systemd/system/skupper-site-service-enabler.service") +} + +func TestSystemctl_NonRoot_AddsUserFlag(t *testing.T) { + var calls [][]string + s := &SiteServiceEnablerInstaller{uid: 1000, command: fakeCommand(&calls)} + err := s.systemctl("start", "foo.service") + assert.NilError(t, err) + assert.Equal(t, len(calls), 1) + assert.DeepEqual(t, calls[0], []string{"systemctl", "--user", "start", "foo.service"}) +} + +func TestSystemctl_Root_NoUserFlag(t *testing.T) { + var calls [][]string + s := &SiteServiceEnablerInstaller{uid: 0, command: fakeCommand(&calls)} + err := s.systemctl("start", "foo.service") + assert.NilError(t, err) + assert.Equal(t, len(calls), 1) + assert.DeepEqual(t, calls[0], []string{"systemctl", "start", "foo.service"}) +} + +func TestRenderFile(t *testing.T) { + tmp := t.TempDir() + dst := filepath.Join(tmp, "out.service") + s := &SiteServiceEnablerInstaller{} + err := s.renderFile(siteServiceEnablerServiceTemplate, siteServiceEnablerData{ + ScriptPath: "/usr/bin/skupper-site-service-enabler", + WantedBy: "multi-user.target", + }, dst, 0644) + assert.NilError(t, err) + + content, err := os.ReadFile(dst) + assert.NilError(t, err) + assert.Assert(t, strings.Contains(string(content), "ExecStart=/usr/bin/skupper-site-service-enabler")) + assert.Assert(t, strings.Contains(string(content), "WantedBy=multi-user.target")) +} + +func TestRenderFile_InvalidTemplate(t *testing.T) { + tmp := t.TempDir() + s := &SiteServiceEnablerInstaller{} + err := s.renderFile("{{.Invalid", siteServiceEnablerData{}, filepath.Join(tmp, "out.service"), 0644) + assert.Assert(t, err != nil) +} + +func TestInstall_CreatesWrapperScript(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 1000, &calls) + + err := s.Install() + assert.NilError(t, err) + + scriptPath := filepath.Join(s.scriptDir, siteServiceEnablerWrapperScript) + content, err := os.ReadFile(scriptPath) + assert.NilError(t, err) + assert.Equal(t, string(content), "#!/bin/sh\nexec skupper system _site-service-enabler\n") + + info, err := os.Stat(scriptPath) + assert.NilError(t, err) + assert.Equal(t, info.Mode(), os.FileMode(0755)) +} + +func TestInstall_CreatesServiceFile(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + _ = os.MkdirAll(s.rootSystemdBasePath, 0755) + + err := s.Install() + assert.NilError(t, err) + + svcPath := s.unitPath(siteServiceEnablerServiceFile) + content, err := os.ReadFile(svcPath) + assert.NilError(t, err) + assert.Assert(t, strings.Contains(string(content), "ExecStart=")) + assert.Assert(t, strings.Contains(string(content), "WantedBy=multi-user.target")) +} + +func TestInstall_SystemctlCallOrder(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + _ = os.MkdirAll(s.rootSystemdBasePath, 0755) + + err := s.Install() + assert.NilError(t, err) + + assert.Equal(t, len(calls), 3) + assert.DeepEqual(t, calls[0], []string{"systemctl", "daemon-reload"}) + assert.DeepEqual(t, calls[1], []string{"systemctl", "enable", siteServiceEnablerServiceFile}) + assert.DeepEqual(t, calls[2], []string{"systemctl", "start", siteServiceEnablerServiceFile}) +} + +func TestInstall_NonRoot_SystemctlUsesUserFlag(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 1000, &calls) + + err := s.Install() + assert.NilError(t, err) + + for _, c := range calls { + assert.Equal(t, c[1], "--user", "expected --user flag in call %v", c) + } +} + +func TestInstall_ScriptDirCreationFailure(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 1000, &calls) + blocker := s.scriptDir + _ = os.MkdirAll(filepath.Dir(blocker), 0755) + _ = os.WriteFile(blocker, []byte("block"), 0644) + s.scriptDir = filepath.Join(blocker, "subdir") + + err := s.Install() + assert.Assert(t, err != nil) + assert.Assert(t, strings.Contains(err.Error(), "unable to create script directory")) +} + +func TestRemove_SystemctlCallOrder(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + + s.Remove() + + assert.Equal(t, len(calls), 3) + assert.DeepEqual(t, calls[0], []string{"systemctl", "stop", siteServiceEnablerServiceFile}) + assert.DeepEqual(t, calls[1], []string{"systemctl", "disable", siteServiceEnablerServiceFile}) + assert.DeepEqual(t, calls[2], []string{"systemctl", "daemon-reload"}) +} + +func TestRemove_DeletesFiles(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + + svcPath := s.unitPath(siteServiceEnablerServiceFile) + _ = os.MkdirAll(filepath.Dir(svcPath), 0755) + _ = os.WriteFile(svcPath, []byte("[Unit]"), 0644) + + scriptPath := filepath.Join(s.scriptDir, siteServiceEnablerWrapperScript) + _ = os.MkdirAll(s.scriptDir, 0755) + _ = os.WriteFile(scriptPath, []byte("#!/bin/sh"), 0755) + + s.Remove() + + _, err := os.Stat(svcPath) + assert.Assert(t, os.IsNotExist(err), "service file should be removed") + + _, err = os.Stat(scriptPath) + assert.Assert(t, os.IsNotExist(err), "wrapper script should be removed") +} + +func TestRemove_NonRoot_SystemctlUsesUserFlag(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 1000, &calls) + + s.Remove() + + for _, c := range calls { + assert.Equal(t, c[1], "--user", "expected --user flag in call %v", c) + } +} + +func TestRemove_ToleratesMissingFiles(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + s.Remove() +} diff --git a/internal/nonkube/bootstrap/site_service_enabler_service.template b/internal/nonkube/bootstrap/site_service_enabler_service.template new file mode 100644 index 000000000..03d97264d --- /dev/null +++ b/internal/nonkube/bootstrap/site_service_enabler_service.template @@ -0,0 +1,12 @@ +[Unit] +Description=Skupper site service enabler +After=network.target + +[Service] +Type=simple +ExecStart={{.ScriptPath}} +Restart=on-failure +RestartSec=5 + +[Install] +WantedBy={{.WantedBy}} diff --git a/internal/nonkube/bootstrap/uninstall.go b/internal/nonkube/bootstrap/uninstall.go index d47ef92cc..f5649d13a 100644 --- a/internal/nonkube/bootstrap/uninstall.go +++ b/internal/nonkube/bootstrap/uninstall.go @@ -72,6 +72,8 @@ func Uninstall(platform string) error { systemdService.Remove() + newSiteServiceEnablerInstaller().Remove() + fmt.Printf("Platform %s infrastructure for Skupper is now uninstalled\n", platform) return nil diff --git a/internal/nonkube/enabler/enabler.go b/internal/nonkube/enabler/enabler.go new file mode 100644 index 000000000..768208601 --- /dev/null +++ b/internal/nonkube/enabler/enabler.go @@ -0,0 +1,136 @@ +package enabler + +import ( + "log/slog" + "os" + "os/exec" + "path/filepath" + "sync" + + "github.com/skupperproject/skupper/internal/filesystem" + "github.com/skupperproject/skupper/pkg/nonkube/api" +) + +const serviceFilePattern = "skupper-*.service" + +type ServiceEnabler struct { + uid int + command func(string, ...string) *exec.Cmd + mu sync.Mutex +} + +func NewServiceEnabler() *ServiceEnabler { + return &ServiceEnabler{ + uid: os.Getuid(), + command: exec.Command, + } +} + +func (e *ServiceEnabler) OnCreate(name string) { + e.mu.Lock() + defer e.mu.Unlock() + e.enableService(name) +} + +func (e *ServiceEnabler) OnUpdate(name string) { + e.mu.Lock() + defer e.mu.Unlock() + e.enableService(name) +} + +func (e *ServiceEnabler) OnRemove(name string) { + e.mu.Lock() + defer e.mu.Unlock() + svcName := filepath.Base(name) + dst := e.systemdUnitPath(svcName) + _ = e.systemctl("disable", "--now", svcName) + _ = os.Remove(dst) + _ = e.systemctl("daemon-reload") +} + +func (e *ServiceEnabler) OnBasePathAdded(_ string) {} + +func (e *ServiceEnabler) Filter(name string) bool { + base := filepath.Base(name) + matched, _ := filepath.Match(serviceFilePattern, base) + return matched && filepath.Base(filepath.Dir(name)) == filepath.Base(string(api.ScriptsPath)) +} + +func (e *ServiceEnabler) enableService(srcPath string) { + svcName := filepath.Base(srcPath) + unitDir := filepath.Dir(e.systemdUnitPath(svcName)) + dst := filepath.Join(unitDir, svcName) + + src, err := os.ReadFile(srcPath) + if err != nil { + slog.Error("failed to read service file", slog.String("path", srcPath), slog.Any("error", err)) + return + } + + if err = os.MkdirAll(unitDir, 0755); err != nil { + slog.Error("failed to create systemd unit directory", slog.String("dir", unitDir), slog.Any("error", err)) + return + } + + existing, readErr := os.ReadFile(dst) + if readErr != nil || string(existing) != string(src) { + if err = os.WriteFile(dst, src, 0644); err != nil { + slog.Error("failed to write service file", slog.String("dst", dst), slog.Any("error", err)) + return + } + } + + if err = e.systemctl("daemon-reload"); err != nil { + slog.Error("failed to reload systemd daemon", slog.Any("error", err)) + return + } + + if err = e.systemctl("enable", "--now", svcName); err != nil { + slog.Error("failed to enable service", slog.String("name", svcName), slog.Any("error", err)) + } +} + +func (e *ServiceEnabler) systemdUnitPath(name string) string { + if e.uid == 0 { + return filepath.Join("/etc/systemd/system", name) + } + return filepath.Join(api.GetConfigHome(), "systemd", "user", name) +} + +func (e *ServiceEnabler) systemctl(args ...string) error { + var fullArgs []string + if e.uid != 0 { + fullArgs = append(fullArgs, "--user") + } + fullArgs = append(fullArgs, args...) + return e.command("systemctl", fullArgs...).Run() +} + +type NamespaceScriptHandler struct { + NamespacesDir string + Watcher *filesystem.FileWatcher + Enabler *ServiceEnabler +} + +func (n *NamespaceScriptHandler) OnCreate(name string) { + scriptsPath := filepath.Join(name, string(api.ScriptsPath)) + n.Watcher.Add(scriptsPath, n.Enabler) +} + +func (n *NamespaceScriptHandler) OnUpdate(name string) { + scriptsPath := filepath.Join(name, string(api.ScriptsPath)) + n.Watcher.Add(scriptsPath, n.Enabler) +} +func (n *NamespaceScriptHandler) OnRemove(_ string) {} +func (n *NamespaceScriptHandler) OnBasePathAdded(_ string) {} + +func (n *NamespaceScriptHandler) Filter(name string) bool { + if filepath.Dir(name) != n.NamespacesDir { + return false + } + stat, err := os.Stat(name) + if err != nil { + return false + } + return stat.IsDir() +} diff --git a/internal/nonkube/enabler/enabler_test.go b/internal/nonkube/enabler/enabler_test.go new file mode 100644 index 000000000..2cd2eff96 --- /dev/null +++ b/internal/nonkube/enabler/enabler_test.go @@ -0,0 +1,217 @@ +package enabler + +import ( + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/skupperproject/skupper/pkg/nonkube/api" + "gotest.tools/v3/assert" +) + +func TestFilter(t *testing.T) { + e, _ := newTestEnabler(t, 1000) + scriptsSegment := "/" + string(api.ScriptsPath) + "/" + + cases := []struct { + path string + want bool + }{ + {filepath.Join("/data/namespaces/west", string(api.ScriptsPath), "skupper-west.service"), true}, + {filepath.Join("/data/namespaces/west", string(api.ScriptsPath), "skupper-west.service"), true}, + {"/data/namespaces/west/runtime/skupper-west.service", false}, + {filepath.Join("/data/namespaces/west" + scriptsSegment + "other.service"), false}, + {filepath.Join("/data/namespaces/west", string(api.ScriptsPath), "skupper-west.sh"), false}, + } + + for _, tc := range cases { + got := e.Filter(tc.path) + assert.Equal(t, tc.want, got, "Filter(%q)", tc.path) + } +} + +func TestSystemdUnitPath(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + eRoot, _ := newTestEnabler(t, 0) + assert.Equal(t, filepath.Join("/etc/systemd/system", "skupper-west.service"), + eRoot.systemdUnitPath("skupper-west.service")) + + eUser, _ := newTestEnabler(t, 1000) + assert.Equal(t, filepath.Join(dir, "systemd", "user", "skupper-west.service"), + eUser.systemdUnitPath("skupper-west.service")) +} + +func TestSystemctlArgs(t *testing.T) { + for _, uid := range []int{0, 1000} { + uid := uid + t.Run(fmt.Sprintf("uid-%d", uid), func(t *testing.T) { + e, calls := newTestEnabler(t, uid) + _ = e.systemctl("enable", "skupper-west.service") + assert.Assert(t, len(*calls) == 1) + args := (*calls)[0] + assert.Equal(t, "systemctl", args[0]) + hasUser := args[1] == "--user" + assert.Equal(t, uid != 0, hasUser) + }) + } +} + +func TestEnableService_CopiesAndEnables(t *testing.T) { + configDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configDir) + + scriptsDir := filepath.Join(t.TempDir(), "namespaces", "west", string(api.ScriptsPath)) + assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) + srcPath := filepath.Join(scriptsDir, "skupper-west.service") + assert.Assert(t, os.WriteFile(srcPath, []byte("[Unit]\nDescription=test\n"), 0644)) + + e, calls := newTestEnabler(t, 1000) + e.enableService(srcPath) + + dstPath := filepath.Join(configDir, "systemd", "user", "skupper-west.service") + data, err := os.ReadFile(dstPath) + assert.Assert(t, err) + assert.Equal(t, "[Unit]\nDescription=test\n", string(data)) + + assert.Assert(t, len(*calls) == 2, "expected 2 systemctl calls, got %d", len(*calls)) + assert.Assert(t, strings.Contains(strings.Join((*calls)[0], " "), "daemon-reload")) + assert.Assert(t, strings.Contains(strings.Join((*calls)[1], " "), "enable")) + assert.Assert(t, strings.Contains(strings.Join((*calls)[1], " "), "--now")) + assert.Assert(t, strings.Contains(strings.Join((*calls)[1], " "), "skupper-west.service")) +} + +func TestEnableService_SkipsWriteWhenUnchanged(t *testing.T) { + configDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configDir) + + content := []byte("[Unit]\nDescription=test\n") + scriptsDir := filepath.Join(t.TempDir(), "namespaces", "west", string(api.ScriptsPath)) + assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) + srcPath := filepath.Join(scriptsDir, "skupper-west.service") + assert.Assert(t, os.WriteFile(srcPath, content, 0644)) + + dstDir := filepath.Join(configDir, "systemd", "user") + assert.Assert(t, os.MkdirAll(dstDir, 0755)) + dstPath := filepath.Join(dstDir, "skupper-west.service") + assert.Assert(t, os.WriteFile(dstPath, content, 0644)) + info, err := os.Stat(dstPath) + assert.Assert(t, err) + modBefore := info.ModTime() + + e, _ := newTestEnabler(t, 1000) + e.enableService(srcPath) + + info, err = os.Stat(dstPath) + assert.Assert(t, err) + assert.Equal(t, modBefore, info.ModTime(), "file should not have been rewritten") +} + +func TestEnableService_MissingSource(t *testing.T) { + configDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configDir) + + e, calls := newTestEnabler(t, 1000) + e.enableService("/nonexistent/scripts/skupper-west.service") + + assert.Equal(t, 0, len(*calls), "expected no systemctl calls for missing source") +} + +func TestOnCreate_CallsEnableService(t *testing.T) { + configDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configDir) + + scriptsDir := filepath.Join(t.TempDir(), "namespaces", "east", string(api.ScriptsPath)) + assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) + srcPath := filepath.Join(scriptsDir, "skupper-east.service") + assert.Assert(t, os.WriteFile(srcPath, []byte("[Unit]\n"), 0644)) + + e, calls := newTestEnabler(t, 1000) + e.OnCreate(srcPath) + + dstPath := filepath.Join(configDir, "systemd", "user", "skupper-east.service") + _, err := os.ReadFile(dstPath) + assert.Assert(t, err) + assert.Assert(t, len(*calls) >= 1) +} + +func TestOnUpdate_CallsEnableService(t *testing.T) { + configDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configDir) + + scriptsDir := filepath.Join(t.TempDir(), "namespaces", "east", string(api.ScriptsPath)) + assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) + srcPath := filepath.Join(scriptsDir, "skupper-east.service") + assert.Assert(t, os.WriteFile(srcPath, []byte("[Unit]\n"), 0644)) + + e, calls := newTestEnabler(t, 1000) + e.OnUpdate(srcPath) + + dstPath := filepath.Join(configDir, "systemd", "user", "skupper-east.service") + _, err := os.ReadFile(dstPath) + assert.Assert(t, err) + assert.Assert(t, len(*calls) >= 1) +} + +func TestOnRemove_DisablesAndDeletesUnit(t *testing.T) { + configDir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", configDir) + + unitDir := filepath.Join(configDir, "systemd", "user") + assert.Assert(t, os.MkdirAll(unitDir, 0755)) + dstPath := filepath.Join(unitDir, "skupper-west.service") + assert.Assert(t, os.WriteFile(dstPath, []byte("[Unit]\n"), 0644)) + + e, calls := newTestEnabler(t, 1000) + e.OnRemove(dstPath) + + _, err := os.Stat(dstPath) + assert.Assert(t, os.IsNotExist(err), "expected unit file to be removed") + + joined := make([]string, len(*calls)) + for i, c := range *calls { + joined[i] = strings.Join(c, " ") + } + all := strings.Join(joined, " | ") + assert.Assert(t, strings.Contains(all, "disable"), "expected disable call, got: %s", all) + assert.Assert(t, strings.Contains(all, "daemon-reload"), "expected daemon-reload call, got: %s", all) +} + +func TestNamespacesHandlerFilter(t *testing.T) { + namespacesDir := t.TempDir() + + subDir := filepath.Join(namespacesDir, "west") + assert.Assert(t, os.MkdirAll(subDir, 0755)) + nestedDir := filepath.Join(subDir, "nested") + assert.Assert(t, os.MkdirAll(nestedDir, 0755)) + filePath := filepath.Join(namespacesDir, "somefile") + assert.Assert(t, os.WriteFile(filePath, []byte{}, 0644)) + + h := &NamespaceScriptHandler{NamespacesDir: namespacesDir} + + assert.Equal(t, true, h.Filter(subDir), "direct subdir should pass") + assert.Equal(t, false, h.Filter(nestedDir), "nested dir should be rejected") + assert.Equal(t, false, h.Filter(filePath), "file should be rejected") + assert.Equal(t, false, h.Filter(namespacesDir), "the namespaces dir itself should be rejected") +} + +func newTestEnabler(t *testing.T, uid int) (*ServiceEnabler, *[][]string) { + t.Helper() + var mu sync.Mutex + var calls [][]string + e := &ServiceEnabler{ + uid: uid, + command: func(name string, args ...string) *exec.Cmd { + mu.Lock() + calls = append(calls, append([]string{name}, args...)) + mu.Unlock() + return exec.Command("true") + }, + } + return e, &calls +} From c0f8402f7ee953251305ce042262eaf0ebaf6faa Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Fri, 21 Aug 2026 21:32:00 +0200 Subject: [PATCH 2/8] fix codespell --- .../system/nonkube/site_service_enabler.go | 59 ------------------- internal/kube/certificates/mgr_test.go | 2 +- 2 files changed, 1 insertion(+), 60 deletions(-) delete mode 100644 internal/cmd/skupper/system/nonkube/site_service_enabler.go diff --git a/internal/cmd/skupper/system/nonkube/site_service_enabler.go b/internal/cmd/skupper/system/nonkube/site_service_enabler.go deleted file mode 100644 index f6b8f8b7c..000000000 --- a/internal/cmd/skupper/system/nonkube/site_service_enabler.go +++ /dev/null @@ -1,59 +0,0 @@ -package nonkube - -import ( - "log/slog" - "os" - "os/signal" - "syscall" - - "github.com/skupperproject/skupper/internal/filesystem" - "github.com/skupperproject/skupper/internal/nonkube/enabler" - "github.com/skupperproject/skupper/internal/version" - "github.com/skupperproject/skupper/pkg/nonkube/api" - "github.com/spf13/cobra" -) - -func NewCmdSiteServiceEnabler() *cobra.Command { - return &cobra.Command{ - Use: "_site-service-enabler", - Hidden: true, - RunE: func(cmd *cobra.Command, args []string) error { - slog.Info("Starting site-service-enabler", slog.String("version", version.Version)) - - stop := make(chan struct{}) - - sigs := make(chan os.Signal, 1) - signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM) - - go func() { - <-sigs - slog.Info("Shutting down site-service-enabler") - close(stop) - }() - - return run(stop) - }, - } -} - -func run(stop <-chan struct{}) error { - namespacesDir := api.GetDefaultOutputNamespacesPath() - - watcher, err := filesystem.NewWatcher(slog.String("component", "site-service-enabler")) - if err != nil { - return err - } - - siteServiceEnabler := enabler.NewServiceEnabler() - //this checks that the script directory is created, where the systemd services are going to be stored - nsHandler := &enabler.NamespaceScriptHandler{ - NamespacesDir: namespacesDir, - Watcher: watcher, - Enabler: siteServiceEnabler, - } - watcher.Add(namespacesDir, nsHandler) - watcher.Start(stop) - - <-stop - return nil -} diff --git a/internal/kube/certificates/mgr_test.go b/internal/kube/certificates/mgr_test.go index 6eb8c8027..81d4d4656 100644 --- a/internal/kube/certificates/mgr_test.go +++ b/internal/kube/certificates/mgr_test.go @@ -411,7 +411,7 @@ func secretWithOwnerRef(secret *corev1.Secret, ref metav1.OwnerReference) *corev return secret } -// managedWithOwnerHosts sets up a Certificiate with skupper controlled and owner hosts annotations +// managedWithOwnerHosts sets up a Certificate with skupper controlled and owner hosts annotations func managedWithOwnerHosts(t *testing.T, cert *skupperv2alpha1.Certificate, ref metav1.OwnerReference, hosts ...string) *skupperv2alpha1.Certificate { t.Helper() cert.ObjectMeta.OwnerReferences = append(cert.ObjectMeta.OwnerReferences, ref) From 31a09d7e0398303618344cd21b1cc2308b9f94de Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Fri, 21 Aug 2026 21:32:54 +0200 Subject: [PATCH 3/8] replace hidden CLI command by rendering a script template --- internal/cmd/skupper/system/system.go | 1 - internal/nonkube/bootstrap/install.go | 14 +++- .../site_service_enabler_installer.go | 42 +++++++++-- .../site_service_enabler_installer_test.go | 70 +++++++++++++++-- .../site_service_enabler_script.template | 75 +++++++++++++++++++ .../site_service_enabler_service.template | 2 +- internal/nonkube/bootstrap/uninstall.go | 4 +- 7 files changed, 188 insertions(+), 20 deletions(-) create mode 100644 internal/nonkube/bootstrap/site_service_enabler_script.template diff --git a/internal/cmd/skupper/system/system.go b/internal/cmd/skupper/system/system.go index 9e7609c51..c4ef0a0d8 100644 --- a/internal/cmd/skupper/system/system.go +++ b/internal/cmd/skupper/system/system.go @@ -35,7 +35,6 @@ approach, which is based on the new set of Custom Resource Definitions (CRDs).`, cmd.AddCommand(CmdSystemGenerateBundleFactory(platform)) cmd.AddCommand(CmdSystemApplyFactory(platform)) cmd.AddCommand(CmdSystemDeleteFactory(platform)) - cmd.AddCommand(nonkube.NewCmdSiteServiceEnabler()) return cmd } diff --git a/internal/nonkube/bootstrap/install.go b/internal/nonkube/bootstrap/install.go index dcd8f0dc8..f5dbbe727 100644 --- a/internal/nonkube/bootstrap/install.go +++ b/internal/nonkube/bootstrap/install.go @@ -51,6 +51,12 @@ func Install(platform string, reloadType string) error { if isContainerAlreadyRunningInPodman { fmt.Printf("Warning: The system controller container %q is already running in Podman.\n", containerName) + if reloadType == types.SystemReloadTypeAuto { + enabler := newSiteServiceEnablerInstaller() + if err = enabler.Install(); err != nil { + return fmt.Errorf("failed to install skupper site service enabler: %v", err) + } + } return nil } @@ -58,6 +64,12 @@ func Install(platform string, reloadType string) error { if isContainerAlreadyRunningInDocker { fmt.Printf("Warning: The system controller container %q is already running in Docker.\n", containerName) + if reloadType == types.SystemReloadTypeAuto { + enabler := newSiteServiceEnablerInstaller() + if err = enabler.Install(); err != nil { + return fmt.Errorf("failed to install skupper site service enabler: %v", err) + } + } return nil } @@ -288,7 +300,7 @@ func IsContainerRunning(containerName string, platform types.Platform) bool { for _, container := range containers { if container.Name == containerName { - return true + return container.Running } } diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer.go b/internal/nonkube/bootstrap/site_service_enabler_installer.go index 19a9de7a5..776b6a696 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_installer.go +++ b/internal/nonkube/bootstrap/site_service_enabler_installer.go @@ -15,6 +15,9 @@ import ( //go:embed site_service_enabler_service.template var siteServiceEnablerServiceTemplate string +//go:embed site_service_enabler_script.template +var siteServiceEnablerScriptTemplate string + const ( siteServiceEnablerRootSystemdBasePath = "/etc/systemd/system" siteServiceEnablerName = "skupper-site-service-enabler" @@ -27,6 +30,12 @@ type siteServiceEnablerData struct { WantedBy string } +type siteServiceEnablerScriptData struct { + NamespacesDir string + SystemdUnitDir string + SystemctlArgs string +} + type SiteServiceEnablerInstaller struct { uid int rootSystemdBasePath string @@ -44,19 +53,23 @@ func newSiteServiceEnablerInstaller() *SiteServiceEnablerInstaller { } func (s *SiteServiceEnablerInstaller) Install() error { + if s.isRunning() { + return nil + } if err := os.MkdirAll(s.scriptDir, 0755); err != nil { return fmt.Errorf("unable to create script directory %q: %w", s.scriptDir, err) } + if err := os.MkdirAll(s.userSystemdDir(), 0755); err != nil { + return fmt.Errorf("unable to create systemd unit directory %q: %w", s.userSystemdDir(), err) + } scriptPath := path.Join(s.scriptDir, siteServiceEnablerWrapperScript) - script := "#!/bin/sh\nexec skupper system _site-service-enabler\n" - if err := os.WriteFile(scriptPath, []byte(script), 0755); err != nil { + if err := s.renderFile(siteServiceEnablerScriptTemplate, s.scriptData(), scriptPath, 0755); err != nil { return fmt.Errorf("unable to write wrapper script %q: %w", scriptPath, err) } - tmplData := s.templateData(scriptPath) serviceFile := s.unitPath(siteServiceEnablerServiceFile) - if err := s.renderFile(siteServiceEnablerServiceTemplate, tmplData, serviceFile, 0644); err != nil { + if err := s.renderFile(siteServiceEnablerServiceTemplate, s.templateData(scriptPath), serviceFile, 0644); err != nil { return fmt.Errorf("unable to write site enabler service unit: %w", err) } @@ -76,10 +89,21 @@ func (s *SiteServiceEnablerInstaller) Install() error { func (s *SiteServiceEnablerInstaller) Remove() { _ = s.systemctl("stop", siteServiceEnablerServiceFile) _ = s.systemctl("disable", siteServiceEnablerServiceFile) - _ = s.systemctl("daemon-reload") - _ = os.Remove(s.unitPath(siteServiceEnablerServiceFile)) _ = os.Remove(path.Join(s.scriptDir, siteServiceEnablerWrapperScript)) + _ = s.systemctl("daemon-reload") +} + +func (s *SiteServiceEnablerInstaller) scriptData() siteServiceEnablerScriptData { + systemctlArgs := "" + if s.uid != 0 { + systemctlArgs = "--user " + } + return siteServiceEnablerScriptData{ + NamespacesDir: api.GetDefaultOutputNamespacesPath(), + SystemdUnitDir: s.userSystemdDir(), + SystemctlArgs: systemctlArgs, + } } func (s *SiteServiceEnablerInstaller) templateData(scriptPath string) siteServiceEnablerData { @@ -104,6 +128,10 @@ func (s *SiteServiceEnablerInstaller) userSystemdDir() string { return path.Join(api.GetConfigHome(), "systemd", "user") } +func (s *SiteServiceEnablerInstaller) isRunning() bool { + return s.systemctl("is-active", "--quiet", siteServiceEnablerServiceFile) == nil +} + func (s *SiteServiceEnablerInstaller) systemctl(args ...string) error { var fullArgs []string if s.uid != 0 { @@ -113,7 +141,7 @@ func (s *SiteServiceEnablerInstaller) systemctl(args ...string) error { return s.command("systemctl", fullArgs...).Run() } -func (s *SiteServiceEnablerInstaller) renderFile(tmplText string, data siteServiceEnablerData, dst string, mode os.FileMode) error { +func (s *SiteServiceEnablerInstaller) renderFile(tmplText string, data any, dst string, mode os.FileMode) error { tmpl, err := template.New("").Parse(tmplText) if err != nil { return err diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go index 617822f98..286158a31 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go +++ b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go @@ -17,9 +17,24 @@ func fakeCommand(calls *[][]string) func(string, ...string) *exec.Cmd { } } + +func fakeCommandNotRunning(calls *[][]string) func(string, ...string) *exec.Cmd { + return func(name string, args ...string) *exec.Cmd { + *calls = append(*calls, append([]string{name}, args...)) + for _, a := range args { + if a == "is-active" { + return exec.Command("false") + } + } + return exec.Command("true") + } +} + func newTestInstaller(t *testing.T, uid int, calls *[][]string) *SiteServiceEnablerInstaller { t.Helper() tmp := t.TempDir() + + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, "config")) return &SiteServiceEnablerInstaller{ uid: uid, rootSystemdBasePath: filepath.Join(tmp, "etc", "systemd", "system"), @@ -77,7 +92,7 @@ func TestSystemctl_Root_NoUserFlag(t *testing.T) { assert.DeepEqual(t, calls[0], []string{"systemctl", "start", "foo.service"}) } -func TestRenderFile(t *testing.T) { +func TestRenderFile_ServiceTemplate(t *testing.T) { tmp := t.TempDir() dst := filepath.Join(tmp, "out.service") s := &SiteServiceEnablerInstaller{} @@ -93,6 +108,25 @@ func TestRenderFile(t *testing.T) { assert.Assert(t, strings.Contains(string(content), "WantedBy=multi-user.target")) } +func TestRenderFile_ScriptTemplate(t *testing.T) { + tmp := t.TempDir() + dst := filepath.Join(tmp, "out.sh") + s := &SiteServiceEnablerInstaller{} + err := s.renderFile(siteServiceEnablerScriptTemplate, siteServiceEnablerScriptData{ + NamespacesDir: "/home/user/.local/share/skupper/namespaces", + SystemdUnitDir: "/home/user/.config/systemd/user", + SystemctlArgs: "--user ", + }, dst, 0755) + assert.NilError(t, err) + + content, err := os.ReadFile(dst) + assert.NilError(t, err) + assert.Assert(t, strings.Contains(string(content), "NAMESPACES_DIR=\"/home/user/.local/share/skupper/namespaces\"")) + assert.Assert(t, strings.Contains(string(content), "UNIT_DIR=\"/home/user/.config/systemd/user\"")) + assert.Assert(t, strings.Contains(string(content), "SYSTEMCTL_ARGS=\"--user \"")) + assert.Assert(t, strings.Contains(string(content), "POLL_INTERVAL")) +} + func TestRenderFile_InvalidTemplate(t *testing.T) { tmp := t.TempDir() s := &SiteServiceEnablerInstaller{} @@ -103,6 +137,7 @@ func TestRenderFile_InvalidTemplate(t *testing.T) { func TestInstall_CreatesWrapperScript(t *testing.T) { var calls [][]string s := newTestInstaller(t, 1000, &calls) + s.command = fakeCommandNotRunning(&calls) err := s.Install() assert.NilError(t, err) @@ -110,7 +145,9 @@ func TestInstall_CreatesWrapperScript(t *testing.T) { scriptPath := filepath.Join(s.scriptDir, siteServiceEnablerWrapperScript) content, err := os.ReadFile(scriptPath) assert.NilError(t, err) - assert.Equal(t, string(content), "#!/bin/sh\nexec skupper system _site-service-enabler\n") + + assert.Assert(t, strings.HasPrefix(string(content), "#!/bin/sh"), "expected shell shebang") + assert.Assert(t, strings.Contains(string(content), "POLL_INTERVAL")) info, err := os.Stat(scriptPath) assert.NilError(t, err) @@ -120,7 +157,8 @@ func TestInstall_CreatesWrapperScript(t *testing.T) { func TestInstall_CreatesServiceFile(t *testing.T) { var calls [][]string s := newTestInstaller(t, 0, &calls) - _ = os.MkdirAll(s.rootSystemdBasePath, 0755) + s.command = fakeCommandNotRunning(&calls) + _ = os.MkdirAll(s.unitPath(""), 0755) err := s.Install() assert.NilError(t, err) @@ -135,20 +173,35 @@ func TestInstall_CreatesServiceFile(t *testing.T) { func TestInstall_SystemctlCallOrder(t *testing.T) { var calls [][]string s := newTestInstaller(t, 0, &calls) - _ = os.MkdirAll(s.rootSystemdBasePath, 0755) + s.command = fakeCommandNotRunning(&calls) + _ = os.MkdirAll(s.unitPath(""), 0755) err := s.Install() assert.NilError(t, err) - assert.Equal(t, len(calls), 3) - assert.DeepEqual(t, calls[0], []string{"systemctl", "daemon-reload"}) - assert.DeepEqual(t, calls[1], []string{"systemctl", "enable", siteServiceEnablerServiceFile}) - assert.DeepEqual(t, calls[2], []string{"systemctl", "start", siteServiceEnablerServiceFile}) + + assert.Equal(t, len(calls), 4) + assert.DeepEqual(t, calls[0], []string{"systemctl", "is-active", "--quiet", siteServiceEnablerServiceFile}) + assert.DeepEqual(t, calls[1], []string{"systemctl", "daemon-reload"}) + assert.DeepEqual(t, calls[2], []string{"systemctl", "enable", siteServiceEnablerServiceFile}) + assert.DeepEqual(t, calls[3], []string{"systemctl", "start", siteServiceEnablerServiceFile}) +} + +func TestInstall_SkipsWhenAlreadyRunning(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + + err := s.Install() + assert.NilError(t, err) + + assert.Equal(t, len(calls), 1) + assert.DeepEqual(t, calls[0], []string{"systemctl", "is-active", "--quiet", siteServiceEnablerServiceFile}) } func TestInstall_NonRoot_SystemctlUsesUserFlag(t *testing.T) { var calls [][]string s := newTestInstaller(t, 1000, &calls) + s.command = fakeCommandNotRunning(&calls) err := s.Install() assert.NilError(t, err) @@ -161,6 +214,7 @@ func TestInstall_NonRoot_SystemctlUsesUserFlag(t *testing.T) { func TestInstall_ScriptDirCreationFailure(t *testing.T) { var calls [][]string s := newTestInstaller(t, 1000, &calls) + s.command = fakeCommandNotRunning(&calls) blocker := s.scriptDir _ = os.MkdirAll(filepath.Dir(blocker), 0755) _ = os.WriteFile(blocker, []byte("block"), 0644) diff --git a/internal/nonkube/bootstrap/site_service_enabler_script.template b/internal/nonkube/bootstrap/site_service_enabler_script.template new file mode 100644 index 000000000..c55da3f4b --- /dev/null +++ b/internal/nonkube/bootstrap/site_service_enabler_script.template @@ -0,0 +1,75 @@ +#!/bin/sh +# skupper-site-service-enabler +# Watches Skupper namespace script directories and registers/deregisters +# site systemd services as they appear or disappear. +# +# Generated by 'skupper system install'. Do not edit manually. + +NAMESPACES_DIR="{{.NamespacesDir}}" +UNIT_DIR="{{.SystemdUnitDir}}" +SYSTEMCTL_ARGS="{{.SystemctlArgs}}" + +SCRIPT_SUBPATH="internal/scripts" +POLL_INTERVAL=5 + +systemctl_run() { + systemctl ${SYSTEMCTL_ARGS}"$@" +} + +enable_service() { + src="$1" + svc=$(basename "$src") + dst="${UNIT_DIR}/${svc}" + mkdir -p "${UNIT_DIR}" + if ! cmp -s "$src" "$dst"; then + cp "$src" "$dst" + systemctl_run daemon-reload || true + fi + systemctl_run enable --now "$svc" || true +} + +disable_service() { + svc="$1" + dst="${UNIT_DIR}/${svc}" + if [ -f "$dst" ]; then + systemctl_run disable --now "$svc" || true + rm -f "$dst" + systemctl_run daemon-reload || true + fi +} + +# List site service unit files we manage. +# Only considers files that match the per-namespace naming pattern +# skupper-.service, excluding infrastructure services. +list_active() { + [ -d "${UNIT_DIR}" ] || return + find "${UNIT_DIR}" -maxdepth 1 -name "skupper-*.service" \ + ! -name "skupper-site-service-enabler.service" \ + ! -name "skupper-controller.service" \ + ! -name "skupper-network-observer-*.service" +} + +while true; do + if [ -d "${NAMESPACES_DIR}" ]; then + find "${NAMESPACES_DIR}" \ + -path "*/${SCRIPT_SUBPATH}/skupper-*.service" \ + -type f | while IFS= read -r src; do + enable_service "$src" + done + fi + + list_active | while IFS= read -r dst; do + svc=$(basename "$dst") + found="" + if [ -d "${NAMESPACES_DIR}" ]; then + found=$(find "${NAMESPACES_DIR}" \ + -path "*/${SCRIPT_SUBPATH}/${svc}" \ + -type f | head -1) + fi + if [ -z "$found" ]; then + disable_service "$svc" + fi + done + + sleep "${POLL_INTERVAL}" +done diff --git a/internal/nonkube/bootstrap/site_service_enabler_service.template b/internal/nonkube/bootstrap/site_service_enabler_service.template index 03d97264d..b8c93c7ec 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_service.template +++ b/internal/nonkube/bootstrap/site_service_enabler_service.template @@ -5,7 +5,7 @@ After=network.target [Service] Type=simple ExecStart={{.ScriptPath}} -Restart=on-failure +Restart=always RestartSec=5 [Install] diff --git a/internal/nonkube/bootstrap/uninstall.go b/internal/nonkube/bootstrap/uninstall.go index f5649d13a..09dca0671 100644 --- a/internal/nonkube/bootstrap/uninstall.go +++ b/internal/nonkube/bootstrap/uninstall.go @@ -15,6 +15,8 @@ import ( func Uninstall(platform string) error { + newSiteServiceEnablerInstaller().Remove() + currentUser, err := user.Current() if err != nil { return fmt.Errorf("failed to get current user: %v", err) @@ -72,8 +74,6 @@ func Uninstall(platform string) error { systemdService.Remove() - newSiteServiceEnablerInstaller().Remove() - fmt.Printf("Platform %s infrastructure for Skupper is now uninstalled\n", platform) return nil From 6e577944c51274157318e19ca0533ab84a50f639 Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Fri, 21 Aug 2026 23:06:19 +0200 Subject: [PATCH 4/8] get reload type early --- internal/nonkube/bootstrap/install.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/nonkube/bootstrap/install.go b/internal/nonkube/bootstrap/install.go index f5dbbe727..676f4152d 100644 --- a/internal/nonkube/bootstrap/install.go +++ b/internal/nonkube/bootstrap/install.go @@ -40,6 +40,11 @@ func Install(platform string, reloadType string) error { return err } + if reloadType == "" { + reloadType = utils.DefaultStr(os.Getenv(types.ENV_SYSTEM_AUTO_RELOAD), + types.SystemReloadTypeManual) + } + config, err := configEnvVariables(platform) if err != nil { return err @@ -84,11 +89,6 @@ func Install(platform string, reloadType string) error { } fmt.Printf("Pulled system-controller image: %s\n", images.GetSystemControllerImageName()) - if reloadType == "" { - reloadType = utils.DefaultStr(os.Getenv(types.ENV_SYSTEM_AUTO_RELOAD), - types.SystemReloadTypeManual) - } - env := map[string]string{ "CONTAINER_ENDPOINT": config.containerEndpoint, "SKUPPER_OUTPUT_PATH": config.hostDataHome, From 2192d0a81f81fb29e23e9fb60d6178fc7c86ee3c Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Fri, 21 Aug 2026 23:08:03 +0200 Subject: [PATCH 5/8] propagate errors form the enabler uninstallation and add ownership marker to systemd services --- .../site_service_enabler_installer.go | 25 ++- .../site_service_enabler_installer_test.go | 153 ++++++++++++++++-- .../site_service_enabler_script.template | 29 ++-- internal/nonkube/bootstrap/uninstall.go | 4 +- 4 files changed, 181 insertions(+), 30 deletions(-) diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer.go b/internal/nonkube/bootstrap/site_service_enabler_installer.go index 776b6a696..67af082e5 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_installer.go +++ b/internal/nonkube/bootstrap/site_service_enabler_installer.go @@ -31,7 +31,7 @@ type siteServiceEnablerData struct { } type siteServiceEnablerScriptData struct { - NamespacesDir string + NamespacesDir string SystemdUnitDir string SystemctlArgs string } @@ -86,12 +86,23 @@ func (s *SiteServiceEnablerInstaller) Install() error { return nil } -func (s *SiteServiceEnablerInstaller) Remove() { - _ = s.systemctl("stop", siteServiceEnablerServiceFile) - _ = s.systemctl("disable", siteServiceEnablerServiceFile) - _ = os.Remove(s.unitPath(siteServiceEnablerServiceFile)) - _ = os.Remove(path.Join(s.scriptDir, siteServiceEnablerWrapperScript)) - _ = s.systemctl("daemon-reload") +func (s *SiteServiceEnablerInstaller) Remove() error { + if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil { + return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err) + } + if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil { + return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err) + } + if err := os.Remove(s.unitPath(siteServiceEnablerServiceFile)); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove unit file: %w", err) + } + if err := os.Remove(path.Join(s.scriptDir, siteServiceEnablerWrapperScript)); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove wrapper script: %w", err) + } + if err := s.systemctl("daemon-reload"); err != nil { + return fmt.Errorf("daemon-reload failed after remove: %w", err) + } + return nil } func (s *SiteServiceEnablerInstaller) scriptData() siteServiceEnablerScriptData { diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go index 286158a31..cc61185f6 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go +++ b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go @@ -1,6 +1,7 @@ package bootstrap import ( + "context" "os" "os/exec" "path/filepath" @@ -13,27 +14,26 @@ import ( func fakeCommand(calls *[][]string) func(string, ...string) *exec.Cmd { return func(name string, args ...string) *exec.Cmd { *calls = append(*calls, append([]string{name}, args...)) - return exec.Command("true") + return exec.CommandContext(context.Background(), "true") } } - func fakeCommandNotRunning(calls *[][]string) func(string, ...string) *exec.Cmd { return func(name string, args ...string) *exec.Cmd { *calls = append(*calls, append([]string{name}, args...)) for _, a := range args { if a == "is-active" { - return exec.Command("false") + return exec.CommandContext(context.Background(), "false") } } - return exec.Command("true") + return exec.CommandContext(context.Background(), "true") } } func newTestInstaller(t *testing.T, uid int, calls *[][]string) *SiteServiceEnablerInstaller { t.Helper() tmp := t.TempDir() - + t.Setenv("XDG_CONFIG_HOME", filepath.Join(tmp, "config")) return &SiteServiceEnablerInstaller{ uid: uid, @@ -127,6 +127,62 @@ func TestRenderFile_ScriptTemplate(t *testing.T) { assert.Assert(t, strings.Contains(string(content), "POLL_INTERVAL")) } +func renderScript(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + dst := filepath.Join(tmp, "out.sh") + s := &SiteServiceEnablerInstaller{} + err := s.renderFile(siteServiceEnablerScriptTemplate, siteServiceEnablerScriptData{ + NamespacesDir: "/ns", + SystemdUnitDir: "/units", + SystemctlArgs: "", + }, dst, 0755) + assert.NilError(t, err) + content, err := os.ReadFile(dst) + assert.NilError(t, err) + return string(content) +} + +func TestScript_RestartsServiceOnUnitChange(t *testing.T) { + body := renderScript(t) + assert.Assert(t, strings.Contains(body, "changed=1"), "expected changed=1 inside change detection branch") + assert.Assert(t, strings.Contains(body, `if [ "$changed" -eq 1 ]`), "expected conditional restart block") + assert.Assert(t, strings.Contains(body, `systemctl_run restart "$svc"`), "expected restart via systemctl_run") +} + +func TestScript_OwnershipMarkerAppendedToUnitCopy(t *testing.T) { + body := renderScript(t) + assert.Assert(t, strings.Contains(body, "OWNERSHIP_MARKER="), "expected OWNERSHIP_MARKER variable") + assert.Assert(t, strings.Contains(body, "X-ManagedBy=skupper-site-service-enabler"), "expected X-ManagedBy marker text") + assert.Assert(t, strings.Contains(body, `awk '/^\[Unit\]/`), "expected awk injection into [Unit] section") + assert.Assert(t, strings.Contains(body, `printf '%s' "$owned" > "$dst"`), "expected owned content written to dst") +} + +func TestScript_ListActiveUsesMarkerNotPrefix(t *testing.T) { + body := renderScript(t) + assert.Assert(t, strings.Contains(body, `grep -rl "${OWNERSHIP_MARKER}"`), "expected grep on ownership marker in list_active") + assert.Assert(t, !strings.Contains(body, `find "${UNIT_DIR}" -maxdepth 1 -name "skupper-*.service"`), "must not use prefix-based find") +} + +func TestScript_KeepsUnitFileWhenDisableFails(t *testing.T) { + tmp := t.TempDir() + dst := filepath.Join(tmp, "out.sh") + s := &SiteServiceEnablerInstaller{} + err := s.renderFile(siteServiceEnablerScriptTemplate, siteServiceEnablerScriptData{ + NamespacesDir: "/ns", + SystemdUnitDir: "/units", + SystemctlArgs: "", + }, dst, 0755) + assert.NilError(t, err) + + content, err := os.ReadFile(dst) + assert.NilError(t, err) + body := string(content) + + assert.Assert(t, strings.Contains(body, `if systemctl_run disable --now "$svc"`), "expected disable guarding rm") + assert.Assert(t, strings.Contains(body, "rm -f"), "expected rm -f inside disable branch") +} + func TestRenderFile_InvalidTemplate(t *testing.T) { tmp := t.TempDir() s := &SiteServiceEnablerInstaller{} @@ -179,7 +235,6 @@ func TestInstall_SystemctlCallOrder(t *testing.T) { err := s.Install() assert.NilError(t, err) - assert.Equal(t, len(calls), 4) assert.DeepEqual(t, calls[0], []string{"systemctl", "is-active", "--quiet", siteServiceEnablerServiceFile}) assert.DeepEqual(t, calls[1], []string{"systemctl", "daemon-reload"}) @@ -229,7 +284,8 @@ func TestRemove_SystemctlCallOrder(t *testing.T) { var calls [][]string s := newTestInstaller(t, 0, &calls) - s.Remove() + err := s.Remove() + assert.NilError(t, err) assert.Equal(t, len(calls), 3) assert.DeepEqual(t, calls[0], []string{"systemctl", "stop", siteServiceEnablerServiceFile}) @@ -249,9 +305,10 @@ func TestRemove_DeletesFiles(t *testing.T) { _ = os.MkdirAll(s.scriptDir, 0755) _ = os.WriteFile(scriptPath, []byte("#!/bin/sh"), 0755) - s.Remove() + err := s.Remove() + assert.NilError(t, err) - _, err := os.Stat(svcPath) + _, err = os.Stat(svcPath) assert.Assert(t, os.IsNotExist(err), "service file should be removed") _, err = os.Stat(scriptPath) @@ -262,7 +319,8 @@ func TestRemove_NonRoot_SystemctlUsesUserFlag(t *testing.T) { var calls [][]string s := newTestInstaller(t, 1000, &calls) - s.Remove() + err := s.Remove() + assert.NilError(t, err) for _, c := range calls { assert.Equal(t, c[1], "--user", "expected --user flag in call %v", c) @@ -272,5 +330,78 @@ func TestRemove_NonRoot_SystemctlUsesUserFlag(t *testing.T) { func TestRemove_ToleratesMissingFiles(t *testing.T) { var calls [][]string s := newTestInstaller(t, 0, &calls) - s.Remove() + err := s.Remove() + assert.NilError(t, err) +} + +func TestRemove_FailsOnStopError(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + s.command = func(name string, args ...string) *exec.Cmd { + calls = append(calls, append([]string{name}, args...)) + for _, a := range args { + if a == "stop" { + return exec.CommandContext(context.Background(), "false") + } + } + return exec.CommandContext(context.Background(), "true") + } + + err := s.Remove() + assert.Assert(t, err != nil) + assert.Assert(t, strings.Contains(err.Error(), "failed to stop")) + // disable and daemon-reload must not have been called + assert.Equal(t, len(calls), 1) +} + +func TestRemove_FailsOnDisableError(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + s.command = func(name string, args ...string) *exec.Cmd { + calls = append(calls, append([]string{name}, args...)) + for _, a := range args { + if a == "disable" { + return exec.CommandContext(context.Background(), "false") + } + } + return exec.CommandContext(context.Background(), "true") + } + + err := s.Remove() + assert.Assert(t, err != nil) + assert.Assert(t, strings.Contains(err.Error(), "failed to disable")) + // daemon-reload must not have been called + assert.Equal(t, len(calls), 2) +} + +func TestRemove_FailsOnUnitFileRemoveError(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + s.command = fakeCommand(&calls) + + unitPath := s.unitPath(siteServiceEnablerServiceFile) + _ = os.MkdirAll(unitPath, 0755) + _ = os.WriteFile(filepath.Join(unitPath, "child"), []byte("x"), 0644) + + err := s.Remove() + assert.Assert(t, err != nil) + assert.Assert(t, strings.Contains(err.Error(), "failed to remove unit file")) +} + +func TestRemove_FailsOnDaemonReloadError(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + s.command = func(name string, args ...string) *exec.Cmd { + calls = append(calls, append([]string{name}, args...)) + for _, a := range args { + if a == "daemon-reload" { + return exec.CommandContext(context.Background(), "false") + } + } + return exec.CommandContext(context.Background(), "true") + } + + err := s.Remove() + assert.Assert(t, err != nil) + assert.Assert(t, strings.Contains(err.Error(), "daemon-reload failed after remove")) } diff --git a/internal/nonkube/bootstrap/site_service_enabler_script.template b/internal/nonkube/bootstrap/site_service_enabler_script.template index c55da3f4b..2adf3ace6 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_script.template +++ b/internal/nonkube/bootstrap/site_service_enabler_script.template @@ -11,6 +11,7 @@ SYSTEMCTL_ARGS="{{.SystemctlArgs}}" SCRIPT_SUBPATH="internal/scripts" POLL_INTERVAL=5 +OWNERSHIP_MARKER="X-ManagedBy=skupper-site-service-enabler" systemctl_run() { systemctl ${SYSTEMCTL_ARGS}"$@" @@ -21,32 +22,38 @@ enable_service() { svc=$(basename "$src") dst="${UNIT_DIR}/${svc}" mkdir -p "${UNIT_DIR}" - if ! cmp -s "$src" "$dst"; then - cp "$src" "$dst" + changed=0 + + owned=$(awk '/^\[Unit\]/{print; print "'"${OWNERSHIP_MARKER}"'"; next}1' "$src") + existing=$(cat "$dst" 2>/dev/null || true) + if [ "$owned" != "$existing" ]; then + printf '%s' "$owned" > "$dst" systemctl_run daemon-reload || true + changed=1 fi systemctl_run enable --now "$svc" || true + if [ "$changed" -eq 1 ]; then + systemctl_run restart "$svc" || true + fi } disable_service() { svc="$1" dst="${UNIT_DIR}/${svc}" if [ -f "$dst" ]; then - systemctl_run disable --now "$svc" || true - rm -f "$dst" - systemctl_run daemon-reload || true + if systemctl_run disable --now "$svc"; then + rm -f "$dst" + systemctl_run daemon-reload || true + fi fi } # List site service unit files we manage. -# Only considers files that match the per-namespace naming pattern -# skupper-.service, excluding infrastructure services. +# Only files bearing the ownership marker are considered owned by this helper. list_active() { [ -d "${UNIT_DIR}" ] || return - find "${UNIT_DIR}" -maxdepth 1 -name "skupper-*.service" \ - ! -name "skupper-site-service-enabler.service" \ - ! -name "skupper-controller.service" \ - ! -name "skupper-network-observer-*.service" + grep -rl "${OWNERSHIP_MARKER}" "${UNIT_DIR}" 2>/dev/null \ + | grep -E '/skupper-[^/]+\.service$' } while true; do diff --git a/internal/nonkube/bootstrap/uninstall.go b/internal/nonkube/bootstrap/uninstall.go index 09dca0671..94b45fb14 100644 --- a/internal/nonkube/bootstrap/uninstall.go +++ b/internal/nonkube/bootstrap/uninstall.go @@ -15,7 +15,9 @@ import ( func Uninstall(platform string) error { - newSiteServiceEnablerInstaller().Remove() + if err := newSiteServiceEnablerInstaller().Remove(); err != nil { + return fmt.Errorf("failed to remove skupper site service enabler: %w", err) + } currentUser, err := user.Current() if err != nil { From 63e69495fc07f8ec56e7f4f9d7463aeeacd60756 Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Fri, 21 Aug 2026 23:17:48 +0200 Subject: [PATCH 6/8] remove unnecesary code --- internal/nonkube/enabler/enabler.go | 136 -------------- internal/nonkube/enabler/enabler_test.go | 217 ----------------------- 2 files changed, 353 deletions(-) delete mode 100644 internal/nonkube/enabler/enabler.go delete mode 100644 internal/nonkube/enabler/enabler_test.go diff --git a/internal/nonkube/enabler/enabler.go b/internal/nonkube/enabler/enabler.go deleted file mode 100644 index 768208601..000000000 --- a/internal/nonkube/enabler/enabler.go +++ /dev/null @@ -1,136 +0,0 @@ -package enabler - -import ( - "log/slog" - "os" - "os/exec" - "path/filepath" - "sync" - - "github.com/skupperproject/skupper/internal/filesystem" - "github.com/skupperproject/skupper/pkg/nonkube/api" -) - -const serviceFilePattern = "skupper-*.service" - -type ServiceEnabler struct { - uid int - command func(string, ...string) *exec.Cmd - mu sync.Mutex -} - -func NewServiceEnabler() *ServiceEnabler { - return &ServiceEnabler{ - uid: os.Getuid(), - command: exec.Command, - } -} - -func (e *ServiceEnabler) OnCreate(name string) { - e.mu.Lock() - defer e.mu.Unlock() - e.enableService(name) -} - -func (e *ServiceEnabler) OnUpdate(name string) { - e.mu.Lock() - defer e.mu.Unlock() - e.enableService(name) -} - -func (e *ServiceEnabler) OnRemove(name string) { - e.mu.Lock() - defer e.mu.Unlock() - svcName := filepath.Base(name) - dst := e.systemdUnitPath(svcName) - _ = e.systemctl("disable", "--now", svcName) - _ = os.Remove(dst) - _ = e.systemctl("daemon-reload") -} - -func (e *ServiceEnabler) OnBasePathAdded(_ string) {} - -func (e *ServiceEnabler) Filter(name string) bool { - base := filepath.Base(name) - matched, _ := filepath.Match(serviceFilePattern, base) - return matched && filepath.Base(filepath.Dir(name)) == filepath.Base(string(api.ScriptsPath)) -} - -func (e *ServiceEnabler) enableService(srcPath string) { - svcName := filepath.Base(srcPath) - unitDir := filepath.Dir(e.systemdUnitPath(svcName)) - dst := filepath.Join(unitDir, svcName) - - src, err := os.ReadFile(srcPath) - if err != nil { - slog.Error("failed to read service file", slog.String("path", srcPath), slog.Any("error", err)) - return - } - - if err = os.MkdirAll(unitDir, 0755); err != nil { - slog.Error("failed to create systemd unit directory", slog.String("dir", unitDir), slog.Any("error", err)) - return - } - - existing, readErr := os.ReadFile(dst) - if readErr != nil || string(existing) != string(src) { - if err = os.WriteFile(dst, src, 0644); err != nil { - slog.Error("failed to write service file", slog.String("dst", dst), slog.Any("error", err)) - return - } - } - - if err = e.systemctl("daemon-reload"); err != nil { - slog.Error("failed to reload systemd daemon", slog.Any("error", err)) - return - } - - if err = e.systemctl("enable", "--now", svcName); err != nil { - slog.Error("failed to enable service", slog.String("name", svcName), slog.Any("error", err)) - } -} - -func (e *ServiceEnabler) systemdUnitPath(name string) string { - if e.uid == 0 { - return filepath.Join("/etc/systemd/system", name) - } - return filepath.Join(api.GetConfigHome(), "systemd", "user", name) -} - -func (e *ServiceEnabler) systemctl(args ...string) error { - var fullArgs []string - if e.uid != 0 { - fullArgs = append(fullArgs, "--user") - } - fullArgs = append(fullArgs, args...) - return e.command("systemctl", fullArgs...).Run() -} - -type NamespaceScriptHandler struct { - NamespacesDir string - Watcher *filesystem.FileWatcher - Enabler *ServiceEnabler -} - -func (n *NamespaceScriptHandler) OnCreate(name string) { - scriptsPath := filepath.Join(name, string(api.ScriptsPath)) - n.Watcher.Add(scriptsPath, n.Enabler) -} - -func (n *NamespaceScriptHandler) OnUpdate(name string) { - scriptsPath := filepath.Join(name, string(api.ScriptsPath)) - n.Watcher.Add(scriptsPath, n.Enabler) -} -func (n *NamespaceScriptHandler) OnRemove(_ string) {} -func (n *NamespaceScriptHandler) OnBasePathAdded(_ string) {} - -func (n *NamespaceScriptHandler) Filter(name string) bool { - if filepath.Dir(name) != n.NamespacesDir { - return false - } - stat, err := os.Stat(name) - if err != nil { - return false - } - return stat.IsDir() -} diff --git a/internal/nonkube/enabler/enabler_test.go b/internal/nonkube/enabler/enabler_test.go deleted file mode 100644 index 2cd2eff96..000000000 --- a/internal/nonkube/enabler/enabler_test.go +++ /dev/null @@ -1,217 +0,0 @@ -package enabler - -import ( - "fmt" - "os" - "os/exec" - "path/filepath" - "strings" - "sync" - "testing" - - "github.com/skupperproject/skupper/pkg/nonkube/api" - "gotest.tools/v3/assert" -) - -func TestFilter(t *testing.T) { - e, _ := newTestEnabler(t, 1000) - scriptsSegment := "/" + string(api.ScriptsPath) + "/" - - cases := []struct { - path string - want bool - }{ - {filepath.Join("/data/namespaces/west", string(api.ScriptsPath), "skupper-west.service"), true}, - {filepath.Join("/data/namespaces/west", string(api.ScriptsPath), "skupper-west.service"), true}, - {"/data/namespaces/west/runtime/skupper-west.service", false}, - {filepath.Join("/data/namespaces/west" + scriptsSegment + "other.service"), false}, - {filepath.Join("/data/namespaces/west", string(api.ScriptsPath), "skupper-west.sh"), false}, - } - - for _, tc := range cases { - got := e.Filter(tc.path) - assert.Equal(t, tc.want, got, "Filter(%q)", tc.path) - } -} - -func TestSystemdUnitPath(t *testing.T) { - dir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", dir) - - eRoot, _ := newTestEnabler(t, 0) - assert.Equal(t, filepath.Join("/etc/systemd/system", "skupper-west.service"), - eRoot.systemdUnitPath("skupper-west.service")) - - eUser, _ := newTestEnabler(t, 1000) - assert.Equal(t, filepath.Join(dir, "systemd", "user", "skupper-west.service"), - eUser.systemdUnitPath("skupper-west.service")) -} - -func TestSystemctlArgs(t *testing.T) { - for _, uid := range []int{0, 1000} { - uid := uid - t.Run(fmt.Sprintf("uid-%d", uid), func(t *testing.T) { - e, calls := newTestEnabler(t, uid) - _ = e.systemctl("enable", "skupper-west.service") - assert.Assert(t, len(*calls) == 1) - args := (*calls)[0] - assert.Equal(t, "systemctl", args[0]) - hasUser := args[1] == "--user" - assert.Equal(t, uid != 0, hasUser) - }) - } -} - -func TestEnableService_CopiesAndEnables(t *testing.T) { - configDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", configDir) - - scriptsDir := filepath.Join(t.TempDir(), "namespaces", "west", string(api.ScriptsPath)) - assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) - srcPath := filepath.Join(scriptsDir, "skupper-west.service") - assert.Assert(t, os.WriteFile(srcPath, []byte("[Unit]\nDescription=test\n"), 0644)) - - e, calls := newTestEnabler(t, 1000) - e.enableService(srcPath) - - dstPath := filepath.Join(configDir, "systemd", "user", "skupper-west.service") - data, err := os.ReadFile(dstPath) - assert.Assert(t, err) - assert.Equal(t, "[Unit]\nDescription=test\n", string(data)) - - assert.Assert(t, len(*calls) == 2, "expected 2 systemctl calls, got %d", len(*calls)) - assert.Assert(t, strings.Contains(strings.Join((*calls)[0], " "), "daemon-reload")) - assert.Assert(t, strings.Contains(strings.Join((*calls)[1], " "), "enable")) - assert.Assert(t, strings.Contains(strings.Join((*calls)[1], " "), "--now")) - assert.Assert(t, strings.Contains(strings.Join((*calls)[1], " "), "skupper-west.service")) -} - -func TestEnableService_SkipsWriteWhenUnchanged(t *testing.T) { - configDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", configDir) - - content := []byte("[Unit]\nDescription=test\n") - scriptsDir := filepath.Join(t.TempDir(), "namespaces", "west", string(api.ScriptsPath)) - assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) - srcPath := filepath.Join(scriptsDir, "skupper-west.service") - assert.Assert(t, os.WriteFile(srcPath, content, 0644)) - - dstDir := filepath.Join(configDir, "systemd", "user") - assert.Assert(t, os.MkdirAll(dstDir, 0755)) - dstPath := filepath.Join(dstDir, "skupper-west.service") - assert.Assert(t, os.WriteFile(dstPath, content, 0644)) - info, err := os.Stat(dstPath) - assert.Assert(t, err) - modBefore := info.ModTime() - - e, _ := newTestEnabler(t, 1000) - e.enableService(srcPath) - - info, err = os.Stat(dstPath) - assert.Assert(t, err) - assert.Equal(t, modBefore, info.ModTime(), "file should not have been rewritten") -} - -func TestEnableService_MissingSource(t *testing.T) { - configDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", configDir) - - e, calls := newTestEnabler(t, 1000) - e.enableService("/nonexistent/scripts/skupper-west.service") - - assert.Equal(t, 0, len(*calls), "expected no systemctl calls for missing source") -} - -func TestOnCreate_CallsEnableService(t *testing.T) { - configDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", configDir) - - scriptsDir := filepath.Join(t.TempDir(), "namespaces", "east", string(api.ScriptsPath)) - assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) - srcPath := filepath.Join(scriptsDir, "skupper-east.service") - assert.Assert(t, os.WriteFile(srcPath, []byte("[Unit]\n"), 0644)) - - e, calls := newTestEnabler(t, 1000) - e.OnCreate(srcPath) - - dstPath := filepath.Join(configDir, "systemd", "user", "skupper-east.service") - _, err := os.ReadFile(dstPath) - assert.Assert(t, err) - assert.Assert(t, len(*calls) >= 1) -} - -func TestOnUpdate_CallsEnableService(t *testing.T) { - configDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", configDir) - - scriptsDir := filepath.Join(t.TempDir(), "namespaces", "east", string(api.ScriptsPath)) - assert.Assert(t, os.MkdirAll(scriptsDir, 0755)) - srcPath := filepath.Join(scriptsDir, "skupper-east.service") - assert.Assert(t, os.WriteFile(srcPath, []byte("[Unit]\n"), 0644)) - - e, calls := newTestEnabler(t, 1000) - e.OnUpdate(srcPath) - - dstPath := filepath.Join(configDir, "systemd", "user", "skupper-east.service") - _, err := os.ReadFile(dstPath) - assert.Assert(t, err) - assert.Assert(t, len(*calls) >= 1) -} - -func TestOnRemove_DisablesAndDeletesUnit(t *testing.T) { - configDir := t.TempDir() - t.Setenv("XDG_CONFIG_HOME", configDir) - - unitDir := filepath.Join(configDir, "systemd", "user") - assert.Assert(t, os.MkdirAll(unitDir, 0755)) - dstPath := filepath.Join(unitDir, "skupper-west.service") - assert.Assert(t, os.WriteFile(dstPath, []byte("[Unit]\n"), 0644)) - - e, calls := newTestEnabler(t, 1000) - e.OnRemove(dstPath) - - _, err := os.Stat(dstPath) - assert.Assert(t, os.IsNotExist(err), "expected unit file to be removed") - - joined := make([]string, len(*calls)) - for i, c := range *calls { - joined[i] = strings.Join(c, " ") - } - all := strings.Join(joined, " | ") - assert.Assert(t, strings.Contains(all, "disable"), "expected disable call, got: %s", all) - assert.Assert(t, strings.Contains(all, "daemon-reload"), "expected daemon-reload call, got: %s", all) -} - -func TestNamespacesHandlerFilter(t *testing.T) { - namespacesDir := t.TempDir() - - subDir := filepath.Join(namespacesDir, "west") - assert.Assert(t, os.MkdirAll(subDir, 0755)) - nestedDir := filepath.Join(subDir, "nested") - assert.Assert(t, os.MkdirAll(nestedDir, 0755)) - filePath := filepath.Join(namespacesDir, "somefile") - assert.Assert(t, os.WriteFile(filePath, []byte{}, 0644)) - - h := &NamespaceScriptHandler{NamespacesDir: namespacesDir} - - assert.Equal(t, true, h.Filter(subDir), "direct subdir should pass") - assert.Equal(t, false, h.Filter(nestedDir), "nested dir should be rejected") - assert.Equal(t, false, h.Filter(filePath), "file should be rejected") - assert.Equal(t, false, h.Filter(namespacesDir), "the namespaces dir itself should be rejected") -} - -func newTestEnabler(t *testing.T, uid int) (*ServiceEnabler, *[][]string) { - t.Helper() - var mu sync.Mutex - var calls [][]string - e := &ServiceEnabler{ - uid: uid, - command: func(name string, args ...string) *exec.Cmd { - mu.Lock() - calls = append(calls, append([]string{name}, args...)) - mu.Unlock() - return exec.Command("true") - }, - } - return e, &calls -} From ebe29b8a3c55036860966c679f484e4187ca5900 Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Fri, 21 Aug 2026 23:24:20 +0200 Subject: [PATCH 7/8] check if the enabler was installed before removing it --- .../site_service_enabler_installer.go | 19 ++++++---- .../site_service_enabler_installer_test.go | 37 +++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer.go b/internal/nonkube/bootstrap/site_service_enabler_installer.go index 67af082e5..d44a36ff3 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_installer.go +++ b/internal/nonkube/bootstrap/site_service_enabler_installer.go @@ -87,14 +87,17 @@ func (s *SiteServiceEnablerInstaller) Install() error { } func (s *SiteServiceEnablerInstaller) Remove() error { - if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil { - return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err) - } - if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil { - return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err) - } - if err := os.Remove(s.unitPath(siteServiceEnablerServiceFile)); err != nil && !os.IsNotExist(err) { - return fmt.Errorf("failed to remove unit file: %w", err) + unitFile := s.unitPath(siteServiceEnablerServiceFile) + if _, err := os.Stat(unitFile); err == nil { + if err := s.systemctl("stop", siteServiceEnablerServiceFile); err != nil { + return fmt.Errorf("failed to stop %s: %w", siteServiceEnablerServiceFile, err) + } + if err := s.systemctl("disable", siteServiceEnablerServiceFile); err != nil { + return fmt.Errorf("failed to disable %s: %w", siteServiceEnablerServiceFile, err) + } + if err := os.Remove(unitFile); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove unit file: %w", err) + } } if err := os.Remove(path.Join(s.scriptDir, siteServiceEnablerWrapperScript)); err != nil && !os.IsNotExist(err) { return fmt.Errorf("failed to remove wrapper script: %w", err) diff --git a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go index cc61185f6..67bc8e6f4 100644 --- a/internal/nonkube/bootstrap/site_service_enabler_installer_test.go +++ b/internal/nonkube/bootstrap/site_service_enabler_installer_test.go @@ -284,6 +284,11 @@ func TestRemove_SystemctlCallOrder(t *testing.T) { var calls [][]string s := newTestInstaller(t, 0, &calls) + // Pre-create the unit file so stop/disable are attempted. + svcPath := s.unitPath(siteServiceEnablerServiceFile) + _ = os.MkdirAll(filepath.Dir(svcPath), 0755) + _ = os.WriteFile(svcPath, []byte("[Unit]"), 0644) + err := s.Remove() assert.NilError(t, err) @@ -319,6 +324,11 @@ func TestRemove_NonRoot_SystemctlUsesUserFlag(t *testing.T) { var calls [][]string s := newTestInstaller(t, 1000, &calls) + // Pre-create the unit file so stop/disable are attempted. + svcPath := s.unitPath(siteServiceEnablerServiceFile) + _ = os.MkdirAll(filepath.Dir(svcPath), 0755) + _ = os.WriteFile(svcPath, []byte("[Unit]"), 0644) + err := s.Remove() assert.NilError(t, err) @@ -332,6 +342,23 @@ func TestRemove_ToleratesMissingFiles(t *testing.T) { s := newTestInstaller(t, 0, &calls) err := s.Remove() assert.NilError(t, err) + // Unit absent: only daemon-reload is called. + assert.Equal(t, len(calls), 1) + assert.DeepEqual(t, calls[0], []string{"systemctl", "daemon-reload"}) +} + +func TestRemove_SkipsStopDisableWhenUnitAbsent(t *testing.T) { + var calls [][]string + s := newTestInstaller(t, 0, &calls) + + err := s.Remove() + assert.NilError(t, err) + + // stop and disable must not have been called; only daemon-reload. + for _, c := range calls { + assert.Assert(t, c[1] != "stop", "stop must not be called when unit is absent") + assert.Assert(t, c[1] != "disable", "disable must not be called when unit is absent") + } } func TestRemove_FailsOnStopError(t *testing.T) { @@ -347,6 +374,11 @@ func TestRemove_FailsOnStopError(t *testing.T) { return exec.CommandContext(context.Background(), "true") } + // Pre-create the unit file so the stop branch is entered. + svcPath := s.unitPath(siteServiceEnablerServiceFile) + _ = os.MkdirAll(filepath.Dir(svcPath), 0755) + _ = os.WriteFile(svcPath, []byte("[Unit]"), 0644) + err := s.Remove() assert.Assert(t, err != nil) assert.Assert(t, strings.Contains(err.Error(), "failed to stop")) @@ -367,6 +399,11 @@ func TestRemove_FailsOnDisableError(t *testing.T) { return exec.CommandContext(context.Background(), "true") } + // Pre-create the unit file so the disable branch is entered. + svcPath := s.unitPath(siteServiceEnablerServiceFile) + _ = os.MkdirAll(filepath.Dir(svcPath), 0755) + _ = os.WriteFile(svcPath, []byte("[Unit]"), 0644) + err := s.Remove() assert.Assert(t, err != nil) assert.Assert(t, strings.Contains(err.Error(), "failed to disable")) From 3aa401b11e8662e332bbc195496a052477c3b892 Mon Sep 17 00:00:00 2001 From: Noe Luaces Date: Mon, 31 Aug 2026 21:38:26 +0200 Subject: [PATCH 8/8] return container status when installing/uninstalling system controller --- internal/nonkube/bootstrap/install.go | 26 ++++++++++----------- internal/nonkube/bootstrap/uninstall.go | 12 +++++----- internal/nonkube/client/compat/container.go | 2 ++ pkg/container/client.go | 1 + 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/internal/nonkube/bootstrap/install.go b/internal/nonkube/bootstrap/install.go index 676f4152d..f0b0e1e52 100644 --- a/internal/nonkube/bootstrap/install.go +++ b/internal/nonkube/bootstrap/install.go @@ -52,10 +52,10 @@ func Install(platform string, reloadType string) error { containerName := fmt.Sprintf("%s-skupper-controller", config.username) - isContainerAlreadyRunningInPodman := IsContainerRunning(containerName, types.PlatformPodman) + foundInPodman, podmanState := FindContainer(containerName, types.PlatformPodman) - if isContainerAlreadyRunningInPodman { - fmt.Printf("Warning: The system controller container %q is already running in Podman.\n", containerName) + if foundInPodman { + fmt.Printf("Warning: The system controller container %q is already present in Podman (state: %s).\n", containerName, podmanState) if reloadType == types.SystemReloadTypeAuto { enabler := newSiteServiceEnablerInstaller() if err = enabler.Install(); err != nil { @@ -65,10 +65,10 @@ func Install(platform string, reloadType string) error { return nil } - isContainerAlreadyRunningInDocker := IsContainerRunning(containerName, types.PlatformDocker) + foundInDocker, dockerState := FindContainer(containerName, types.PlatformDocker) - if isContainerAlreadyRunningInDocker { - fmt.Printf("Warning: The system controller container %q is already running in Docker.\n", containerName) + if foundInDocker { + fmt.Printf("Warning: The system controller container %q is already present in Docker (state: %s).\n", containerName, dockerState) if reloadType == types.SystemReloadTypeAuto { enabler := newSiteServiceEnablerInstaller() if err = enabler.Install(); err != nil { @@ -281,7 +281,7 @@ func createSystemdService(container container.Container, platform string) error return nil } -func IsContainerRunning(containerName string, platform types.Platform) bool { +func FindContainer(containerName string, platform types.Platform) (bool, string) { endpoint := fmt.Sprintf("unix://%s/podman/podman.sock", api.GetRuntimeDir()) if platform == types.PlatformDocker { @@ -290,19 +290,19 @@ func IsContainerRunning(containerName string, platform types.Platform) bool { cli, err := internalclient.NewCompatClient(endpoint, "") if err != nil { - return false + return false, "" } containers, err := cli.ContainerList() if err != nil { - return false + return false, "" } - for _, container := range containers { - if container.Name == containerName { - return container.Running + for _, c := range containers { + if c.Name == containerName { + return true, c.State } } - return false + return false, "" } diff --git a/internal/nonkube/bootstrap/uninstall.go b/internal/nonkube/bootstrap/uninstall.go index 94b45fb14..86f394685 100644 --- a/internal/nonkube/bootstrap/uninstall.go +++ b/internal/nonkube/bootstrap/uninstall.go @@ -26,17 +26,17 @@ func Uninstall(platform string) error { containerName := fmt.Sprintf("%s-skupper-controller", currentUser.Username) - isContainerAlreadyRunningInPodman := IsContainerRunning(containerName, types.PlatformPodman) + foundInPodman, podmanState := FindContainer(containerName, types.PlatformPodman) - if isContainerAlreadyRunningInPodman && platform == "docker" { - fmt.Printf("Warning: The system controller container %q is already running in Podman but the selected platform is Docker.\n", containerName) + if foundInPodman && platform == "docker" { + fmt.Printf("Warning: The system controller container %q is already present in Podman (state: %s) but the selected platform is Docker.\n", containerName, podmanState) return nil } - isContainerAlreadyRunningInDocker := IsContainerRunning(containerName, types.PlatformDocker) + foundInDocker, dockerState := FindContainer(containerName, types.PlatformDocker) - if isContainerAlreadyRunningInDocker && platform == "podman" { - fmt.Printf("Warning: The system controller container %q is already running in Docker but the selected platform is Podman.\n", containerName) + if foundInDocker && platform == "podman" { + fmt.Printf("Warning: The system controller container %q is already present in Docker (state: %s) but the selected platform is Podman.\n", containerName, dockerState) return nil } diff --git a/internal/nonkube/client/compat/container.go b/internal/nonkube/client/compat/container.go index b233817cd..e6949531d 100644 --- a/internal/nonkube/client/compat/container.go +++ b/internal/nonkube/client/compat/container.go @@ -43,6 +43,7 @@ func (c *CompatClient) ContainerList() ([]*container.Container, error) { FileMounts: make([]container.FileMount, 0), Ports: make([]container.Port, 0), Command: []string{cMap["Command"].(string)}, + State: cMap["State"].(string), Running: cMap["State"].(string) == "running", CreatedAt: time.Unix(jsonNumberAsInt(cMap["Created"]), 0), } @@ -216,6 +217,7 @@ func FromInspectContainer(c *containers_compat.ContainerInspectOKBody) *containe // State info if c.State != nil { ct.Running = c.State.Running + ct.State = c.State.Status startedAt, _ := time.Parse(time.RFC3339, c.State.StartedAt) exitedAt, _ := time.Parse(time.RFC3339, c.State.FinishedAt) ct.StartedAt = startedAt diff --git a/pkg/container/client.go b/pkg/container/client.go index 5ff9fb3be..e105ff26d 100644 --- a/pkg/container/client.go +++ b/pkg/container/client.go @@ -77,6 +77,7 @@ type Container struct { MaxMemoryBytes int64 RestartCount int Running bool + State string CreatedAt time.Time StartedAt time.Time ExitedAt time.Time