From 0384f03026a1525e2a5f4da99390bc9fe8011099 Mon Sep 17 00:00:00 2001 From: rekby Date: Thu, 2 Jul 2020 22:38:18 +0300 Subject: [PATCH 1/6] Fix nil exception while domain authorize Fix #134 --- internal/cert_manager/manager.go | 10 ++++++---- internal/cert_manager/manager_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/internal/cert_manager/manager.go b/internal/cert_manager/manager.go index dde9298b..bda449b3 100644 --- a/internal/cert_manager/manager.go +++ b/internal/cert_manager/manager.go @@ -422,7 +422,7 @@ authorizeOrderLoop: } var err error order, err = m.Client.AuthorizeOrder(ctx, authIDs) - log.DebugError(logger, err, "Create authorization order.") + log.DebugError(logger, err, "Create authorization order.", zap.Reflect("order", order)) if err != nil { return nil, err } @@ -458,7 +458,8 @@ authorizeOrderLoop: authDomainLoop: for _, zurl := range order.AuthzURLs { z, err := client.GetAuthorization(ctx, zurl) - log.DebugError(logger, err, "Get authorization object.", zap.String("domain", z.Identifier.Value)) + + log.DebugError(logger, err, "Get authorization object.", zap.Reflect("authorization", z)) if err != nil { return nil, err } @@ -514,7 +515,7 @@ authorizeOrderLoop: // All authorizations are satisfied. // Wait for the CA to update the order status. order, err = client.WaitOrder(ctx, order.URI) - log.DebugWarning(logger, err, "Wait order authorization.") + log.DebugWarning(logger, err, "Wait order authorization.", zap.Reflect("order", order)) if err == nil { break authorizeOrderLoop } @@ -590,7 +591,7 @@ func (m *Manager) deactivatePendingAuthz(ctx context.Context, uries []string) { for _, uri := range uries { localLogger := logger.With(zap.String("uri", uri)) authorization, err := m.Client.GetAuthorization(ctx, uri) - log.DebugError(localLogger, err, "Get authorization", zap.Any("authorization", authorization)) + log.DebugError(localLogger, err, "Get authorization", zap.Reflect("authorization", authorization)) if err != nil { continue } @@ -626,6 +627,7 @@ func (m *Manager) fulfill(ctx context.Context, challenge *acme.Challenge, domain switch challenge.Type { case tlsAlpn01: cert, err := m.Client.TLSALPN01ChallengeCert(challenge.Token, domain.String()) + log.DebugError(logger, err, "Got TLSALPN01ChallengeCert", log.Cert(&cert)) if err != nil { return nil, err } diff --git a/internal/cert_manager/manager_test.go b/internal/cert_manager/manager_test.go index 91b193a1..a169a6b0 100644 --- a/internal/cert_manager/manager_test.go +++ b/internal/cert_manager/manager_test.go @@ -11,6 +11,7 @@ import ( "crypto/tls" "crypto/x509" "fmt" + "golang.org/x/xerrors" "net" "net/http" "testing" @@ -216,3 +217,29 @@ func createManager(t *testing.T) (res testManagerContext, cancel func()) { ctxCancel() } } + +// Test nil in getAuthorized +func TestIssue_134(t *testing.T) { + ctx, ctxCancel := th.TestContext(t) + defer ctxCancel() + + mc := minimock.NewController(t) + defer mc.Finish() + + td := testdeep.NewT(t) + + testURL := "http://test" + client := NewAcmeClientMock(mc) + client.AuthorizeOrderMock.Return(&acme.Order{ + Status: acme.StatusPending, + AuthzURLs: []string{testURL}, + }, nil) + + testErr := xerrors.New("testErr") + client.GetAuthorizationMock.Expect(ctx, testURL).Return(nil, testErr) + + m := &Manager{Client: client} + res, err := m.createOrderForDomains(ctx, "test") + td.Nil(res) + td.True(xerrors.Is(err, testErr)) +} From 49e260c6379b12b45b1a91d787bd5b389ac0ed90 Mon Sep 17 00:00:00 2001 From: rekby Date: Thu, 2 Jul 2020 23:04:45 +0300 Subject: [PATCH 2/6] Improove logs --- internal/cache/disk.go | 14 +++++++++++--- internal/cert_manager/manager.go | 2 +- internal/log/log.go | 10 ++++++++++ 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/internal/cache/disk.go b/internal/cache/disk.go index 224b2bd0..dba10203 100644 --- a/internal/cache/disk.go +++ b/internal/cache/disk.go @@ -2,6 +2,8 @@ package cache import ( "context" + "github.com/rekby/lets-proxy2/internal/log" + "go.uber.org/zap/zapcore" "io/ioutil" "os" "path/filepath" @@ -28,13 +30,19 @@ func (c *DiskCache) Get(ctx context.Context, key string) ([]byte, error) { filePath := c.filepath(key) + logLevel := zapcore.DebugLevel res, err := ioutil.ReadFile(filePath) - if os.IsNotExist(err) { - err = ErrCacheMiss + if err != nil { + if os.IsNotExist(err) { + err = ErrCacheMiss + } else { + logLevel = zapcore.ErrorLevel + } } - zc.L(ctx).Debug("Got from disk cache", zap.String("dir", c.Dir), zap.String("key", key), + log.LevelParamCtx(ctx, logLevel, "Got from disk cache", zap.String("dir", c.Dir), zap.String("key", key), zap.String("file", filePath), zap.Error(err)) + return res, err } diff --git a/internal/cert_manager/manager.go b/internal/cert_manager/manager.go index bda449b3..40b0cdc0 100644 --- a/internal/cert_manager/manager.go +++ b/internal/cert_manager/manager.go @@ -608,7 +608,7 @@ func (m *Manager) certKeyGetOrCreate(ctx context.Context, cd CertDescription) (c logger := zc.L(ctx) key, err := getCertificateKey(ctx, m.Cache, cd) - log.DebugError(logger, err, "Got certificate key from cache and reuse old key") + logger.Debug("Got certificate key from cache and reuse old key", zap.Error(err)) if err == nil { return key, nil } diff --git a/internal/log/log.go b/internal/log/log.go index 3ec0449e..c0377854 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -190,6 +190,16 @@ func infoPanic(logger *zap.Logger, err error, mess string, fields ...zap.Field) } func LevelParam(logger *zap.Logger, level zapcore.Level, mess string, fields ...zap.Field) { + levelParam(logger, level, mess, fields...) +} + +func LevelParamCtx(ctx context.Context, level zapcore.Level, mess string, fields ...zap.Field) { + logger := zc.L(ctx) + levelParam(logger, level, mess, fields...) +} + +func levelParam(logger *zap.Logger, level zapcore.Level, mess string, fields ...zap.Field) { + logger = logger.WithOptions(zap.AddCallerSkip(addSkipCallers)) if ce := logger.Check(level, mess); ce != nil { ce.Write(fields...) } From 57c41fa840fac97fe3def8805fdc45ce8c84c5fa Mon Sep 17 00:00:00 2001 From: rekby Date: Fri, 3 Jul 2020 00:03:01 +0300 Subject: [PATCH 3/6] Fix unhandled panic in background goroutines. --- cmd/main.go | 6 +++++ .../acme_client_manager/client_manager.go | 9 ++++++- internal/cache/value_lru.go | 2 ++ internal/cert_manager/cert-state_test.go | 3 ++- internal/cert_manager/manager.go | 22 ++++++++-------- internal/contexthelper/contexthelper.go | 1 + internal/dns/parallel.go | 8 +++++- internal/dns/resolver.go | 13 +++++++--- internal/domain_checker/ip_list.go | 9 ++++++- internal/domain_checker/ip_list_sources.go | 9 +++++-- internal/log/handle_panic.go | 25 +++++++++++++++++++ internal/tlslistener/context_conn.go | 6 ++++- internal/tlslistener/tlslistenershandler.go | 6 +++++ 13 files changed, 99 insertions(+), 20 deletions(-) create mode 100644 internal/log/handle_panic.go diff --git a/cmd/main.go b/cmd/main.go index dd254aaf..23c7e5b7 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -90,6 +90,8 @@ func startMetrics(ctx context.Context, r prometheus.Gatherer, config config.Conf secretMetric := secrethandler.New(zc.L(ctx).Named("metrics_secret"), config.GetSecretHandlerConfig(), m) go func() { + defer log.HandlePanic(loggerLocal) + err := http.Serve(listener, secretMetric) var effectiveError = err if effectiveError == http.ErrServerClosed { @@ -156,6 +158,8 @@ func startProgram(config *configType) { log.InfoFatal(logger, err, "Apply proxy config") go func() { + defer log.HandlePanic(logger) + <-ctx.Done() err := p.Close() log.DebugError(logger, err, "Stop proxy") @@ -178,6 +182,8 @@ func startProfiler(ctx context.Context, config profiler.Config) { } go func() { + defer log.HandlePanic(logger) + httpServer := http.Server{ Addr: config.BindAddress, Handler: profiler.New(logger.Named("profiler"), config), diff --git a/internal/acme_client_manager/client_manager.go b/internal/acme_client_manager/client_manager.go index 72d133c9..ac8f0114 100644 --- a/internal/acme_client_manager/client_manager.go +++ b/internal/acme_client_manager/client_manager.go @@ -130,6 +130,7 @@ func (m *AcmeManager) GetClient(ctx context.Context) (*acme.Client, error) { } if m.client != nil { + // handlepanic: in accountRenew go m.accountRenew() } @@ -160,7 +161,12 @@ func (m *AcmeManager) accountRenew() { log.InfoCtx(m.ctx, "Stop renew acme account because cancel context", zap.Error(m.ctx.Err())) return case <-ticker.C: - newAccount := renewTos(m.ctx, m.client, m.account) + var newAccount *acme.Account + func() { + defer log.HandlePanic(logger) + + newAccount = renewTos(m.ctx, m.client, m.account) + }() m.mu.Lock() m.account = newAccount m.mu.Unlock() @@ -201,6 +207,7 @@ func (m *AcmeManager) loadFromCache(ctx context.Context) (err error) { m.client = &acme.Client{DirectoryURL: m.DirectoryURL, Key: state.PrivateKey} m.account = state.AcmeAccount + // handlepanic: in accountRenew go m.accountRenew() return nil diff --git a/internal/cache/value_lru.go b/internal/cache/value_lru.go index 207725e5..a01cbb1b 100644 --- a/internal/cache/value_lru.go +++ b/internal/cache/value_lru.go @@ -69,6 +69,7 @@ func (c *MemoryValueLRU) Put(ctx context.Context, key string, value interface{}) c.mu.Lock() c.m[key] = &memoryValueLRUItem{key: key, value: value, lastUsedTime: c.time()} if len(c.m) > c.MaxSize { + // handlepanic: no external call go c.clean() } c.mu.Unlock() @@ -92,6 +93,7 @@ func (c *MemoryValueLRU) Delete(ctx context.Context, key string) (err error) { func (c *MemoryValueLRU) time() uint64 { res := atomic.AddUint64(&c.lastTime, 1) if res == math.MaxUint64/2 { + // handlepanic: no external call go c.renumberTime() } return res diff --git a/internal/cert_manager/cert-state_test.go b/internal/cert_manager/cert-state_test.go index e1322da8..0a5586b4 100644 --- a/internal/cert_manager/cert-state_test.go +++ b/internal/cert_manager/cert-state_test.go @@ -94,8 +94,9 @@ func TestCertStateManyIssuers(t *testing.T) { for i := 0; i < cnt; i++ { go func() { + defer wg.Done() + lockTimesChan <- lockFunc() - wg.Done() }() } wg.Wait() diff --git a/internal/cert_manager/manager.go b/internal/cert_manager/manager.go index 40b0cdc0..da209e73 100644 --- a/internal/cert_manager/manager.go +++ b/internal/cert_manager/manager.go @@ -144,7 +144,7 @@ func (m *Manager) GetCertificate(hello *tls.ClientHelloInfo) (resultCert *tls.Ce logger = logger.With(logDomain(needDomain)) ctx = zc.WithLogger(ctx, logger) - defer handlePanic(logger) + defer log.HandlePanic(logger) logger.Info("Get certificate", zap.String("original_domain", hello.ServerName)) if isTLSALPN01Hello(hello) { @@ -188,6 +188,7 @@ func (m *Manager) getCertificate(ctx context.Context, needDomain DomainName, cer } } if !locked { + // handlepanic: in renewCertInBackground go m.renewCertInBackground(ctx, needDomain, certDescription) } } @@ -308,6 +309,7 @@ func filterDomains(ctx context.Context, checker DomainChecker, originalDomains [ go func() { defer wg.Done() + defer log.HandlePanic(logger) allow, err := checker.IsDomainAllowed(ctx, domain.ASCII()) logger.Debug("Check domain", logDomain(domain), zap.Bool("allowed", allow), zap.Error(err)) @@ -430,6 +432,8 @@ authorizeOrderLoop: //noinspection GoDeferInLoop defer func() { go func() { + defer log.HandlePanic(logger) + revokeLogger := logger.Named("background_auth_revoker") revokeCtx, cancel := context.WithTimeout(context.Background(), revokeAuthorizationTimeout) @@ -575,6 +579,7 @@ func (m *Manager) issueCertificate(ctx context.Context, cd CertDescription, orde func (m *Manager) renewCertInBackground(ctx context.Context, needDomain DomainName, cd CertDescription) { // detach from request lifetime, but save log context logger := zc.L(ctx).Named("background") + defer log.HandlePanic(logger) ctx, ctxCancel := context.WithTimeout(context.Background(), m.CertificateIssueTimeout) defer ctxCancel() @@ -632,7 +637,10 @@ func (m *Manager) fulfill(ctx context.Context, challenge *acme.Challenge, domain return nil, err } m.putCertToken(ctx, domain, &cert) - return func(localContext context.Context) { go m.deleteCertToken(localContext, domain) }, nil + return func(localContext context.Context) { + // handlepanic: in deleteCertToken + go m.deleteCertToken(localContext, domain) + }, nil case http01: resp, err := m.Client.HTTP01ChallengeResponse(challenge.Token) if err != nil { @@ -701,6 +709,8 @@ func (m *Manager) putCertToken(ctx context.Context, key DomainName, certificate } func (m *Manager) deleteCertToken(ctx context.Context, key DomainName) { + defer log.HandlePanicCtx(ctx) + err := m.certForDomainAuthorize.Delete(ctx, key.String()) log.DebugDPanicCtx(ctx, err, "Delete cert token", zap.String("key", key.String())) } @@ -919,14 +929,6 @@ func isNeedRenew(cert *tls.Certificate, now time.Time) bool { return cert.Leaf.NotAfter.Add(-renewBeforeExpire).Before(now) } -// must called as defer handlepanic(logger) -func handlePanic(logger *zap.Logger) { - err := recover() - if err != nil { - logger.DPanic("Panic handled", zap.Any("panic", err)) - } -} - func isCertLocked(ctx context.Context, storage cache.Bytes, certName CertDescription) (bool, error) { lockName := certName.LockName() diff --git a/internal/contexthelper/contexthelper.go b/internal/contexthelper/contexthelper.go index ab63d18c..b9e6a4f0 100644 --- a/internal/contexthelper/contexthelper.go +++ b/internal/contexthelper/contexthelper.go @@ -77,6 +77,7 @@ func CombineContext(contexts ...context.Context) *CombinedContext { deadlineMin = deadline } res.deadline, res.deadlineOk = deadlineMin, deadlineOk + // handlepanic: no external call go res.waitCloseAny() return res } diff --git a/internal/dns/parallel.go b/internal/dns/parallel.go index a0651946..efa28fd9 100644 --- a/internal/dns/parallel.go +++ b/internal/dns/parallel.go @@ -2,6 +2,8 @@ package dns import ( "context" + "github.com/rekby/lets-proxy2/internal/log" + zc "github.com/rekby/zapcontext" "net" "sync" ) @@ -24,6 +26,8 @@ func NewParallel(resolvers ...ResolverInterface) Parallel { // If any of resolvers return ips - return sum array of the ips (may duplicated) // If all resolvers return error - return any of they errors func (p Parallel) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, error) { + logger := zc.L(ctx) + switch len(p) { case 0: return nil, nil @@ -40,8 +44,10 @@ func (p Parallel) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, wg.Add(len(p)) // nolint:wsl for i := range p { // nolint:wsl go func(i int) { + defer wg.Done() + defer log.HandlePanic(logger) + ips[i], errs[i] = p[i].LookupIPAddr(ctx, host) - wg.Done() }(i) } diff --git a/internal/dns/resolver.go b/internal/dns/resolver.go index 31960ccf..739537c2 100644 --- a/internal/dns/resolver.go +++ b/internal/dns/resolver.go @@ -49,7 +49,8 @@ func NewResolver(dnsServer string) *Resolver { } func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, error) { - ctx = zc.WithLogger(ctx, zc.L(ctx).With(zap.String("dns_server", r.server))) + logger := zc.L(ctx).With(zap.String("dns_server", r.server)) + ctx = zc.WithLogger(ctx, logger) if !strings.HasSuffix(host, ".") { host += "." } @@ -62,6 +63,8 @@ func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, wg.Add(1) go func() { //nolint:wsl defer wg.Done() + defer log.HandlePanic(logger) + errA = errPanic ipAddrA, errA = r.lookup(ctx, host, mdns.TypeA) }() @@ -69,6 +72,8 @@ func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, wg.Add(1) go func() { //nolint:wsl defer wg.Done() + defer log.HandlePanic(logger) + errAAAA = errPanic ipAddrAAAA, errAAAA = r.lookup(ctx, host, mdns.TypeAAAA) }() @@ -83,7 +88,7 @@ func (r *Resolver) LookupIPAddr(ctx context.Context, host string) ([]net.IPAddr, resultErr = errA } - log.DebugErrorCtx(ctx, resultErr, "Host lookup", zap.NamedError("errA", errA), + log.DebugError(logger, resultErr, "Host lookup", zap.NamedError("errA", errA), zap.NamedError("errAAAA", errAAAA), zap.Any("ipAddrA", ipAddrA), zap.Any("ipAddrAAAA", ipAddrAAAA)) @@ -142,8 +147,10 @@ func lookupWithClient(ctx context.Context, host string, server string, recordTyp var dnsAnswer *mdns.Msg go func() { // nolint:wsl + defer close(exchangeCompleted) + defer log.HandlePanic(logger) + dnsAnswer, _, err = client.Exchange(&msg, server) - close(exchangeCompleted) }() select { diff --git a/internal/domain_checker/ip_list.go b/internal/domain_checker/ip_list.go index 174c53e1..dd8ab343 100644 --- a/internal/domain_checker/ip_list.go +++ b/internal/domain_checker/ip_list.go @@ -134,6 +134,7 @@ func (s *IPList) StartAutoRenew() { } s.started = true if s.AutoUpdateInterval > 0 { + // handlepanic: in updateIPsByTimer go s.updateIPsByTimer() } } @@ -155,12 +156,18 @@ func (s *IPList) updateIPsByTimer() { ticker := time.NewTicker(s.AutoUpdateInterval) defer ticker.Stop() + logger := zc.L(s.ctx) + for { select { case <-contextDone: return case <-ticker.C: - s.updateIPs() + func() { + defer log.HandlePanic(logger) + + s.updateIPs() + }() } } } diff --git a/internal/domain_checker/ip_list_sources.go b/internal/domain_checker/ip_list_sources.go index ef676938..b73cc2c8 100644 --- a/internal/domain_checker/ip_list_sources.go +++ b/internal/domain_checker/ip_list_sources.go @@ -188,12 +188,17 @@ func getIPByExternalRequest(ctx context.Context, url string) ([]net.IP, error) { wg.Add(2) var errTCP4, errTCP6 error go func() { + defer wg.Done() + defer log.HandlePanic(logger) + ipsFromDetector[0], errTCP4 = fGetIP("tcp4") - wg.Done() }() + go func() { + defer wg.Done() + defer log.HandlePanic(logger) + ipsFromDetector[1], errTCP6 = fGetIP("tcp6") - wg.Done() }() wg.Wait() diff --git a/internal/log/handle_panic.go b/internal/log/handle_panic.go new file mode 100644 index 00000000..e91aaa27 --- /dev/null +++ b/internal/log/handle_panic.go @@ -0,0 +1,25 @@ +package log + +import ( + "context" + zc "github.com/rekby/zapcontext" + "go.uber.org/zap" +) + +// must called as defer HandlePanic(logger) +func HandlePanic(logger *zap.Logger) { + err := recover() + if err != nil { + logger.DPanic("Panic handled", zap.Any("panic", err)) + } +} + +// must called as defer HandlePanicCtx(ctx) +func HandlePanicCtx(ctx context.Context) { + logger := zc.L(ctx) + if logger == nil { + logger, _ = zap.NewDevelopment() + logger.Error("No context logger. Create tmp dev logger.") + } + HandlePanic(logger) +} diff --git a/internal/tlslistener/context_conn.go b/internal/tlslistener/context_conn.go index 6f386c88..7533adb6 100644 --- a/internal/tlslistener/context_conn.go +++ b/internal/tlslistener/context_conn.go @@ -2,6 +2,7 @@ package tlslistener import ( "context" + "github.com/rekby/lets-proxy2/internal/log" "net" "golang.org/x/xerrors" @@ -35,11 +36,14 @@ func (c ContextConnextion) Close() error { func finalizeContextConnection(conn *ContextConnextion) { go func() { + logger := zc.L(conn.Context) + defer log.HandlePanic(logger) + if conn.connCloseHandler != nil { conn.connCloseHandler(xerrors.New("Leak connection")) conn.connCloseHandler = nil } - zc.L(conn.Context).Warn("Leaked connection") + logger.Warn("Leaked connection") _ = conn.Close() }() } diff --git a/internal/tlslistener/tlslistenershandler.go b/internal/tlslistener/tlslistenershandler.go index 00f92629..9b93bb4c 100644 --- a/internal/tlslistener/tlslistenershandler.go +++ b/internal/tlslistener/tlslistenershandler.go @@ -79,14 +79,18 @@ func (p *ListenersHandler) Start(ctx context.Context, r prometheus.Registerer) e logger.Info("StartAutoRenew handleListeners") for _, listenerForTLS := range p.ListenersForHandleTLS { + // handlepanic: in handleConnections go handleConnections(ctx, listenerForTLS, p.handleTCPTLSConnection, listenerClosed) } for _, listener := range p.Listeners { + // handlepanic: in handleConnections go handleConnections(ctx, listener, p.handleTCPConnection, listenerClosed) } go func() { + defer log.HandlePanic(logger) + listenersCount := len(p.ListenersForHandleTLS) + len(p.Listeners) for i := 0; i < listenersCount; i++ { select { @@ -106,6 +110,7 @@ func (p *ListenersHandler) Start(ctx context.Context, r prometheus.Registerer) e func handleConnections(ctx context.Context, l net.Listener, handleFunc func(ctx context.Context, conn net.Conn), listenerClosed chan<- struct{}) { logger := zc.L(ctx) + defer log.HandlePanic(logger) for { conn, err := l.Accept() @@ -120,6 +125,7 @@ func handleConnections(ctx context.Context, l net.Listener, handleFunc func(ctx listenerClosed <- struct{}{} return } + // handlepanic: in handleFunc go handleFunc(ctx, conn) } } From ee5b25e51cf2405de12770857e443d39ef1fb449 Mon Sep 17 00:00:00 2001 From: rekby Date: Fri, 3 Jul 2020 00:59:58 +0300 Subject: [PATCH 4/6] Fix nil panic for order error --- internal/cert_manager/manager.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/cert_manager/manager.go b/internal/cert_manager/manager.go index da209e73..452defb0 100644 --- a/internal/cert_manager/manager.go +++ b/internal/cert_manager/manager.go @@ -430,8 +430,8 @@ authorizeOrderLoop: } //noinspection GoDeferInLoop - defer func() { - go func() { + defer func(order *acme.Order) { + go func(order *acme.Order) { defer log.HandlePanic(logger) revokeLogger := logger.Named("background_auth_revoker") @@ -441,8 +441,8 @@ authorizeOrderLoop: revokeCtx = zc.WithLogger(revokeCtx, revokeLogger) m.deactivatePendingAuthz(revokeCtx, order.AuthzURLs) - }() - }() + }(order) + }(order) switch order.Status { case acme.StatusReady: From 74d3a889febc1a2bd222d49f3438d1bae9754759 Mon Sep 17 00:00:00 2001 From: rekby Date: Fri, 3 Jul 2020 01:02:15 +0300 Subject: [PATCH 5/6] Cert timeout in tests --- internal/cert_manager/manager_functional_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/cert_manager/manager_functional_test.go b/internal/cert_manager/manager_functional_test.go index c7321e32..a4908e14 100644 --- a/internal/cert_manager/manager_functional_test.go +++ b/internal/cert_manager/manager_functional_test.go @@ -27,6 +27,7 @@ import ( ) const forceRsaDomain = "force-rsa.ru" +const testCertIssueTimeout = time.Second * 30 func TestManager_GetCertificateHttp01(t *testing.T) { ctx, flush := th.TestContext(t) @@ -38,6 +39,7 @@ func TestManager_GetCertificateHttp01(t *testing.T) { defer mc.Finish() manager := New(createTestClient(t), newCacheMock(mc), nil) + manager.CertificateIssueTimeout = testCertIssueTimeout manager.AutoSubdomains = []string{"www."} manager.EnableTLSValidation = false manager.EnableHTTPValidation = true @@ -87,7 +89,10 @@ func TestManager_GetCertificateTls(t *testing.T) { defer mc.Finish() manager := New(createTestClient(t), newCacheMock(mc), nil) + manager.CertificateIssueTimeout = testCertIssueTimeout manager.AutoSubdomains = []string{"www."} + manager.EnableTLSValidation = true + manager.EnableHTTPValidation = false lisneter, err := net.ListenTCP("tcp", &net.TCPAddr{Port: 5001}) From d3448bd3e6d7e1ee6fb57b8987805a4b48524b4e Mon Sep 17 00:00:00 2001 From: rekby Date: Fri, 3 Jul 2020 01:55:38 +0300 Subject: [PATCH 6/6] paralell tests --- go.sum | 2 + .../cert_manager/manager_functional_test.go | 15 +++++- internal/cert_manager/manager_issues_test.go | 52 +++++++++++++++++++ internal/cert_manager/manager_test.go | 27 ---------- 4 files changed, 67 insertions(+), 29 deletions(-) create mode 100644 internal/cert_manager/manager_issues_test.go diff --git a/go.sum b/go.sum index 524fbac0..d4275c36 100644 --- a/go.sum +++ b/go.sum @@ -125,6 +125,7 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+ github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= +github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0= github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:aYKd//L2LvnjZzWKhF00oedf4jCCReLcmhLdhm1A27Q= @@ -188,4 +189,5 @@ gopkg.in/natefinch/lumberjack.v2 v2.0.0/go.mod h1:l0ndWWf7gzL7RNwBG7wST/UCcT4T24 gopkg.in/yaml.v2 v2.2.1/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= gopkg.in/yaml.v2 v2.2.4/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= +gopkg.in/yaml.v2 v2.2.5 h1:ymVxjfMaHvXD8RqPRmzHHsB3VvucivSkIAvJFDI5O3c= gopkg.in/yaml.v2 v2.2.5/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI= diff --git a/internal/cert_manager/manager_functional_test.go b/internal/cert_manager/manager_functional_test.go index a4908e14..58554404 100644 --- a/internal/cert_manager/manager_functional_test.go +++ b/internal/cert_manager/manager_functional_test.go @@ -30,6 +30,8 @@ const forceRsaDomain = "force-rsa.ru" const testCertIssueTimeout = time.Second * 30 func TestManager_GetCertificateHttp01(t *testing.T) { + t.Parallel() + ctx, flush := th.TestContext(t) defer flush() @@ -80,6 +82,8 @@ func TestManager_GetCertificateHttp01(t *testing.T) { } func TestManager_GetCertificateTls(t *testing.T) { + t.Parallel() + ctx, flush := th.TestContext(t) defer flush() @@ -200,18 +204,22 @@ func getCertificatesTests(t *testing.T, manager *Manager, ctx context.Context, l func getCertificatesTestsKeyType(t *testing.T, manager *Manager, keyType KeyType, ctx context.Context, logger *zap.Logger) { t.Run("OneCert", func(t *testing.T) { + t.Parallel() checkOkDomain(ctx, t, manager, keyType, keyType, "onecert.ru") }) t.Run("punycode-domain", func(t *testing.T) { + t.Parallel() checkOkDomain(ctx, t, manager, keyType, keyType, "xn--80adjurfhd.xn--p1ai") // проверка.рф }) t.Run("OneCertCamelCase", func(t *testing.T) { + t.Parallel() checkOkDomain(ctx, t, manager, keyType, keyType, "onecertCamelCase.ru") }) t.Run("Locked", func(t *testing.T) { + t.Parallel() domain := "locked.ru" cert, err := manager.GetCertificate(createTLSHello(ctx, keyType, domain)) @@ -221,8 +229,9 @@ func getCertificatesTestsKeyType(t *testing.T, manager *Manager, keyType KeyType //nolint[:dupl] t.Run("ParallelCert", func(t *testing.T) { - // change top loevel logger - // no parallelize + t.Parallel() + + // change top level logger oldLogger := logger logger = zap.NewNop() defer func() { @@ -257,6 +266,7 @@ func getCertificatesTestsKeyType(t *testing.T, manager *Manager, keyType KeyType }) t.Run("RenewSoonExpiredCert", func(t *testing.T) { + t.Parallel() const domain = "soon-expired.com" // issue certificate @@ -299,6 +309,7 @@ func getCertificatesTestsKeyType(t *testing.T, manager *Manager, keyType KeyType }) t.Run("Force-rsa", func(t *testing.T) { + t.Parallel() checkOkDomain(ctx, t, manager, keyType, KeyRSA, forceRsaDomain) }) } diff --git a/internal/cert_manager/manager_issues_test.go b/internal/cert_manager/manager_issues_test.go new file mode 100644 index 00000000..311e1d9c --- /dev/null +++ b/internal/cert_manager/manager_issues_test.go @@ -0,0 +1,52 @@ +package cert_manager + +import ( + "context" + "errors" + "github.com/gojuno/minimock/v3" + "github.com/maxatome/go-testdeep" + "github.com/rekby/lets-proxy2/internal/th" + "golang.org/x/crypto/acme" + "golang.org/x/xerrors" + "testing" +) + +// Test nil in getAuthorized +func TestIssue_134(t *testing.T) { + t.Parallel() + + ctx, ctxCancel := th.TestContext(t) + defer ctxCancel() + + mc := minimock.NewController(t) + defer mc.Finish() + + td := testdeep.NewT(t) + + const testDomain = "test.com" + testURL := "http://test" + client := NewAcmeClientMock(mc) + client.AuthorizeOrderMock.Set(func(ctx context.Context, id []acme.AuthzID, opt ...acme.OrderOption) (op1 *acme.Order, err error) { + if len(id) == 1 && id[0].Value == testDomain { + return &acme.Order{ + Status: acme.StatusPending, + AuthzURLs: []string{testURL}, + }, nil + } + t.Fatalf("Unexpected args: %#v", id) + return nil, errors.New("Unexpected args") + }) + + testErr := xerrors.New("testErr") + client.GetAuthorizationMock.Set(func(ctx context.Context, url string) (ap1 *acme.Authorization, err error) { + if url != testURL { + t.Fatalf("Unexpected args: %#v", url) + } + return nil, testErr + }) + + m := &Manager{Client: client} + res, err := m.createOrderForDomains(ctx, testDomain) + td.Nil(res) + td.True(xerrors.Is(err, testErr)) +} diff --git a/internal/cert_manager/manager_test.go b/internal/cert_manager/manager_test.go index a169a6b0..91b193a1 100644 --- a/internal/cert_manager/manager_test.go +++ b/internal/cert_manager/manager_test.go @@ -11,7 +11,6 @@ import ( "crypto/tls" "crypto/x509" "fmt" - "golang.org/x/xerrors" "net" "net/http" "testing" @@ -217,29 +216,3 @@ func createManager(t *testing.T) (res testManagerContext, cancel func()) { ctxCancel() } } - -// Test nil in getAuthorized -func TestIssue_134(t *testing.T) { - ctx, ctxCancel := th.TestContext(t) - defer ctxCancel() - - mc := minimock.NewController(t) - defer mc.Finish() - - td := testdeep.NewT(t) - - testURL := "http://test" - client := NewAcmeClientMock(mc) - client.AuthorizeOrderMock.Return(&acme.Order{ - Status: acme.StatusPending, - AuthzURLs: []string{testURL}, - }, nil) - - testErr := xerrors.New("testErr") - client.GetAuthorizationMock.Expect(ctx, testURL).Return(nil, testErr) - - m := &Manager{Client: client} - res, err := m.createOrderForDomains(ctx, "test") - td.Nil(res) - td.True(xerrors.Is(err, testErr)) -}