From 5990d32fd36e7d267eb6ef74cff57ebed3511099 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Tue, 19 Oct 2021 11:44:49 +0200 Subject: [PATCH] More logging and refactor (#457) * only use "context" * enable 'h2' support at server * trace log remote and database config * log loglevel on start --- cmd/agent/agent.go | 3 +- cmd/server/server.go | 15 +++--- cmd/server/setup.go | 47 +++++++++++------ server/grpc/rpc.go | 17 +++--- server/remote/bitbucket/bitbucket.go | 15 ++++-- server/remote/bitbucket/bitbucket_test.go | 4 +- server/remote/context.go | 2 +- server/shared/configFetcher.go | 1 + server/store/context.go | 2 +- server/store/datastore/store.go | 63 +++++++---------------- server/store/datastore/store_test.go | 52 +++++++++++++++++++ server/store/store.go | 3 +- 12 files changed, 135 insertions(+), 89 deletions(-) create mode 100644 server/store/datastore/store_test.go diff --git a/cmd/agent/agent.go b/cmd/agent/agent.go index faa1a1d7a..094c7753b 100644 --- a/cmd/agent/agent.go +++ b/cmd/agent/agent.go @@ -25,7 +25,6 @@ import ( "github.com/rs/zerolog/log" "github.com/tevino/abool" "github.com/urfave/cli" - oldcontext "golang.org/x/net/context" "google.golang.org/grpc" grpccredentials "google.golang.org/grpc/credentials" "google.golang.org/grpc/keepalive" @@ -160,7 +159,7 @@ type credentials struct { password string } -func (c *credentials) GetRequestMetadata(oldcontext.Context, ...string) (map[string]string, error) { +func (c *credentials) GetRequestMetadata(context.Context, ...string) (map[string]string, error) { return map[string]string{ "username": c.username, "password": c.password, diff --git a/cmd/server/server.go b/cmd/server/server.go index 3ae8abac5..dd97a1ded 100644 --- a/cmd/server/server.go +++ b/cmd/server/server.go @@ -31,7 +31,6 @@ import ( "github.com/rs/zerolog/log" "github.com/urfave/cli" "golang.org/x/crypto/acme/autocert" - oldcontext "golang.org/x/net/context" "golang.org/x/sync/errgroup" "google.golang.org/grpc" "google.golang.org/grpc/keepalive" @@ -68,7 +67,6 @@ func loop(c *cli.Context) error { log.Warn().Msg("--debug is deprecated, use --log-level instead") zerolog.SetGlobalLevel(zerolog.DebugLevel) } - if c.IsSet("log-level") { logLevelFlag := c.String("log-level") lvl, err := zerolog.ParseLevel(logLevelFlag) @@ -77,6 +75,7 @@ func loop(c *cli.Context) error { } zerolog.SetGlobalLevel(lvl) } + log.Log().Msgf("LogLevel = %s", zerolog.GlobalLevel().String()) if c.String("server-host") == "" { log.Fatal().Msg("WOODPECKER_HOST is not properly configured") @@ -105,7 +104,11 @@ func loop(c *cli.Context) error { log.Fatal().Err(err).Msg("") } - store_ := setupStore(c) + store_, err := setupStore(c) + if err != nil { + log.Fatal().Err(err).Msg("") + } + setupEvilGlobals(c, store_, remote_) proxyWebUI := c.String("www-proxy") @@ -189,7 +192,7 @@ func loop(c *cli.Context) error { Addr: ":https", Handler: handler, TLSConfig: &tls.Config{ - NextProtos: []string{"http/1.1"}, // disable h2 because Safari :( + NextProtos: []string{"h2", "http/1.1"}, }, } return serve.ListenAndServeTLS( @@ -232,7 +235,7 @@ func loop(c *cli.Context) error { Handler: handler, TLSConfig: &tls.Config{ GetCertificate: manager.GetCertificate, - NextProtos: []string{"http/1.1"}, // disable h2 because Safari :( + NextProtos: []string{"h2", "http/1.1"}, }, } return serve.ListenAndServeTLS("", "") @@ -296,7 +299,7 @@ func (a *authorizer) streamInterceptor(srv interface{}, stream grpc.ServerStream return handler(srv, stream) } -func (a *authorizer) unaryIntercaptor(ctx oldcontext.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { +func (a *authorizer) unaryIntercaptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp interface{}, err error) { if err := a.authorize(ctx); err != nil { return nil, err } diff --git a/cmd/server/setup.go b/cmd/server/setup.go index 423520298..40b3773f4 100644 --- a/cmd/server/setup.go +++ b/cmd/server/setup.go @@ -44,11 +44,13 @@ import ( "github.com/woodpecker-ci/woodpecker/server/web" ) -func setupStore(c *cli.Context) store.Store { - return datastore.New( - c.String("driver"), - c.String("datasource"), - ) +func setupStore(c *cli.Context) (store.Store, error) { + opts := &datastore.Opts{ + Driver: c.String("driver"), + Config: c.String("datasource"), + } + log.Trace().Msgf("setup datastore: %#v", opts) + return datastore.New(opts) } func setupQueue(c *cli.Context, s store.Store) queue.Queue { @@ -98,21 +100,25 @@ func SetupRemote(c *cli.Context) (remote.Remote, error) { // helper function to setup the Bitbucket remote from the CLI arguments. func setupBitbucket(c *cli.Context) (remote.Remote, error) { - return bitbucket.New( - c.String("bitbucket-client"), - c.String("bitbucket-secret"), - ), nil + opts := &bitbucket.Opts{ + Client: c.String("bitbucket-client"), + Secret: c.String("bitbucket-secret"), + } + log.Trace().Msgf("Remote (bitbucket) opts: %#v", opts) + return bitbucket.New(opts) } // helper function to setup the Gogs remote from the CLI arguments. func setupGogs(c *cli.Context) (remote.Remote, error) { - return gogs.New(gogs.Opts{ + opts := gogs.Opts{ URL: c.String("gogs-server"), Username: c.String("gogs-git-username"), Password: c.String("gogs-git-password"), PrivateMode: c.Bool("gogs-private-mode"), SkipVerify: c.Bool("gogs-skip-verify"), - }) + } + log.Trace().Msgf("Remote (gogs) opts: %#v", opts) + return gogs.New(opts) } // helper function to setup the Gitea remote from the CLI arguments. @@ -130,12 +136,13 @@ func setupGitea(c *cli.Context) (remote.Remote, error) { if len(opts.URL) == 0 { log.Fatal().Msg("WOODPECKER_GITEA_URL must be set") } + log.Trace().Msgf("Remote (gitea) opts: %#v", opts) return gitea.New(opts) } // helper function to setup the Stash remote from the CLI arguments. func setupStash(c *cli.Context) (remote.Remote, error) { - return bitbucketserver.New(bitbucketserver.Opts{ + opts := bitbucketserver.Opts{ URL: c.String("stash-server"), Username: c.String("stash-git-username"), Password: c.String("stash-git-password"), @@ -143,7 +150,9 @@ func setupStash(c *cli.Context) (remote.Remote, error) { ConsumerRSA: c.String("stash-consumer-rsa"), ConsumerRSAString: c.String("stash-consumer-rsa-string"), SkipVerify: c.Bool("stash-skip-verify"), - }) + } + log.Trace().Msgf("Remote (bitbucketserver) opts: %#v", opts) + return bitbucketserver.New(opts) } // helper function to setup the Gitlab remote from the CLI arguments. @@ -161,7 +170,7 @@ func setupGitlab(c *cli.Context) (remote.Remote, error) { // helper function to setup the GitHub remote from the CLI arguments. func setupGithub(c *cli.Context) (remote.Remote, error) { - return github.New(github.Opts{ + opts := github.Opts{ URL: c.String("github-server"), Context: c.String("github-context"), Client: c.String("github-client"), @@ -172,12 +181,14 @@ func setupGithub(c *cli.Context) (remote.Remote, error) { PrivateMode: c.Bool("github-private-mode"), SkipVerify: c.Bool("github-skip-verify"), MergeRef: c.BoolT("github-merge-ref"), - }) + } + log.Trace().Msgf("Remote (github) opts: %#v", opts) + return github.New(opts) } // helper function to setup the Coding remote from the CLI arguments. func setupCoding(c *cli.Context) (remote.Remote, error) { - return coding.New(coding.Opts{ + opts := coding.Opts{ URL: c.String("coding-server"), Client: c.String("coding-client"), Secret: c.String("coding-secret"), @@ -186,7 +197,9 @@ func setupCoding(c *cli.Context) (remote.Remote, error) { Username: c.String("coding-git-username"), Password: c.String("coding-git-password"), SkipVerify: c.Bool("coding-skip-verify"), - }) + } + log.Trace().Msgf("Remote (coding) opts: %#v", opts) + return coding.New(opts) } func setupTree(c *cli.Context) *gin.Engine { diff --git a/server/grpc/rpc.go b/server/grpc/rpc.go index 0cb2ff9b7..c5e7d0f53 100644 --- a/server/grpc/rpc.go +++ b/server/grpc/rpc.go @@ -28,7 +28,6 @@ import ( "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promauto" - oldcontext "golang.org/x/net/context" grpcMetadata "google.golang.org/grpc/metadata" "github.com/woodpecker-ci/expr" @@ -505,7 +504,7 @@ func NewWoodpeckerServer(remote remote.Remote, queue queue.Queue, logger logging return &WoodpeckerServer{peer: peer} } -func (s *WoodpeckerServer) Next(c oldcontext.Context, req *proto.NextRequest) (*proto.NextReply, error) { +func (s *WoodpeckerServer) Next(c context.Context, req *proto.NextRequest) (*proto.NextReply, error) { filter := rpc.Filter{ Labels: req.GetFilter().GetLabels(), Expr: req.GetFilter().GetExpr(), @@ -528,7 +527,7 @@ func (s *WoodpeckerServer) Next(c oldcontext.Context, req *proto.NextRequest) (* return res, err } -func (s *WoodpeckerServer) Init(c oldcontext.Context, req *proto.InitRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Init(c context.Context, req *proto.InitRequest) (*proto.Empty, error) { state := rpc.State{ Error: req.GetState().GetError(), ExitCode: int(req.GetState().GetExitCode()), @@ -542,7 +541,7 @@ func (s *WoodpeckerServer) Init(c oldcontext.Context, req *proto.InitRequest) (* return res, err } -func (s *WoodpeckerServer) Update(c oldcontext.Context, req *proto.UpdateRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Update(c context.Context, req *proto.UpdateRequest) (*proto.Empty, error) { state := rpc.State{ Error: req.GetState().GetError(), ExitCode: int(req.GetState().GetExitCode()), @@ -556,7 +555,7 @@ func (s *WoodpeckerServer) Update(c oldcontext.Context, req *proto.UpdateRequest return res, err } -func (s *WoodpeckerServer) Upload(c oldcontext.Context, req *proto.UploadRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Upload(c context.Context, req *proto.UploadRequest) (*proto.Empty, error) { file := &rpc.File{ Data: req.GetFile().GetData(), Mime: req.GetFile().GetMime(), @@ -572,7 +571,7 @@ func (s *WoodpeckerServer) Upload(c oldcontext.Context, req *proto.UploadRequest return res, err } -func (s *WoodpeckerServer) Done(c oldcontext.Context, req *proto.DoneRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Done(c context.Context, req *proto.DoneRequest) (*proto.Empty, error) { state := rpc.State{ Error: req.GetState().GetError(), ExitCode: int(req.GetState().GetExitCode()), @@ -586,19 +585,19 @@ func (s *WoodpeckerServer) Done(c oldcontext.Context, req *proto.DoneRequest) (* return res, err } -func (s *WoodpeckerServer) Wait(c oldcontext.Context, req *proto.WaitRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Wait(c context.Context, req *proto.WaitRequest) (*proto.Empty, error) { res := new(proto.Empty) err := s.peer.Wait(c, req.GetId()) return res, err } -func (s *WoodpeckerServer) Extend(c oldcontext.Context, req *proto.ExtendRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Extend(c context.Context, req *proto.ExtendRequest) (*proto.Empty, error) { res := new(proto.Empty) err := s.peer.Extend(c, req.GetId()) return res, err } -func (s *WoodpeckerServer) Log(c oldcontext.Context, req *proto.LogRequest) (*proto.Empty, error) { +func (s *WoodpeckerServer) Log(c context.Context, req *proto.LogRequest) (*proto.Empty, error) { line := &rpc.Line{ Out: req.GetLine().GetOut(), Pos: int(req.GetLine().GetPos()), diff --git a/server/remote/bitbucket/bitbucket.go b/server/remote/bitbucket/bitbucket.go index d071843b6..42df8ec2c 100644 --- a/server/remote/bitbucket/bitbucket.go +++ b/server/remote/bitbucket/bitbucket.go @@ -34,6 +34,12 @@ const ( DefaultURL = "https://bitbucket.org" ) +// Opts are remote options for bitbucket +type Opts struct { + Client string + Secret string +} + type config struct { API string URL string @@ -43,13 +49,14 @@ type config struct { // New returns a new remote Configuration for integrating with the Bitbucket // repository hosting service at https://bitbucket.org -func New(client, secret string) remote.Remote { +func New(opts *Opts) (remote.Remote, error) { return &config{ API: DefaultAPI, URL: DefaultURL, - Client: client, - Secret: secret, - } + Client: opts.Client, + Secret: opts.Secret, + }, nil + // TODO: add checks } // Login authenticates an account with Bitbucket using the oauth2 protocol. The diff --git a/server/remote/bitbucket/bitbucket_test.go b/server/remote/bitbucket/bitbucket_test.go index 5273c3bd2..a4f489316 100644 --- a/server/remote/bitbucket/bitbucket_test.go +++ b/server/remote/bitbucket/bitbucket_test.go @@ -44,7 +44,7 @@ func Test_bitbucket(t *testing.T) { }) g.It("Should return client with default endpoint", func() { - remote := New("4vyW6b49Z", "a5012f6c6") + remote, _ := New(&Opts{Client: "4vyW6b49Z", Secret: "a5012f6c6"}) g.Assert(remote.(*config).URL).Equal(DefaultURL) g.Assert(remote.(*config).API).Equal(DefaultAPI) g.Assert(remote.(*config).Client).Equal("4vyW6b49Z") @@ -52,7 +52,7 @@ func Test_bitbucket(t *testing.T) { }) g.It("Should return the netrc file", func() { - remote := New("", "") + remote, _ := New(&Opts{}) netrc, _ := remote.Netrc(fakeUser, nil) g.Assert(netrc.Machine).Equal("bitbucket.org") g.Assert(netrc.Login).Equal("x-token-auth") diff --git a/server/remote/context.go b/server/remote/context.go index 5bfccc036..e076c508a 100644 --- a/server/remote/context.go +++ b/server/remote/context.go @@ -15,7 +15,7 @@ package remote import ( - "golang.org/x/net/context" + "context" ) const key = "remote" diff --git a/server/shared/configFetcher.go b/server/shared/configFetcher.go index 549c6e584..90b3a5ad5 100644 --- a/server/shared/configFetcher.go +++ b/server/shared/configFetcher.go @@ -39,6 +39,7 @@ func (cf *configFetcher) Fetch(ctx context.Context) (files []*remote.FileMeta, e // try to fetch 3 times, timeout is one second longer each time for i := 0; i < 3; i++ { files, err = cf.fetch(ctx, time.Second*time.Duration(configFetchTimeout), strings.TrimSpace(cf.repo.Config)) + log.Trace().Msgf("%d try failed: %v", i, err) if errors.Is(err, context.DeadlineExceeded) { continue } diff --git a/server/store/context.go b/server/store/context.go index 135c9c840..4f3fc2d2b 100644 --- a/server/store/context.go +++ b/server/store/context.go @@ -15,7 +15,7 @@ package store import ( - "golang.org/x/net/context" + "context" ) const key = "store" diff --git a/server/store/datastore/store.go b/server/store/datastore/store.go index 6fd0c65cb..7f27f35d1 100644 --- a/server/store/datastore/store.go +++ b/server/store/datastore/store.go @@ -16,7 +16,7 @@ package datastore import ( "database/sql" - "os" + "fmt" "time" "github.com/rs/zerolog/log" @@ -35,14 +35,21 @@ type datastore struct { config string } +// Opts are options for a new database connection +type Opts struct { + Driver string + Config string +} + // New creates a database connection for the given driver and datasource // and returns a new Store. -func New(driver, config string) store.Store { +func New(opts *Opts) (store.Store, error) { + db, err := open(opts.Driver, opts.Config) return &datastore{ - DB: open(driver, config), - driver: driver, - config: config, - } + DB: db, + driver: opts.Driver, + config: opts.Config, + }, err } // From returns a Store using an existing database connection. @@ -52,10 +59,10 @@ func From(db *sql.DB) store.Store { // open opens a new database connection with the specified // driver and connection string and returns a store. -func open(driver, config string) *sql.DB { +func open(driver, config string) (*sql.DB, error) { db, err := sql.Open(driver, config) if err != nil { - log.Fatal().Err(err).Msg("database connection failed") + return nil, fmt.Errorf("database connection failed: %v", err) } if driver == "mysql" { // per issue https://github.com/go-sql-driver/mysql/issues/257 @@ -65,47 +72,13 @@ func open(driver, config string) *sql.DB { setupMeddler(driver) if err := pingDatabase(db); err != nil { - log.Fatal().Err(err).Msg("database ping attempts failed") + return nil, fmt.Errorf("database ping attempts failed: %v", err) } if err := setupDatabase(driver, db); err != nil { - log.Fatal().Err(err).Msg("migration failed") - } - return db -} - -// openTest opens a new database connection for testing purposes. -// The database driver and connection string are provided by -// environment variables, with fallback to in-memory sqlite. -func openTest() *sql.DB { - var ( - driver = "sqlite3" - config = ":memory:" - ) - if os.Getenv("WOODPECKER_DATABASE_DRIVER") != "" { - driver = os.Getenv("WOODPECKER_DATABASE_DRIVER") - config = os.Getenv("WOODPECKER_DATABASE_CONFIG") - } - return open(driver, config) -} - -// newTest creates a new database connection for testing purposes. -// The database driver and connection string are provided by -// environment variables, with fallback to in-memory sqlite. -func newTest() *datastore { - var ( - driver = "sqlite3" - config = ":memory:" - ) - if os.Getenv("WOODPECKER_DATABASE_DRIVER") != "" { - driver = os.Getenv("WOODPECKER_DATABASE_DRIVER") - config = os.Getenv("WOODPECKER_DATABASE_CONFIG") - } - return &datastore{ - DB: open(driver, config), - driver: driver, - config: config, + return nil, fmt.Errorf("database migration failed: %v", err) } + return db, nil } // helper function to ping the database with backoff to ensure diff --git a/server/store/datastore/store_test.go b/server/store/datastore/store_test.go new file mode 100644 index 000000000..0fee0057f --- /dev/null +++ b/server/store/datastore/store_test.go @@ -0,0 +1,52 @@ +// Copyright 2021 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package datastore + +import ( + "database/sql" + "os" +) + +func testDriverConfig() (driver, config string) { + driver = "sqlite3" + config = ":memory:" + + if os.Getenv("WOODPECKER_DATABASE_DRIVER") != "" { + driver = os.Getenv("WOODPECKER_DATABASE_DRIVER") + config = os.Getenv("WOODPECKER_DATABASE_CONFIG") + } + + return +} + +// openTest opens a new database connection for testing purposes. +// The database driver and connection string are provided by +// environment variables, with fallback to in-memory sqlite. +func openTest() *sql.DB { + db, _ := open(testDriverConfig()) + return db +} + +// newTest creates a new database connection for testing purposes. +// The database driver and connection string are provided by +// environment variables, with fallback to in-memory sqlite. +func newTest() *datastore { + driver, config := testDriverConfig() + return &datastore{ + DB: openTest(), + driver: driver, + config: config, + } +} diff --git a/server/store/store.go b/server/store/store.go index 2012d7359..d834b3561 100644 --- a/server/store/store.go +++ b/server/store/store.go @@ -15,10 +15,9 @@ package store import ( + "context" "io" - "golang.org/x/net/context" - "github.com/woodpecker-ci/woodpecker/server/model" )