Repository: kumahq/kuma
Pull request #1222 (2020-11-26 15:46:02)

fix(kuma-cp) errors on delete secrets
### Summary

* We were not preserving the scope on Universal
* On Kubernetes, I described the problem in the comment so we don't forget, the secret was deleted with some increased latency but it resulted in many error logs.

### Documentation

- [X] No docs.



Patch:
diff --git a/pkg/core/managers/apis/mesh/mesh_manager.go b/pkg/core/managers/apis/mesh/mesh_manager.go
index 886232c3f..a2120f710 100644
--- a/pkg/core/managers/apis/mesh/mesh_manager.go
+++ b/pkg/core/managers/apis/mesh/mesh_manager.go
@@ -103,7 +103,7 @@ func (m *meshManager) Delete(ctx context.Context, resource core_model.Resource,
 	}
 	opts := core_store.NewDeleteOptions(fs...)
 	// delete all secrets
-	if err := m.otherManagers.DeleteAll(ctx, &system.SecretResourceList{}, core_store.DeleteAllByMesh(opts.Mesh)); err != nil {
+	if err := m.otherManagers.DeleteAll(ctx, &system.SecretResourceList{}, core_store.DeleteAllByMesh(opts.Name)); err != nil {
 		return errors.Wrap(err, "could not delete associated secrets")
 	}
 	return notFoundErr
diff --git a/pkg/core/secrets/manager/validator.go b/pkg/core/secrets/manager/validator.go
index d5b5a1ae1..97a687040 100644
--- a/pkg/core/secrets/manager/validator.go
+++ b/pkg/core/secrets/manager/validator.go
@@ -43,6 +43,9 @@ func (s *secretValidator) ValidateDelete(ctx context.Context, name string, mesh
 	meshRes := &mesh_core.MeshResource{}
 	err := s.store.Get(ctx, meshRes, core_store.GetByKey(mesh, model.NoMesh))
 	if err != nil {
+		if core_store.IsResourceNotFound(err) {
+			return nil // when Mesh no longer exist we should be able to safely delete a secret because it's not referenced anywhere
+		}
 		return err
 	}
 
diff --git a/pkg/plugins/bootstrap/k8s/plugin.go b/pkg/plugins/bootstrap/k8s/plugin.go
index 3cd5bb75c..2a15b43e3 100644
--- a/pkg/plugins/bootstrap/k8s/plugin.go
+++ b/pkg/plugins/bootstrap/k8s/plugin.go
@@ -3,6 +3,7 @@ package k8s
 import (
 	kube_runtime "k8s.io/apimachinery/pkg/runtime"
 	kube_ctrl "sigs.k8s.io/controller-runtime"
+	kube_client "sigs.k8s.io/controller-runtime/pkg/client"
 	kube_manager "sigs.k8s.io/controller-runtime/pkg/manager"
 
 	kuma_kube_cache "github.com/kumahq/kuma/pkg/plugins/bootstrap/k8s/cache"
@@ -25,8 +26,9 @@ func init() {
 
 func (p *plugin) BeforeBootstrap(b *core_runtime.Builder, _ core_plugins.PluginConfig) error {
 	scheme := kube_runtime.NewScheme()
+	config := kube_ctrl.GetConfigOrDie()
 	mgr, err := kube_ctrl.NewManager(
-		kube_ctrl.GetConfigOrDie(),
+		config,
 		kube_ctrl.Options{
 			Scheme:   scheme,
 			NewCache: kuma_kube_cache.New,
@@ -42,8 +44,22 @@ func (p *plugin) BeforeBootstrap(b *core_runtime.Builder, _ core_plugins.PluginC
 	if err != nil {
 		return err
 	}
+
+	// We need non cached client for resources that we don't have (get/list/watch) RBAC for all namespaces / cluster scope. Right now the only such resource is Secret
+	// Kubernetes cache lists resources under the hood from all Namespace unless we specify the "Namespace" in Options.
+	// If we don't do this the result is the following error: E1126 10:42:52.097662       1 reflector.go:178] pkg/mod/k8s.io/client-go@v0.18.9/tools/cache/reflector.go:125: Failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:kuma-system:kuma-control-plane" cannot list resource "secrets" in API group "" at the cluster scope
+	// We cannot specify this Namespace parameter because it affect all the resources, therefore we need separate client for Secrets.
+	nonCachedClient, err := kube_client.New(config, kube_client.Options{
+		Scheme: scheme,
+		Mapper: mgr.GetRESTMapper(),
+	})
+	if err != nil {
+		return err
+	}
+
 	b.WithComponentManager(&kubeComponentManager{mgr})
 	b.WithExtensions(k8s_extensions.NewManagerContext(b.Extensions(), mgr))
+	b.WithExtensions(k8s_extensions.NewNonCachedClientContext(b.Extensions(), nonCachedClient))
 	if expTime := b.Config().Runtime.Kubernetes.MarshalingCacheExpirationTime; expTime > 0 {
 		b.WithExtensions(k8s_extensions.NewResourceConverterContext(b.Extensions(), k8s.NewCachingConverter(expTime)))
 	} else {
diff --git a/pkg/plugins/extensions/k8s/context.go b/pkg/plugins/extensions/k8s/context.go
index 1e1f002c1..cb2335dd1 100644
--- a/pkg/plugins/extensions/k8s/context.go
+++ b/pkg/plugins/extensions/k8s/context.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	kube_ctrl "sigs.k8s.io/controller-runtime"
+	kube_client "sigs.k8s.io/controller-runtime/pkg/client"
 
 	k8s_common "github.com/kumahq/kuma/pkg/plugins/common/k8s"
 )
@@ -33,3 +34,14 @@ func FromResourceConverterContext(ctx context.Context) (converter k8s_common.Con
 	converter, ok = ctx.Value(converterKey{}).(k8s_common.Converter)
 	return
 }
+
+type nonCachedClient struct{}
+
+func NewNonCachedClientContext(ctx context.Context, client kube_client.Client) context.Context {
+	return context.WithValue(ctx, nonCachedClient{}, client)
+}
+
+func FromNonCachedClientContext(ctx context.Context) (client kube_client.Client, ok bool) {
+	client, ok = ctx.Value(nonCachedClient{}).(kube_client.Client)
+	return
+}
diff --git a/pkg/plugins/runtime/k8s/plugin.go b/pkg/plugins/runtime/k8s/plugin.go
index 8c11d7526..3961d87da 100644
--- a/pkg/plugins/runtime/k8s/plugin.go
+++ b/pkg/plugins/runtime/k8s/plugin.go
@@ -222,8 +222,12 @@ func addValidators(mgr kube_ctrl.Manager, rt core_runtime.Runtime, converter k8s
 	mgr.GetWebhookServer().Register("/validate-v1-service", &kube_webhook.Admission{Handler: &k8s_webhooks.ServiceValidator{}})
 	log.Info("Registering a validation webhook for v1/Service", "path", "/validate-v1-service")
 
+	client, ok := k8s_extensions.FromNonCachedClientContext(rt.Extensions())
+	if !ok {
+		return errors.Errorf("non cached client hasn't been configured")
+	}
 	secretValidator := &k8s_webhooks.SecretValidator{
-		Client:    mgr.GetClient(),
+		Client:    client,
 		Validator: manager.NewSecretValidator(rt.CaManagers(), rt.ResourceStore()),
 	}
 	mgr.GetWebhookServer().Register("/validate-v1-secret", &kube_webhook.Admission{Handler: secretValidator})
diff --git a/pkg/plugins/secrets/k8s/plugin.go b/pkg/plugins/secrets/k8s/plugin.go
index 444efed31..14ffdcb1c 100644
--- a/pkg/plugins/secrets/k8s/plugin.go
+++ b/pkg/plugins/secrets/k8s/plugin.go
@@ -26,5 +26,9 @@ func (p *plugin) NewSecretStore(pc core_plugins.PluginContext, _ core_plugins.Pl
 	if err := kube_core.AddToScheme(mgr.GetScheme()); err != nil {
 		return nil, errors.Wrapf(err, "could not add %q to scheme", kube_core.SchemeGroupVersion)
 	}
-	return NewStore(mgr.GetAPIReader(), mgr.GetClient(), pc.Config().Store.Kubernetes.SystemNamespace)
+	client, ok := k8s_extensions.FromNonCachedClientContext(pc.Extensions())
+	if !ok {
+		return nil, errors.Errorf("non cached client hasn't been configured")
+	}
+	return NewStore(client, client, pc.Config().Store.Kubernetes.SystemNamespace)
 }


Test patch:
diff --git a/pkg/core/managers/apis/mesh/mesh_manager_test.go b/pkg/core/managers/apis/mesh/mesh_manager_test.go
index 2f11c175d..42d91d8d5 100644
--- a/pkg/core/managers/apis/mesh/mesh_manager_test.go
+++ b/pkg/core/managers/apis/mesh/mesh_manager_test.go
@@ -212,6 +212,34 @@ var _ = Describe("Mesh Manager", func() {
 		})
 	})
 
+	Describe("Delete()", func() {
+		It("should delete secrets within one mesh", func() {
+			// given two meshes
+			err := resManager.Create(context.Background(), &core_mesh.MeshResource{}, store.CreateByKey("demo-1", model.NoMesh))
+			Expect(err).ToNot(HaveOccurred())
+			err = resManager.Create(context.Background(), &core_mesh.MeshResource{}, store.CreateByKey("demo-2", model.NoMesh))
+			Expect(err).ToNot(HaveOccurred())
+
+			// when demo-1 is deleted
+			err = resManager.Delete(context.Background(), &core_mesh.MeshResource{}, store.DeleteByKey("demo-1", model.NoMesh))
+
+			// then
+			Expect(err).ToNot(HaveOccurred())
+
+			// and all secrets are deleted
+			secrets := &system.SecretResourceList{}
+			err = secretManager.List(context.Background(), secrets, store.ListByMesh("demo-1"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(secrets.Items).To(BeEmpty())
+
+			// and all secrets from other mesh are preserved
+			secrets = &system.SecretResourceList{}
+			err = secretManager.List(context.Background(), secrets, store.ListByMesh("demo-2"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(secrets.Items).To(HaveLen(1)) // default signing key
+		})
+	})
+
 	Describe("Update()", func() {
 		It("should not allow to change CA when mTLS is enabled", func() {
 			// given
diff --git a/pkg/core/secrets/manager/validator_test.go b/pkg/core/secrets/manager/validator_test.go
index 2d3cfa1a0..e91202257 100644
--- a/pkg/core/secrets/manager/validator_test.go
+++ b/pkg/core/secrets/manager/validator_test.go
@@ -109,11 +109,19 @@ var _ = Describe("Secret Validator", func() {
 		}),
 	)
 
-	It("should validate that secret is not in use", func() {
+	It("should pass validation of secrets that are not in use", func() {
 		// when
 		err := validator.ValidateDelete(context.Background(), "some-not-used-secret", "default")
 
 		// then
 		Expect(err).ToNot(HaveOccurred())
 	})
+
+	It("should pass validation of secrets in mesh that non exist", func() {
+		// when
+		err := validator.ValidateDelete(context.Background(), "some-not-used-secret", "non-existing-mesh")
+
+		// then
+		Expect(err).ToNot(HaveOccurred())
+	})
 })