From 88260db4a9186a6eafdb9951716172d7f5e179a8 Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Mon, 22 Jan 2024 06:58:48 -0400 Subject: [PATCH 1/3] Allow to create broker daemon with custom name The daemon used to rely on the executable name to define its internal name. This is far from ideal when dealing with tests (especially in parallel). Now, the New() function takes a name argument that allows setting a custom name for the daemon. This is very useful for tests. --- cmd/oidc-broker/daemon/config.go | 2 +- cmd/oidc-broker/daemon/daemon.go | 22 ++++++++++------------ cmd/oidc-broker/daemon/daemon_test.go | 15 +++++---------- cmd/oidc-broker/daemon/export_test.go | 2 +- cmd/oidc-broker/daemon/version.go | 6 +++--- cmd/oidc-broker/main.go | 3 ++- 6 files changed, 22 insertions(+), 28 deletions(-) diff --git a/cmd/oidc-broker/daemon/config.go b/cmd/oidc-broker/daemon/config.go index 2b0be1c9..09d6c932 100644 --- a/cmd/oidc-broker/daemon/config.go +++ b/cmd/oidc-broker/daemon/config.go @@ -37,7 +37,7 @@ func initViperConfig(name string, cmd *cobra.Command, vip *viper.Viper) (err err vip.AddConfigPath("./") vip.AddConfigPath("$HOME/") vip.AddConfigPath("$SNAP_DATA/") - vip.AddConfigPath(filepath.Join("/etc", cmdName)) + vip.AddConfigPath(filepath.Join("/etc", name)) // Add the executable path to the config search path. if binPath, err := os.Executable(); err != nil { slog.Warn(fmt.Sprintf("Failed to get current executable path, not adding it as a config dir: %v", err)) diff --git a/cmd/oidc-broker/daemon/daemon.go b/cmd/oidc-broker/daemon/daemon.go index a5cc8754..25b6b822 100644 --- a/cmd/oidc-broker/daemon/daemon.go +++ b/cmd/oidc-broker/daemon/daemon.go @@ -17,9 +17,6 @@ import ( "github.com/ubuntu/oidc-broker/internal/daemon" ) -// cmdName is the binary name for the agent. -var cmdName = filepath.Base(os.Args[0]) - // App encapsulate commands and options of the daemon, which can be controlled by env variables and config files. type App struct { rootCmd cobra.Command @@ -27,6 +24,7 @@ type App struct { config daemonConfig daemon *daemon.Daemon + name string ready chan struct{} } @@ -44,31 +42,31 @@ type daemonConfig struct { } // New registers commands and return a new App. -func New() *App { - a := App{ready: make(chan struct{})} +func New(name string) *App { + a := App{ready: make(chan struct{}), name: name} a.rootCmd = cobra.Command{ - Use: fmt.Sprintf("%s COMMAND", cmdName), - Short: fmt.Sprintf("%s authentication broker", cmdName), - Long: fmt.Sprintf("Authentication daemon %s to communicate with our authentication daemon.", cmdName), + Use: fmt.Sprintf("%s COMMAND", name), + Short: fmt.Sprintf("%s authentication broker", name), + Long: fmt.Sprintf("Authentication daemon %s to communicate with our authentication daemon.", name), Args: cobra.NoArgs, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { // Command parsing has been successful. Returns to not print usage anymore. a.rootCmd.SilenceUsage = true // Set config defaults - systemCache := filepath.Join("/var", "lib", cmdName) + systemCache := filepath.Join("/var", "lib", name) if snapData := os.Getenv("SNAP_DATA"); snapData != "" { systemCache = filepath.Join(snapData, "cache") } a.config = daemonConfig{ Paths: systemPaths{ - BrokersConf: filepath.Join(consts.DefaultBrokersConfPath, cmdName), + BrokersConf: filepath.Join(consts.DefaultBrokersConfPath, name), Cache: systemCache, }, } // Install and unmarshall configuration - if err := initViperConfig(cmdName, &a.rootCmd, a.viper); err != nil { + if err := initViperConfig(name, &a.rootCmd, a.viper); err != nil { return err } if err := a.viper.Unmarshal(&a.config); err != nil { @@ -108,7 +106,7 @@ func (a *App) serve(config daemonConfig) error { return fmt.Errorf("error initializing users cache directory at %q: %v", config.Paths.Cache, err) } - s, err := brokerservice.New(ctx, cmdName) + s, err := brokerservice.New(ctx, a.name) if err != nil { close(a.ready) return err diff --git a/cmd/oidc-broker/daemon/daemon_test.go b/cmd/oidc-broker/daemon/daemon_test.go index c2a68c11..07ceb9c2 100644 --- a/cmd/oidc-broker/daemon/daemon_test.go +++ b/cmd/oidc-broker/daemon/daemon_test.go @@ -48,9 +48,7 @@ func TestVersion(t *testing.T) { fields := strings.Fields(out) require.Len(t, fields, 2, "wrong number of fields in version: %s", out) - want := "authd" - - require.Equal(t, want, fields[0], "Wrong executable name") + require.Equal(t, t.Name(), fields[0], "Wrong executable name") require.Equal(t, consts.Version, fields[1], "Wrong version") } @@ -224,8 +222,7 @@ func TestAutoDetectConfig(t *testing.T) { // Remove configuration next binary for other tests to not pick it up. defer os.Remove(configNextToBinaryPath) - a := daemon.New() - + a := daemon.New(t.Name()) wg := sync.WaitGroup{} wg.Add(1) go func() { @@ -248,21 +245,19 @@ func TestNoConfigSetDefaults(t *testing.T) { // TODO t.Fail() - a := daemon.New() - // Use version to still run preExec to load no config but without running server + a := daemon.New(t.Name()) // Use version to still run preExec to load no config but without running server a.SetArgs("version") err := a.Run() require.NoError(t, err, "Run should not return an error") require.Equal(t, 0, a.Config().Verbosity, "Default Verbosity") - require.Equal(t, consts.DefaultBrokersConfPath, a.Config().Paths.BrokersConf, "Default brokers configuration path") + require.Equal(t, filepath.Join(consts.DefaultBrokersConfPath, t.Name()), a.Config().Paths.BrokersConf, "Default brokers configuration path") //require.Equal(t, consts.DefaultCacheDir, a.Config().Paths.Cache, "Default cache directory") } func TestBadConfigReturnsError(t *testing.T) { - a := daemon.New() - // Use version to still run preExec to load no config but without running server + a := daemon.New(t.Name()) // Use version to still run preExec to load no config but without running server a.SetArgs("version", "--config", "/does/not/exist.yaml") err := a.Run() diff --git a/cmd/oidc-broker/daemon/export_test.go b/cmd/oidc-broker/daemon/export_test.go index a2f67ef9..27abc1ab 100644 --- a/cmd/oidc-broker/daemon/export_test.go +++ b/cmd/oidc-broker/daemon/export_test.go @@ -21,7 +21,7 @@ func NewForTests(t *testing.T, conf *DaemonConfig, args ...string) *App { argsWithConf := []string{"--config", p} argsWithConf = append(argsWithConf, args...) - a := New() + a := New(t.Name()) a.rootCmd.SetArgs(argsWithConf) return a } diff --git a/cmd/oidc-broker/daemon/version.go b/cmd/oidc-broker/daemon/version.go index 5136a17b..b8145019 100644 --- a/cmd/oidc-broker/daemon/version.go +++ b/cmd/oidc-broker/daemon/version.go @@ -12,13 +12,13 @@ func (a *App) installVersion() { Use: "version", Short:/*i18n.G(*/ "Returns version of daemon and exits", /*)*/ Args: cobra.NoArgs, - RunE: func(cmd *cobra.Command, args []string) error { return getVersion() }, + RunE: func(cmd *cobra.Command, args []string) error { return a.getVersion() }, } a.rootCmd.AddCommand(cmd) } // getVersion returns the current service version. -func getVersion() (err error) { - fmt.Printf( /*i18n.G(*/ "%s\t%s" /*)*/ +"\n", cmdName, consts.Version) +func (a *App) getVersion() (err error) { + fmt.Printf( /*i18n.G(*/ "%s\t%s" /*)*/ +"\n", a.name, consts.Version) return nil } diff --git a/cmd/oidc-broker/main.go b/cmd/oidc-broker/main.go index 212104dc..f630b1ab 100644 --- a/cmd/oidc-broker/main.go +++ b/cmd/oidc-broker/main.go @@ -5,6 +5,7 @@ import ( "log/slog" "os" "os/signal" + "path/filepath" "sync" "syscall" @@ -20,7 +21,7 @@ import ( func main() { i18n.InitI18nDomain(consts.TEXTDOMAIN, po.Files) - a := daemon.New() + a := daemon.New(filepath.Base(os.Args[0])) os.Exit(run(a)) } From f2222e49adc9c9b8cc4b7463def43ac970a6ad94 Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Mon, 22 Jan 2024 07:01:41 -0400 Subject: [PATCH 2/3] Adjust naming for systemPaths This is a broker daemon, it should only know about its configuration and not about the general broker configuration directory --- cmd/oidc-broker/daemon/daemon.go | 8 ++++---- cmd/oidc-broker/daemon/daemon_test.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/oidc-broker/daemon/daemon.go b/cmd/oidc-broker/daemon/daemon.go index 25b6b822..6a6d619f 100644 --- a/cmd/oidc-broker/daemon/daemon.go +++ b/cmd/oidc-broker/daemon/daemon.go @@ -31,8 +31,8 @@ type App struct { // only overriable for tests. type systemPaths struct { - BrokersConf string - Cache string + BrokerConf string + Cache string } // daemonConfig defines configuration parameters of the daemon. @@ -60,8 +60,8 @@ func New(name string) *App { } a.config = daemonConfig{ Paths: systemPaths{ - BrokersConf: filepath.Join(consts.DefaultBrokersConfPath, name), - Cache: systemCache, + BrokerConf: filepath.Join(consts.DefaultBrokersConfPath, name), + Cache: systemCache, }, } diff --git a/cmd/oidc-broker/daemon/daemon_test.go b/cmd/oidc-broker/daemon/daemon_test.go index 07ceb9c2..7f39f21d 100644 --- a/cmd/oidc-broker/daemon/daemon_test.go +++ b/cmd/oidc-broker/daemon/daemon_test.go @@ -252,7 +252,7 @@ func TestNoConfigSetDefaults(t *testing.T) { require.NoError(t, err, "Run should not return an error") require.Equal(t, 0, a.Config().Verbosity, "Default Verbosity") - require.Equal(t, filepath.Join(consts.DefaultBrokersConfPath, t.Name()), a.Config().Paths.BrokersConf, "Default brokers configuration path") + require.Equal(t, filepath.Join(consts.DefaultBrokersConfPath, t.Name()), a.Config().Paths.BrokerConf, "Default broker configuration path") //require.Equal(t, consts.DefaultCacheDir, a.Config().Paths.Cache, "Default cache directory") } From 17f960241b3ecde411938577c22e0d8e1e991022 Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Mon, 22 Jan 2024 07:17:08 -0400 Subject: [PATCH 3/3] Prevent s.Stop function from panicking if called many times --- internal/brokerservice/brokerservice.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/brokerservice/brokerservice.go b/internal/brokerservice/brokerservice.go index 177c2bfa..5f311ce8 100644 --- a/internal/brokerservice/brokerservice.go +++ b/internal/brokerservice/brokerservice.go @@ -107,7 +107,12 @@ func (s *Service) Serve() error { // Stop stop the service and do all the necessary cleanup operation. func (s *Service) Stop() error { - close(s.serve) - s.disconnect() + // Check if already stopped. + select { + case <-s.serve: + default: + close(s.serve) + s.disconnect() + } return nil }