Merge pull request #5691 from irbekrm/remove_redundant_secret_cache

Ensures that only one secrets cache is created for cert-manager controller

pkg/issuer/acme/dns/dns.go

@@ -488,9 +488,10 @@ func (s *Solver) dns01SolverForConfig(config *cmacme.ACMEChallengeSolverDNS01) (
 // NewSolver creates a Solver which can instantiate the appropriate DNS
 // provider.
 func NewSolver(ctx *controller.Context) (*Solver, error) {
+	secretsLister := ctx.KubeSharedInformerFactory.Core().V1().Secrets().Lister()
 	webhookSolvers := []webhook.Solver{
 		&webhookslv.Webhook{},
-		rfc2136.New(rfc2136.WithNamespace(ctx.Namespace)),
+		rfc2136.New(rfc2136.WithNamespace(ctx.Namespace), rfc2136.WithSecretsLister(secretsLister)),
 	}
 
 	initialized := make(map[string]webhook.Solver)

pkg/issuer/acme/dns/rfc2136/provider.go

@@ -33,6 +33,8 @@ import (
 	logf "github.com/cert-manager/cert-manager/pkg/logs"
 )
 
+const SolverName = "rfc2136"
+
 type Solver struct {
 	secretLister corelisters.SecretLister
 
@@ -50,6 +52,12 @@ func WithNamespace(ns string) Option {
 	}
 }
 
+func WithSecretsLister(secretLister corelisters.SecretLister) Option {
+	return func(s *Solver) {
+		s.secretLister = secretLister
+	}
+}
+
 func New(opts ...Option) *Solver {
 	s := &Solver{}
 	for _, o := range opts {
@@ -59,7 +67,7 @@ func New(opts ...Option) *Solver {
 }
 
 func (s *Solver) Name() string {
-	return "rfc2136"
+	return SolverName
 }
 
 func (s *Solver) Present(ch *whapi.ChallengeRequest) error {
@@ -91,18 +99,25 @@ func (s *Solver) CleanUp(ch *whapi.ChallengeRequest) error {
 }
 
 func (s *Solver) Initialize(kubeClientConfig *restclient.Config, stopCh <-chan struct{}) error {
-	cl, err := kubernetes.NewForConfig(kubeClientConfig)
-	if err != nil {
-		return err
+	// Only start a secrets informerfactory if it is needed (if the solver
+	// is not already initialized with a secrets lister) This is legacy
+	// functionality. If you have a secrets watcher already available in the
+	// caller, you probably want to use that to avoid double caching the
+	// Secrets
+	// TODO: refactor and remove this functionality
+	if s.secretLister == nil {
+		cl, err := kubernetes.NewForConfig(kubeClientConfig)
+		if err != nil {
+			return err
+		}
+
+		// obtain a secret lister and start the informer factory to populate the
+		// secret cache
+		factory := informers.NewSharedInformerFactoryWithOptions(cl, time.Minute*5, informers.WithNamespace(s.namespace))
+		s.secretLister = factory.Core().V1().Secrets().Lister()
+		factory.Start(stopCh)
+		factory.WaitForCacheSync(stopCh)
 	}
-
-	// obtain a secret lister and start the informer factory to populate the
-	// secret cache
-	factory := informers.NewSharedInformerFactoryWithOptions(cl, time.Minute*5, informers.WithNamespace(s.namespace))
-	s.secretLister = factory.Core().V1().Secrets().Lister()
-	factory.Start(stopCh)
-	factory.WaitForCacheSync(stopCh)
-
 	return nil
 }
 

test/acme/dns/fixture.go

@@ -24,10 +24,12 @@ import (
 	"time"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
 	"github.com/cert-manager/cert-manager/pkg/acme/webhook"
+	"github.com/cert-manager/cert-manager/pkg/issuer/acme/dns/rfc2136"
 	"github.com/cert-manager/cert-manager/test/internal/apiserver"
 )
 
@@ -42,7 +44,8 @@ func init() {
 type fixture struct {
 	// testSolver is the actual DNS solver that is under test.
 	// It is set when calling the NewFixture function.
-	testSolver webhook.Solver
+	testSolver     webhook.Solver
+	testSolverType string
 
 	resolvedFQDN            string
 	resolvedZone            string
@@ -96,7 +99,28 @@ func (f *fixture) setup(t *testing.T) func() {
 	f.clientset = cl
 
 	stopCh := make(chan struct{})
-	f.testSolver.Initialize(env.Config, stopCh)
+
+	var testSolver webhook.Solver
+	switch f.testSolverType {
+	case rfc2136.SolverName:
+		cl, err := kubernetes.NewForConfig(env.Config)
+		if err != nil {
+			t.Errorf("error initializing solver: %#+v", err)
+		}
+
+		// obtain a secret lister and start the informer factory to populate the
+		// secret cache
+		factory := informers.NewSharedInformerFactoryWithOptions(cl, time.Minute*5)
+		secretLister := factory.Core().V1().Secrets().Lister()
+		factory.Start(stopCh)
+		factory.WaitForCacheSync(stopCh)
+		testSolver = rfc2136.New(rfc2136.WithSecretsLister(secretLister))
+		f.testSolver = testSolver
+	default:
+		t.Errorf("unknown solver type: %s", f.testSolverType)
+	}
+
+	testSolver.Initialize(env.Config, stopCh)
 
 	return func() {
 		close(stopCh)

test/acme/dns/options.go

@@ -24,8 +24,6 @@ import (
 	"time"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-
-	"github.com/cert-manager/cert-manager/pkg/acme/webhook"
 )
 
 // Option applies a configuration option to the test fixture being built
@@ -33,9 +31,9 @@ type Option func(*fixture)
 
 // NewFixture constructs a new *fixture, applying the given Options before
 // returning.
-func NewFixture(solver webhook.Solver, opts ...Option) *fixture {
+func NewFixture(solverType string, opts ...Option) *fixture {
 	f := &fixture{
-		testSolver: solver,
+		testSolverType: solverType,
 	}
 	for _, o := range opts {
 		o(f)

test/integration/rfc2136_dns01/provider_test.go

@@ -59,7 +59,7 @@ func TestRunSuiteWithTSIG(t *testing.T) {
 		TSIGKeyName: rfc2136TestTsigKeyName,
 	}
 
-	fixture := dns.NewFixture(&rfc2136.Solver{},
+	fixture := dns.NewFixture(rfc2136.SolverName,
 		dns.SetResolvedZone(rfc2136TestZone),
 		dns.SetResolvedFQDN(rfc2136TestFqdn),
 		dns.SetAllowAmbientCredentials(false),
@@ -91,7 +91,7 @@ func TestRunSuiteNoTSIG(t *testing.T) {
 		Nameserver: server.ListenAddr(),
 	}
 
-	fixture := dns.NewFixture(&rfc2136.Solver{},
+	fixture := dns.NewFixture(rfc2136.SolverName,
 		dns.SetResolvedZone(rfc2136TestZone),
 		dns.SetResolvedFQDN(rfc2136TestFqdn),
 		dns.SetAllowAmbientCredentials(false),