diff --git a/pkg/extension/extension.go b/pkg/extension/extension.go new file mode 100644 index 00000000..d5fee2cb --- /dev/null +++ b/pkg/extension/extension.go @@ -0,0 +1,110 @@ +package extension + +import ( + "context" + + "helm.sh/helm/v3/pkg/chartutil" + "helm.sh/helm/v3/pkg/release" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/client-go/rest" +) + +// ReconcilerExtension defines an extension for the reconciler. +// It consists of several sub-interfaces. +type ReconcilerExtension interface { + Name() string + BeginReconciliationExtension + EndReconciliationExtension +} + +// BeginReconciliationExtension defines the extension point to execute at the beginning of a reconciliation flow. +type BeginReconciliationExtension interface { + BeginReconcile(ctx context.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, obj *unstructured.Unstructured) error +} + +// NoOpReconcilerExtension implements all extension methods as no-ops and can be used for convenience +// when only a subset of the available methods needs to be implemented. +type NoOpReconcilerExtension struct{} + +func (e NoOpReconcilerExtension) BeginReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return nil +} + +func (e NoOpReconcilerExtension) EndReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return nil +} + +type helmReleaseType int + +var helmReleaseKey helmReleaseType + +type helmValuesType int + +var helmValuesKey helmValuesType + +type kubernetesConfigType int + +var kubernetesConfigKey kubernetesConfigType + +type Context struct { + KubernetesConfig *rest.Config + HelmRelease *release.Release + HelmValues chartutil.Values +} + +func newContextWithValues(ctx context.Context, vals ...interface{}) context.Context { + if len(vals)%2 == 1 { + // Uneven number of vals, which is supposed to consist of key-value pairs. + // Add trailing nil value to fix it. + vals = append(vals, nil) + } + for i := 0; i < len(vals); i += 2 { + k := vals[i] + v := vals[i+1] + ctx = context.WithValue(ctx, k, v) + } + return ctx +} + +func NewContext(ctx context.Context, reconciliationContext *Context) context.Context { + if reconciliationContext == nil { + return ctx + } + return newContextWithValues(ctx, + helmReleaseKey, reconciliationContext.HelmRelease, + helmValuesKey, reconciliationContext.HelmValues, + kubernetesConfigKey, reconciliationContext.KubernetesConfig, + ) +} + +func HelmReleaseFromContext(ctx context.Context) release.Release { + v, ok := ctx.Value(helmReleaseKey).(*release.Release) + if !ok { + return release.Release{} + } + if v == nil { + return release.Release{} + } + return *v +} + +func KubernetesConfigFromContext(ctx context.Context) *rest.Config { + v, ok := ctx.Value(kubernetesConfigKey).(*rest.Config) + if !ok { + return nil + } + return v +} + +func HelmValuesFromContext(ctx context.Context) chartutil.Values { + v, ok := ctx.Value(helmValuesKey).(chartutil.Values) + if !ok { + return nil + } + return v +} diff --git a/pkg/hook/hook.go b/pkg/hook/hook.go index d1f42cc9..1eafb2f1 100644 --- a/pkg/hook/hook.go +++ b/pkg/hook/hook.go @@ -17,28 +17,69 @@ limitations under the License. package hook import ( + "context" + "github.com/go-logr/logr" + "github.com/operator-framework/helm-operator-plugins/pkg/extension" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/release" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) -type PreHook interface { - Exec(*unstructured.Unstructured, chartutil.Values, logr.Logger) error +type PreHookFunc func(context.Context, *unstructured.Unstructured, logr.Logger) error + +func WrapPreHookFunc(f PreHookFunc) PreHookFunc { + wrappedF := func(ctx context.Context, obj *unstructured.Unstructured, log logr.Logger) error { + err := f(ctx, obj, log) + if err != nil { + log.Error(err, "pre-release hook failed") + } + return nil + } + + return PreHookFunc(wrappedF) } -type PreHookFunc func(*unstructured.Unstructured, chartutil.Values, logr.Logger) error +func WrapPostHookFunc(f PostHookFunc) PostHookFunc { + wrappedF := func(ctx context.Context, obj *unstructured.Unstructured, rel release.Release, vals chartutil.Values, log logr.Logger) error { + err := f(ctx, obj, rel, vals, log) + if err != nil { + log.Error(err, "post-release hook failed", "name", rel.Name, "version", rel.Version) + } + return nil + } -func (f PreHookFunc) Exec(obj *unstructured.Unstructured, vals chartutil.Values, log logr.Logger) error { - return f(obj, vals, log) + return PostHookFunc(wrappedF) } -type PostHook interface { - Exec(*unstructured.Unstructured, release.Release, logr.Logger) error +func (h PreHookFunc) Name() string { + return "pre-hook" } -type PostHookFunc func(*unstructured.Unstructured, release.Release, logr.Logger) error +func (h PreHookFunc) BeginReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + log := logr.FromContextOrDiscard(ctx) + return h(ctx, obj, log) +} -func (f PostHookFunc) Exec(obj *unstructured.Unstructured, rel release.Release, log logr.Logger) error { - return f(obj, rel, log) +func (h PreHookFunc) EndReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return nil } + +var _ extension.ReconcilerExtension = (PreHookFunc)(nil) + +type PostHookFunc func(context.Context, *unstructured.Unstructured, release.Release, chartutil.Values, logr.Logger) error + +func (h PostHookFunc) Name() string { + return "post-hook" +} + +func (f PostHookFunc) BeginReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return nil +} + +func (f PostHookFunc) EndReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + log := logr.FromContextOrDiscard(ctx) + return f(ctx, obj, extension.HelmReleaseFromContext(ctx), extension.HelmValuesFromContext(ctx), log) +} + +var _ extension.ReconcilerExtension = (*PostHookFunc)(nil) diff --git a/pkg/hook/hook_test.go b/pkg/hook/hook_test.go index 7a499453..74ef0862 100644 --- a/pkg/hook/hook_test.go +++ b/pkg/hook/hook_test.go @@ -17,6 +17,8 @@ limitations under the License. package hook_test import ( + "context" + "github.com/go-logr/logr" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -24,29 +26,32 @@ import ( "helm.sh/helm/v3/pkg/release" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - . "github.com/operator-framework/helm-operator-plugins/pkg/hook" + "github.com/operator-framework/helm-operator-plugins/pkg/extension" + "github.com/operator-framework/helm-operator-plugins/pkg/hook" ) var _ = Describe("Hook", func() { var _ = Describe("PreHookFunc", func() { It("should implement the PreHook interface", func() { called := false - var h PreHook = PreHookFunc(func(*unstructured.Unstructured, chartutil.Values, logr.Logger) error { - called = true - return nil - }) - Expect(h.Exec(nil, nil, logr.Discard())).To(Succeed()) + var h extension.ReconcilerExtension = hook.PreHookFunc( + func(context.Context, *unstructured.Unstructured, logr.Logger) error { + called = true + return nil + }) + Expect(h.BeginReconcile(context.TODO(), nil)).To(Succeed()) Expect(called).To(BeTrue()) }) }) var _ = Describe("PostHookFunc", func() { It("should implement the PostHook interface", func() { called := false - var h PostHook = PostHookFunc(func(*unstructured.Unstructured, release.Release, logr.Logger) error { - called = true - return nil - }) - Expect(h.Exec(nil, release.Release{}, logr.Discard())).To(Succeed()) + var h extension.ReconcilerExtension = hook.PostHookFunc( + func(context.Context, *unstructured.Unstructured, release.Release, chartutil.Values, logr.Logger) error { + called = true + return nil + }) + Expect(h.EndReconcile(context.TODO(), nil)).To(Succeed()) Expect(called).To(BeTrue()) }) }) diff --git a/pkg/reconciler/extensions.go b/pkg/reconciler/extensions.go new file mode 100644 index 00000000..b45af653 --- /dev/null +++ b/pkg/reconciler/extensions.go @@ -0,0 +1,51 @@ +package reconciler + +import ( + "context" + "fmt" + + "github.com/operator-framework/helm-operator-plugins/pkg/extension" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +type extensions []extension.ReconcilerExtension + +func (es extensions) forEach(f func(e extension.ReconcilerExtension) error) error { + var err error + for _, e := range es { + err = f(e) + if err != nil { + return err + } + } + return err +} + +func (r *Reconciler) extBeginReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return r.extensions.forEach(func(ext extension.ReconcilerExtension) error { + e, ok := ext.(extension.BeginReconciliationExtension) + if !ok { + return nil + } + err := e.BeginReconcile(ctx, obj) + if err != nil { + return fmt.Errorf("extension %s failed during begin-reconcile phase: %v", ext.Name(), err) + } + return nil + }) +} + +func (r *Reconciler) extEndReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return r.extensions.forEach(func(ext extension.ReconcilerExtension) error { + e, ok := ext.(extension.EndReconciliationExtension) + if !ok { + return nil + } + + err := e.EndReconcile(ctx, obj) + if err != nil { + return fmt.Errorf("extension %s failed during end-reconcile phase: %v", ext.Name(), err) + } + return nil + }) +} diff --git a/pkg/reconciler/internal/hook/hook.go b/pkg/reconciler/internal/hook/hook.go index c9a37e59..c4b7ab3d 100644 --- a/pkg/reconciler/internal/hook/hook.go +++ b/pkg/reconciler/internal/hook/hook.go @@ -17,11 +17,11 @@ limitations under the License. package hook import ( + "context" "sync" "github.com/go-logr/logr" sdkhandler "github.com/operator-framework/operator-lib/handler" - "helm.sh/helm/v3/pkg/release" "helm.sh/helm/v3/pkg/releaseutil" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -33,12 +33,12 @@ import ( "sigs.k8s.io/yaml" "github.com/operator-framework/helm-operator-plugins/internal/sdk/controllerutil" - "github.com/operator-framework/helm-operator-plugins/pkg/hook" + "github.com/operator-framework/helm-operator-plugins/pkg/extension" "github.com/operator-framework/helm-operator-plugins/pkg/internal/predicate" "github.com/operator-framework/helm-operator-plugins/pkg/manifestutil" ) -func NewDependentResourceWatcher(c controller.Controller, rm meta.RESTMapper) hook.PostHook { +func NewDependentResourceWatcher(c controller.Controller, rm meta.RESTMapper) extension.ReconcilerExtension { return &dependentResourceWatcher{ controller: c, restMapper: rm, @@ -53,9 +53,20 @@ type dependentResourceWatcher struct { m sync.Mutex watches map[schema.GroupVersionKind]struct{} + + extension.NoOpReconcilerExtension } -func (d *dependentResourceWatcher) Exec(owner *unstructured.Unstructured, rel release.Release, log logr.Logger) error { +var _ extension.EndReconciliationExtension = (*dependentResourceWatcher)(nil) + +func (d *dependentResourceWatcher) Name() string { + return "internal-dependent-resource-watcher" +} + +func (d *dependentResourceWatcher) EndReconcile(ctx context.Context, owner *unstructured.Unstructured) error { + log := logr.FromContextOrDiscard(ctx) + rel := extension.HelmReleaseFromContext(ctx) + // using predefined functions for filtering events dependentPredicate := predicate.DependentPredicateFuncs() diff --git a/pkg/reconciler/internal/hook/hook_test.go b/pkg/reconciler/internal/hook/hook_test.go index dc4394a1..16521af7 100644 --- a/pkg/reconciler/internal/hook/hook_test.go +++ b/pkg/reconciler/internal/hook/hook_test.go @@ -17,9 +17,9 @@ limitations under the License. package hook_test import ( + "context" "strings" - "github.com/go-logr/logr" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" sdkhandler "github.com/operator-framework/operator-lib/handler" @@ -29,7 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/handler" - "github.com/operator-framework/helm-operator-plugins/pkg/hook" + "github.com/operator-framework/helm-operator-plugins/pkg/extension" "github.com/operator-framework/helm-operator-plugins/pkg/internal/fake" internalhook "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/hook" ) @@ -37,18 +37,16 @@ import ( var _ = Describe("Hook", func() { Describe("dependentResourceWatcher", func() { var ( - drw hook.PostHook + drw extension.ReconcilerExtension c *fake.Controller rm *meta.DefaultRESTMapper owner *unstructured.Unstructured rel *release.Release - log logr.Logger ) BeforeEach(func() { rm = meta.NewDefaultRESTMapper([]schema.GroupVersion{}) c = &fake.Controller{} - log = logr.Discard() }) Context("with unknown APIs", func() { @@ -70,18 +68,18 @@ var _ = Describe("Hook", func() { }) It("should fail with an invalid release manifest", func() { rel.Manifest = "---\nfoobar" - err := drw.Exec(owner, *rel, log) + err := drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner) Expect(err).NotTo(BeNil()) }) It("should fail with unknown owner kind", func() { - Expect(drw.Exec(owner, *rel, log)).To(MatchError(&meta.NoKindMatchError{ + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(MatchError(&meta.NoKindMatchError{ GroupKind: schema.GroupKind{Group: "apps", Kind: "Deployment"}, SearchedVersions: []string{"v1"}, })) }) It("should fail with unknown dependent kind", func() { rm.Add(schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, meta.RESTScopeNamespace) - Expect(drw.Exec(owner, *rel, log)).To(MatchError(&meta.NoKindMatchError{ + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(MatchError(&meta.NoKindMatchError{ GroupKind: schema.GroupKind{Group: "apps", Kind: "ReplicaSet"}, SearchedVersions: []string{"v1"}, })) @@ -111,7 +109,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{clusterRole, clusterRole, rsOwnerNamespace, rsOwnerNamespace}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -134,7 +132,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{rsOwnerNamespace, ssOtherNamespace}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -145,7 +143,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{clusterRole, clusterRoleBinding}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -155,7 +153,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{rsOwnerNamespaceWithKeep, ssOtherNamespaceWithKeep, clusterRoleWithKeep, clusterRoleBindingWithKeep}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(4)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) @@ -182,7 +180,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{rsOwnerNamespace}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(1)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) }) @@ -191,7 +189,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{clusterRole}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(1)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) }) @@ -200,7 +198,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{ssOtherNamespace}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(1)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) }) @@ -209,7 +207,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{rsOwnerNamespaceWithKeep, ssOtherNamespaceWithKeep, clusterRoleWithKeep}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(3)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) @@ -220,7 +218,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{replicaSetList}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - Expect(drw.Exec(owner, *rel, log)).To(Succeed()) + Expect(drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) Expect(c.WatchCalls[1].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -230,7 +228,7 @@ var _ = Describe("Hook", func() { Manifest: strings.Join([]string{errReplicaSetList}, "---\n"), } drw = internalhook.NewDependentResourceWatcher(c, rm) - err := drw.Exec(owner, *rel, log) + err := drw.EndReconcile(extension.NewContext(context.TODO(), &extension.Context{HelmRelease: rel}), owner) Expect(err).To(HaveOccurred()) }) }) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index f1c8f001..01c9c20a 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -49,6 +49,7 @@ import ( "github.com/operator-framework/helm-operator-plugins/internal/sdk/controllerutil" "github.com/operator-framework/helm-operator-plugins/pkg/annotation" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" + "github.com/operator-framework/helm-operator-plugins/pkg/extension" "github.com/operator-framework/helm-operator-plugins/pkg/hook" "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/conditions" internalhook "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/hook" @@ -66,8 +67,7 @@ type Reconciler struct { valueTranslator values.Translator valueMapper values.Mapper // nolint:staticcheck eventRecorder record.EventRecorder - preHooks []hook.PreHook - postHooks []hook.PostHook + extensions extensions log logr.Logger gvk *schema.GroupVersionKind @@ -360,23 +360,27 @@ func WithUninstallAnnotations(as ...annotation.Uninstall) Option { } } -// WithPreHook is an Option that configures the reconciler to run the given -// PreHook just before performing any actions (e.g. install, upgrade, uninstall, -// or reconciliation). -func WithPreHook(h hook.PreHook) Option { +// WithExtension is an Option that registers an extension into the reconciler. +// For example, extensions may be necessary when need arises to manage certain +// resources outsides of the standard Helm-based workflow. +func WithExtension(e extension.ReconcilerExtension) Option { return func(r *Reconciler) error { - r.preHooks = append(r.preHooks, h) + r.extensions = append(r.extensions, e) return nil } } +// WithPreHook is an Option that configures the reconciler to run the given +// PreHook just before performing any actions (e.g. install, upgrade, uninstall, +// or reconciliation). +func WithPreHook(f hook.PreHookFunc) Option { + return WithExtension(hook.WrapPreHookFunc(f)) +} + // WithPostHook is an Option that configures the reconciler to run the given // PostHook just after performing any non-uninstall release actions. -func WithPostHook(h hook.PostHook) Option { - return func(r *Reconciler) error { - r.postHooks = append(r.postHooks, h) - return nil - } +func WithPostHook(f hook.PostHookFunc) Option { + return WithExtension(hook.WrapPostHookFunc(f)) } // WithValueTranslator is an Option that configures a function that translates a @@ -455,6 +459,7 @@ func WithSelector(s metav1.LabelSelector) Option { // - Irreconcilable - an error occurred during reconciliation func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.Result, err error) { log := r.log.WithValues(strings.ToLower(r.gvk.Kind), req.NamespacedName) + ctx = logr.NewContext(ctx, log) obj := &unstructured.Unstructured{} obj.SetGroupVersionKind(*r.gvk) @@ -511,9 +516,21 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } u.UpdateStatus(updater.EnsureCondition(conditions.Initialized(corev1.ConditionTrue, "", ""))) + err = r.extBeginReconcile(ctx, obj) + if err != nil { + return ctrl.Result{}, err + } + if obj.GetDeletionTimestamp() != nil { err := r.handleDeletion(ctx, actionClient, obj, log) - return ctrl.Result{}, err + if err != nil { + return ctrl.Result{}, err + } + err = r.extEndReconcile(ctx, obj) + if err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil } vals, err := r.getValues(ctx, obj) @@ -537,12 +554,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } u.UpdateStatus(updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", ""))) - for _, h := range r.preHooks { - if err := h.Exec(obj, vals, log); err != nil { - log.Error(err, "pre-release hook failed") - } - } - switch state { case stateNeedsInstall: rel, err = r.doInstall(actionClient, &u, obj, vals.AsMap(), log) @@ -564,10 +575,12 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{}, fmt.Errorf("unexpected release state: %s", state) } - for _, h := range r.postHooks { - if err := h.Exec(obj, *rel, log); err != nil { - log.Error(err, "post-release hook failed", "name", rel.Name, "version", rel.Version) - } + reconciliationContext := extension.Context{HelmRelease: rel, HelmValues: vals} + ctx = extension.NewContext(ctx, &reconciliationContext) + + err = r.extEndReconcile(ctx, obj) + if err != nil { + return ctrl.Result{}, err } ensureDeployedRelease(&u, rel) @@ -854,7 +867,7 @@ func (r *Reconciler) setupWatches(mgr ctrl.Manager, c controller.Controller) err } if !r.skipDependentWatches { - r.postHooks = append([]hook.PostHook{internalhook.NewDependentResourceWatcher(c, mgr.GetRESTMapper())}, r.postHooks...) + r.extensions = append(r.extensions, internalhook.NewDependentResourceWatcher(c, mgr.GetRESTMapper())) } return nil } diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 61b5bc93..8e430f3d 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -52,6 +52,7 @@ import ( "github.com/operator-framework/helm-operator-plugins/internal/sdk/controllerutil" "github.com/operator-framework/helm-operator-plugins/pkg/annotation" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" + "github.com/operator-framework/helm-operator-plugins/pkg/extension" "github.com/operator-framework/helm-operator-plugins/pkg/hook" "github.com/operator-framework/helm-operator-plugins/pkg/internal/status" "github.com/operator-framework/helm-operator-plugins/pkg/internal/testutil" @@ -339,26 +340,30 @@ var _ = Describe("Reconciler", func() { var _ = Describe("WithPreHook", func() { It("should set a reconciler prehook", func() { called := false - preHook := hook.PreHookFunc(func(*unstructured.Unstructured, chartutil.Values, logr.Logger) error { + preHook := hook.PreHookFunc(func(context.Context, *unstructured.Unstructured, logr.Logger) error { called = true return nil }) + nExtensions := len(r.extensions) Expect(WithPreHook(preHook)(r)).To(Succeed()) - Expect(r.preHooks).To(HaveLen(1)) - Expect(r.preHooks[0].Exec(nil, nil, logr.Discard())).To(Succeed()) + Expect(len(r.extensions)).To(Equal(nExtensions + 1)) + hook := r.extensions[nExtensions].(extension.BeginReconciliationExtension) + Expect(hook.BeginReconcile(context.TODO(), nil)).To(Succeed()) Expect(called).To(BeTrue()) }) }) var _ = Describe("WithPostHook", func() { It("should set a reconciler posthook", func() { called := false - postHook := hook.PostHookFunc(func(*unstructured.Unstructured, release.Release, logr.Logger) error { + postHook := hook.PostHookFunc(func(context.Context, *unstructured.Unstructured, release.Release, chartutil.Values, logr.Logger) error { called = true return nil }) + nExtensions := len(r.extensions) Expect(WithPostHook(postHook)(r)).To(Succeed()) - Expect(r.postHooks).To(HaveLen(1)) - Expect(r.postHooks[0].Exec(nil, release.Release{}, logr.Discard())).To(Succeed()) + Expect(len(r.extensions)).To(Equal(nExtensions + 1)) + hook := r.extensions[nExtensions].(extension.EndReconciliationExtension) + Expect(hook.EndReconcile(context.TODO(), nil)).To(Succeed()) Expect(called).To(BeTrue()) }) }) @@ -477,6 +482,33 @@ var _ = Describe("Reconciler", func() { cancel() }) + When("an extension fails", func() { + It("subsequent extensions are not executed", func() { + var ( + failingPreReconciliationExtCalled bool + succeedingPreReconciliationExtCalled bool + ) + + failingPreReconciliationExt := &testBeginReconcileExtension{f: func() error { + failingPreReconciliationExtCalled = true + return errors.New("error!") + }} + + succeedingPreReconciliationExt := &testBeginReconcileExtension{f: func() error { + succeedingPreReconciliationExtCalled = true + return nil + }} + + r.extensions = append(r.extensions, failingPreReconciliationExt) + r.extensions = append(r.extensions, succeedingPreReconciliationExt) + + err := r.extBeginReconcile(ctx, &unstructured.Unstructured{}) + Expect(err).To(HaveOccurred()) + Expect(failingPreReconciliationExtCalled).To(BeTrue()) + Expect(succeedingPreReconciliationExtCalled).To(BeFalse()) + }) + }) + When("requested CR is not found", func() { It("returns successfully with no action", func() { res, err := r.Reconcile(ctx, req) @@ -1354,15 +1386,15 @@ func verifyNoRelease(ctx context.Context, cl client.Client, ns string, name stri func verifyHooksCalled(ctx context.Context, r *Reconciler, req reconcile.Request) { buf := &bytes.Buffer{} By("setting up a pre and post hook", func() { - preHook := hook.PreHookFunc(func(*unstructured.Unstructured, chartutil.Values, logr.Logger) error { + preHook := func(context.Context, *unstructured.Unstructured, logr.Logger) error { return errors.New("pre hook foobar") - }) - postHook := hook.PostHookFunc(func(*unstructured.Unstructured, release.Release, logr.Logger) error { + } + postHook := func(context.Context, *unstructured.Unstructured, release.Release, chartutil.Values, logr.Logger) error { return errors.New("post hook foobar") - }) + } r.log = zap.New(zap.WriteTo(buf)) - r.preHooks = append(r.preHooks, preHook) - r.postHooks = append(r.postHooks, postHook) + r.extensions = append(r.extensions, hook.PreHookFunc(hook.WrapPreHookFunc(preHook))) + r.extensions = append(r.extensions, hook.PostHookFunc(hook.WrapPostHookFunc(postHook))) }) By("successfully reconciling a request", func() { res, err := r.Reconcile(ctx, req) @@ -1390,3 +1422,18 @@ func verifyEvent(ctx context.Context, cl client.Reader, obj metav1.Object, event Reason: %q Message: %q`, eventType, reason, message)) } + +type testBeginReconcileExtension struct { + f func() error + extension.NoOpReconcilerExtension +} + +func (e *testBeginReconcileExtension) Name() string { + return "test-extension" +} + +func (e *testBeginReconcileExtension) BeginReconcile(ctx context.Context, obj *unstructured.Unstructured) error { + return e.f() +} + +var _ extension.ReconcilerExtension = (*testBeginReconcileExtension)(nil)