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 <[email protected]>
Co-authored-by: Giuseppe Capizzi <[email protected]>
Co-authored-by: Georgi Sabev <[email protected]>
Co-authored-by: Danail Branekov <[email protected]>

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,

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,24 +25,48 @@ 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() {
 	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
 
 	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())
 })

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
 

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

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)

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
 			}
 		}

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())
 

helm/korifi/controllers/role.yaml

@@ -95,7 +95,10 @@ rules:
   verbs:
   - create
   - delete
+  - get
+  - list
   - patch
+  - watch
 - apiGroups:
   - korifi.cloudfoundry.org
   resources:

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() {

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")
+}