From 5d639ab2002d5d4a7e92c315c4df43137d8ad40a Mon Sep 17 00:00:00 2001 From: James Houlahan Date: Mon, 15 Aug 2022 15:01:55 +0200 Subject: [PATCH] refactor: Improve data ownership interface --- .../gluon_bench/gluon_benchmarks/sync.go | 8 +- .../imap_benchmarks/server/local.go | 5 +- builder.go | 69 ++++++++++++ demo/demo.go | 21 ++-- internal/backend/backend.go | 39 +++++-- internal/backend/db.go | 17 +-- option.go | 42 ++++---- server.go | 68 +++--------- store/store.go | 4 +- tests/deletion_pool_test.go | 12 +-- tests/server_test.go | 101 ++++-------------- tests/store_test.go | 2 +- 12 files changed, 189 insertions(+), 199 deletions(-) create mode 100644 builder.go diff --git a/benchmarks/gluon_bench/gluon_benchmarks/sync.go b/benchmarks/gluon_bench/gluon_benchmarks/sync.go index b8689922..8605ced6 100644 --- a/benchmarks/gluon_bench/gluon_benchmarks/sync.go +++ b/benchmarks/gluon_bench/gluon_benchmarks/sync.go @@ -42,12 +42,12 @@ func (s *Sync) Setup(ctx context.Context, benchmarkDir string) error { loggerIn := logrus.StandardLogger().WriterLevel(logrus.TraceLevel) loggerOut := logrus.StandardLogger().WriterLevel(logrus.TraceLevel) - opts := []gluon.Option{gluon.WithLogger(loggerIn, loggerOut), - gluon.WithDataPath(benchmarkDir), + opts := []gluon.Option{ + gluon.WithDataDir(benchmarkDir), + gluon.WithLogger(loggerIn, loggerOut), } - server, err := gluon.New(benchmarkDir, opts...) - + server, err := gluon.New(opts...) if err != nil { return err } diff --git a/benchmarks/gluon_bench/imap_benchmarks/server/local.go b/benchmarks/gluon_bench/imap_benchmarks/server/local.go index f8d912a9..b0a5d7f6 100644 --- a/benchmarks/gluon_bench/imap_benchmarks/server/local.go +++ b/benchmarks/gluon_bench/imap_benchmarks/server/local.go @@ -38,10 +38,9 @@ func (*LocalServerBuilder) New(ctx context.Context, serverPath string, profiler opts = append(opts, gluon.WithLogger(loggerIn, loggerOut)) opts = append(opts, gluon.WithCmdProfiler(profiler)) - opts = append(opts, gluon.WithDataPath(serverPath)) - - server, err := gluon.New(serverPath, opts...) + opts = append(opts, gluon.WithDataDir(serverPath)) + server, err := gluon.New(opts...) if err != nil { return nil, err } diff --git a/builder.go b/builder.go new file mode 100644 index 00000000..b0e9f669 --- /dev/null +++ b/builder.go @@ -0,0 +1,69 @@ +package gluon + +import ( + "crypto/tls" + "io" + "net" + "os" + "path/filepath" + + "github.com/ProtonMail/gluon/events" + "github.com/ProtonMail/gluon/internal" + "github.com/ProtonMail/gluon/internal/backend" + "github.com/ProtonMail/gluon/internal/session" + "github.com/ProtonMail/gluon/profiling" + "github.com/ProtonMail/gluon/store" +) + +type serverBuilder struct { + dir string + delim string + tlsConfig *tls.Config + inLogger io.Writer + outLogger io.Writer + versionInfo internal.VersionInfo + cmdExecProfBuilder profiling.CmdProfilerBuilder + storeBuilder store.Builder +} + +func newBuilder() (*serverBuilder, error) { + return &serverBuilder{ + delim: "/", + cmdExecProfBuilder: &profiling.NullCmdExecProfilerBuilder{}, + storeBuilder: &store.OnDiskStoreBuilder{}, + }, nil +} + +func (builder *serverBuilder) build() (*Server, error) { + if builder.dir == "" { + dir, err := os.MkdirTemp("", "gluon-*") + if err != nil { + return nil, err + } + + builder.dir = dir + } + + if err := os.MkdirAll(builder.dir, 0o700); err != nil { + return nil, err + } + + backend, err := backend.New(filepath.Join(builder.dir, "backend"), builder.storeBuilder, builder.delim) + if err != nil { + return nil, err + } + + return &Server{ + dir: builder.dir, + backend: backend, + listeners: make(map[net.Listener]struct{}), + sessions: make(map[int]*session.Session), + inLogger: builder.inLogger, + outLogger: builder.outLogger, + tlsConfig: builder.tlsConfig, + watchers: make(map[chan events.Event]struct{}), + storeBuilder: builder.storeBuilder, + cmdExecProfBuilder: builder.cmdExecProfBuilder, + versionInfo: builder.versionInfo, + }, nil +} diff --git a/demo/demo.go b/demo/demo.go index ba056135..b6f1ca24 100644 --- a/demo/demo.go +++ b/demo/demo.go @@ -20,6 +20,8 @@ var blockProfileFlag = flag.Bool("profile-lock", false, "Enable lock profiling." var profilePathFlag = flag.String("profile-path", "", "Path where to write profile data.") func main() { + ctx := context.Background() + flag.Parse() if *cpuProfileFlag { @@ -41,23 +43,16 @@ func main() { logrus.SetLevel(level) } - loggerIn := logrus.StandardLogger().WriterLevel(logrus.TraceLevel) - loggerOut := logrus.StandardLogger().WriterLevel(logrus.TraceLevel) - - ctx := context.Background() - server, err := gluon.New(temp(), gluon.WithLogger( - loggerIn, - loggerOut, - ), - gluon.WithDataPath(temp()), - ) - - defer server.Close(ctx) - + server, err := gluon.New(gluon.WithLogger( + logrus.StandardLogger().WriterLevel(logrus.TraceLevel), + logrus.StandardLogger().WriterLevel(logrus.TraceLevel), + )) if err != nil { logrus.WithError(err).Fatal("Failed to create server") } + defer server.Close(ctx) + if err := addUser(ctx, server, []string{"user1@example.com", "alias1@example.com"}, "password1"); err != nil { logrus.WithError(err).Fatal("Failed to add user") } diff --git a/internal/backend/backend.go b/internal/backend/backend.go index 495c62e0..e43a49b7 100644 --- a/internal/backend/backend.go +++ b/internal/backend/backend.go @@ -3,6 +3,7 @@ package backend import ( "context" "fmt" + "path/filepath" "sync" "github.com/ProtonMail/gluon/connector" @@ -13,6 +14,9 @@ import ( ) type Backend struct { + // dir is the directory in which backend files should be stored. + dir string + // remote manages operations to be performed on the API. remote *remote.Manager @@ -22,18 +26,23 @@ type Backend struct { // users holds all registered backend users. users map[string]*user usersLock sync.Mutex + + // storeBuilder builds stores for the backend users. + storeBuilder store.Builder } -func New(dir string) (*Backend, error) { - manager, err := remote.New(dir) +func New(dir string, storeBuilder store.Builder, delim string) (*Backend, error) { + manager, err := remote.New(filepath.Join(dir, "remote")) if err != nil { return nil, err } return &Backend{ - remote: manager, - delim: "/", - users: make(map[string]*user), + dir: dir, + remote: manager, + storeBuilder: storeBuilder, + delim: delim, + users: make(map[string]*user), }, nil } @@ -41,18 +50,28 @@ func (b *Backend) NewUserID() string { return uuid.NewString() } -func (b *Backend) SetDelimiter(delim string) { - b.delim = delim -} - func (b *Backend) GetDelimiter() string { return b.delim } -func (b *Backend) AddUser(ctx context.Context, userID string, conn connector.Connector, store store.Store, db *DB) error { +func (b *Backend) AddUser(ctx context.Context, userID string, conn connector.Connector, passphrase []byte) error { b.usersLock.Lock() defer b.usersLock.Unlock() + store, err := b.storeBuilder.New(filepath.Join(b.dir, "store"), userID, passphrase) + if err != nil { + return err + } + + db, err := b.newDB(userID) + if err != nil { + if err := store.Close(); err != nil { + logrus.WithError(err).Error("Failed to close storage") + } + + return err + } + remote, err := b.remote.AddUser(ctx, userID, conn) if err != nil { return err diff --git a/internal/backend/db.go b/internal/backend/db.go index a31a2301..766283f4 100644 --- a/internal/backend/db.go +++ b/internal/backend/db.go @@ -3,6 +3,7 @@ package backend import ( "context" "fmt" + "os" "path/filepath" "sync" @@ -67,17 +68,21 @@ func (d *DB) Close() error { return d.db.Close() } -func getDatabasePath(dataPath, userID string) string { - return fmt.Sprintf("file:%v?cache=shared&_fk=1", filepath.Join(dataPath, fmt.Sprintf("%v.db", userID))) -} +func (b *Backend) newDB(userID string) (*DB, error) { + dir := filepath.Join(b.dir, "db") -func NewDB(dataPath, userID string) (*DB, error) { - dbPath := getDatabasePath(dataPath, userID) - client, err := ent.Open(dialect.SQLite, dbPath) + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, err + } + client, err := ent.Open(dialect.SQLite, getDatabasePath(dir, userID)) if err != nil { return nil, err } return &DB{db: client}, nil } + +func getDatabasePath(dir, userID string) string { + return fmt.Sprintf("file:%v?cache=shared&_fk=1", filepath.Join(dir, fmt.Sprintf("%v.db", userID))) +} diff --git a/option.go b/option.go index 1b9c4f12..99755903 100644 --- a/option.go +++ b/option.go @@ -11,7 +11,7 @@ import ( // Option represents a type that can be used to configure the server. type Option interface { - config(server *Server) + config(*serverBuilder) } // WithDelimiter instructs the server to use the given path delimiter instead of the default ('/'). @@ -25,8 +25,8 @@ type withDelimiter struct { delimiter string } -func (opt withDelimiter) config(server *Server) { - server.backend.SetDelimiter(opt.delimiter) +func (opt withDelimiter) config(builder *serverBuilder) { + builder.delim = opt.delimiter } // WithTLS instructs the server to use the given TLS config. @@ -40,8 +40,8 @@ type withTLS struct { cfg *tls.Config } -func (opt withTLS) config(server *Server) { - server.tlsConfig = opt.cfg +func (opt withTLS) config(builder *serverBuilder) { + builder.tlsConfig = opt.cfg } // WithLogger instructs the server to write incoming and outgoing IMAP communication to the given io.Writers. @@ -56,17 +56,17 @@ type withLogger struct { in, out io.Writer } -func (opt withLogger) config(server *Server) { - server.inLogger = opt.in - server.outLogger = opt.out +func (opt withLogger) config(builder *serverBuilder) { + builder.inLogger = opt.in + builder.outLogger = opt.out } type withVersionInfo struct { versionInfo internal.VersionInfo } -func (vi *withVersionInfo) config(server *Server) { - server.versionInfo = vi.versionInfo +func (vi *withVersionInfo) config(builder *serverBuilder) { + builder.versionInfo = vi.versionInfo } func WithVersionInfo(vmajor, vminor, vpatch int, name, vendor, supportURL string) Option { @@ -88,8 +88,8 @@ type withCmdExecProfiler struct { builder profiling.CmdProfilerBuilder } -func (c *withCmdExecProfiler) config(server *Server) { - server.cmdExecProfBuilder = c.builder +func (c *withCmdExecProfiler) config(builder *serverBuilder) { + builder.cmdExecProfBuilder = c.builder } // WithCmdProfiler allows a specific CmdProfilerBuilder to be set for the server's execution. @@ -98,25 +98,25 @@ func WithCmdProfiler(builder profiling.CmdProfilerBuilder) Option { } type withStoreBuilder struct { - builder store.StoreBuilder + builder store.Builder } -func (w *withStoreBuilder) config(server *Server) { - server.storeBuilder = w.builder +func (w *withStoreBuilder) config(builder *serverBuilder) { + builder.storeBuilder = w.builder } -func WithStoreBuilder(builder store.StoreBuilder) Option { +func WithStoreBuilder(builder store.Builder) Option { return &withStoreBuilder{builder: builder} } -type withDataPath struct { +type withDataDir struct { path string } -func (w *withDataPath) config(server *Server) { - server.dataPath = w.path +func (w *withDataDir) config(builder *serverBuilder) { + builder.dir = w.path } -func WithDataPath(path string) Option { - return &withDataPath{path: path} +func WithDataDir(path string) Option { + return &withDataDir{path: path} } diff --git a/server.go b/server.go index 8d140856..7d35e633 100644 --- a/server.go +++ b/server.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "net" - "os" "path/filepath" "runtime/pprof" "strconv" @@ -27,6 +26,9 @@ import ( // Server is the gluon IMAP server. type Server struct { + // dir holds the path to all of Gluon's data. + dir string + // backend provides the server with access to the IMAP backend. backend *backend.Backend @@ -52,40 +54,28 @@ type Server struct { watchers map[chan events.Event]struct{} watchersLock sync.RWMutex - versionInfo internal.VersionInfo + // storeBuilder builds message stores. + storeBuilder store.Builder + + // cmdExecProfBuilder builds command profiling collectors. cmdExecProfBuilder profiling.CmdProfilerBuilder - storeBuilder store.StoreBuilder - dataPath string + // versionInfo holds info about the Gluon version. + versionInfo internal.VersionInfo } // New creates a new server with the given options. -// It stores data in the given directory. -func New(dir string, withOpt ...Option) (*Server, error) { - backend, err := backend.New(dir) +func New(withOpt ...Option) (*Server, error) { + builder, err := newBuilder() if err != nil { return nil, err } - server := &Server{ - backend: backend, - listeners: make(map[net.Listener]struct{}), - sessions: make(map[int]*session.Session), - watchers: make(map[chan events.Event]struct{}), - cmdExecProfBuilder: &profiling.NullCmdExecProfilerBuilder{}, - storeBuilder: &store.OnDiskStoreBuilder{}, - dataPath: os.TempDir(), - } - for _, opt := range withOpt { - opt.config(server) - } - - if err := os.MkdirAll(server.dataPath, 0o700); err != nil { - return nil, err + opt.config(builder) } - return server, nil + return builder.build() } // AddUser creates a new user and generates new unique ID for this user. If you have an existing userID, please use @@ -102,32 +92,8 @@ func (s *Server) AddUser(ctx context.Context, conn connector.Connector, encrypti // LoadUser loads an existing user's data from disk. This function can also be used to assign a custom userID to a mail // server user. -func (s *Server) LoadUser(ctx context.Context, conn connector.Connector, userID string, encryptionPassphrase []byte) error { - userPath, err := s.GetUserDataPath(userID) - if err != nil { - return err - } - - if err := os.MkdirAll(userPath, 0o700); err != nil { - return err - } - - store, err := s.storeBuilder.New(s.dataPath, userID, encryptionPassphrase) - if err != nil { - return err - } - - client, err := backend.NewDB(s.dataPath, userID) - - if err != nil { - if err := store.Close(); err != nil { - logrus.WithError(err).Error("Failed to close storage") - } - - return err - } - - if err := s.backend.AddUser(ctx, userID, conn, store, client); err != nil { +func (s *Server) LoadUser(ctx context.Context, conn connector.Connector, userID string, passphrase []byte) error { + if err := s.backend.AddUser(ctx, userID, conn, passphrase); err != nil { return err } @@ -228,7 +194,7 @@ func (s *Server) GetVersionInfo() internal.VersionInfo { } func (s *Server) GetDataPath() string { - return s.dataPath + return s.dir } func (s *Server) GetUserDataPath(userID string) (string, error) { @@ -236,7 +202,7 @@ func (s *Server) GetUserDataPath(userID string) (string, error) { return "", fmt.Errorf("not a valid user id") } - return filepath.Join(s.dataPath, userID), nil + return filepath.Join(s.dir, userID), nil } func (s *Server) addListener(l net.Listener) { diff --git a/store/store.go b/store/store.go index 3bd3d1d8..2410e7bc 100644 --- a/store/store.go +++ b/store/store.go @@ -8,6 +8,6 @@ type Store interface { Close() error } -type StoreBuilder interface { - New(directory, userID string, encryptionPassphrase []byte) (Store, error) +type Builder interface { + New(dir, userID string, passphrase []byte) (Store, error) } diff --git a/tests/deletion_pool_test.go b/tests/deletion_pool_test.go index 384b3bd8..53ee9446 100644 --- a/tests/deletion_pool_test.go +++ b/tests/deletion_pool_test.go @@ -390,16 +390,10 @@ func TestMessageErasedFromDB(t *testing.T) { }) } +// This test checks whether any messages from a previous server that are written to the db and are cleared +// on the next startup. We force the server to use the same directories and state to check for this. func TestMessageErasedFromDBOnStartup(t *testing.T) { - // This test checks whether any messages from a previous server that are written to the db and are cleared - // on the next startup. We force the server to use the same directories and state to check for this. - pathGenerator := newFixedPathGenerator( - t.TempDir(), - t.TempDir(), - t.TempDir(), - ) - - options := defaultServerOptions(t, withPathGenerator(pathGenerator)) + options := defaultServerOptions(t, withDataDir(t.TempDir())) runOneToOneTest(t, options, func(c *testConnection, s *testSession) { // Create a mailbox. diff --git a/tests/server_test.go b/tests/server_test.go index c0dda041..41ab0e7b 100644 --- a/tests/server_test.go +++ b/tests/server_test.go @@ -45,72 +45,10 @@ var ( } ) -type pathGenerator interface { - GenerateBackendPath() string - GenerateStorePath() string - GenerateUserPath(user string) string -} - -type defaultPathGenerator struct { - tb testing.TB -} - -func (dpg *defaultPathGenerator) GenerateBackendPath() string { - return dpg.tb.TempDir() -} - -func (dpg *defaultPathGenerator) GenerateStorePath() string { - return dpg.tb.TempDir() -} - -func (dpg *defaultPathGenerator) GenerateUserPath(user string) string { - tmpDir := dpg.tb.TempDir() - return getEntPath(tmpDir) -} - -type fixedPathGenerator struct { - backendPath string - storePath string - userPath string - userPaths map[string]string -} - -func newFixedPathGenerator(backendPath, storePath, userPath string) pathGenerator { - return &fixedPathGenerator{ - backendPath: backendPath, - userPath: userPath, - storePath: storePath, - userPaths: make(map[string]string), - } -} - -func (fpg *fixedPathGenerator) GenerateBackendPath() string { - return fpg.storePath -} - -func (fpg *fixedPathGenerator) GenerateStorePath() string { - return fpg.storePath -} - -func (fpg *fixedPathGenerator) GenerateUserPath(user string) string { - if v, ok := fpg.userPaths[user]; ok { - return v - } - - newUserPath := getEntPath(fpg.userPath) - fpg.userPaths[user] = newUserPath - - return newUserPath -} - -func newDefaultPathGenerator(tb testing.TB) pathGenerator { - return &defaultPathGenerator{tb: tb} -} - type serverOptions struct { credentials []credentials delimiter string - pathGenerator + dataDir string } func (s *serverOptions) defaultUsername() string { @@ -133,12 +71,12 @@ func (d *delimiterServerOption) apply(options *serverOptions) { options.delimiter = d.delimiter } -type pathGeneratorOption struct { - pg pathGenerator +type dataDirOption struct { + dir string } -func (pg *pathGeneratorOption) apply(options *serverOptions) { - options.pathGenerator = pg.pg +func (opt *dataDirOption) apply(options *serverOptions) { + options.dataDir = opt.dir } type credentialsSeverOption struct { @@ -153,8 +91,8 @@ func withDelimiter(delimiter string) serverOption { return &delimiterServerOption{delimiter: delimiter} } -func withPathGenerator(pg pathGenerator) serverOption { - return &pathGeneratorOption{pg: pg} +func withDataDir(dir string) serverOption { + return &dataDirOption{dir: dir} } func withCredentials(credentials []credentials) serverOption { @@ -167,8 +105,8 @@ func defaultServerOptions(tb testing.TB, modifiers ...serverOption) *serverOptio usernames: []string{"user"}, password: "pass", }}, - delimiter: "/", - pathGenerator: newDefaultPathGenerator(tb), + delimiter: "/", + dataDir: tb.TempDir(), } for _, op := range modifiers { @@ -194,12 +132,12 @@ func runServer(tb testing.TB, options *serverOptions, tests func(*testSession)) // Setup goroutine leak detector here so that it doesn't report the goroutines created by logrus. defer goleak.VerifyNone(tb, goleak.IgnoreCurrent()) - gluonPath := options.pathGenerator.GenerateBackendPath() - storePath := options.pathGenerator.GenerateStorePath() + // Log the (temporary?) directory to store gluon data. + logrus.Tracef("Gluon Data Dir: %v", options.dataDir) - logrus.Tracef("Backend Path: %v", gluonPath) + // Create a new gluon server. server, err := gluon.New( - gluonPath, + gluon.WithDataDir(options.dataDir), gluon.WithDelimiter(options.delimiter), gluon.WithTLS(&tls.Config{ Certificates: []tls.Certificate{testCert}, @@ -209,9 +147,14 @@ func runServer(tb testing.TB, options *serverOptions, tests func(*testSession)) loggerIn, loggerOut, ), - gluon.WithVersionInfo(TestServerVersionInfo.Version.Major, TestServerVersionInfo.Version.Minor, TestServerVersionInfo.Version.Patch, - TestServerVersionInfo.Name, TestServerVersionInfo.Vendor, TestServerVersionInfo.SupportURL), - gluon.WithDataPath(storePath), + gluon.WithVersionInfo( + TestServerVersionInfo.Version.Major, + TestServerVersionInfo.Version.Minor, + TestServerVersionInfo.Version.Patch, + TestServerVersionInfo.Name, + TestServerVersionInfo.Vendor, + TestServerVersionInfo.SupportURL, + ), gluon.WithStoreBuilder(&testBadgerStoreBuilder{}), ) require.NoError(tb, err) @@ -247,7 +190,7 @@ func runServer(tb testing.TB, options *serverOptions, tests func(*testSession)) } conns[userID] = conn - dbPaths[userID] = filepath.Join(server.GetDataPath(), fmt.Sprintf("%v.db", userID)) + dbPaths[userID] = filepath.Join(server.GetDataPath(), "backend", "db", fmt.Sprintf("%v.db", userID)) } listener, err := net.Listen("tcp", net.JoinHostPort("localhost", "0")) diff --git a/tests/store_test.go b/tests/store_test.go index ae429d57..ab598d35 100644 --- a/tests/store_test.go +++ b/tests/store_test.go @@ -218,7 +218,7 @@ func TestFlagsDuplicateAndCaseInsensitive(t *testing.T) { } func TestStoreFlagsPersistBetweenRuns(t *testing.T) { - options := defaultServerOptions(t, withPathGenerator(newFixedPathGenerator(t.TempDir(), t.TempDir(), t.TempDir()))) + options := defaultServerOptions(t, withDataDir(t.TempDir())) runOneToOneTestWithAuth(t, options, func(c *testConnection, _ *testSession) { c.C("b001 CREATE saved-messages")