From 53cdac184cc6dfeee7b6502f4b808a9bdc53b4b4 Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Wed, 21 Aug 2024 17:52:02 +0200 Subject: [PATCH] refactor: renames ctrl packages and funcs (#59) We have both `controllers/{routing,authorization}` and `pkg/{routing,authorization}` in place. When both are used in some other package, there's a need for using import aliases, which hints that the abstraction they represent is blurry. We can define clear separation of responsibilities by renaming controller packages to `{authz,routing}ctrl`. Additionally, we can simplify functions and data types, such as constructor func, now called `New`. --- .../controller.go} | 32 +++++++++++-------- .../controller_test.go} | 2 +- .../reconcile_authconfig.go} | 8 ++--- .../reconcile_authpolicy.go} | 4 +-- .../suite_test.go | 8 ++--- .../controller.go} | 30 +++++++++-------- .../controller_test.go} | 2 +- .../delete_resources.go} | 6 ++-- .../exported_svc_locator.go | 2 +- .../fixtures_test.go} | 2 +- .../routing_gvk.go => routingctrl/gvk.go} | 2 +- .../reconcile_resources.go} | 6 ++-- .../{routing => routingctrl}/suite_test.go | 6 ++-- main.go | 10 +++--- 14 files changed, 64 insertions(+), 56 deletions(-) rename controllers/{authorization/authorization_controller.go => authzctrl/controller.go} (83%) rename controllers/{authorization/authorization_controller_test.go => authzctrl/controller_test.go} (99%) rename controllers/{authorization/authorization_reconcile_authconfig.go => authzctrl/reconcile_authconfig.go} (89%) rename controllers/{authorization/authorization_reconcile_authpolicy.go => authzctrl/reconcile_authpolicy.go} (96%) rename controllers/{authorization => authzctrl}/suite_test.go (89%) rename controllers/{routing/routing_controller.go => routingctrl/controller.go} (80%) rename controllers/{routing/routing_controller_test.go => routingctrl/controller_test.go} (99%) rename controllers/{routing/routing_delete_resources.go => routingctrl/delete_resources.go} (87%) rename controllers/{routing => routingctrl}/exported_svc_locator.go (98%) rename controllers/{routing/routing_fixtures_test.go => routingctrl/fixtures_test.go} (98%) rename controllers/{routing/routing_gvk.go => routingctrl/gvk.go} (98%) rename controllers/{routing/routing_reconcile_resources.go => routingctrl/reconcile_resources.go} (93%) rename controllers/{routing => routingctrl}/suite_test.go (91%) diff --git a/controllers/authorization/authorization_controller.go b/controllers/authzctrl/controller.go similarity index 83% rename from controllers/authorization/authorization_controller.go rename to controllers/authzctrl/controller.go index 67bf2c1..d51c4b0 100644 --- a/controllers/authorization/authorization_controller.go +++ b/controllers/authzctrl/controller.go @@ -1,4 +1,4 @@ -package authorization +package authzctrl import ( "context" @@ -23,15 +23,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -const ctrlName = "authorization" +const name = "authorization" -func NewPlatformAuthorizationController(cli client.Client, log logr.Logger, - component spi.AuthorizationComponent, config PlatformAuthorizationConfig) *PlatformAuthorizationController { - return &PlatformAuthorizationController{ +func New(cli client.Client, log logr.Logger, + component spi.AuthorizationComponent, config PlatformAuthorizationConfig) *Controller { + return &Controller{ active: true, Client: cli, log: log.WithValues( - "controller", ctrlName, + "controller", name, "component", component.ObjectReference.Kind, ), config: config, @@ -54,8 +54,8 @@ type PlatformAuthorizationConfig struct { ProviderName string } -// PlatformAuthorizationController holds the controller configuration. -type PlatformAuthorizationController struct { +// Controller holds the authorization controller configuration. +type Controller struct { client.Client active bool log logr.Logger @@ -69,8 +69,8 @@ type PlatformAuthorizationController struct { // +kubebuilder:rbac:groups=authorino.kuadrant.io,resources=authconfigs,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=security.istio.io,resources=authorizationpolicies,verbs=get;list;watch;create;update;patch;delete -// Reconcile ensures that the namespace has all required resources needed to be part of the Service Mesh of Open Data Hub. -func (r *PlatformAuthorizationController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +// Reconcile ensures that the component has all required resources needed to use authorization capability of the platform. +func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { if !r.active { r.log.V(5).Info("controller is not active") @@ -104,7 +104,11 @@ func (r *PlatformAuthorizationController) Reconcile(ctx context.Context, req ctr return ctrl.Result{}, errors.Join(errs...) } -func (r *PlatformAuthorizationController) SetupWithManager(mgr ctrl.Manager) error { +func (r *Controller) Name() string { + return name + "-" + strings.ToLower(r.authComponent.ObjectReference.Kind) +} + +func (r *Controller) SetupWithManager(mgr ctrl.Manager) error { if r.Client == nil { // Ensures client is set - fall back to the one defined for the passed manager r.Client = mgr.GetClient() @@ -113,7 +117,7 @@ func (r *PlatformAuthorizationController) SetupWithManager(mgr ctrl.Manager) err // TODO(mvp): define predicates so we do not reconcile unnecessarily //nolint:wrapcheck //reason there is no point in wrapping it return ctrl.NewControllerManagedBy(mgr). - Named(ctrlName+"-"+strings.ToLower(r.authComponent.ObjectReference.Kind)). + Named(r.Name()). For(&metav1.PartialObjectMetadata{ TypeMeta: metav1.TypeMeta{ APIVersion: r.authComponent.ObjectReference.GroupVersion().String(), @@ -125,11 +129,11 @@ func (r *PlatformAuthorizationController) SetupWithManager(mgr ctrl.Manager) err Complete(r) } -func (r *PlatformAuthorizationController) Activate() { +func (r *Controller) Activate() { r.active = true } -func (r *PlatformAuthorizationController) Deactivate() { +func (r *Controller) Deactivate() { r.active = false } diff --git a/controllers/authorization/authorization_controller_test.go b/controllers/authzctrl/controller_test.go similarity index 99% rename from controllers/authorization/authorization_controller_test.go rename to controllers/authzctrl/controller_test.go index 6b216c4..c2e5c57 100644 --- a/controllers/authorization/authorization_controller_test.go +++ b/controllers/authzctrl/controller_test.go @@ -1,4 +1,4 @@ -package authorization_test +package authzctrl_test import ( "context" diff --git a/controllers/authorization/authorization_reconcile_authconfig.go b/controllers/authzctrl/reconcile_authconfig.go similarity index 89% rename from controllers/authorization/authorization_reconcile_authconfig.go rename to controllers/authzctrl/reconcile_authconfig.go index 242a116..5fb6260 100644 --- a/controllers/authorization/authorization_reconcile_authconfig.go +++ b/controllers/authzctrl/reconcile_authconfig.go @@ -1,4 +1,4 @@ -package authorization +package authzctrl import ( "context" @@ -17,7 +17,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func (r *PlatformAuthorizationController) reconcileAuthConfig(ctx context.Context, target *unstructured.Unstructured) error { +func (r *Controller) reconcileAuthConfig(ctx context.Context, target *unstructured.Unstructured) error { hosts, err := r.extractHosts(target) if err != nil { return err @@ -112,7 +112,7 @@ func CompareAuthConfigs(m1, m2 *authorinov1beta2.AuthConfig) bool { reflect.DeepEqual(m1.Spec, m2.Spec) } -func (r *PlatformAuthorizationController) createAuthConfigTemplate(ctx context.Context, target *unstructured.Unstructured) (authorinov1beta2.AuthConfig, error) { +func (r *Controller) createAuthConfigTemplate(ctx context.Context, target *unstructured.Unstructured) (authorinov1beta2.AuthConfig, error) { authType, err := r.typeDetector.Detect(ctx, target) if err != nil { return authorinov1beta2.AuthConfig{}, fmt.Errorf("could not detect authtype: %w", err) @@ -126,7 +126,7 @@ func (r *PlatformAuthorizationController) createAuthConfigTemplate(ctx context.C return templ, nil } -func (r *PlatformAuthorizationController) extractHosts(target *unstructured.Unstructured) ([]string, error) { +func (r *Controller) extractHosts(target *unstructured.Unstructured) ([]string, error) { hosts, err := r.hostExtractor(target) if err != nil { return nil, fmt.Errorf("could not extract host: %w", err) diff --git a/controllers/authorization/authorization_reconcile_authpolicy.go b/controllers/authzctrl/reconcile_authpolicy.go similarity index 96% rename from controllers/authorization/authorization_reconcile_authpolicy.go rename to controllers/authzctrl/reconcile_authpolicy.go index bc6f32a..16fe186 100644 --- a/controllers/authorization/authorization_reconcile_authpolicy.go +++ b/controllers/authzctrl/reconcile_authpolicy.go @@ -1,4 +1,4 @@ -package authorization +package authzctrl import ( "context" @@ -18,7 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func (r *PlatformAuthorizationController) reconcileAuthPolicy(ctx context.Context, target *unstructured.Unstructured) error { +func (r *Controller) reconcileAuthPolicy(ctx context.Context, target *unstructured.Unstructured) error { desired := createAuthzPolicy(r.authComponent.Ports, r.authComponent.WorkloadSelector, r.config.ProviderName, target) found := &istiosecurityv1beta1.AuthorizationPolicy{} justCreated := false diff --git a/controllers/authorization/suite_test.go b/controllers/authzctrl/suite_test.go similarity index 89% rename from controllers/authorization/suite_test.go rename to controllers/authzctrl/suite_test.go index 942b011..a6121ab 100644 --- a/controllers/authorization/suite_test.go +++ b/controllers/authzctrl/suite_test.go @@ -1,4 +1,4 @@ -package authorization_test +package authzctrl_test import ( "context" @@ -6,7 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/opendatahub-io/odh-platform/controllers/authorization" + "github.com/opendatahub-io/odh-platform/controllers/authzctrl" "github.com/opendatahub-io/odh-platform/pkg/config" "github.com/opendatahub-io/odh-platform/pkg/platform" "github.com/opendatahub-io/odh-platform/pkg/spi" @@ -30,7 +30,7 @@ var _ = SynchronizedBeforeSuite(func(ctx context.Context) { } envTest, cancelFunc = test.StartWithControllers( - authorization.NewPlatformAuthorizationController( + authzctrl.New( nil, ctrl.Log.WithName("controllers").WithName("platform"), spi.AuthorizationComponent{ @@ -48,7 +48,7 @@ var _ = SynchronizedBeforeSuite(func(ctx context.Context) { HostPaths: []string{"spec.host"}, }, }, - authorization.PlatformAuthorizationConfig{ + authzctrl.PlatformAuthorizationConfig{ Label: config.GetAuthorinoLabel(), Audiences: config.GetAuthAudience(), ProviderName: config.GetAuthProvider(), diff --git a/controllers/routing/routing_controller.go b/controllers/routingctrl/controller.go similarity index 80% rename from controllers/routing/routing_controller.go rename to controllers/routingctrl/controller.go index 9a0e0c6..531b1d9 100644 --- a/controllers/routing/routing_controller.go +++ b/controllers/routingctrl/controller.go @@ -1,4 +1,4 @@ -package routing +package routingctrl import ( "context" @@ -22,14 +22,14 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) -const ctrlName = "routing" +const name = "routing" -func NewPlatformRoutingController(cli client.Client, log logr.Logger, component spi.RoutingComponent, config spi.PlatformRoutingConfiguration) *PlatformRoutingController { - return &PlatformRoutingController{ +func New(cli client.Client, log logr.Logger, component spi.RoutingComponent, config spi.PlatformRoutingConfiguration) *Controller { + return &Controller{ active: true, Client: cli, log: log.WithValues( - "controller", ctrlName, + "controller", name, "component", component.ObjectReference.Kind, ), component: component, @@ -38,8 +38,8 @@ func NewPlatformRoutingController(cli client.Client, log logr.Logger, component } } -// PlatformRoutingController holds the controller configuration. -type PlatformRoutingController struct { +// Controller holds the routing controller configuration. +type Controller struct { client.Client active bool log logr.Logger @@ -53,8 +53,8 @@ type PlatformRoutingController struct { // +kubebuilder:rbac:groups="networking.istio.io",resources=gateways,verbs=* // +kubebuilder:rbac:groups="networking.istio.io",resources=destinationrules,verbs=* -// Reconcile ensures that the namespace has all required resources needed to be part of the Service Mesh of Open Data Hub. -func (r *PlatformRoutingController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { +// Reconcile ensures that the component has all required resources needed to use routing capability of the platform. +func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { if !r.active { r.log.V(5).Info("controller is not active") @@ -101,7 +101,11 @@ func (r *PlatformRoutingController) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, errors.Join(errs...) } -func (r *PlatformRoutingController) SetupWithManager(mgr ctrl.Manager) error { +func (r *Controller) Name() string { + return name + "-" + strings.ToLower(r.component.ObjectReference.Kind) +} + +func (r *Controller) SetupWithManager(mgr ctrl.Manager) error { if r.Client == nil { // Ensures client is set - fall back to the one defined for the passed manager r.Client = mgr.GetClient() @@ -110,7 +114,7 @@ func (r *PlatformRoutingController) SetupWithManager(mgr ctrl.Manager) error { // TODO(mvp) define predicates for labels, annotation and generation changes //nolint:wrapcheck //reason there is no point in wrapping it return ctrl.NewControllerManagedBy(mgr). - Named(ctrlName+"-"+strings.ToLower(r.component.ObjectReference.Kind)). + Named(r.Name()). For(&metav1.PartialObjectMetadata{ TypeMeta: metav1.TypeMeta{ APIVersion: r.component.ObjectReference.GroupVersion().String(), @@ -124,10 +128,10 @@ func (r *PlatformRoutingController) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -func (r *PlatformRoutingController) Activate() { +func (r *Controller) Activate() { r.active = true } -func (r *PlatformRoutingController) Deactivate() { +func (r *Controller) Deactivate() { r.active = false } diff --git a/controllers/routing/routing_controller_test.go b/controllers/routingctrl/controller_test.go similarity index 99% rename from controllers/routing/routing_controller_test.go rename to controllers/routingctrl/controller_test.go index bbac4dc..f66af8f 100644 --- a/controllers/routing/routing_controller_test.go +++ b/controllers/routingctrl/controller_test.go @@ -1,4 +1,4 @@ -package routing_test +package routingctrl_test import ( "context" diff --git a/controllers/routing/routing_delete_resources.go b/controllers/routingctrl/delete_resources.go similarity index 87% rename from controllers/routing/routing_delete_resources.go rename to controllers/routingctrl/delete_resources.go index b2e3605..1096fa0 100644 --- a/controllers/routing/routing_delete_resources.go +++ b/controllers/routingctrl/delete_resources.go @@ -1,4 +1,4 @@ -package routing +package routingctrl import ( "context" @@ -14,7 +14,7 @@ import ( ) // HandleResourceDeletion handles the removal of dependent resources when the target resource is being deleted. -func (r *PlatformRoutingController) HandleResourceDeletion(ctx context.Context, sourceRes *unstructured.Unstructured) (ctrl.Result, error) { +func (r *Controller) HandleResourceDeletion(ctx context.Context, sourceRes *unstructured.Unstructured) (ctrl.Result, error) { exportModes, found := extractExportModes(sourceRes) if !found { r.log.Info("No export modes found, skipping deletion logic", "sourceRes", sourceRes) @@ -33,7 +33,7 @@ func (r *PlatformRoutingController) HandleResourceDeletion(ctx context.Context, return removeFinalizer(ctx, r.Client, sourceRes) } -func (r *PlatformRoutingController) deleteOwnedResources(ctx context.Context, target *unstructured.Unstructured, gvkList []schema.GroupVersionKind) error { +func (r *Controller) deleteOwnedResources(ctx context.Context, target *unstructured.Unstructured, gvkList []schema.GroupVersionKind) error { deleteOptions := []client.DeleteAllOfOption{ client.InNamespace(r.config.GatewayNamespace), labels.MatchingLabels( diff --git a/controllers/routing/exported_svc_locator.go b/controllers/routingctrl/exported_svc_locator.go similarity index 98% rename from controllers/routing/exported_svc_locator.go rename to controllers/routingctrl/exported_svc_locator.go index 6f09b76..4bde0ab 100644 --- a/controllers/routing/exported_svc_locator.go +++ b/controllers/routingctrl/exported_svc_locator.go @@ -1,4 +1,4 @@ -package routing +package routingctrl import ( "context" diff --git a/controllers/routing/routing_fixtures_test.go b/controllers/routingctrl/fixtures_test.go similarity index 98% rename from controllers/routing/routing_fixtures_test.go rename to controllers/routingctrl/fixtures_test.go index 94a56e4..cd8c880 100644 --- a/controllers/routing/routing_fixtures_test.go +++ b/controllers/routingctrl/fixtures_test.go @@ -1,4 +1,4 @@ -package routing_test +package routingctrl_test import ( "context" diff --git a/controllers/routing/routing_gvk.go b/controllers/routingctrl/gvk.go similarity index 98% rename from controllers/routing/routing_gvk.go rename to controllers/routingctrl/gvk.go index 066c9a7..08e5f17 100644 --- a/controllers/routing/routing_gvk.go +++ b/controllers/routingctrl/gvk.go @@ -1,4 +1,4 @@ -package routing +package routingctrl import ( "github.com/opendatahub-io/odh-platform/pkg/spi" diff --git a/controllers/routing/routing_reconcile_resources.go b/controllers/routingctrl/reconcile_resources.go similarity index 93% rename from controllers/routing/routing_reconcile_resources.go rename to controllers/routingctrl/reconcile_resources.go index 938a7db..db83f20 100644 --- a/controllers/routing/routing_reconcile_resources.go +++ b/controllers/routingctrl/reconcile_resources.go @@ -1,4 +1,4 @@ -package routing +package routingctrl import ( "context" @@ -17,7 +17,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -func (r *PlatformRoutingController) reconcileResources(ctx context.Context, target *unstructured.Unstructured) error { +func (r *Controller) reconcileResources(ctx context.Context, target *unstructured.Unstructured) error { // TODO shouldn't we make it a predicate for ctrl watch instead _, exportModeFound := extractExportModes(target) if !exportModeFound { @@ -53,7 +53,7 @@ func (r *PlatformRoutingController) reconcileResources(ctx context.Context, targ return errors.Join(errSvcExport...) } -func (r *PlatformRoutingController) exportService(ctx context.Context, target *unstructured.Unstructured, exportedSvc *corev1.Service, domain string) error { +func (r *Controller) exportService(ctx context.Context, target *unstructured.Unstructured, exportedSvc *corev1.Service, domain string) error { exportModes, found := extractExportModes(target) if !found { return fmt.Errorf("could not extract export modes from target %s", target.GetName()) diff --git a/controllers/routing/suite_test.go b/controllers/routingctrl/suite_test.go similarity index 91% rename from controllers/routing/suite_test.go rename to controllers/routingctrl/suite_test.go index 4ef3e56..d3ff11b 100644 --- a/controllers/routing/suite_test.go +++ b/controllers/routingctrl/suite_test.go @@ -1,4 +1,4 @@ -package routing_test +package routingctrl_test import ( "context" @@ -6,7 +6,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/opendatahub-io/odh-platform/controllers/routing" + "github.com/opendatahub-io/odh-platform/controllers/routingctrl" "github.com/opendatahub-io/odh-platform/pkg/platform" "github.com/opendatahub-io/odh-platform/pkg/spi" "github.com/opendatahub-io/odh-platform/test" @@ -37,7 +37,7 @@ var _ = SynchronizedBeforeSuite(func() { return } - routingCtrl := routing.NewPlatformRoutingController( + routingCtrl := routingctrl.New( nil, ctrl.Log.WithName("controllers").WithName("platform"), spi.RoutingComponent{ diff --git a/main.go b/main.go index e9cdd61..aea4428 100644 --- a/main.go +++ b/main.go @@ -4,8 +4,8 @@ import ( "flag" "os" - "github.com/opendatahub-io/odh-platform/controllers/authorization" - "github.com/opendatahub-io/odh-platform/controllers/routing" + "github.com/opendatahub-io/odh-platform/controllers/authzctrl" + "github.com/opendatahub-io/odh-platform/controllers/routingctrl" "github.com/opendatahub-io/odh-platform/pkg/config" pschema "github.com/opendatahub-io/odh-platform/pkg/schema" "github.com/opendatahub-io/odh-platform/pkg/spi" @@ -66,14 +66,14 @@ func main() { os.Exit(1) } - authorizationConfig := authorization.PlatformAuthorizationConfig{ + authorizationConfig := authzctrl.PlatformAuthorizationConfig{ Label: config.GetAuthorinoLabel(), Audiences: config.GetAuthAudience(), ProviderName: config.GetAuthProvider(), } for _, component := range authorizationComponents { - if err = authorization.NewPlatformAuthorizationController(mgr.GetClient(), ctrlLog, component, authorizationConfig). + if err = authzctrl.New(mgr.GetClient(), ctrlLog, component, authorizationConfig). SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "authorization", "component", component.ObjectReference.Kind) os.Exit(1) @@ -94,7 +94,7 @@ func main() { } for _, component := range routingComponents { - if err = routing.NewPlatformRoutingController( + if err = routingctrl.New( mgr.GetClient(), ctrlLog, component,