diff --git a/pkg/acme/accounts/client.go b/pkg/acme/accounts/client.go index 9bc41d43a..b11f87b04 100644 --- a/pkg/acme/accounts/client.go +++ b/pkg/acme/accounts/client.go @@ -32,6 +32,13 @@ import ( "github.com/cert-manager/cert-manager/pkg/metrics" ) +const ( + // defaultACMEHTTPTimeout sets the default maximum time that an individual HTTP request can take when doing ACME operations. + // Note that there may be other timeouts - e.g. dial timeouts or TLS handshake timeouts - which will be smaller than this. This + // timeout is the overall timeout for the entire request. + defaultACMEHTTPTimeout = time.Second * 90 +) + // NewClientFunc is a function type for building a new ACME client. type NewClientFunc func(*http.Client, cmacme.ACMEIssuer, *rsa.PrivateKey, string) acmecl.Interface @@ -70,6 +77,6 @@ func BuildHTTPClient(metrics *metrics.Metrics, skipTLSVerify bool) *http.Client TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, }, - Timeout: time.Second * 30, + Timeout: defaultACMEHTTPTimeout, }) } diff --git a/pkg/acme/client/middleware/logger.go b/pkg/acme/client/middleware/logger.go index 36ace3471..73c8dbfd1 100644 --- a/pkg/acme/client/middleware/logger.go +++ b/pkg/acme/client/middleware/logger.go @@ -18,7 +18,6 @@ package middleware import ( "context" - "time" "github.com/go-logr/logr" "golang.org/x/crypto/acme" @@ -27,10 +26,6 @@ import ( logf "github.com/cert-manager/cert-manager/pkg/logs" ) -const ( - timeout = time.Second * 10 -) - func NewLogger(baseCl client.Interface) client.Interface { return &Logger{ baseCl: baseCl, @@ -49,135 +44,95 @@ var _ client.Interface = &Logger{} func (l *Logger) AuthorizeOrder(ctx context.Context, id []acme.AuthzID, opt ...acme.OrderOption) (*acme.Order, error) { l.log.V(logf.TraceLevel).Info("Calling AuthorizeOrder") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.AuthorizeOrder(ctx, id, opt...) } func (l *Logger) GetOrder(ctx context.Context, url string) (*acme.Order, error) { l.log.V(logf.TraceLevel).Info("Calling GetOrder") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.GetOrder(ctx, url) } func (l *Logger) FetchCert(ctx context.Context, url string, bundle bool) ([][]byte, error) { l.log.V(logf.TraceLevel).Info("Calling FetchCert") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.FetchCert(ctx, url, bundle) } func (l *Logger) ListCertAlternates(ctx context.Context, url string) ([]string, error) { l.log.V(logf.TraceLevel).Info("Calling ListCertAlternates") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.ListCertAlternates(ctx, url) } func (l *Logger) WaitOrder(ctx context.Context, url string) (*acme.Order, error) { l.log.V(logf.TraceLevel).Info("Calling WaitOrder") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.WaitOrder(ctx, url) } func (l *Logger) CreateOrderCert(ctx context.Context, finalizeURL string, csr []byte, bundle bool) (der [][]byte, certURL string, err error) { l.log.V(logf.TraceLevel).Info("Calling CreateOrderCert") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.CreateOrderCert(ctx, finalizeURL, csr, bundle) } func (l *Logger) Accept(ctx context.Context, chal *acme.Challenge) (*acme.Challenge, error) { l.log.V(logf.TraceLevel).Info("Calling Accept") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.Accept(ctx, chal) } func (l *Logger) GetChallenge(ctx context.Context, url string) (*acme.Challenge, error) { l.log.V(logf.TraceLevel).Info("Calling GetChallenge") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.GetChallenge(ctx, url) } func (l *Logger) GetAuthorization(ctx context.Context, url string) (*acme.Authorization, error) { l.log.V(logf.TraceLevel).Info("Calling GetAuthorization") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.GetAuthorization(ctx, url) } func (l *Logger) WaitAuthorization(ctx context.Context, url string) (*acme.Authorization, error) { l.log.V(logf.TraceLevel).Info("Calling WaitAuthorization") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.WaitAuthorization(ctx, url) } func (l *Logger) Register(ctx context.Context, a *acme.Account, prompt func(tosURL string) bool) (*acme.Account, error) { l.log.V(logf.TraceLevel).Info("Calling Register") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.Register(ctx, a, prompt) } func (l *Logger) GetReg(ctx context.Context, url string) (*acme.Account, error) { l.log.V(logf.TraceLevel).Info("Calling GetReg") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.GetReg(ctx, url) } func (l *Logger) HTTP01ChallengeResponse(token string) (string, error) { l.log.V(logf.TraceLevel).Info("Calling HTTP01ChallengeResponse") + return l.baseCl.HTTP01ChallengeResponse(token) } func (l *Logger) DNS01ChallengeRecord(token string) (string, error) { l.log.V(logf.TraceLevel).Info("Calling DNS01ChallengeRecord") + return l.baseCl.DNS01ChallengeRecord(token) } func (l *Logger) Discover(ctx context.Context) (acme.Directory, error) { l.log.V(logf.TraceLevel).Info("Calling Discover") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.Discover(ctx) } func (l *Logger) UpdateReg(ctx context.Context, a *acme.Account) (*acme.Account, error) { l.log.V(logf.TraceLevel).Info("Calling UpdateReg") - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - return l.baseCl.UpdateReg(ctx, a) } diff --git a/pkg/controller/BUILD.bazel b/pkg/controller/BUILD.bazel index eec331c26..edaef663a 100644 --- a/pkg/controller/BUILD.bazel +++ b/pkg/controller/BUILD.bazel @@ -70,6 +70,7 @@ filegroup( "//pkg/controller/certificates:all-srcs", "//pkg/controller/certificatesigningrequests:all-srcs", "//pkg/controller/clusterissuers:all-srcs", + "//pkg/controller/globals:all-srcs", "//pkg/controller/issuers:all-srcs", "//pkg/controller/test:all-srcs", ], diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index f4af1d209..5660c0dfe 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -157,7 +157,8 @@ func NewController( } func (c *controller) ProcessItem(ctx context.Context, key string) error { - // Set context deadline for full sync in 10 seconds + // TODO: Change to globals.DefaultControllerContextTimeout as part of a wider effort to ensure we have + // failsafe timeouts in every controller ctx, cancel := context.WithTimeout(ctx, time.Second*10) defer cancel() diff --git a/pkg/controller/clusterissuers/BUILD.bazel b/pkg/controller/clusterissuers/BUILD.bazel index 9475e70cc..018fd2886 100644 --- a/pkg/controller/clusterissuers/BUILD.bazel +++ b/pkg/controller/clusterissuers/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//pkg/client/clientset/versioned:go_default_library", "//pkg/client/listers/certmanager/v1:go_default_library", "//pkg/controller:go_default_library", + "//pkg/controller/globals:go_default_library", "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", "//pkg/util/feature:go_default_library", diff --git a/pkg/controller/clusterissuers/sync.go b/pkg/controller/clusterissuers/sync.go index 3167ac3a7..9b09c9e40 100644 --- a/pkg/controller/clusterissuers/sync.go +++ b/pkg/controller/clusterissuers/sync.go @@ -18,7 +18,6 @@ package clusterissuers import ( "context" - "time" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" @@ -28,6 +27,7 @@ import ( "github.com/cert-manager/cert-manager/internal/controller/feature" internalissuers "github.com/cert-manager/cert-manager/internal/controller/issuers" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/cert-manager/cert-manager/pkg/controller/globals" logf "github.com/cert-manager/cert-manager/pkg/logs" utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" ) @@ -41,8 +41,7 @@ const ( func (c *controller) Sync(ctx context.Context, iss *cmapi.ClusterIssuer) (err error) { log := logf.FromContext(ctx) - // allow a maximum of 10s - ctx, cancel := context.WithTimeout(ctx, time.Second*10) + ctx, cancel := context.WithTimeout(ctx, globals.DefaultControllerContextTimeout) defer cancel() issuerCopy := iss.DeepCopy() diff --git a/pkg/controller/globals/BUILD.bazel b/pkg/controller/globals/BUILD.bazel new file mode 100644 index 000000000..f0d1134b0 --- /dev/null +++ b/pkg/controller/globals/BUILD.bazel @@ -0,0 +1,22 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["timeout.go"], + importpath = "github.com/cert-manager/cert-manager/pkg/controller/globals", + visibility = ["//visibility:public"], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) diff --git a/pkg/controller/globals/timeout.go b/pkg/controller/globals/timeout.go new file mode 100644 index 000000000..197630521 --- /dev/null +++ b/pkg/controller/globals/timeout.go @@ -0,0 +1,27 @@ +/* +Copyright 2022 The cert-manager 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 globals + +import "time" + +const ( + // DefaultControllerContextTimeout is the default maximum amount of time which a single synchronize action in some controllers + // may take before the sync will be cancelled by a context timeout. + // This timeout might not be respected on all controllers thanks to backwards compatibility concerns, but it's a goal to have + // all issuers have some default timeout which represents a default upper bound on the time they're permitted to take. + DefaultControllerContextTimeout = 2 * time.Minute +) diff --git a/pkg/controller/issuers/BUILD.bazel b/pkg/controller/issuers/BUILD.bazel index f3d56c873..cc5d759d8 100644 --- a/pkg/controller/issuers/BUILD.bazel +++ b/pkg/controller/issuers/BUILD.bazel @@ -16,6 +16,7 @@ go_library( "//pkg/client/clientset/versioned:go_default_library", "//pkg/client/listers/certmanager/v1:go_default_library", "//pkg/controller:go_default_library", + "//pkg/controller/globals:go_default_library", "//pkg/issuer:go_default_library", "//pkg/logs:go_default_library", "//pkg/util/feature:go_default_library", diff --git a/pkg/controller/issuers/sync.go b/pkg/controller/issuers/sync.go index cc9e5405f..32f3a5328 100644 --- a/pkg/controller/issuers/sync.go +++ b/pkg/controller/issuers/sync.go @@ -18,7 +18,6 @@ package issuers import ( "context" - "time" corev1 "k8s.io/api/core/v1" apiequality "k8s.io/apimachinery/pkg/api/equality" @@ -28,6 +27,7 @@ import ( "github.com/cert-manager/cert-manager/internal/controller/feature" internalissuers "github.com/cert-manager/cert-manager/internal/controller/issuers" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" + "github.com/cert-manager/cert-manager/pkg/controller/globals" logf "github.com/cert-manager/cert-manager/pkg/logs" utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature" ) @@ -41,8 +41,7 @@ const ( func (c *controller) Sync(ctx context.Context, iss *cmapi.Issuer) (err error) { log := logf.FromContext(ctx) - // allow a maximum of 10s - ctx, cancel := context.WithTimeout(ctx, time.Second*10) + ctx, cancel := context.WithTimeout(ctx, globals.DefaultControllerContextTimeout) defer cancel() issuerCopy := iss.DeepCopy()