diff --git a/pkg/controller/certificates/BUILD.bazel b/pkg/controller/certificates/BUILD.bazel index 1525be02c..381c70b09 100644 --- a/pkg/controller/certificates/BUILD.bazel +++ b/pkg/controller/certificates/BUILD.bazel @@ -46,6 +46,7 @@ filegroup( "//pkg/controller/certificates/metrics:all-srcs", "//pkg/controller/certificates/readiness:all-srcs", "//pkg/controller/certificates/requestmanager:all-srcs", + "//pkg/controller/certificates/revisionmanager:all-srcs", "//pkg/controller/certificates/trigger:all-srcs", ], tags = ["automanaged"], diff --git a/pkg/controller/certificates/revisionmanager/BUILD.bazel b/pkg/controller/certificates/revisionmanager/BUILD.bazel new file mode 100644 index 000000000..1b981bc83 --- /dev/null +++ b/pkg/controller/certificates/revisionmanager/BUILD.bazel @@ -0,0 +1,57 @@ +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "go_default_library", + srcs = ["revisionmanager_controller.go"], + importpath = "github.com/jetstack/cert-manager/pkg/controller/certificates/revisionmanager", + visibility = ["//visibility:public"], + deps = [ + "//pkg/api/util:go_default_library", + "//pkg/apis/certmanager/v1:go_default_library", + "//pkg/apis/meta/v1:go_default_library", + "//pkg/client/clientset/versioned:go_default_library", + "//pkg/client/informers/externalversions:go_default_library", + "//pkg/client/listers/certmanager/v1:go_default_library", + "//pkg/controller:go_default_library", + "//pkg/controller/certificates:go_default_library", + "//pkg/logs:go_default_library", + "//pkg/util/predicate:go_default_library", + "@com_github_go_logr_logr//:go_default_library", + "@io_k8s_apimachinery//pkg/api/errors:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_apimachinery//pkg/labels:go_default_library", + "@io_k8s_client_go//tools/cache:go_default_library", + "@io_k8s_client_go//util/workqueue:go_default_library", + ], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) + +go_test( + name = "go_default_test", + srcs = ["revisionmanager_controller_test.go"], + embed = [":go_default_library"], + deps = [ + "//pkg/apis/certmanager/v1:go_default_library", + "//pkg/apis/meta/v1:go_default_library", + "//pkg/controller:go_default_library", + "//pkg/controller/test:go_default_library", + "//pkg/logs/testing:go_default_library", + "//test/unit/gen:go_default_library", + "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", + "@io_k8s_apimachinery//pkg/runtime:go_default_library", + "@io_k8s_client_go//testing:go_default_library", + ], +) diff --git a/pkg/controller/certificates/revisionmanager/revisionmanager_controller.go b/pkg/controller/certificates/revisionmanager/revisionmanager_controller.go new file mode 100644 index 000000000..fb77d4a4c --- /dev/null +++ b/pkg/controller/certificates/revisionmanager/revisionmanager_controller.go @@ -0,0 +1,216 @@ +/* +Copyright 2021 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 revisionmanager + +import ( + "context" + "sort" + "strconv" + "time" + + "github.com/go-logr/logr" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/workqueue" + + apiutil "github.com/jetstack/cert-manager/pkg/api/util" + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" + cmclient "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" + cminformers "github.com/jetstack/cert-manager/pkg/client/informers/externalversions" + cmlisters "github.com/jetstack/cert-manager/pkg/client/listers/certmanager/v1" + controllerpkg "github.com/jetstack/cert-manager/pkg/controller" + "github.com/jetstack/cert-manager/pkg/controller/certificates" + logf "github.com/jetstack/cert-manager/pkg/logs" + "github.com/jetstack/cert-manager/pkg/util/predicate" +) + +const ( + ControllerName = "CertificateRevisionManager" +) + +var ( + certificateGvk = cmapi.SchemeGroupVersion.WithKind("Certificate") +) + +type controller struct { + certificateLister cmlisters.CertificateLister + certificateRequestLister cmlisters.CertificateRequestLister + client cmclient.Interface +} + +type revision struct { + rev int + req *cmapi.CertificateRequest +} + +func NewController(log logr.Logger, client cmclient.Interface, cmFactory cminformers.SharedInformerFactory) (*controller, workqueue.RateLimitingInterface, []cache.InformerSynced) { + // create a queue used to queue up items to be processed + queue := workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(time.Second*1, time.Second*30), ControllerName) + + // obtain references to all the informers used by this controller + certificateInformer := cmFactory.Certmanager().V1().Certificates() + certificateRequestInformer := cmFactory.Certmanager().V1().CertificateRequests() + + certificateInformer.Informer().AddEventHandler(&controllerpkg.QueuingEventHandler{Queue: queue}) + certificateRequestInformer.Informer().AddEventHandler(&controllerpkg.BlockingEventHandler{ + // Trigger reconciles on changes to any 'owned' CertificateRequest resources + WorkFunc: certificates.EnqueueCertificatesForResourceUsingPredicates(log, queue, certificateInformer.Lister(), labels.Everything(), + predicate.ResourceOwnerOf, + ), + }) + + // build a list of InformerSynced functions that will be returned by the Register method. + // the controller will only begin processing items once all of these informers have synced. + mustSync := []cache.InformerSynced{ + certificateRequestInformer.Informer().HasSynced, + certificateInformer.Informer().HasSynced, + } + + return &controller{ + certificateLister: certificateInformer.Lister(), + certificateRequestLister: certificateRequestInformer.Lister(), + client: client, + }, queue, mustSync +} + +// ProcessItem will attempt to garbage collect old CertificateRequests based +// upon `spec.revisionHistoryLimit`. This controller will only act on +// Certificates which are in a Ready state and this value is set. +func (c *controller) ProcessItem(ctx context.Context, key string) error { + log := logf.FromContext(ctx).WithValues("key", key) + + ctx = logf.NewContext(ctx, log) + namespace, name, err := cache.SplitMetaNamespaceKey(key) + if err != nil { + log.Error(err, "invalid resource key passed to ProcessItem") + return nil + } + + crt, err := c.certificateLister.Certificates(namespace).Get(name) + if apierrors.IsNotFound(err) { + log.Error(err, "certificate not found for key") + return nil + } + if err != nil { + return err + } + + log = logf.WithResource(log, crt) + + // If RevisionHistoryLimit is nil, don't attempt to garbage collect old + // CertificateRequests + if crt.Spec.RevisionHistoryLimit == nil { + return nil + } + + limit := int(*crt.Spec.RevisionHistoryLimit) + + // Only garbage collect over Certificates that are in a Ready=True condition. + if !apiutil.CertificateHasCondition(crt, cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionReady, + Status: cmmeta.ConditionTrue, + }) { + return nil + } + + // Get all CertificateRequests that are owned by this Certificate + requests, err := certificates.ListCertificateRequestsMatchingPredicates( + c.certificateRequestLister.CertificateRequests(crt.Namespace), labels.Everything(), predicate.ResourceOwnedBy(crt)) + if err != nil { + return err + } + + // Prune and sort all CertificateRequests by their revision number. + revisions := pruneSortRequestsWithRevisions(log, requests) + + // If the number of owned CertificateRequests with revisions is less than the + // revision limit, exit early. + if limit >= len(revisions) { + log.V(logf.DebugLevel).Info("request revisions within limit") + return nil + } + + // Delete requests until we hit the revision limit, oldest first. + for i := 0; i < (len(revisions) - limit); i++ { + req := revisions[i].req + logf.WithRelatedResource(log, req).WithValues("revision", revisions[i].rev).Info("garbage collecting old certificate request revsion") + + err = c.client.CertmanagerV1().CertificateRequests(req.Namespace).Delete(ctx, req.Name, metav1.DeleteOptions{}) + if err != nil { + return err + } + } + + return nil +} + +// pruneSortRequestsWithRevisions will prune the given CertificateRequests for +// those that have a valid revision number set, and return a sorted slice by +// oldest first. +func pruneSortRequestsWithRevisions(log logr.Logger, reqs []*cmapi.CertificateRequest) []revision { + var revisions []revision + + for _, req := range reqs { + log = logf.WithRelatedResource(log, req) + + if req.Annotations == nil || req.Annotations[cmapi.CertificateRequestRevisionAnnotationKey] == "" { + log.V(logf.DebugLevel).Info("skipping processing request with missing revsion") + continue + } + + rn, err := strconv.Atoi(req.Annotations[cmapi.CertificateRequestRevisionAnnotationKey]) + if err != nil { + log.Error(err, "failed to parse request revsion") + continue + } + + revisions = append(revisions, revision{rn, req}) + } + + sort.SliceStable(revisions, func(i, j int) bool { + return revisions[i].rev < revisions[j].rev + }) + + return revisions +} + +// controllerWrapper wraps the `controller` structure to make it implement +// the controllerpkg.queueingController interface +type controllerWrapper struct { + *controller +} + +func (c *controllerWrapper) Register(ctx *controllerpkg.Context) (workqueue.RateLimitingInterface, []cache.InformerSynced, error) { + // construct a new named logger to be reused throughout the controller + log := logf.FromContext(ctx.RootContext, ControllerName) + + ctrl, queue, mustSync := NewController(log, ctx.CMClient, ctx.SharedInformerFactory) + c.controller = ctrl + + return queue, mustSync, nil +} + +func init() { + controllerpkg.Register(ControllerName, func(ctx *controllerpkg.Context) (controllerpkg.Interface, error) { + return controllerpkg.NewBuilder(ctx, ControllerName). + For(&controllerWrapper{}). + Complete() + }) +} diff --git a/pkg/controller/certificates/revisionmanager/revisionmanager_controller_test.go b/pkg/controller/certificates/revisionmanager/revisionmanager_controller_test.go new file mode 100644 index 000000000..6ac6de567 --- /dev/null +++ b/pkg/controller/certificates/revisionmanager/revisionmanager_controller_test.go @@ -0,0 +1,386 @@ +/* +Copyright 2021 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 revisionmanager + +import ( + "context" + "reflect" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + coretesting "k8s.io/client-go/testing" + + cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" + cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" + controllerpkg "github.com/jetstack/cert-manager/pkg/controller" + testpkg "github.com/jetstack/cert-manager/pkg/controller/test" + logtest "github.com/jetstack/cert-manager/pkg/logs/testing" + "github.com/jetstack/cert-manager/test/unit/gen" +) + +func TestProcessItem(t *testing.T) { + baseCrt := gen.Certificate("test-cert", + gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("uid-1"), + ) + baseCRNoOwner := gen.CertificateRequest("test-cr", + gen.SetCertificateRequestNamespace("testns"), + ) + baseCR := gen.CertificateRequestFrom(baseCRNoOwner, + gen.AddCertificateRequestOwnerReferences(*metav1.NewControllerRef( + baseCrt, cmapi.SchemeGroupVersion.WithKind("Certificate")), + ), + ) + + tests := map[string]struct { + // key that should be passed to ProcessItem. + // if not set, the 'namespace/name' of the 'Certificate' field will be used. + // if neither is set, the key will be "" + key string + + // Certificate to be synced for the test. + // if not set, the 'key' will be passed to ProcessItem instead. + certificate *cmapi.Certificate + + // Request, if set, will exist in the apiserver before the test is run. + requests []runtime.Object + + expectedActions []testpkg.Action + + // err is the expected error text returned by the controller, if any. + err string + }{ + "do nothing if an empty 'key' is used": {}, + "do nothing if an invalid 'key' is used": { + key: "abc/def/ghi", + }, + "do nothing if a key references a Certificate that does not exist": { + key: "namespace/name", + }, + "do nothing if Certificate is not in a Ready=True state": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionFalse}), + gen.SetCertificateRevisionHistoryLimit(1), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("1"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("2"), + ), + }, + }, + "do nothing if no requests exist": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevisionHistoryLimit(1), + ), + }, + "do nothing if requests don't have or bad revisions set": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevisionHistoryLimit(1), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("abc"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-2"), + ), + }, + }, + "do nothing if requests aren't owned by this Certificate": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevisionHistoryLimit(1), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCRNoOwner, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("1"), + ), + gen.CertificateRequestFrom(baseCRNoOwner, + gen.SetCertificateRequestName("cr-2"), + gen.SetCertificateRequestRevision("2"), + ), + }, + }, + "do nothing if number of revisions matches that of the limit": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevisionHistoryLimit(2), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("1"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-2"), + gen.SetCertificateRequestRevision("2"), + ), + }, + }, + "do nothing if revision limit is not set": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("1"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-2"), + gen.SetCertificateRequestRevision("2"), + ), + }, + }, + "delete 1 request if limit is 1 and 2 requests exist": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevisionHistoryLimit(1), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-2"), + gen.SetCertificateRequestRevision("2"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("1"), + ), + }, + expectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewDeleteAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", "cr-1")), + }, + }, + "delete 3 requests if limit is 3 and 6 requests exist": { + certificate: gen.CertificateFrom(baseCrt, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionReady, Status: cmmeta.ConditionTrue}), + gen.SetCertificateRevisionHistoryLimit(3), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-2"), + gen.SetCertificateRequestRevision("2"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-3"), + gen.SetCertificateRequestRevision("3"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-1"), + gen.SetCertificateRequestRevision("1"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-4"), + gen.SetCertificateRequestRevision("11"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-5"), + gen.SetCertificateRequestRevision("11"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestName("cr-6"), + gen.SetCertificateRequestRevision("2"), + ), + }, + expectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewDeleteAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", "cr-1")), + testpkg.NewAction(coretesting.NewDeleteAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", "cr-2")), + testpkg.NewAction(coretesting.NewDeleteAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", "cr-6")), + }, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + // Create and initialise a new unit test builder + builder := &testpkg.Builder{ + T: t, + ExpectedEvents: nil, + ExpectedActions: test.expectedActions, + StringGenerator: func(i int) string { return "notrandom" }, + } + if test.certificate != nil { + builder.CertManagerObjects = append(builder.CertManagerObjects, test.certificate) + } + for _, req := range test.requests { + builder.CertManagerObjects = append(builder.CertManagerObjects, req) + } + builder.Init() + + // Register informers used by the controller using the registration wrapper + w := &controllerWrapper{} + _, _, err := w.Register(builder.Context) + if err != nil { + t.Fatal(err) + } + // Start the informers and begin processing updates + builder.Start() + defer builder.Stop() + + key := test.key + if key == "" && test.certificate != nil { + key, err = controllerpkg.KeyFunc(test.certificate) + if err != nil { + t.Fatal(err) + } + } + + // Call ProcessItem + err = w.controller.ProcessItem(context.Background(), key) + switch { + case err != nil: + if test.err != err.Error() { + t.Errorf("error text did not match, got=%s, exp=%s", err.Error(), test.err) + } + default: + if test.err != "" { + t.Errorf("got no error but expected: %s", test.err) + } + } + + if err := builder.AllEventsCalled(); err != nil { + builder.T.Error(err) + } + if err := builder.AllActionsExecuted(); err != nil { + builder.T.Error(err) + } + if err := builder.AllReactorsCalled(); err != nil { + builder.T.Error(err) + } + }) + } +} + +func TestPruneSortRequestsWithRevisions(t *testing.T) { + baseCR := gen.CertificateRequest("test") + + tests := map[string]struct { + input []*cmapi.CertificateRequest + exp []revision + }{ + "an empty list of request should return empty": { + input: nil, + exp: nil, + }, + "a single request with no revision set should return empty": { + input: []*cmapi.CertificateRequest{ + baseCR, + }, + exp: nil, + }, + "a single request with revision set should return single request": { + input: []*cmapi.CertificateRequest{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("123"), + ), + }, + exp: []revision{ + { + rev: 123, + req: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("123"), + ), + }, + }, + }, + "two requests with one badly formed revision should return single request": { + input: []*cmapi.CertificateRequest{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("123"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("hello"), + ), + }, + exp: []revision{ + { + rev: 123, + req: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("123"), + ), + }, + }, + }, + "multiple requests with some with good revsions should return list in order": { + input: []*cmapi.CertificateRequest{ + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("123"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("hello"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("3"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("cert-manager"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("900"), + ), + gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("1"), + ), + }, + exp: []revision{ + { + rev: 1, + req: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("1"), + ), + }, + { + rev: 3, + req: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("3"), + ), + }, + { + rev: 123, + req: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("123"), + ), + }, + { + rev: 900, + req: gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestRevision("900"), + ), + }, + }, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + log := logtest.TestLogger{T: t} + output := pruneSortRequestsWithRevisions(log, test.input) + if !reflect.DeepEqual(test.exp, output) { + t.Errorf("unexpected prune sort response, exp=%v got=%v", + test.exp, output) + } + }) + } +}