Skip to content

Commit

Permalink
refactor: renames ctrl packages and funcs (#59)
Browse files Browse the repository at this point in the history
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`.
  • Loading branch information
bartoszmajsak authored Aug 21, 2024
1 parent afae24d commit 53cdac1
Show file tree
Hide file tree
Showing 14 changed files with 64 additions and 56 deletions.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package authorization
package authzctrl

import (
"context"
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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")

Expand Down Expand Up @@ -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()
Expand All @@ -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(),
Expand All @@ -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
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package authorization_test
package authzctrl_test

import (
"context"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package authorization
package authzctrl

import (
"context"
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package authorization
package authzctrl

import (
"context"
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package authorization_test
package authzctrl_test

import (
"context"
"testing"

. "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"
Expand All @@ -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{
Expand All @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing
package routingctrl

import (
"context"
Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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")

Expand Down Expand Up @@ -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()
Expand All @@ -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(),
Expand All @@ -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
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing_test
package routingctrl_test

import (
"context"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing
package routingctrl

import (
"context"
Expand All @@ -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)
Expand All @@ -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(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing
package routingctrl

import (
"context"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing_test
package routingctrl_test

import (
"context"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing
package routingctrl

import (
"github.com/opendatahub-io/odh-platform/pkg/spi"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package routing
package routingctrl

import (
"context"
Expand All @@ -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 {
Expand Down Expand Up @@ -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())
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
package routing_test
package routingctrl_test

import (
"context"
"testing"

. "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"
Expand Down Expand Up @@ -37,7 +37,7 @@ var _ = SynchronizedBeforeSuite(func() {
return
}

routingCtrl := routing.NewPlatformRoutingController(
routingCtrl := routingctrl.New(
nil,
ctrl.Log.WithName("controllers").WithName("platform"),
spi.RoutingComponent{
Expand Down
10 changes: 5 additions & 5 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -94,7 +94,7 @@ func main() {
}

for _, component := range routingComponents {
if err = routing.NewPlatformRoutingController(
if err = routingctrl.New(
mgr.GetClient(),
ctrlLog,
component,
Expand Down

0 comments on commit 53cdac1

Please sign in to comment.