New Reconciler Extensions

[mtesseract]

Implement general extensions which are better suited for hybrid operators with customized reconciliation workflows.
Shows how pre/post hooks can be implemented in terms of extensions.

Signed-off-by: Moritz Clasmeier <moritz@stackrox.com>

This PR demonstrates on implementation strategy for extensions.
Looking forward to reviews & discussion.

Based on the discussions we've had, I'd like to comment on a few aspects:

• It was suggested to go forward with a minimal interface that would suit our needs: The new interface is minimal in the sense that it only contains two methods, similar to the PreHook/PostHook mechanism.

• This proposal tries to keep most of the advantages of static typing but in order to make future extensibility easy there is a NoOp value, which can be used for inheriting no-op-default-implementations for extension methods.

• The nomenclature has been changed: It's not anymore pre- & post-reconcile, but rather begin- & end-reconcile, to highlight that these custom steps are still to be regarded as part of the overall reconciliation workflow.

• The logger is now injected into the context as done in the kubebuilder scaffolding.

• In this proposal I am deliberately not using context.Context for untyped injection of more data potentially required by extensions because the understanding I have acquired from my research suggests that this is in rather sharp contradiction to the current best-practices. For instance, the official documentation says "Use context Values only for request-scoped data that transits processes and APIs, not for passing optional parameters to functions.". This is not 100% our use-case, but I understand it so that also our use-case would be questionable. This is against what we have discussed in our meeting. What I have done instead is I created a dedicated "reconciliation context", that is passed to all extension methods and is distinct from context.Context but used for providing all sorts of potentially-available context for the reconciliation workflow. My hope here is that this would allow for easy extensibility in the future and would help us to not box ourselves in.

• I have also shown how to implement PreHooks and PostHooks using the general extension interface. This compatibility code could remain, but doesn't have to remain.

I will be back from vacation on April 4, opening this draft so that you can already have a look at the current state. @varshaprasad96 @joelanford @misberner

[varshaprasad96]

This would mean that if we have to add any additional extension points in future, then there would be a user facing breaking change in extension.ReconcilerExtension since we would be adding additional sub-interfaces. It would be helpful if we can implement this to avoid breaking changes in future when we have to introduce more extensions. Would something like this work?

// 1. We have interfaces defined for extension points as mentioned in this PR:

// BeginReconciliationExtension defines the extension point to execute at the beginning of a reconciliation flow.
 type BeginReconciliationExtension interface {
 	BeginReconcile(ctx context.Context, reconciliationContext *Context, obj *unstructured.Unstructured) error
 }

 // EndReconciliationExtension defiens the extension point to execute at the end of a reconciliation flow.
 type EndReconciliationExtension interface {
 	EndReconcile(ctx context.Context, reconciliationContext *Context, obj *unstructured.Unstructured) error
 }

// 2. We have an internal struct that encapsulates this
type allextensions interface {
       BeginExts []BeginReconciliationExtension
       EndExts []EndReconciliationExtension
}

// 3. Reconciler in turn encapsulates the above `allextensions`
type Reconciler struct {
      ....
      extensions    allextensions
      ....
}

// 4. WithExtensions accepts interfaces and we type assert here to check which extension point it belongs to.
func WithExtensions(ext ...interface{}) Option {
	return func(r *Reconciler) error {
		for _, e := range ext {
			if b, ok := e.(BeginReconciliationExtension); ok {
				r.extensions.BeginExts = append(r.extensions.BeginExts, b)
			}
			if end, ok := e.(EndReconciliationExtension); ok {
				r.extensions.EndExts = append(r.extensions.EndExts, end)
			}
		}
		return nil
	}
}

As we keep on adding extension points, we can expand WithExtensions to type assert on each of them without causing user facing breaking changes.

This does look convoluted and I am not sure if it is the best way. Please feel free to disregard this approach, but it would be nice if we can come up with something which would cause minimal breaking changes later.

[varshaprasad96]

Do the users need to know the rest.Config entirely? Wouldn't it be sufficient if we pass just the client instead?

[varshaprasad96]

Do we need NoOpReconcilerExtension? Instead can we can let the users handle this, since as we keep on adding extension points, we would have to keep updating this. In an ideal scenario, we would not want breaking changes when we add extension points, but in case we still end up having predefined interfaces, would this be necessary?

[openshift-ci]

@mtesseract: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.