Merge pull request #5226 from SgtCoDFish/issuer-timeout

ACME Timeout Increases

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,
 		})
 }

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)
 }

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",
     ],

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()
 

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",

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()

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"],
+)

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
+)

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",

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()