weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Merge pull request #7161 from wallrj/7147-cainjector-metadata-only-cache

Browse Source 
Reduce memory usage by only caching the metadata of Secret resources
This commit is contained in:
cert-manager-prow[bot] 2024-07-12 10:31:19 +00:00 committed by GitHub
commit c746fdf356
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 106 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
cmd/cainjector/app/controller.go
View File

@ -23,6 +23,7 @@ import (
"net/http"
"time"
corev1 "k8s.io/api/core/v1"
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
@ -81,6 +82,46 @@ func Run(opts *config.CAInjectorConfiguration, ctx context.Context) error {
ReaderFailOnMissingInformer: true,
DefaultNamespaces: defaultNamespaces,
},
Client: client.Options{
Cache: &client.CacheOptions{
// Why do we disable the cache for v1.Secret?
//
// 1. To reduce memory use of cainjector, by disabling
// in-memory cache of Secret resources.
// 2. To reduce the load on the K8S API server when
// cainjector starts up, caused by the initial listing of
// Secret resources in the cluster.
//
// Clusters may contain many and / or large Secret
// resources.
// For example OpenShift clusters may have thousands of
// ServiceAccounts and each of these has a Secret with the
// associated token.
// Or where helm is used, there will be large Secret
// resources containing the configuration of each Helm
// deployment.
//
// Ordinarily, the controller-runtime client would implicitly
// initialize a client-go cache which would list every
// Secret, including the entire data of every Secret.
// This initial list operation can place enormous load on
// the K8S API server.
//
// The problem can be alleviated by disabling the implicit cache:
// * Here in the client CacheOptions and,
// * in NewControllerManagedBy.Watches, by supplying the
// builder.OnlyMetadata option.
//
// The disadvantage is that this will cause *increased*
// ongoing load on the K8S API server later, because the
// reconciler for each injectable will GET the source Secret
// directly from the K8S API server every time the
// injectable is reconciled.
DisableFor: []client.Object{
&corev1.Secret{},
},
},
},
LeaderElection: opts.LeaderElectionConfig.Enabled,
LeaderElectionNamespace: opts.LeaderElectionConfig.Namespace,
LeaderElectionID: "cert-manager-cainjector-leader-election",

2
cmd/cainjector/go.mod
View File

@ -21,6 +21,7 @@ require (
github.com/cert-manager/cert-manager v0.0.0-00010101000000-000000000000
github.com/spf13/cobra v1.8.0
github.com/spf13/pflag v1.0.5
k8s.io/api v0.30.2
k8s.io/apiextensions-apiserver v0.30.2
k8s.io/apimachinery v0.30.2
k8s.io/client-go v0.30.2
@ -76,7 +77,6 @@ require (
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/yaml.v2 v2.4.0 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/api v0.30.2 // indirect
k8s.io/klog/v2 v2.120.1 // indirect
k8s.io/kube-openapi v0.0.0-20240521193020-835d969ad83a // indirect
k8s.io/utils v0.0.0-20240502163921-fe8a2dddb1d0 // indirect

20
design/20221205-memory-management.md
View File

@ -90,6 +90,22 @@ This proposal suggests a mechanism how to avoid caching cert-manager unrelated `
- use the same mechanism to improve memory consumption by cainjector. This proposal focuses on controller only as it is the more complex part however we need to fix this problem in cainjector too and it would be nice to be consistent
> 📖 Update: In [#7161: Reduce memory usage by only caching the metadata of Secret resources](https://github.com/cert-manager/cert-manager/pull/716199)
> we addressed the high startup memory usage of cainjector with metatdata-only caching features of controller-runtime.
> We did not use the split cache design that was implemented for the
> controller, and this contradicts the goal above: "use the same mechanism to
> improve memory consumption by cainjector ... to be consistent".
> Why? Because the split cache mechanism is overkill for cainjector.
> The split cache design is designed to reduce memory use **and** minimize the
> ongoing load on the K8S API server; which is appropriate for the controller
> because it has multiple controller loops each reading Secret resources every
> time a Certificate is reconciled.
> It is not necessary for cainjector, because cainjector reads relatively few
> Secret resources, infrequently; `cainjector` only reads Secrets having the
> `cert-manager.io/allow-direct-injection` or Secrets created from
> Certificates having that annotation. And it only reads the Secret data once
> during while reconciling the target resource.
#### Must not
- make our controllers less reliable (i.e by introducing edge cases where a cert-manager related event does not trigger a reconcile). Given the wide usage of cert-manager and the various different usage scenarios, any such edge case would be likely to occur for some users
@ -509,7 +525,7 @@ The configured `Secret` will be retrieved when the issuer is reconciled (events
#### Upstream mechanisms
There are a number of existing upstream mechanisms how to limit what gets stored in the cache. This section focuses on what is available for client-go informers which we use in cert-manager controllers, but there is a controller-runtime wrapper available for each of these mechanisms that should make it usable in cainjector as well.
##### Filtering
Filtering which objects get watched using [label or field selectors](https://github.com/kubernetes/apimachinery/blob/v0.26.0/pkg/apis/meta/v1/types.go#L328-L332). These selectors allow to filter what resources are retrieved during the initial list call and watch calls to kube apiserver by informer's `ListerWatcher` component (and therefore will end up in the cache). client-go informer factory allows configuring individual informers with [list options](https://github.com/kubernetes/client-go/blob/v12.0.0/informers/factory.go#L78-L84) that will be used [for list and watch calls](https://github.com/kubernetes/client-go/blob/v12.0.0/informers/core/v1/secret.go#L59-L72).
@ -688,7 +704,7 @@ See an example flag implementation for cainjector in https://github.com/cert-man
It might work well for cases where 'known' selectors need to be passed that we could event document such as `type!=helm.sh/release.v1`.
#### Drawbacks
- bad user experience- no straightforward way to tell if the selector actually does what was expected and an easy footgun especially when users attempt to specify which `Secret`s _should_ (rather than _shouldn't_) be watched
- users should aim to use 'negative' selectors, but that be complicated if there is a large number of random `Secret`s in cluster that don't have a unifying selector

4
pkg/controller/cainjector/indexers.go
View File

@ -20,8 +20,8 @@ import (
"context"
"github.com/go-logr/logr"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
@ -48,7 +48,7 @@ const (
func certFromSecretToInjectableMapFuncBuilder(cl client.Reader, log logr.Logger, config setup) handler.MapFunc {
return func(ctx context.Context, obj client.Object) []ctrl.Request {
secretName := types.NamespacedName{Name: obj.GetName(), Namespace: obj.GetNamespace()}
certName := owningCertForSecret(obj.(*corev1.Secret))
certName := owningCertForSecret(obj.(*metav1.PartialObjectMetadata))
if certName == nil {
return nil
}

20
pkg/controller/cainjector/reconciler.go
View File

@ -22,7 +22,6 @@ import (
"strings"
"github.com/go-logr/logr"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@ -163,16 +162,25 @@ func dropNotFound(err error) error {
return err
}
// owningCertForSecret gets the name of the owning certificate for a
// given secret, returning nil if no such object exists.
func owningCertForSecret(secret *corev1.Secret) *types.NamespacedName {
val, ok := secret.Annotations[certmanager.CertificateNameKey]
// owningCertForSecret gets the name of the owning certificate for a given
// secret, returning nil if the supplied secret does not have a
// `cert-manager.io/certificate-name` annotation.
// The secret may be a v1.Secret or a v1.PartialObjectMetadata.
//
// NOTE: "owning" here does not mean [ownerReference][1], because
// cert-manager does not set the ownerReference of the Secret,
// unless the [`--enable-certificate-owner-ref` flag is true][2].
//
// [1]: https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/
// [2]: https://cert-manager.io/docs/cli/controller/
func owningCertForSecret(secret client.Object) *types.NamespacedName {
val, ok := secret.GetAnnotations()[certmanager.CertificateNameKey]
if !ok {
return nil
}
return &types.NamespacedName{
Name: val,
Namespace: secret.Namespace,
Namespace: secret.GetNamespace(),
}
}

28
pkg/controller/cainjector/setup.go
View File

@ -165,7 +165,20 @@ func RegisterAllInjectors(ctx context.Context, mgr ctrl.Manager, opts SetupOptio
// injectables is here where we define which
// objects' events should trigger a reconcile.
builder.WithPredicates(predicates)).
Watches(new(corev1.Secret), handler.EnqueueRequestsFromMapFunc(secretForInjectableMapFuncBuilder(mgr.GetClient(), log, setup)))
Watches(
new(corev1.Secret),
handler.EnqueueRequestsFromMapFunc(secretForInjectableMapFuncBuilder(mgr.GetClient(), log, setup)),
// Why do we use builder.OnlyMetadata?
//
// 1. To reduce memory use of cainjector, by only caching the
// metadata of Secrets, not the data.
// 2. To reduce the load on the K8S API server, by only listing
// the metadata of the Secrets when cainjector starts up.
//
// This configuration works in conjunction with the `DisableFor`
// option in the manager client.Options and client.CacheOptions.
builder.OnlyMetadata,
)
if opts.EnableCertificatesDataSource {
// Index injectable with a new field. If the injectable's CA is
// to be sourced from a Certificate's Secret, the field's value will be the
@ -176,10 +189,15 @@ func RegisterAllInjectors(ctx context.Context, mgr ctrl.Manager, opts SetupOptio
err := fmt.Errorf("error making injectable indexable by inject-ca-from path: %w", err)
return err
}
b.Watches(new(corev1.Secret), handler.EnqueueRequestsFromMapFunc(
certFromSecretToInjectableMapFuncBuilder(mgr.GetClient(), log, setup))).
Watches(new(cmapi.Certificate),
handler.EnqueueRequestsFromMapFunc(certToInjectableMapFuncBuilder(mgr.GetClient(), log, setup)))
b.Watches(
new(corev1.Secret),
handler.EnqueueRequestsFromMapFunc(certFromSecretToInjectableMapFuncBuilder(mgr.GetClient(), log, setup)),
// See "Why do we use builder.OnlyMetadata?" above.
builder.OnlyMetadata,
).Watches(
new(cmapi.Certificate),
handler.EnqueueRequestsFromMapFunc(certToInjectableMapFuncBuilder(mgr.GetClient(), log, setup)),
)
}
if err := b.Complete(r); err != nil {
return fmt.Errorf("error registering controller for %s: %w", setup.objType.GetName(), err)

7
pkg/controller/cainjector/sources.go
View File

@ -117,6 +117,13 @@ func (c *certificateDataSource) ReadCA(ctx context.Context, log logr.Logger, met
// don't requeue if we're just not found, we'll get called when the secret gets created
return nil, dropNotFound(err)
}
// Only use Secrets that have been created by this Certificate.
// The Secret must have a `cert-manager.io/certificate-name` annotation
// value matching the name of this Certificate..
// NOTE: "owner" is not the `ownerReference`, because cert-manager does not
// usually set the ownerReference of the Secret.
// TODO: The logged warning below is misleading because it contains the
// ownerReference, which is not the reason for ignoring the Secret.
owner := owningCertForSecret(&secret)
if owner == nil || *owner != certName {
log.V(logf.WarnLevel).Info("refusing to target secret not owned by certificate", "owner", metav1.GetControllerOf(&secret))