Move role controller to standard Teleport controller

integrations/operator/controllers/resources/role_controller.go

@@ -20,190 +20,53 @@ package resources
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/gravitational/trace"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-	ctrl "sigs.k8s.io/controller-runtime"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/gravitational/teleport/api/client"
 	"github.com/gravitational/teleport/api/types"
-	v5 "github.com/gravitational/teleport/integrations/operator/apis/resources/v5"
+	resourcesv5 "github.com/gravitational/teleport/integrations/operator/apis/resources/v5"
 )
 
-const teleportRoleKind = "TeleportRole"
-
-// TODO(for v12): Have the Role controller to use the generic Teleport reconciler
-// This means we'll have to move back to a statically typed client.
-// This will require removing the crdgen hack, fixing TeleportRole JSON serialization
-
-var TeleportRoleGVKV5 = schema.GroupVersionKind{
-	Group:   v5.GroupVersion.Group,
-	Version: v5.GroupVersion.Version,
-	Kind:    teleportRoleKind,
+// roleClient implements TeleportResourceClient and offers CRUD methods needed to reconcile roles
+type roleClient struct {
+	teleportClient *client.Client
 }
 
-// RoleReconciler reconciles a TeleportRole object
-type RoleReconciler struct {
-	kclient.Client
-	Scheme         *runtime.Scheme
-	TeleportClient *client.Client
+// Get gets the Teleport role of a given name
+func (r roleClient) Get(ctx context.Context, name string) (types.Role, error) {
+	role, err := r.teleportClient.GetRole(ctx, name)
+	return role, trace.Wrap(err)
 }
 
-//+kubebuilder:rbac:groups=resources.teleport.dev,resources=roles,verbs=get;list;watch;create;update;patch;delete
-//+kubebuilder:rbac:groups=resources.teleport.dev,resources=roles/status,verbs=get;update;patch
-//+kubebuilder:rbac:groups=resources.teleport.dev,resources=roles/finalizers,verbs=update
-
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.11.0/pkg/reconcile
-func (r *RoleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	// The TeleportRole OpenAPI spec does not validate typing of Label fields like `node_labels`.
-	// This means we can receive invalid data, by default it won't be unmarshalled properly and will crash the operator.
-	// To handle this more gracefully we unmarshall first in an unstructured object.
-	// The unstructured object will be converted later to a typed one, in r.UpsertExternal.
-	// See `/operator/crdgen/schemagen.go` and https://github.com/gravitational/teleport/issues/15204 for context.
-	// TODO: (Check how to handle multiple versions)
-	obj, err := GetUnstructuredObjectFromGVK(TeleportRoleGVKV5)
-	if err != nil {
-		return ctrl.Result{}, trace.Wrap(err, "creating object in which the CR will be unmarshalled")
-	}
-	return ResourceBaseReconciler{
-		Client:         r.Client,
-		DeleteExternal: r.Delete,
-		UpsertExternal: r.Upsert,
-	}.Do(ctx, req, obj)
+// Create creates a Teleport role
+func (r roleClient) Create(ctx context.Context, role types.Role) error {
+	_, err := r.teleportClient.CreateRole(ctx, role)
+	return trace.Wrap(err)
 }
 
-// SetupWithManager sets up the controller with the Manager.
-func (r *RoleReconciler) SetupWithManager(mgr ctrl.Manager) error {
-	// The TeleportRole OpenAPI spec does not validate typing of Label fields like `node_labels`.
-	// This means we can receive invalid data, by default it won't be unmarshalled properly and will crash the operator
-	// To handle this more gracefully we unmarshall first in an unstructured object.
-	// The unstructured object will be converted later to a typed one, in r.UpsertExternal.
-	// See `/operator/crdgen/schemagen.go` and https://github.com/gravitational/teleport/issues/15204 for context
-	// TODO: (Check how to handle multiple versions)
-	obj, err := GetUnstructuredObjectFromGVK(TeleportRoleGVKV5)
-	if err != nil {
-		return trace.Wrap(err, "creating the model object for the manager watcher/client")
-	}
-
-	return ctrl.NewControllerManagedBy(mgr).
-		For(obj).
-		WithEventFilter(buildPredicate()).
-		Complete(r)
+// Update updates a Teleport role
+func (r roleClient) Update(ctx context.Context, role types.Role) error {
+	_, err := r.teleportClient.UpdateRole(ctx, role)
+	return trace.Wrap(err)
 }
 
-func (r *RoleReconciler) Delete(ctx context.Context, obj kclient.Object) error {
-	return r.TeleportClient.DeleteRole(ctx, obj.GetName())
+// Delete deletes a Teleport role
+func (r roleClient) Delete(ctx context.Context, name string) error {
+	return trace.Wrap(r.teleportClient.DeleteRole(ctx, name))
 }
 
-func (r *RoleReconciler) Upsert(ctx context.Context, obj kclient.Object) error {
-	// We receive an unstructured object. We convert it to a typed TeleportRole object and gracefully handle errors.
-	u, ok := obj.(*unstructured.Unstructured)
-	if !ok {
-		return fmt.Errorf("failed to convert Object into resource object: %T", obj)
+// NewRoleReconciler instantiates a new Kubernetes controller reconciling role resources
+func NewRoleReconciler(client kclient.Client, tClient *client.Client) (Reconciler, error) {
+	roleClient := &roleClient{
+		teleportClient: tClient,
 	}
-	k8sResource := &v5.TeleportRole{}
 
-	// If an error happens we want to put it in status.conditions before returning.
-	err := runtime.DefaultUnstructuredConverter.FromUnstructuredWithValidation(
-		u.Object,
-		k8sResource, true, /* returnUnknownFields */
+	resourceReconciler, err := NewTeleportResourceReconciler[types.Role, *resourcesv5.TeleportRole](
+		client,
+		roleClient,
 	)
-	updateErr := updateStatus(updateStatusConfig{
-		ctx:         ctx,
-		client:      r.Client,
-		k8sResource: k8sResource,
-		condition:   getStructureConditionFromError(err),
-	})
-	if err != nil || updateErr != nil {
-		return trace.NewAggregate(err, updateErr)
-	}
-
-	// Converting the Kubernetes resource into a Teleport one, checking potential ownership issues.
-	teleportResource := k8sResource.ToTeleport()
-	existingResource, err := r.TeleportClient.GetRole(ctx, teleportResource.GetName())
-	updateErr = updateStatus(updateStatusConfig{
-		ctx:         ctx,
-		client:      r.Client,
-		k8sResource: k8sResource,
-		condition:   getReconciliationConditionFromError(err, true /* ignoreNotFound */),
-	})
-	if err != nil && !trace.IsNotFound(err) || updateErr != nil {
-		return trace.NewAggregate(err, updateErr)
-	}
-
-	if err == nil {
-		// The resource already exists
-		newOwnershipCondition, isOwned := checkOwnership(existingResource)
-		if updateErr = updateStatus(updateStatusConfig{
-			ctx:         ctx,
-			client:      r.Client,
-			k8sResource: k8sResource,
-			condition:   newOwnershipCondition,
-		}); updateErr != nil {
-			return trace.Wrap(updateErr)
-		}
-		if !isOwned {
-			return trace.AlreadyExists("unowned resource '%s' already exists", existingResource.GetName())
-		}
-	} else {
-		// The resource does not yet exist
-		if updateErr = updateStatus(updateStatusConfig{
-			ctx:         ctx,
-			client:      r.Client,
-			k8sResource: k8sResource,
-			condition:   newResourceCondition,
-		}); updateErr != nil {
-			return trace.Wrap(updateErr)
-		}
-	}
 
-	if existingResource != nil {
-		teleportResource.SetRevision(existingResource.GetRevision())
-	}
-	r.AddTeleportResourceOrigin(teleportResource)
-
-	// If an error happens we want to put it in status.conditions before returning.
-	_, err = r.TeleportClient.UpsertRole(ctx, teleportResource)
-	updateErr = updateStatus(updateStatusConfig{
-		ctx:         ctx,
-		client:      r.Client,
-		k8sResource: k8sResource,
-		condition:   getReconciliationConditionFromError(err, false /* ignoreNotFound */),
-	})
-	// We update the status conditions on exit
-	return trace.NewAggregate(err, updateErr)
-}
-
-func (r *RoleReconciler) AddTeleportResourceOrigin(resource types.Role) {
-	metadata := resource.GetMetadata()
-	if metadata.Labels == nil {
-		metadata.Labels = make(map[string]string)
-	}
-	metadata.Labels[types.OriginLabel] = types.OriginKubernetes
-	resource.SetMetadata(metadata)
-}
-
-func GetUnstructuredObjectFromGVK(gvk schema.GroupVersionKind) (*unstructured.Unstructured, error) {
-	if gvk.Empty() {
-		return nil, trace.BadParameter("cannot create an object for an empty GVK, aborting")
-	}
-	obj := unstructured.Unstructured{}
-	obj.SetGroupVersionKind(gvk)
-	return &obj, nil
-}
-
-func NewRoleReconciler(client kclient.Client, tClient *client.Client) (Reconciler, error) {
-	return &RoleReconciler{
-		Client:         client,
-		Scheme:         Scheme,
-		TeleportClient: tClient,
-	}, nil
+	return resourceReconciler, trace.Wrap(err, "building teleport resource reconciler")
 }

integrations/operator/controllers/resources/role_controller_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/stretchr/testify/require"
 	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/yaml"
 	"k8s.io/client-go/util/retry"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -39,6 +40,12 @@ import (
 	"github.com/gravitational/teleport/integrations/operator/controllers/resources"
 )
 
+var TeleportRoleGVKV5 = schema.GroupVersionKind{
+	Group:   resourcesv5.GroupVersion.Group,
+	Version: resourcesv5.GroupVersion.Version,
+	Kind:    "TeleportRole",
+}
+
 // When I create or delete a TeleportRole CR in Kubernetes,
 // the corresponding TeleportRole must be created/deleted in Teleport.
 func TestRoleCreation(t *testing.T) {
@@ -190,7 +197,7 @@ allow:
 
 			roleName := validRandomResourceName("role-")
 
-			obj, err := resources.GetUnstructuredObjectFromGVK(resources.TeleportRoleGVKV5)
+			obj, err := resources.GetUnstructuredObjectFromGVK(TeleportRoleGVKV5)
 			require.NoError(t, err)
 			obj.Object["spec"] = roleManifest
 			obj.SetName(roleName)
@@ -400,58 +407,6 @@ func k8sCreateRole(ctx context.Context, t *testing.T, kc kclient.Client, role *r
 	require.NoError(t, err)
 }
 
-func TestAddTeleportResourceOriginRole(t *testing.T) {
-	r := resources.RoleReconciler{}
-	tests := []struct {
-		name     string
-		resource types.Role
-	}{
-		{
-			name: "origin already set correctly",
-			resource: &types.RoleV6{
-				Metadata: types.Metadata{
-					Name:   "user with correct origin",
-					Labels: map[string]string{types.OriginLabel: types.OriginKubernetes},
-				},
-			},
-		},
-		{
-			name: "origin already set incorrectly",
-			resource: &types.RoleV6{
-				Metadata: types.Metadata{
-					Name:   "user with correct origin",
-					Labels: map[string]string{types.OriginLabel: types.OriginConfigFile},
-				},
-			},
-		},
-		{
-			name: "origin not set",
-			resource: &types.RoleV6{
-				Metadata: types.Metadata{
-					Name:   "user with correct origin",
-					Labels: map[string]string{"foo": "bar"},
-				},
-			},
-		},
-		{
-			name: "no labels",
-			resource: &types.RoleV6{
-				Metadata: types.Metadata{
-					Name: "user with no labels",
-				},
-			},
-		},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			r.AddTeleportResourceOrigin(tc.resource)
-			metadata := tc.resource.GetMetadata()
-			require.Contains(t, metadata.Labels, types.OriginLabel)
-			require.Equal(t, types.OriginKubernetes, metadata.Labels[types.OriginLabel])
-		})
-	}
-}
-
 func getRoleStatusConditionError(object map[string]interface{}) []metav1.Condition {
 	var conditionsWithError []metav1.Condition
 	var status resourcesv5.TeleportRoleStatus

integrations/operator/controllers/resources/utils.go

@@ -25,6 +25,8 @@ import (
 	"github.com/gravitational/trace"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 	ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
 
@@ -156,3 +158,14 @@ func updateStatus(config updateStatusConfig) error {
 	}
 	return trace.Wrap(statusErr)
 }
+
+// GetUnstructuredObjectFromGVK creates a new empty unstructured object with the
+// given Group Version and Kind.
+func GetUnstructuredObjectFromGVK(gvk schema.GroupVersionKind) (*unstructured.Unstructured, error) {
+	if gvk.Empty() {
+		return nil, trace.BadParameter("cannot create an object for an empty GVK, aborting")
+	}
+	obj := unstructured.Unstructured{}
+	obj.SetGroupVersionKind(gvk)
+	return &obj, nil
+}