Skip to content

Commit

Permalink
Split controllers and webhooks into separate packages
Browse files Browse the repository at this point in the history
Having controllers in separate packages allows having a suite per a
controller. This has several benefits:
* The suite setup is significantly simpler - it does not have to wire
  all the controllers/webhooks. Instead, the setup wires only the
  controller/webhook that is being tested
* Not wiring all the controllers in the test env reduces the noise of
  unrelated controllers/webhooks. This allows tests to not care about
  creating "valid" dependent objects, they only create dependent
  objects with properties that are just relevant to the
  controller/webhook under test
* Tests are no longer required to take side effects from neighbour
  controllers/webhooks into consideration
* Instead of relying on a state change of a dependent object caused by a
  neightbour controller, the test could simply set the desired state of
  the dependent object. This makes the test simpler and easy to reason
  about

fixes #3304
  • Loading branch information
danail-branekov committed May 31, 2024
1 parent 24a5442 commit 0c1c260
Show file tree
Hide file tree
Showing 104 changed files with 3,268 additions and 2,616 deletions.
4 changes: 2 additions & 2 deletions api/authorization/user_client_factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
k8sclient "k8s.io/client-go/kubernetes"

apierrors "code.cloudfoundry.org/korifi/api/errors"
"code.cloudfoundry.org/korifi/controllers/webhooks"
"code.cloudfoundry.org/korifi/controllers/webhooks/validation"
"code.cloudfoundry.org/korifi/tools/k8s"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
Expand Down Expand Up @@ -83,7 +83,7 @@ func isForbidden(err error) bool {
return false
}

