TopologyReconciler

[ChunyiLyu]

This closes https://github.com/rabbitmq/service-operator-experience/issues/100

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

I will squash the commits or re adjust the commit history before merging.

• Created TopologyReconciler which implements the controller runtime Reconciler interface with Reconcile() and SetupWithManager(). The TopologyReconciler.Reconcile() takes care of generating a rabbit hole client, add finalizer to the given object. It calls a DeclareFunc and a DeleteFunc which are configured by each custom resource Reconciler themselves. TopologyReconciler.Reconcile() also logs/events/set status for the given obj after calling the DeclareFunc (success and failure) and DeleteFunc (success and failure) .
• With this change, when we need to add a new CRD in the future, people can still use kubebuilder to scaffold the code but would need to delete the Reconcile() and SetupWithManager() method from the generated Reconciler. Any custom reconcile logic will go into DeclareFunc and DeleteFunc.
• Logging has slightly changed for several resources. For example, for a failed delete in federation, log/event used to be failed to delete federation upstream parameter it will now become failed to delete federation. This is because failed delete (also declare) logging and events are handled in a generic way in the TopologyReconciler.Reconcile() where it formats the message as "failed to delete lowercase-resource-kind". For declare, it would be either "failed to declare lowercase-resource-kind" and "Successfully declared lowercase-resource-kind". For several resources, like Exchange, Binding, Queue, this is the same format as it used to be. For Federation, Shove, Schema replication there is a small change.
• Happy to take suggestions for any of the interface and function names.
• SuperStreamReconciler was not refactored since its reconcile logic doesn't directly map to declare and delete. I will look into it later separately.

Additional Context

The TopologyReconciler.setObservedGeneration() are from reconciler-runtime.

[ablease]

Thanks for this Chunyi!

[coro]

Love the changes so far, but I'm wondering if there can be a couple of changes to the nested data structures we're now using.

[coro]

I'm not convinced this function SetInternalDomainName needs to exist (before or after this PR!).

In the product code all it does is set a property of the Reconciler, which is already set in main.go.
In the test code, it's only used in common_test.go, to see the responses when the internal domain is either provided or not.

The whole scope of this test file feels weird to me as well - it's not actually testing anything in the common.go despite the name. It also is doing setup of the domain names within the It clauses of each branch (instead of (Just)BeforeEach)

Some suggestions I have:

• Rename this file to topology_controller_test.go
• Delete the SetInternalDomainName function
• In this test, set the domain name when the controllers are instantiated in suite_test.go to a variable and use JustBeforeEach to change it in each test case here

[ChunyiLyu]

Good point. Method removed and I've updated the test.

[coro]

This TopologyReconciler object contains essentially an embedded version of itself, since the QueueReconciler is not too different from the TopologyReconciler object. We're even instantiating it with the same Recorder and Client as the parent type, and that duplication implies to me that there's some bleed between the two in responsibility.

Looking at the old reconcilers, I'm not actually convinced we need to keep them as objects anymore - the two methods DeclareFunc and DeleteFunc very rarely access properties on their respective objects anymore, and so could be functions.

I think with that in mind, the DeclareFunc and DeleteFunc could be converted from methods into functions, and then this property of the TopologyReconciler can literally be a function signature, i.e.

// TopologyReconciler reconciles any topology rabbitmq objects
type TopologyReconciler struct {
	client.Client
	Type                    client.Object
	WatchTypes              []client.Object
	Log                     logr.Logger
	Scheme                  *runtime.Scheme
	Recorder                record.EventRecorder
	RabbitmqClientFactory   rabbitmqclient.Factory
	KubernetesClusterDomain string
        DeleteFunc              func(ctx context.Context, client rabbitmqclient.Client, resource topology.TopologyResource) error
	DeleteFunc              func(ctx context.Context, client rabbitmqclient.Client, resource topology.TopologyResource) error
}

TopologyReconciler{
  Client:                  mgr.GetClient(),
  Type:                    &topology.Queue{},
  Log:                     ctrl.Log.WithName(controllers.QueueControllerName),
  Scheme:                  mgr.GetScheme(),
  Recorder:                mgr.GetEventRecorderFor(controllers.QueueControllerName),
  RabbitmqClientFactory:   rabbitmqclient.RabbitholeClientFactory,
  KubernetesClusterDomain: clusterDomain,
  DeclareFunc:             &controllers.QueueDeclare,
  DeleteFunc:              &controllers.QueueDelete,
}

What do you think?

[ChunyiLyu]

I think having the declare and delete function directly in TopologyReconciler is going to limit the extensibility. Each resource reconciler needs different input to their declare and delete functions. For example the User and the Permission reconciler needs the Scheme to be able to set owner reference, and federation/shovel needs the k8s client to be able to fetch k8s secret. Having a embeded struct means we have the flexibility of adding fields and use them in declare and delete functions without changing the signature/type.

That said, the Recorder can be removed, and the k8s client can be removed for Reconciler that's not using it. I've updated and re pushed.