From 07fd7459e09574bcfbbf1d149e344778495632f3 Mon Sep 17 00:00:00 2001 From: Georgi Sabev Date: Thu, 6 Jul 2023 13:26:19 +0000 Subject: [PATCH] Allow the name registry to get, list and watch leases In order to check whether a name has been already reserved by the object being updated (i.e. the lease for the old name does not exist, but a lease owned by the updated object for the new name does), the name registry has to be allowed to get leases. However, when the name registry is using a client with cache, once the lease is got, the client's cache starts watching and listing the lease. Therefore, the client's user (i.e. the controllers user) has to be permitted to watch and list leases as well. This requirement was not covered by our integration tests as webhooks used to be run with the admin client (that is configured by env test). In order to close this gap, webhooks are now run as an user that is bound to the controllers role. Co-authored-by: Danail Branekov Co-authored-by: Giuseppe Capizzi Co-authored-by: Georgi Sabev Co-authored-by: Danail Branekov --- api/repositories/app_repository_test.go | 3 +- .../integration/integration_suite_test.go | 39 ++++++++-- .../integration/name_registry_test.go | 10 ++- controllers/coordination/name_registry.go | 2 +- .../coordination/name_registry_test.go | 8 +- controllers/webhooks/duplicate_validator.go | 5 ++ .../workloads/suite_integration_test.go | 24 +++--- helm/korifi/controllers/role.yaml | 3 + .../buildworkload_controller_test.go | 1 - tests/helpers/controllers_user.go | 73 +++++++++++++++++++ 10 files changed, 141 insertions(+), 27 deletions(-) create mode 100644 tests/helpers/controllers_user.go diff --git a/api/repositories/app_repository_test.go b/api/repositories/app_repository_test.go index 75714c9a2..a9a9cbaf6 100644 --- a/api/repositories/app_repository_test.go +++ b/api/repositories/app_repository_test.go @@ -9,7 +9,6 @@ import ( "time" apierrors "code.cloudfoundry.org/korifi/api/errors" - "code.cloudfoundry.org/korifi/api/repositories" . "code.cloudfoundry.org/korifi/api/repositories" "code.cloudfoundry.org/korifi/api/repositories/conditions" korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1" @@ -646,7 +645,7 @@ var _ = Describe("AppRepository", func() { }) It("does not change the app lifecyle", func() { - Expect(patchedAppRecord.Lifecycle).To(Equal(repositories.Lifecycle{ + Expect(patchedAppRecord.Lifecycle).To(Equal(Lifecycle{ Type: string(originalCFApp.Spec.Lifecycle.Type), Data: LifecycleData{ Buildpacks: originalCFApp.Spec.Lifecycle.Data.Buildpacks, diff --git a/controllers/coordination/integration/integration_suite_test.go b/controllers/coordination/integration/integration_suite_test.go index 913217f57..10835375a 100644 --- a/controllers/coordination/integration/integration_suite_test.go +++ b/controllers/coordination/integration/integration_suite_test.go @@ -1,12 +1,15 @@ package integration_test import ( + "context" "testing" "time" + "code.cloudfoundry.org/korifi/tests/helpers" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -22,8 +25,10 @@ func TestIntegration(t *testing.T) { } var ( - testEnv *envtest.Environment - k8sClient client.Client + testEnv *envtest.Environment + k8sClient client.Client + controllersClient client.Client + cacheStop context.CancelFunc ) var _ = BeforeSuite(func() { @@ -31,15 +36,37 @@ var _ = BeforeSuite(func() { testEnv = &envtest.Environment{} - cfg, err := testEnv.Start() + adminConfig, err := testEnv.Start() Expect(err).NotTo(HaveOccurred()) - Expect(cfg).NotTo(BeNil()) - k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + k8sClient, err = client.New(adminConfig, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + + controllersConf := helpers.SetupControllersUser(testEnv) + userCache, err := cache.New(controllersConf, cache.Options{}) + Expect(err).NotTo(HaveOccurred()) + + var cacheCtx context.Context + cacheCtx, cacheStop = context.WithCancel(context.Background()) + go func() { + GinkgoRecover() + Expect(userCache.Start(cacheCtx)).To(Succeed()) + }() + userCache.WaitForCacheSync(cacheCtx) + + controllersClient, err = client.New( + controllersConf, + client.Options{ + Scheme: scheme.Scheme, + Cache: &client.CacheOptions{ + Reader: userCache, + }, + }, + ) Expect(err).NotTo(HaveOccurred()) - Expect(k8sClient).NotTo(BeNil()) }) var _ = AfterSuite(func() { + cacheStop() Expect(testEnv.Stop()).To(Succeed()) }) diff --git a/controllers/coordination/integration/name_registry_test.go b/controllers/coordination/integration/name_registry_test.go index f1df79410..93c2758a9 100644 --- a/controllers/coordination/integration/name_registry_test.go +++ b/controllers/coordination/integration/name_registry_test.go @@ -2,6 +2,7 @@ package integration_test import ( "context" + "time" "code.cloudfoundry.org/korifi/controllers/coordination" "github.com/google/uuid" @@ -16,17 +17,22 @@ var _ = Describe("Name Registry", func() { var ( ns1 *corev1.Namespace ctx context.Context + cancelCtx context.CancelFunc nameRegistry coordination.NameRegistry name string ) BeforeEach(func() { - nameRegistry = coordination.NewNameRegistry(k8sClient, "my-entity") - ctx = context.Background() + nameRegistry = coordination.NewNameRegistry(controllersClient, "my-entity") + ctx, cancelCtx = context.WithTimeout(context.Background(), 10*time.Second) ns1 = createNamespace(ctx, uuid.NewString()) name = uuid.NewString() }) + AfterEach(func() { + cancelCtx() + }) + Describe("Registering a Name", func() { var ns2 *corev1.Namespace diff --git a/controllers/coordination/name_registry.go b/controllers/coordination/name_registry.go index 88eb48c41..2348715eb 100644 --- a/controllers/coordination/name_registry.go +++ b/controllers/coordination/name_registry.go @@ -30,7 +30,7 @@ var ( lockedIdentity = "locked" ) -//+kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=create;patch;delete +//+kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;create;patch;delete;list;watch type NameRegistry struct { client client.Client diff --git a/controllers/coordination/name_registry_test.go b/controllers/coordination/name_registry_test.go index 7f6d350f4..4f9c43ba9 100644 --- a/controllers/coordination/name_registry_test.go +++ b/controllers/coordination/name_registry_test.go @@ -47,7 +47,7 @@ var _ = Describe("NameRegistry", func() { lease := obj.(*coordinationv1.Lease) Expect(lease.Namespace).To(Equal("the-namespace")) - Expect(lease.Name).To(HavePrefix("n-")) + Expect(lease.Name).To(Equal("n-23d52b682183ce677443e399507b80c921c68a69")) Expect(lease.Spec.HolderIdentity).To(gstruct.PointTo(Equal("none"))) Expect(lease.Annotations).To(SatisfyAll( HaveKeyWithValue("coordination.cloudfoundry.org/entity-type", "my-type"), @@ -98,7 +98,7 @@ var _ = Describe("NameRegistry", func() { Expect(obj).To(BeAssignableToTypeOf(&coordinationv1.Lease{})) lease := obj.(*coordinationv1.Lease) Expect(lease.Namespace).To(Equal("the-namespace")) - Expect(lease.Name).To(HavePrefix("n-")) + Expect(lease.Name).To(Equal("n-23d52b682183ce677443e399507b80c921c68a69")) }) When("the lease does not exist", func() { @@ -153,7 +153,7 @@ var _ = Describe("NameRegistry", func() { Expect(obj).To(BeAssignableToTypeOf(&coordinationv1.Lease{})) lease := obj.(*coordinationv1.Lease) Expect(lease.Namespace).To(Equal("the-namespace")) - Expect(lease.Name).To(HavePrefix("n-")) + Expect(lease.Name).To(Equal("n-23d52b682183ce677443e399507b80c921c68a69")) Expect(patch.Type()).To(Equal(types.JSONPatchType)) data, dataErr := patch.Data(nil) @@ -207,7 +207,7 @@ var _ = Describe("NameRegistry", func() { Expect(obj).To(BeAssignableToTypeOf(&coordinationv1.Lease{})) lease := obj.(*coordinationv1.Lease) Expect(lease.Namespace).To(Equal("the-namespace")) - Expect(lease.Name).To(HavePrefix("n-")) + Expect(lease.Name).To(Equal("n-23d52b682183ce677443e399507b80c921c68a69")) Expect(patch.Type()).To(Equal(types.JSONPatchType)) data, dataErr := patch.Data(nil) diff --git a/controllers/webhooks/duplicate_validator.go b/controllers/webhooks/duplicate_validator.go index a42b02496..22cc638c6 100644 --- a/controllers/webhooks/duplicate_validator.go +++ b/controllers/webhooks/duplicate_validator.go @@ -80,6 +80,11 @@ func (v DuplicateValidator) ValidateUpdate(ctx context.Context, logger logr.Logg } if isOwned { + logger.Info("unique name is already owned by updated object", + "name", obj.UniqueName(), + "updatedObjectKind", obj.GetObjectKind(), + "object", client.ObjectKeyFromObject(obj), + ) return nil } } diff --git a/controllers/webhooks/workloads/suite_integration_test.go b/controllers/webhooks/workloads/suite_integration_test.go index a9533a63f..02726610e 100644 --- a/controllers/webhooks/workloads/suite_integration_test.go +++ b/controllers/webhooks/workloads/suite_integration_test.go @@ -23,8 +23,9 @@ import ( admissionv1beta1 "k8s.io/api/admission/v1beta1" coordinationv1 "k8s.io/api/coordination/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/envtest" @@ -65,22 +66,22 @@ var _ = BeforeSuite(func() { }, } - cfg, err := testEnv.Start() + adminConfig, err := testEnv.Start() Expect(err).NotTo(HaveOccurred()) - Expect(cfg).NotTo(BeNil()) + Expect(adminConfig).NotTo(BeNil()) - scheme := runtime.NewScheme() - Expect(korifiv1alpha1.AddToScheme(scheme)).To(Succeed()) - Expect(admissionv1beta1.AddToScheme(scheme)).To(Succeed()) - Expect(corev1.AddToScheme(scheme)).To(Succeed()) - Expect(coordinationv1.AddToScheme(scheme)).To(Succeed()) + Expect(korifiv1alpha1.AddToScheme(scheme.Scheme)).To(Succeed()) + Expect(admissionv1beta1.AddToScheme(scheme.Scheme)).To(Succeed()) + Expect(corev1.AddToScheme(scheme.Scheme)).To(Succeed()) + Expect(coordinationv1.AddToScheme(scheme.Scheme)).To(Succeed()) + Expect(rbacv1.AddToScheme(scheme.Scheme)).To(Succeed()) //+kubebuilder:scaffold:scheme // start webhook server using Manager webhookInstallOptions := &testEnv.WebhookInstallOptions - mgr, err := ctrl.NewManager(cfg, ctrl.Options{ - Scheme: scheme, + mgr, err := ctrl.NewManager(helpers.SetupControllersUser(testEnv), ctrl.Options{ + Scheme: scheme.Scheme, Host: webhookInstallOptions.LocalServingHost, Port: webhookInstallOptions.LocalServingPort, CertDir: webhookInstallOptions.LocalServingCertDir, @@ -89,7 +90,8 @@ var _ = BeforeSuite(func() { }) Expect(err).NotTo(HaveOccurred()) - k8sClient = helpers.NewCacheSyncingClient(mgr.GetClient()) + k8sClient, err = client.New(adminConfig, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) Expect((&korifiv1alpha1.CFApp{}).SetupWebhookWithManager(mgr)).To(Succeed()) diff --git a/helm/korifi/controllers/role.yaml b/helm/korifi/controllers/role.yaml index d32dd019f..2d6fc2105 100644 --- a/helm/korifi/controllers/role.yaml +++ b/helm/korifi/controllers/role.yaml @@ -95,7 +95,10 @@ rules: verbs: - create - delete + - get + - list - patch + - watch - apiGroups: - korifi.cloudfoundry.org resources: diff --git a/kpack-image-builder/controllers/buildworkload_controller_test.go b/kpack-image-builder/controllers/buildworkload_controller_test.go index 5ad65cf8d..a35b825bc 100644 --- a/kpack-image-builder/controllers/buildworkload_controller_test.go +++ b/kpack-image-builder/controllers/buildworkload_controller_test.go @@ -254,7 +254,6 @@ var _ = Describe("BuildWorkloadReconciler", func() { originalImageUID = kpackImage.UID g.Expect(originalImageUID).NotTo(BeEmpty()) }).Should(Succeed()) - }) It("deletes the kpack image and recreates it", func() { diff --git a/tests/helpers/controllers_user.go b/tests/helpers/controllers_user.go new file mode 100644 index 000000000..0644594c5 --- /dev/null +++ b/tests/helpers/controllers_user.go @@ -0,0 +1,73 @@ +package helpers + +import ( + "context" + "os" + "path/filepath" + "runtime" + + . "github.com/onsi/gomega" //lint:ignore ST1001 this is a test file + + . "github.com/onsi/ginkgo/v2" //lint:ignore ST1001 this is a test file + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" +) + +func SetupControllersUser(testEnv *envtest.Environment) *rest.Config { + controllersUser, err := testEnv.ControlPlane.AddUser(envtest.User{Name: "envtest-controller"}, testEnv.Config) + Expect(err).NotTo(HaveOccurred()) + + adminClient, err := client.New(testEnv.Config, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + + bindUserToControllersRole(adminClient, "envtest-controller") + + return controllersUser.Config() +} + +func bindUserToControllersRole(k8sClient client.Client, userName string) { + GinkgoHelper() + + controllersRole := ensureControllersClusterRole(k8sClient) + + Expect(k8sClient.Create(context.Background(), &rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "envtest-controller", + }, + Subjects: []rbacv1.Subject{{ + Kind: "User", + Name: userName, + }}, + RoleRef: rbacv1.RoleRef{ + Kind: "ClusterRole", + Name: controllersRole.Name, + }, + })).To(Succeed()) +} + +func ensureControllersClusterRole(k8sClient client.Client) *rbacv1.ClusterRole { + clusterRoleDefinition, err := os.ReadFile(controllersRoleYamlPath()) + Expect(err).NotTo(HaveOccurred()) + + roleObject, _, err := scheme.Codecs.UniversalDeserializer().Decode(clusterRoleDefinition, nil, new(rbacv1.ClusterRole)) + Expect(err).NotTo(HaveOccurred()) + + clusterRole, ok := roleObject.(*rbacv1.ClusterRole) + Expect(ok).To(BeTrue()) + + Expect(client.IgnoreAlreadyExists(k8sClient.Create(context.Background(), clusterRole))).To(Succeed()) + + return clusterRole +} + +func controllersRoleYamlPath() string { + _, thisFilePath, _, ok := runtime.Caller(0) + Expect(ok).To(BeTrue()) + thisFileDir := filepath.Dir(thisFilePath) + + return filepath.Join(thisFileDir, "..", "..", "helm", "korifi", "controllers", "role.yaml") +}