From 87bef523374b0f73ea293a99fb48fcd503d67c51 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Thu, 5 Jan 2023 18:15:19 +0000 Subject: [PATCH 1/2] Fix cainjector's namespace flag Ensures that when cainjector has the namespace flag passed, namespaced resource caching is scoped to that namespace Signed-off-by: irbekrm --- cmd/cainjector/app/start.go | 4 ++-- pkg/controller/cainjector/setup.go | 13 ++++++++----- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/cmd/cainjector/app/start.go b/cmd/cainjector/app/start.go index 713c80ed8..948e7442e 100644 --- a/cmd/cainjector/app/start.go +++ b/cmd/cainjector/app/start.go @@ -215,7 +215,7 @@ func (o InjectorControllerOptions) RunInjectorController(ctx context.Context) er // Never retry if the controller exits cleanly. g.Go(func() (err error) { for { - err = cainjector.RegisterCertificateBased(gctx, mgr) + err = cainjector.RegisterCertificateBased(gctx, mgr, o.Namespace) if err == nil { return } @@ -234,7 +234,7 @@ func (o InjectorControllerOptions) RunInjectorController(ctx context.Context) er // We do not retry this controller because it only interacts with core APIs // which should always be in a working state. g.Go(func() (err error) { - if err = cainjector.RegisterSecretBased(gctx, mgr); err != nil { + if err = cainjector.RegisterSecretBased(gctx, mgr, o.Namespace); err != nil { return fmt.Errorf("error registering secret controller: %v", err) } return diff --git a/pkg/controller/cainjector/setup.go b/pkg/controller/cainjector/setup.go index acb7743f8..52f200640 100644 --- a/pkg/controller/cainjector/setup.go +++ b/pkg/controller/cainjector/setup.go @@ -180,8 +180,8 @@ func dataFromSliceOrFile(data []byte, file string) ([]byte, error) { // indices. // The registered controllers require the cert-manager API to be available // in order to run. -func RegisterCertificateBased(ctx context.Context, mgr ctrl.Manager) error { - cache, client, err := newIndependentCacheAndDelegatingClient(mgr) +func RegisterCertificateBased(ctx context.Context, mgr ctrl.Manager, namespace string) error { + cache, client, err := newIndependentCacheAndDelegatingClient(mgr, namespace) if err != nil { return err } @@ -202,8 +202,8 @@ func RegisterCertificateBased(ctx context.Context, mgr ctrl.Manager) error { // indices. // The registered controllers only require the corev1 APi to be available in // order to run. -func RegisterSecretBased(ctx context.Context, mgr ctrl.Manager) error { - cache, client, err := newIndependentCacheAndDelegatingClient(mgr) +func RegisterSecretBased(ctx context.Context, mgr ctrl.Manager, namespace string) error { + cache, client, err := newIndependentCacheAndDelegatingClient(mgr, namespace) if err != nil { return err } @@ -226,11 +226,14 @@ func RegisterSecretBased(ctx context.Context, mgr ctrl.Manager) error { // cert-manager Certificates CRDs have been installed and before the CA bundles // have been injected into the cert-manager CRDs, by the secrets based injector, // which is running in a separate goroutine. -func newIndependentCacheAndDelegatingClient(mgr ctrl.Manager) (cache.Cache, client.Client, error) { +func newIndependentCacheAndDelegatingClient(mgr ctrl.Manager, namespace string) (cache.Cache, client.Client, error) { cacheOptions := cache.Options{ Scheme: mgr.GetScheme(), Mapper: mgr.GetRESTMapper(), } + if namespace != "" { + cacheOptions.Namespace = namespace + } ca, err := cache.New(mgr.GetConfig(), cacheOptions) if err != nil { return nil, nil, err From ff800307374aff81d69b16f7e2d13c78a542c845 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Thu, 5 Jan 2023 18:15:19 +0000 Subject: [PATCH 2/2] Log error if CA source is in a namespace that is not in scope cainjector will still watch cluster-scoped resources such as CRDs, so it can get references to Secrets or Certificates in namespaces that are out of scope Signed-off-by: irbekrm --- pkg/controller/cainjector/controller.go | 10 ++++++++- pkg/controller/cainjector/setup.go | 9 +++++--- pkg/controller/cainjector/sources.go | 28 +++++++++++++++++++------ 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/pkg/controller/cainjector/controller.go b/pkg/controller/cainjector/controller.go index 1bcfc4ec3..44c063afb 100644 --- a/pkg/controller/cainjector/controller.go +++ b/pkg/controller/cainjector/controller.go @@ -106,6 +106,9 @@ type genericInjectReconciler struct { log logr.Logger client.Client + // if set, the reconciler is namespace scoped + namespace string + resourceName string // just used for logging } @@ -157,11 +160,16 @@ func (r *genericInjectReconciler) Reconcile(_ context.Context, req ctrl.Request) return ctrl.Result{}, nil } - caData, err := dataSource.ReadCA(ctx, log, metaObj) + caData, err := dataSource.ReadCA(ctx, log, metaObj, r.namespace) + if apierrors.IsForbidden(err) { + log.V(logf.InfoLevel).Info("cainjector was forbidden to retrieve the ca data source") + return ctrl.Result{}, nil + } if err != nil { log.Error(err, "failed to read CA from data source") return ctrl.Result{}, err } + if caData == nil { log.V(logf.InfoLevel).Info("could not find any ca data in data source for target") return ctrl.Result{}, nil diff --git a/pkg/controller/cainjector/setup.go b/pkg/controller/cainjector/setup.go index 52f200640..87892a749 100644 --- a/pkg/controller/cainjector/setup.go +++ b/pkg/controller/cainjector/setup.go @@ -77,10 +77,10 @@ var ( // registerAllInjectors registers all injectors and based on the // graduation state of the injector decides how to log no kind/resource match errors -func registerAllInjectors(ctx context.Context, groupName string, mgr ctrl.Manager, sources []caDataSource, client client.Client, ca cache.Cache) error { +func registerAllInjectors(ctx context.Context, groupName string, mgr ctrl.Manager, sources []caDataSource, client client.Client, ca cache.Cache, namespace string) error { controllers := make([]controller.Controller, len(injectorSetups)) for i, setup := range injectorSetups { - controller, err := newGenericInjectionController(ctx, groupName, mgr, setup, sources, ca, client) + controller, err := newGenericInjectionController(ctx, groupName, mgr, setup, sources, ca, client, namespace) if err != nil { if !meta.IsNoMatchError(err) || !setup.injector.IsAlpha() { return err @@ -126,7 +126,7 @@ func registerAllInjectors(ctx context.Context, groupName string, mgr ctrl.Manage // * https://github.com/kubernetes-sigs/controller-runtime/issues/764 func newGenericInjectionController(ctx context.Context, groupName string, mgr ctrl.Manager, setup injectorSetup, sources []caDataSource, ca cache.Cache, - client client.Client) (controller.Controller, error) { + client client.Client, namespace string) (controller.Controller, error) { log := ctrl.Log.WithName(groupName).WithName(setup.resourceName) typ := setup.injector.NewTarget().AsObject() @@ -140,6 +140,7 @@ func newGenericInjectionController(ctx context.Context, groupName string, mgr ct log: log.WithName("generic-inject-reconciler"), resourceName: setup.resourceName, injector: setup.injector, + namespace: namespace, }, LogConstructor: func(request *reconcile.Request) logr.Logger { return log }, }) @@ -194,6 +195,7 @@ func RegisterCertificateBased(ctx context.Context, mgr ctrl.Manager, namespace s }, client, cache, + namespace, ) } @@ -217,6 +219,7 @@ func RegisterSecretBased(ctx context.Context, mgr ctrl.Manager, namespace string }, client, cache, + namespace, ) } diff --git a/pkg/controller/cainjector/sources.go b/pkg/controller/cainjector/sources.go index 044d667af..8eb2e8080 100644 --- a/pkg/controller/cainjector/sources.go +++ b/pkg/controller/cainjector/sources.go @@ -18,11 +18,11 @@ package cainjector import ( "context" - - logf "github.com/cert-manager/cert-manager/pkg/logs" + "fmt" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" @@ -34,6 +34,7 @@ import ( cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + logf "github.com/cert-manager/cert-manager/pkg/logs" ) // caDataSource knows how to extract CA data given a provided InjectTarget. @@ -52,7 +53,7 @@ type caDataSource interface { // In this case, the caller should not retry the operation. // It is up to the ReadCA implementation to inform the user why the CA // failed to read. - ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object) (ca []byte, err error) + ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object, namespace string) (ca []byte, err error) // ApplyTo applies any required watchers to the given controller. ApplyTo(ctx context.Context, mgr ctrl.Manager, setup injectorSetup, controller controller.Controller, ca cache.Cache) error @@ -69,7 +70,7 @@ func (c *kubeconfigDataSource) Configured(log logr.Logger, metaObj metav1.Object return metaObj.GetAnnotations()[cmapi.WantInjectAPIServerCAAnnotation] == "true" } -func (c *kubeconfigDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object) (ca []byte, err error) { +func (c *kubeconfigDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object, namespace string) (ca []byte, err error) { return c.apiserverCABundle, nil } @@ -99,15 +100,22 @@ func (c *certificateDataSource) Configured(log logr.Logger, metaObj metav1.Objec return true } -func (c *certificateDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object) (ca []byte, err error) { +func (c *certificateDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object, namespace string) (ca []byte, err error) { certNameRaw := metaObj.GetAnnotations()[cmapi.WantInjectAnnotation] certName := splitNamespacedName(certNameRaw) log = log.WithValues("certificate", certName) if certName.Namespace == "" { log.Error(nil, "invalid certificate name; needs a namespace/ prefix") + // TODO: should an error be returned here to prevent the caller from proceeding? // don't return an error, requeuing won't help till this is changed return nil, nil } + if namespace != "" && certName.Namespace != namespace { + err := fmt.Errorf("cannot read CA data from Certificate in namespace %s, cainjector is scoped to namespace %s", certName.Namespace, namespace) + forbidenErr := apierrors.NewForbidden(cmapi.Resource("certificates"), certName.Name, err) + log.Error(forbidenErr, "cannot read data source") + return nil, forbidenErr + } var cert cmapi.Certificate if err := c.client.Get(ctx, certName, &cert); err != nil { @@ -185,16 +193,24 @@ func (c *secretDataSource) Configured(log logr.Logger, metaObj metav1.Object) bo return true } -func (c *secretDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object) ([]byte, error) { +func (c *secretDataSource) ReadCA(ctx context.Context, log logr.Logger, metaObj metav1.Object, namespace string) ([]byte, error) { secretNameRaw := metaObj.GetAnnotations()[cmapi.WantInjectFromSecretAnnotation] secretName := splitNamespacedName(secretNameRaw) log = log.WithValues("secret", secretName) if secretName.Namespace == "" { log.Error(nil, "invalid certificate name") + // TODO: should we return error here to prevent the caller from proceeding? // don't return an error, requeuing won't help till this is changed return nil, nil } + if namespace != "" && secretName.Namespace != namespace { + err := fmt.Errorf("cannot read CA data from Secret in namespace %s, cainjector is scoped to namespace %s", secretName.Namespace, namespace) + forbidenErr := apierrors.NewForbidden(cmapi.Resource("certificates"), secretName.Name, err) + log.Error(forbidenErr, "cannot read data source") + return nil, forbidenErr + } + // grab the associated secret var secret corev1.Secret if err := c.client.Get(ctx, secretName, &secret); err != nil {