From 1834afaa00605a4c21ff7a4247cef7e2c32afa3b Mon Sep 17 00:00:00 2001 From: irbekrm Date: Wed, 18 Jan 2023 17:41:02 +0000 Subject: [PATCH 1/4] A bunch of comments on webhook solver functionality With the goal of making folks working on these parts of code be aware that this is the one bit that will be imported in external projects Signed-off-by: irbekrm --- pkg/acme/webhook/apiserver/apiserver.go | 5 ++- pkg/acme/webhook/cmd/cmd.go | 5 +++ pkg/acme/webhook/cmd/server/start.go | 5 +++ pkg/acme/webhook/webhook.go | 5 ++- test/acme/dns/fixture.go | 50 +++++++++++++------------ 5 files changed, 44 insertions(+), 26 deletions(-) diff --git a/pkg/acme/webhook/apiserver/apiserver.go b/pkg/acme/webhook/apiserver/apiserver.go index acfc12b21..e97c5580c 100644 --- a/pkg/acme/webhook/apiserver/apiserver.go +++ b/pkg/acme/webhook/apiserver/apiserver.go @@ -112,7 +112,10 @@ func (c *Config) Complete() CompletedConfig { return CompletedConfig{&completedCfg} } -// New returns a new instance of AdmissionServer from the given config. +// New returns a new instance of apiserver from the given config. Each of the +// configured solvers will have an API GroupVersion registered with the new +// apiserver and will have its Initialize function passed as post-start hook +// with the server. func (c completedConfig) New() (*ChallengeServer, error) { genericServer, err := c.GenericConfig.New("challenge-server", genericapiserver.NewEmptyDelegate()) // completion is done in Complete, no need for a second time if err != nil { diff --git a/pkg/acme/webhook/cmd/cmd.go b/pkg/acme/webhook/cmd/cmd.go index 5e6c0ea18..e824b8e93 100644 --- a/pkg/acme/webhook/cmd/cmd.go +++ b/pkg/acme/webhook/cmd/cmd.go @@ -29,6 +29,11 @@ import ( logf "github.com/cert-manager/cert-manager/pkg/logs" ) +// RunWebhookServer creates and starts a new apiserver that acts as a external +// webhook server for solving DNS challenges using the provided solver +// implementations. This can be used as an entry point by external webhook +// implementations, see +// https://github.com/cert-manager/webhook-example/blob/899c408751425f8d0842b61c0e62fd8035d00316/main.go#L23-L31 func RunWebhookServer(groupName string, hooks ...webhook.Solver) { stopCh, exit := util.SetupExitHandler(util.GracefulShutdown) defer exit() // This function might call os.Exit, so defer last diff --git a/pkg/acme/webhook/cmd/server/start.go b/pkg/acme/webhook/cmd/server/start.go index 46e44d706..400070216 100644 --- a/pkg/acme/webhook/cmd/server/start.go +++ b/pkg/acme/webhook/cmd/server/start.go @@ -97,6 +97,9 @@ func (o *WebhookServerOptions) Complete() error { return nil } +// Config creates a new webhook server config that includes generic upstream +// apiserver options, rest client config and the Solvers configured for this +// webhook server func (o WebhookServerOptions) Config() (*apiserver.Config, error) { // TODO have a "real" external address if err := o.RecommendedOptions.SecureServing.MaybeDefaultWithSelfSignedCerts("localhost", nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil { @@ -118,6 +121,8 @@ func (o WebhookServerOptions) Config() (*apiserver.Config, error) { return config, nil } +// RunWebhookServer creates a new apiserver, registers an API Group for each of +// the configured solvers and runs the new apiserver. func (o WebhookServerOptions) RunWebhookServer(stopCh <-chan struct{}) error { config, err := o.Config() if err != nil { diff --git a/pkg/acme/webhook/webhook.go b/pkg/acme/webhook/webhook.go index dfccbc265..c1480a13b 100644 --- a/pkg/acme/webhook/webhook.go +++ b/pkg/acme/webhook/webhook.go @@ -24,7 +24,9 @@ import ( whapi "github.com/cert-manager/cert-manager/pkg/acme/webhook/apis/acme/v1alpha1" ) -// Solver has the functionality to solve ACME challenges. +// Solver has the functionality to solve ACME challenges. This interface is +// implemented internally by RFC2136 DNS provider and by external webhook solver +// implementations see https://github.com/cert-manager/webhook-example type Solver interface { // Name is the name of this ACME solver as part of the API group. // This must match what you configure in the ACME Issuer's DNS01 config. @@ -41,5 +43,6 @@ type Solver interface { CleanUp(ch *whapi.ChallengeRequest) error // Initialize is called as a post-start hook when the apiserver starts. + // https://github.com/kubernetes/apiserver/blob/release-1.26/pkg/server/hooks.go#L32-L42 Initialize(kubeClientConfig *restclient.Config, stopCh <-chan struct{}) error } diff --git a/test/acme/dns/fixture.go b/test/acme/dns/fixture.go index abdac67f2..43bd998ae 100644 --- a/test/acme/dns/fixture.go +++ b/test/acme/dns/fixture.go @@ -81,6 +81,32 @@ type fixture struct { propagationLimit time.Duration } +// RunConformance will execute all conformance tests using the supplied +// configuration These conformance tests should be run by all external DNS +// solver webhook implementations, see +// https://github.com/cert-manager/webhook-example +func (f *fixture) RunConformance(t *testing.T) { + defer f.setup(t)() + t.Run("Conformance", func(t *testing.T) { + f.RunBasic(t) + f.RunExtended(t) + }) +} + +func (f *fixture) RunBasic(t *testing.T) { + defer f.setup(t)() + t.Run("Basic", func(t *testing.T) { + t.Run("PresentRecord", f.TestBasicPresentRecord) + }) +} + +func (f *fixture) RunExtended(t *testing.T) { + defer f.setup(t)() + t.Run("Extended", func(t *testing.T) { + t.Run("DeletingOneRecordRetainsOthers", f.TestExtendedDeletingOneRecordRetainsOthers) + }) +} + func (f *fixture) setup(t *testing.T) func() { f.setupLock.Lock() defer f.setupLock.Unlock() @@ -127,27 +153,3 @@ func (f *fixture) setup(t *testing.T) func() { stopFunc() } } - -// RunConformance will execute all conformance tests using the supplied -// configuration -func (f *fixture) RunConformance(t *testing.T) { - defer f.setup(t)() - t.Run("Conformance", func(t *testing.T) { - f.RunBasic(t) - f.RunExtended(t) - }) -} - -func (f *fixture) RunBasic(t *testing.T) { - defer f.setup(t)() - t.Run("Basic", func(t *testing.T) { - t.Run("PresentRecord", f.TestBasicPresentRecord) - }) -} - -func (f *fixture) RunExtended(t *testing.T) { - defer f.setup(t)() - t.Run("Extended", func(t *testing.T) { - t.Run("DeletingOneRecordRetainsOthers", f.TestExtendedDeletingOneRecordRetainsOthers) - }) -} From 216b60e98b2bb2ee7eb368d5b8774b2cafc56acb Mon Sep 17 00:00:00 2001 From: irbekrm Date: Wed, 18 Jan 2023 17:41:51 +0000 Subject: [PATCH 2/4] RFC2136 solver has an init option to reset secrets lister Signed-off-by: irbekrm --- pkg/issuer/acme/dns/rfc2136/provider.go | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/pkg/issuer/acme/dns/rfc2136/provider.go b/pkg/issuer/acme/dns/rfc2136/provider.go index 874b8689d..271b369cb 100644 --- a/pkg/issuer/acme/dns/rfc2136/provider.go +++ b/pkg/issuer/acme/dns/rfc2136/provider.go @@ -37,6 +37,8 @@ const SolverName = "rfc2136" type Solver struct { secretLister corelisters.SecretLister + // options to apply when the lister gets initialized + initOpts []Option // If specified, namespace will cause the rfc2136 provider to limit the // scope of the lister/watcher to a single namespace, to allow for @@ -58,6 +60,21 @@ func WithSecretsLister(secretLister corelisters.SecretLister) Option { } } +// InitializeResetLister is a hack to make RFC2136 solver fit the Solver +// interface. Unlike external solvers that are run as apiserver implementations, +// this solver is created as part of challenge controller initialization. That +// makes its Initialize method not fit the Solver interface very well as we want +// a way to initialize the solver with the existing Secrets lister rather than a +// new kube apiserver client. InitializeResetLister allows to reset secrets +// lister when Initialize function is called so that a new lister can be +// created. This is useful in tests where a kube clientset can get recreated for +// an existing solver (which would not happen when this solver runs normally). +func InitializeResetLister() Option { + return func(s *Solver) { + s.initOpts = []Option{func(s *Solver) { s.secretLister = nil }} + } +} + func New(opts ...Option) *Solver { s := &Solver{} for _, o := range opts { @@ -99,12 +116,12 @@ func (s *Solver) CleanUp(ch *whapi.ChallengeRequest) error { } func (s *Solver) Initialize(kubeClientConfig *restclient.Config, stopCh <-chan struct{}) error { + for _, opt := range s.initOpts { + opt(s) + } // 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 + // functionality and is currently only used in integration tests. if s.secretLister == nil { cl, err := kubernetes.NewForConfig(kubeClientConfig) if err != nil { From 644a46c8fe6c0edba9a31802582113db2bdd7eaa Mon Sep 17 00:00:00 2001 From: irbekrm Date: Wed, 18 Jan 2023 17:43:34 +0000 Subject: [PATCH 3/4] Resets secrets lister in RFC2136 conformance tests The way the tests run (a new kube apiserver with a different client created for the same initialized solver) is not how this solver would actually run Signed-off-by: irbekrm --- test/acme/dns/fixture.go | 28 ++----------------- test/acme/dns/options.go | 10 +++++-- .../rfc2136_dns01/provider_test.go | 4 +-- 3 files changed, 11 insertions(+), 31 deletions(-) diff --git a/test/acme/dns/fixture.go b/test/acme/dns/fixture.go index 43bd998ae..8e2a7b30c 100644 --- a/test/acme/dns/fixture.go +++ b/test/acme/dns/fixture.go @@ -24,12 +24,10 @@ 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" ) @@ -44,9 +42,7 @@ 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 - testSolverType string - + testSolver webhook.Solver resolvedFQDN string resolvedZone string allowAmbientCredentials bool @@ -126,27 +122,7 @@ func (f *fixture) setup(t *testing.T) func() { stopCh := make(chan struct{}) - 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) + f.testSolver.Initialize(env.Config, stopCh) return func() { close(stopCh) diff --git a/test/acme/dns/options.go b/test/acme/dns/options.go index 053eb358e..d5c36bffc 100644 --- a/test/acme/dns/options.go +++ b/test/acme/dns/options.go @@ -23,6 +23,7 @@ import ( "strings" "time" + "github.com/cert-manager/cert-manager/pkg/acme/webhook" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ) @@ -30,10 +31,13 @@ import ( type Option func(*fixture) // NewFixture constructs a new *fixture, applying the given Options before -// returning. -func NewFixture(solverType string, opts ...Option) *fixture { +// returning. Solver is an implementation of +// https://github.com/cert-manager/cert-manager/blob/v1.11.0/pkg/acme/webhook/webhook.go#L27-L45 +// and could be RFC2136 solver or any of external solvers that run these +// conformance tests. +func NewFixture(solver webhook.Solver, opts ...Option) *fixture { f := &fixture{ - testSolverType: solverType, + testSolver: solver, } for _, o := range opts { o(f) diff --git a/test/integration/rfc2136_dns01/provider_test.go b/test/integration/rfc2136_dns01/provider_test.go index e22128b68..b950657b1 100644 --- a/test/integration/rfc2136_dns01/provider_test.go +++ b/test/integration/rfc2136_dns01/provider_test.go @@ -59,7 +59,7 @@ func TestRunSuiteWithTSIG(t *testing.T) { TSIGKeyName: rfc2136TestTsigKeyName, } - fixture := dns.NewFixture(rfc2136.SolverName, + fixture := dns.NewFixture(rfc2136.New(rfc2136.InitializeResetLister()), 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.SolverName, + fixture := dns.NewFixture(rfc2136.New(rfc2136.InitializeResetLister()), dns.SetResolvedZone(rfc2136TestZone), dns.SetResolvedFQDN(rfc2136TestFqdn), dns.SetAllowAmbientCredentials(false), From 438c79d4e371cbb3dd96ba5f33e84e9930c1a2a4 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Thu, 19 Jan 2023 12:05:56 +0000 Subject: [PATCH 4/4] Code review feedback: fix imports Signed-off-by: irbekrm --- test/acme/dns/options.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/acme/dns/options.go b/test/acme/dns/options.go index d5c36bffc..ee3541ce4 100644 --- a/test/acme/dns/options.go +++ b/test/acme/dns/options.go @@ -23,8 +23,9 @@ import ( "strings" "time" - "github.com/cert-manager/cert-manager/pkg/acme/webhook" 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