if _, isValidationErr := webhooks.WebhookErrorToValidationError(err); isValidationErr {
if _, isValidationErr := validation.WebhookErrorToValidationError(err); isValidationErr {
return false
}

Expand Down
5 changes: 2 additions & 3 deletions api/errors/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@ import (
"reflect"
"strings"

"code.cloudfoundry.org/korifi/controllers/webhooks"

"code.cloudfoundry.org/korifi/controllers/webhooks/validation"
"github.com/go-logr/logr"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -372,7 +371,7 @@ func NewRollingDeployNotSupportedError(runnerName string) RollingDeployNotSuppor
}

func FromK8sError(err error, resourceType string) error {
if webhookValidationError, ok := webhooks.WebhookErrorToValidationError(err); ok {
if webhookValidationError, ok := validation.WebhookErrorToValidationError(err); ok {
return NewUnprocessableEntityError(err, webhookValidationError.GetMessage())
}

Expand Down
6 changes: 3 additions & 3 deletions api/repositories/app_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
apierrors "code.cloudfoundry.org/korifi/api/errors"
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/controllers/workloads/env"
"code.cloudfoundry.org/korifi/controllers/webhooks"
"code.cloudfoundry.org/korifi/controllers/webhooks/validation"
"code.cloudfoundry.org/korifi/tools/k8s"

"github.com/google/uuid"
Expand Down Expand Up @@ -247,8 +247,8 @@ func (f *AppRepo) CreateApp(ctx context.Context, authInfo authorization.Info, ap
cfApp := appCreateMessage.toCFApp()
err = userClient.Create(ctx, &cfApp)
if err != nil {
if validationError, ok := webhooks.WebhookErrorToValidationError(err); ok {
if validationError.Type == webhooks.DuplicateNameErrorType {
if validationError, ok := validation.WebhookErrorToValidationError(err); ok {
if validationError.Type == validation.DuplicateNameErrorType {
return AppRecord{}, apierrors.NewUniquenessError(err, validationError.GetMessage())
}
}
Expand Down
4 changes: 2 additions & 2 deletions api/repositories/package_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"code.cloudfoundry.org/korifi/api/authorization"
apierrors "code.cloudfoundry.org/korifi/api/errors"
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/controllers/workloads"
"code.cloudfoundry.org/korifi/controllers/controllers/workloads/packages"
"code.cloudfoundry.org/korifi/tools/dockercfg"
"code.cloudfoundry.org/korifi/tools/k8s"

Expand Down Expand Up @@ -187,7 +187,7 @@ func (r *PackageRepo) CreatePackage(ctx context.Context, authInfo authorization.
}
}

cfPackage, err = r.awaiter.AwaitCondition(ctx, userClient, cfPackage, workloads.InitializedConditionType)
cfPackage, err = r.awaiter.AwaitCondition(ctx, userClient, cfPackage, packages.InitializedConditionType)
if err != nil {
return PackageRecord{}, fmt.Errorf("failed waiting for Initialized condition: %w", err)
}
Expand Down
8 changes: 4 additions & 4 deletions api/repositories/service_binding_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import (
"code.cloudfoundry.org/korifi/api/authorization"
apierrors "code.cloudfoundry.org/korifi/api/errors"
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/webhooks"
"code.cloudfoundry.org/korifi/controllers/webhooks/services"
"code.cloudfoundry.org/korifi/controllers/webhooks/services/bindings"
"code.cloudfoundry.org/korifi/controllers/webhooks/validation"
"code.cloudfoundry.org/korifi/tools/k8s"

"github.com/google/uuid"
Expand Down Expand Up @@ -135,8 +135,8 @@ func (r *ServiceBindingRepo) CreateServiceBinding(ctx context.Context, authInfo

err = userClient.Create(ctx, cfServiceBinding)
if err != nil {
if validationError, ok := webhooks.WebhookErrorToValidationError(err); ok {
if validationError.Type == services.ServiceBindingErrorType {
if validationError, ok := validation.WebhookErrorToValidationError(err); ok {
if validationError.Type == bindings.ServiceBindingErrorType {
return ServiceBindingRecord{}, apierrors.NewUniquenessError(err, validationError.GetMessage())
}
}
Expand Down
4 changes: 2 additions & 2 deletions api/repositories/task_repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"code.cloudfoundry.org/korifi/api/authorization"
apierrors "code.cloudfoundry.org/korifi/api/errors"
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/controllers/workloads"
"code.cloudfoundry.org/korifi/controllers/controllers/workloads/tasks"
"code.cloudfoundry.org/korifi/tools/k8s"
"github.com/google/uuid"
v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -273,7 +273,7 @@ func taskToRecord(task *korifiv1alpha1.CFTask) TaskRecord {
if failedCond != nil && failedCond.Status == metav1.ConditionTrue {
taskRecord.FailureReason = failedCond.Message

if failedCond.Reason == workloads.TaskCanceledReason {
if failedCond.Reason == tasks.TaskCanceledReason {
taskRecord.FailureReason = "task was cancelled"
}
}
Expand Down
6 changes: 3 additions & 3 deletions controllers/api/v1alpha1/cfapp_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package v1alpha1_test

import (
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -20,7 +20,7 @@ var _ = Describe("CFAppMutatingWebhook", func() {
BeforeEach(func() {
cfApp = &korifiv1alpha1.CFApp{
ObjectMeta: metav1.ObjectMeta{
Name: GenerateGUID(),
Name: uuid.NewString(),
Namespace: namespace,
Labels: map[string]string{
"anotherLabel": "app-label",
Expand All @@ -30,7 +30,7 @@ var _ = Describe("CFAppMutatingWebhook", func() {
},
},
Spec: korifiv1alpha1.CFAppSpec{
DisplayName: GenerateGUID(),
DisplayName: uuid.NewString(),
DesiredState: "STARTED",
Lifecycle: korifiv1alpha1.Lifecycle{
Type: "buildpack",
Expand Down
8 changes: 4 additions & 4 deletions controllers/api/v1alpha1/cfbuild_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package v1alpha1_test

import (
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
v1 "k8s.io/api/core/v1"
Expand All @@ -24,9 +24,9 @@ var _ = Describe("CFBuildMutatingWebhook", func() {
)

BeforeEach(func() {
cfAppGUID = GenerateGUID()
cfPackageGUID = GenerateGUID()
cfBuildGUID = GenerateGUID()
cfAppGUID = uuid.NewString()
cfPackageGUID = uuid.NewString()
cfBuildGUID = uuid.NewString()

cfBuild = &korifiv1alpha1.CFBuild{
ObjectMeta: metav1.ObjectMeta{
Expand Down
6 changes: 3 additions & 3 deletions controllers/api/v1alpha1/cfpackage_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ package v1alpha1_test

import (
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
v1 "k8s.io/api/core/v1"
Expand All @@ -17,11 +17,11 @@ var _ = Describe("CFPackageMutatingWebhook", func() {
)

BeforeEach(func() {
cfAppGUID = GenerateGUID()
cfAppGUID = uuid.NewString()

cfPackage = &korifiv1alpha1.CFPackage{
ObjectMeta: metav1.ObjectMeta{
Name: GenerateGUID(),
Name: uuid.NewString(),
Namespace: namespace,
Labels: map[string]string{"foo": "bar"},
},
Expand Down
6 changes: 3 additions & 3 deletions controllers/api/v1alpha1/cfprocess_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ import (
"context"

korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"
"code.cloudfoundry.org/korifi/tools"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/onsi/gomega/gstruct"
Expand All @@ -27,8 +27,8 @@ var _ = Describe("CFProcessMutatingWebhook", func() {
)

BeforeEach(func() {
cfAppGUID = GenerateGUID()
cfProcessGUID = GenerateGUID()
cfAppGUID = uuid.NewString()
cfProcessGUID = uuid.NewString()
cfProcess = &korifiv1alpha1.CFProcess{
ObjectMeta: metav1.ObjectMeta{
Name: cfProcessGUID,
Expand Down
5 changes: 2 additions & 3 deletions controllers/api/v1alpha1/cfroute_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package v1alpha1_test

import (
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
Expand All @@ -26,7 +25,7 @@ var _ = Describe("CFRouteMutatingWebhook Integration Tests", func() {
BeforeEach(func() {
cfDomain = &korifiv1alpha1.CFDomain{
ObjectMeta: metav1.ObjectMeta{
Name: GenerateGUID(),
Name: uuid.NewString(),
Namespace: namespace,
},
Spec: korifiv1alpha1.CFDomainSpec{
Expand All @@ -37,7 +36,7 @@ var _ = Describe("CFRouteMutatingWebhook Integration Tests", func() {

cfRoute = &korifiv1alpha1.CFRoute{
ObjectMeta: metav1.ObjectMeta{
Name: GenerateGUID(),
Name: uuid.NewString(),
Namespace: namespace,
Labels: map[string]string{"foo": "bar"},
},
Expand Down
38 changes: 21 additions & 17 deletions controllers/api/v1alpha1/webhook_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,19 @@ import (

korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/controllers/shared"
"code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"
"code.cloudfoundry.org/korifi/controllers/coordination"
"code.cloudfoundry.org/korifi/controllers/webhooks"
"code.cloudfoundry.org/korifi/controllers/webhooks/finalizer"
"code.cloudfoundry.org/korifi/controllers/webhooks/networking"
"code.cloudfoundry.org/korifi/controllers/webhooks/networking/domains"
"code.cloudfoundry.org/korifi/controllers/webhooks/networking/routes"
"code.cloudfoundry.org/korifi/controllers/webhooks/validation"
"code.cloudfoundry.org/korifi/controllers/webhooks/version"
"code.cloudfoundry.org/korifi/controllers/webhooks/workloads"
"code.cloudfoundry.org/korifi/controllers/webhooks/workloads/apps"
"code.cloudfoundry.org/korifi/controllers/webhooks/workloads/orgs"
packageswebhook "code.cloudfoundry.org/korifi/controllers/webhooks/workloads/packages"
"code.cloudfoundry.org/korifi/controllers/webhooks/workloads/spaces"
"code.cloudfoundry.org/korifi/tests/helpers"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
admissionv1beta1 "k8s.io/api/admission/v1beta1"
Expand Down Expand Up @@ -83,7 +87,7 @@ var _ = BeforeSuite(func() {
},
}

namespace = testutils.PrefixedGUID("webhooks-test-ns")
namespace = uuid.NewString()

_, err := testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expand All @@ -100,18 +104,18 @@ var _ = BeforeSuite(func() {

uncachedClient := helpers.NewUncachedClient(k8sManager.GetConfig())
Expect((&korifiv1alpha1.CFApp{}).SetupWebhookWithManager(k8sManager)).To(Succeed())
Expect(workloads.NewCFAppValidator(
webhooks.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, workloads.AppEntityType)),
Expect(apps.NewValidator(
validation.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, apps.AppEntityType)),
).SetupWebhookWithManager(k8sManager)).To(Succeed())

Expect((&korifiv1alpha1.CFRoute{}).SetupWebhookWithManager(k8sManager)).To(Succeed())
Expect(networking.NewCFRouteValidator(
webhooks.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, networking.RouteEntityType)),
Expect(routes.NewValidator(
validation.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, routes.RouteEntityType)),
namespace,
uncachedClient,
).SetupWebhookWithManager(k8sManager)).To(Succeed())

Expect(networking.NewCFDomainValidator(uncachedClient).SetupWebhookWithManager(k8sManager)).To(Succeed())
Expect(domains.NewValidator(uncachedClient).SetupWebhookWithManager(k8sManager)).To(Succeed())

Expect((&korifiv1alpha1.CFPackage{}).SetupWebhookWithManager(k8sManager)).To(Succeed())

Expand All @@ -120,16 +124,16 @@ var _ = BeforeSuite(func() {

Expect((&korifiv1alpha1.CFBuild{}).SetupWebhookWithManager(k8sManager)).To(Succeed())

orgNameDuplicateValidator := webhooks.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, workloads.CFOrgEntityType))
orgPlacementValidator := webhooks.NewPlacementValidator(uncachedClient, namespace)
Expect(workloads.NewCFOrgValidator(orgNameDuplicateValidator, orgPlacementValidator).SetupWebhookWithManager(k8sManager)).To(Succeed())
orgNameDuplicateValidator := validation.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, orgs.CFOrgEntityType))
orgPlacementValidator := validation.NewPlacementValidator(uncachedClient, namespace)
Expect(orgs.NewValidator(orgNameDuplicateValidator, orgPlacementValidator).SetupWebhookWithManager(k8sManager)).To(Succeed())

spaceNameDuplicateValidator := webhooks.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, workloads.CFSpaceEntityType))
spacePlacementValidator := webhooks.NewPlacementValidator(uncachedClient, namespace)
Expect(workloads.NewCFSpaceValidator(spaceNameDuplicateValidator, spacePlacementValidator).SetupWebhookWithManager(k8sManager)).To(Succeed())
spaceNameDuplicateValidator := validation.NewDuplicateValidator(coordination.NewNameRegistry(uncachedClient, spaces.CFSpaceEntityType))
spacePlacementValidator := validation.NewPlacementValidator(uncachedClient, namespace)
Expect(spaces.NewValidator(spaceNameDuplicateValidator, spacePlacementValidator).SetupWebhookWithManager(k8sManager)).To(Succeed())
version.NewVersionWebhook("some-version").SetupWebhookWithManager(k8sManager)
finalizer.NewControllersFinalizerWebhook().SetupWebhookWithManager(k8sManager)
Expect(workloads.NewCFPackageValidator().SetupWebhookWithManager(k8sManager)).To(Succeed())
Expect(packageswebhook.NewValidator().SetupWebhookWithManager(k8sManager)).To(Succeed())

Expect(adminClient.Create(ctx, &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Expand Down
6 changes: 3 additions & 3 deletions controllers/cleanup/build_cleaner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import (

korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/cleanup"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"
"code.cloudfoundry.org/korifi/statefulset-runner/controllers"
"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
Expand All @@ -28,14 +28,14 @@ var _ = Describe("BuildCleaner", func() {
BeforeEach(func() {
cleaner = cleanup.NewBuildCleaner(controllersClient, 1)

namespace = GenerateGUID()
namespace = uuid.NewString()
Expect(k8sClient.Create(ctx, &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: namespace,
},
})).To(Succeed())

appGUID = GenerateGUID()
appGUID = uuid.NewString()

// sleeps are needed as creation timestamps can't be manipulated
// directly, and they have a 1 second granularity
Expand Down
8 changes: 4 additions & 4 deletions controllers/cleanup/package_cleaner_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import (

korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
"code.cloudfoundry.org/korifi/controllers/cleanup"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"
"code.cloudfoundry.org/korifi/statefulset-runner/controllers"

"github.com/google/uuid"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
Expand All @@ -29,15 +29,15 @@ var _ = Describe("PackageCleaner", func() {
BeforeEach(func() {
cleaner = cleanup.NewPackageCleaner(controllersClient, 1)

namespace = GenerateGUID()
namespace = uuid.NewString()
Expect(k8sClient.Create(ctx, &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: namespace,
},
})).To(Succeed())

appGUID = GenerateGUID()
buildGUID := GenerateGUID()
appGUID = uuid.NewString()
buildGUID := uuid.NewString()

cfApp = &korifiv1alpha1.CFApp{
ObjectMeta: metav1.ObjectMeta{
Expand Down
3 changes: 1 addition & 2 deletions controllers/controllers/workloads/apps/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package apps_test

import (
korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
. "code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"
. "code.cloudfoundry.org/korifi/tests/matchers"
"code.cloudfoundry.org/korifi/tools"
"code.cloudfoundry.org/korifi/tools/k8s"
Expand Down Expand Up @@ -461,7 +460,7 @@ var _ = Describe("CFAppReconciler Integration Tests", func() {

cfServiceBinding := korifiv1alpha1.CFServiceBinding{
ObjectMeta: metav1.ObjectMeta{
Name: PrefixedGUID("service-binding"),
Name: uuid.NewString(),
Namespace: testNamespace,
},
Spec: korifiv1alpha1.CFServiceBindingSpec{
Expand Down
Loading

0 comments on commit 0c1c260

Please sign in to comment.