From 963a1cfa93f09644d9a9c40fbca8d2d6e6663ec1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20B=C3=A4umer?= Date: Tue, 18 May 2021 16:03:23 +0200 Subject: [PATCH 01/14] Add annotation package docs (#1) --- pkg/annotation/annotation.go | 39 ++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/pkg/annotation/annotation.go b/pkg/annotation/annotation.go index 329ef1e..b286e31 100644 --- a/pkg/annotation/annotation.go +++ b/pkg/annotation/annotation.go @@ -14,6 +14,30 @@ See the License for the specific language governing permissions and limitations under the License. */ +// Package annotation allows to set custom install, upgrade or uninstall options on custom resource objects with annotations. +// To create custom annotations implement the Install, Upgrade or Uninstall interface. +// +// Example: +// +// To disable hooks based on annotations the InstallDisableHooks is passed to the reconciler as an option. +// +// r, err := reconciler.New( +// reconciler.WithChart(*w.Chart), +// reconciler.WithGroupVersionKind(w.GroupVersionKind), +// reconciler.WithInstallAnnotations(annotation.InstallDisableHooks{}), +// ) +// +// If the reconciler detects an annotation named "helm.sdk.operatorframework.io/install-disable-hooks" +// on the watched custom resource, it sets the install.DisableHooks option to the annotations value. For more information +// take a look at the InstallDisableHooks.InstallOption method. +// +// kind: OperatorHelmKind +// apiVersion: test.example.com/v1 +// metadata: +// name: nginx-sample +// annotations: +// "helm.sdk.operatorframework.io/install-disable-hooks": true +// package annotation import ( @@ -30,27 +54,24 @@ var ( DefaultUninstallAnnotations = []Uninstall{UninstallDescription{}, UninstallDisableHooks{}} ) +// Install configures an install annotation. type Install interface { Name() string InstallOption(string) helmclient.InstallOption } +// Upgrade configures an upgrade annotation. type Upgrade interface { Name() string UpgradeOption(string) helmclient.UpgradeOption } +// Uninstall configures an uninstall annotation. type Uninstall interface { Name() string UninstallOption(string) helmclient.UninstallOption } -type InstallDisableHooks struct { - CustomName string -} - -var _ Install = &InstallDisableHooks{} - const ( defaultDomain = "helm.sdk.operatorframework.io" defaultInstallDisableHooksName = defaultDomain + "/install-disable-hooks" @@ -64,6 +85,12 @@ const ( defaultUninstallDescriptionName = defaultDomain + "/uninstall-description" ) +type InstallDisableHooks struct { + CustomName string +} + +var _ Install = &InstallDisableHooks{} + func (i InstallDisableHooks) Name() string { if i.CustomName != "" { return i.CustomName From a116e55f3cec0f5edd1474ad79f29767a4fe9b21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20B=C3=A4umer?= Date: Tue, 18 May 2021 19:21:56 +0200 Subject: [PATCH 02/14] Fork helm-operator adjustments for CI and documentation (#4) --- .github/labeler.yml | 2 ++ .github/workflows/ci.yml | 6 +---- .github/workflows/deploy.yml | 17 +++++++------- README.md | 44 ++++++++++++++++++++++++++++++++++-- 4 files changed, 54 insertions(+), 15 deletions(-) diff --git a/.github/labeler.yml b/.github/labeler.yml index 164dc38..c169049 100644 --- a/.github/labeler.yml +++ b/.github/labeler.yml @@ -1,3 +1,5 @@ +upstream-triage: + - "./*" area/main-binary: - 'main.go' - 'internal/**/*' diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9b97925..13c7342 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1,11 +1,7 @@ name: CI on: - push: - branches: - - '**' - pull_request: - branches: [ main ] + - push jobs: diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index cb4d112..2cf5b1f 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -1,13 +1,14 @@ name: Deploy -on: - push: - branches: - - '**' - tags: - - 'v*' - pull_request: - branches: [ main ] +# Disabled as we don't need docker images to use the helm-operator as a library. +#on: +# push: +# branches: +# - '**' +# tags: +# - 'v*' +# pull_request: +# branches: [ main ] jobs: goreleaser: diff --git a/README.md b/README.md index 0a8e4ac..023d008 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,46 @@ # helm-operator -[![Build Status](https://github.com/joelanford/helm-operator/workflows/CI/badge.svg?branch=master) -[![Coverage Status](https://coveralls.io/repos/github/joelanford/helm-operator/badge.svg?branch=master)](https://coveralls.io/github/joelanford/helm-operator?branch=master) +[![Build Status](https://github.com/stackrox/helm-operator/workflows/CI/badge.svg?branch=main) Experimental refactoring of the operator-framework's helm operator + +## Why a fork? + +Initially the Helm operator type was designed to automate Helm chart operations +by mapping the [values](https://helm.sh/docs/chart_template_guide/values_files/) of a Helm chart exactly to a +`CustomResourceDefinition` and defining its watched resources in a `watches.yaml` +[configuration](https://sdk.operatorframework.io/docs/building-operators/helm/tutorial/#watch-the-nginx-cr). + +To write a [Level II+](https://sdk.operatorframework.io/docs/advanced-topics/operator-capabilities/operator-capabilities/) operator +which reuses an already existing Helm chart a [hybrid](https://github.com/operator-framework/operator-sdk/issues/670) +between the Go and Helm operator type is necessary. + +The hybrid approach allows to add customizations to the Helm operator like value mapping based on cluster state or +executing code in on specific events. + +## Quickstart + +Add this module as a replace directive to your `go.mod`: + +``` +go mod edit -replace=github.com/joelanford/helm-operator=github.com/stackrox/helm-operator@main +``` + +Example: + +``` +chart, err := loader.Load("path/to/chart") +if err != nil { + panic(err) +} + +reconciler := reconciler.New( + reconciler.WithChart(*chart), + reconciler.WithGroupVersionKind(gvk), +) + +if err := reconciler.SetupWithManager(mgr); err != nil { + panic(fmt.Sprintf("unable to create reconciler: %s", err)) +} +``` + From 3d560ce50ca8e0d105f3fd855ff5a0660e616ef1 Mon Sep 17 00:00:00 2001 From: Gaurav Nelson Date: Thu, 20 May 2021 17:22:49 +1000 Subject: [PATCH 03/14] Updated README (#5) --- README.md | 57 ++++++++++++++++++++++++++++--------------------------- 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/README.md b/README.md index 023d008..42142d4 100644 --- a/README.md +++ b/README.md @@ -1,46 +1,47 @@ # helm-operator -[![Build Status](https://github.com/stackrox/helm-operator/workflows/CI/badge.svg?branch=main) +![Build Status](https://github.com/stackrox/helm-operator/workflows/CI/badge.svg?branch=main) -Experimental refactoring of the operator-framework's helm operator +Experimental refactoring of the operator-framework's helm operator. ## Why a fork? -Initially the Helm operator type was designed to automate Helm chart operations +The Helm operator type automates Helm chart operations by mapping the [values](https://helm.sh/docs/chart_template_guide/values_files/) of a Helm chart exactly to a `CustomResourceDefinition` and defining its watched resources in a `watches.yaml` -[configuration](https://sdk.operatorframework.io/docs/building-operators/helm/tutorial/#watch-the-nginx-cr). +[configuration](https://sdk.operatorframework.io/docs/building-operators/helm/tutorial/#watch-the-nginx-cr) file. -To write a [Level II+](https://sdk.operatorframework.io/docs/advanced-topics/operator-capabilities/operator-capabilities/) operator -which reuses an already existing Helm chart a [hybrid](https://github.com/operator-framework/operator-sdk/issues/670) -between the Go and Helm operator type is necessary. +For creating a [Level II+](https://sdk.operatorframework.io/docs/advanced-topics/operator-capabilities/operator-capabilities/) operator +that reuses an already existing Helm chart, we need a [hybrid](https://github.com/operator-framework/operator-sdk/issues/670) +between the Go and Helm operator types is. -The hybrid approach allows to add customizations to the Helm operator like value mapping based on cluster state or -executing code in on specific events. +The hybrid approach allows adding customizations to the Helm operator, such as: +- value mapping based on cluster state, or +- executing code in specific events. -## Quickstart +## Quick start -Add this module as a replace directive to your `go.mod`: +- Add this module as a replace directive to your `go.mod`: -``` -go mod edit -replace=github.com/joelanford/helm-operator=github.com/stackrox/helm-operator@main -``` + ``` + go mod edit -replace=github.com/joelanford/helm-operator=github.com/stackrox/helm-operator@main + ``` -Example: + For example: -``` -chart, err := loader.Load("path/to/chart") -if err != nil { - panic(err) -} + ```go + chart, err := loader.Load("path/to/chart") + if err != nil { + panic(err) + } -reconciler := reconciler.New( - reconciler.WithChart(*chart), - reconciler.WithGroupVersionKind(gvk), -) + reconciler := reconciler.New( + reconciler.WithChart(*chart), + reconciler.WithGroupVersionKind(gvk), + ) -if err := reconciler.SetupWithManager(mgr); err != nil { - panic(fmt.Sprintf("unable to create reconciler: %s", err)) -} -``` + if err := reconciler.SetupWithManager(mgr); err != nil { + panic(fmt.Sprintf("unable to create reconciler: %s", err)) + } + ``` From 22a740812837ccabb45308c41fb268303797797b Mon Sep 17 00:00:00 2001 From: Gaurav Nelson Date: Fri, 21 May 2021 09:57:56 +1000 Subject: [PATCH 04/14] fixed typo in README (#7) --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 42142d4..4a6c54d 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ by mapping the [values](https://helm.sh/docs/chart_template_guide/values_files/) For creating a [Level II+](https://sdk.operatorframework.io/docs/advanced-topics/operator-capabilities/operator-capabilities/) operator that reuses an already existing Helm chart, we need a [hybrid](https://github.com/operator-framework/operator-sdk/issues/670) -between the Go and Helm operator types is. +between the Go and Helm operator types. The hybrid approach allows adding customizations to the Helm operator, such as: - value mapping based on cluster state, or From 88508a237f9f3752c928cd30ac8f568613ddb428 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Tue, 25 May 2021 11:25:25 +0200 Subject: [PATCH 05/14] Add a WithValueTranslator option to Reconciller. (#6) This lets the user inject code which produces helm values based on the fetched object itself (unlike `Mapper` which can only see `Values`). This way the custom code can convert the object to a typed one and work with proper structs rather than a tree of maps from `string` to `interface{}`. --- pkg/reconciler/internal/values/values.go | 8 ++++++++ pkg/reconciler/reconciler.go | 23 ++++++++++++++++++++--- pkg/values/values.go | 13 +++++++++++++ 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/internal/values/values.go b/pkg/reconciler/internal/values/values.go index 5eef1b3..163e907 100644 --- a/pkg/reconciler/internal/values/values.go +++ b/pkg/reconciler/internal/values/values.go @@ -68,3 +68,11 @@ func (v *Values) ApplyOverrides(in map[string]string) error { } var DefaultMapper = values.MapperFunc(func(v chartutil.Values) chartutil.Values { return v }) + +var DefaultTranslator = values.TranslatorFunc(func(u *unstructured.Unstructured) (chartutil.Values, error) { + internalValues, err := FromUnstructured(u) + if err != nil { + return chartutil.Values{}, err + } + return internalValues.Map(), err +}) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index abecaa9..75b6fd2 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -61,6 +61,7 @@ const uninstallFinalizer = "uninstall-helm-release" type Reconciler struct { client client.Client actionClientGetter helmclient.ActionClientGetter + valueTranslator values.Translator valueMapper values.Mapper eventRecorder record.EventRecorder preHooks []hook.PreHook @@ -362,8 +363,20 @@ func WithPostHook(h hook.PostHook) Option { } } +// WithValueTranslator is an Option that configures a function that translates a +// custom resource to the values passed to Helm. +// Use this if you need to customize the logic that translates your custom resource to Helm values. +func WithValueTranslator(t values.Translator) Option { + return func(r *Reconciler) error { + r.valueTranslator = t + return nil + } +} + // WithValueMapper is an Option that configures a function that maps values -// from a custom resource spec to the values passed to Helm +// from a custom resource spec to the values passed to Helm. +// Use this if you want to apply a transformation on the values obtained from your custom resource, before +// they are passed to Helm. func WithValueMapper(m values.Mapper) Option { return func(r *Reconciler) error { r.valueMapper = m @@ -522,14 +535,15 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } func (r *Reconciler) getValues(obj *unstructured.Unstructured) (chartutil.Values, error) { - crVals, err := internalvalues.FromUnstructured(obj) + vals, err := r.valueTranslator.Translate(obj) if err != nil { return chartutil.Values{}, err } + crVals := internalvalues.New(vals) if err := crVals.ApplyOverrides(r.overrideValues); err != nil { return chartutil.Values{}, err } - vals := r.valueMapper.Map(crVals.Map()) + vals = r.valueMapper.Map(crVals.Map()) vals, err = chartutil.CoalesceValues(r.chrt, vals) if err != nil { return chartutil.Values{}, err @@ -736,6 +750,9 @@ func (r *Reconciler) addDefaults(mgr ctrl.Manager, controllerName string) { if r.eventRecorder == nil { r.eventRecorder = mgr.GetEventRecorderFor(controllerName) } + if r.valueTranslator == nil { + r.valueTranslator = internalvalues.DefaultTranslator + } if r.valueMapper == nil { r.valueMapper = internalvalues.DefaultMapper } diff --git a/pkg/values/values.go b/pkg/values/values.go index 46e1941..e39d0e1 100644 --- a/pkg/values/values.go +++ b/pkg/values/values.go @@ -18,8 +18,11 @@ package values import ( "helm.sh/helm/v3/pkg/chartutil" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) +// TODO: Consider deprecating Mapper and overrides in favour of Translator. + type Mapper interface { Map(chartutil.Values) chartutil.Values } @@ -29,3 +32,13 @@ type MapperFunc func(chartutil.Values) chartutil.Values func (m MapperFunc) Map(v chartutil.Values) chartutil.Values { return m(v) } + +type Translator interface { + Translate(unstructured *unstructured.Unstructured) (chartutil.Values, error) +} + +type TranslatorFunc func(*unstructured.Unstructured) (chartutil.Values, error) + +func (t TranslatorFunc) Translate(u *unstructured.Unstructured) (chartutil.Values, error) { + return t(u) +} From c3df552af1b1114407c22f96aeb85c6311ed24ce Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Tue, 1 Jun 2021 11:34:10 +0200 Subject: [PATCH 06/14] Pass context to Translate(). (#8) --- pkg/reconciler/internal/values/values.go | 3 ++- pkg/reconciler/reconciler.go | 6 +++--- pkg/values/values.go | 9 +++++---- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/internal/values/values.go b/pkg/reconciler/internal/values/values.go index 163e907..b188c20 100644 --- a/pkg/reconciler/internal/values/values.go +++ b/pkg/reconciler/internal/values/values.go @@ -17,6 +17,7 @@ limitations under the License. package values import ( + "context" "fmt" "os" @@ -69,7 +70,7 @@ func (v *Values) ApplyOverrides(in map[string]string) error { var DefaultMapper = values.MapperFunc(func(v chartutil.Values) chartutil.Values { return v }) -var DefaultTranslator = values.TranslatorFunc(func(u *unstructured.Unstructured) (chartutil.Values, error) { +var DefaultTranslator = values.TranslatorFunc(func(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) { internalValues, err := FromUnstructured(u) if err != nil { return chartutil.Values{}, err diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 75b6fd2..6bbf1df 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -471,7 +471,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{}, err } - vals, err := r.getValues(obj) + vals, err := r.getValues(ctx, obj) if err != nil { u.UpdateStatus( updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonErrorGettingValues, err)), @@ -534,8 +534,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{RequeueAfter: r.reconcilePeriod}, nil } -func (r *Reconciler) getValues(obj *unstructured.Unstructured) (chartutil.Values, error) { - vals, err := r.valueTranslator.Translate(obj) +func (r *Reconciler) getValues(ctx context.Context, obj *unstructured.Unstructured) (chartutil.Values, error) { + vals, err := r.valueTranslator.Translate(ctx, obj) if err != nil { return chartutil.Values{}, err } diff --git a/pkg/values/values.go b/pkg/values/values.go index e39d0e1..e1c094f 100644 --- a/pkg/values/values.go +++ b/pkg/values/values.go @@ -17,6 +17,7 @@ limitations under the License. package values import ( + "context" "helm.sh/helm/v3/pkg/chartutil" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -34,11 +35,11 @@ func (m MapperFunc) Map(v chartutil.Values) chartutil.Values { } type Translator interface { - Translate(unstructured *unstructured.Unstructured) (chartutil.Values, error) + Translate(ctx context.Context, unstructured *unstructured.Unstructured) (chartutil.Values, error) } -type TranslatorFunc func(*unstructured.Unstructured) (chartutil.Values, error) +type TranslatorFunc func(context.Context, *unstructured.Unstructured) (chartutil.Values, error) -func (t TranslatorFunc) Translate(u *unstructured.Unstructured) (chartutil.Values, error) { - return t(u) +func (t TranslatorFunc) Translate(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) { + return t(ctx, u) } From 7710c0ba33cff6a41a223aea482abb91b18d38ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20B=C3=A4umer?= Date: Tue, 8 Jun 2021 13:36:16 +0200 Subject: [PATCH 07/14] Disable scheme registration in SetupWithManager (#10) --- internal/cmd/run/cmd.go | 2 +- pkg/reconciler/reconciler.go | 10 ++++++++-- pkg/reconciler/reconciler_test.go | 2 +- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/cmd/run/cmd.go b/internal/cmd/run/cmd.go index 84ccee9..9131cf2 100644 --- a/internal/cmd/run/cmd.go +++ b/internal/cmd/run/cmd.go @@ -169,7 +169,7 @@ func (r *run) run(cmd *cobra.Command) { os.Exit(1) } - if err := r.SetupWithManager(mgr); err != nil { + if err := r.SetupWithManager(mgr, reconciler.SetupOpts{}); err != nil { log.Error(err, "unable to create controller", "controller", "Helm") os.Exit(1) } diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 6bbf1df..aa3282b 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -116,6 +116,10 @@ func (r *Reconciler) setupAnnotationMaps() { r.uninstallAnnotations = make(map[string]annotation.Uninstall) } +type SetupOpts struct { + DisableSetupScheme bool +} + // SetupWithManager configures a controller for the Reconciler and registers // watches. It also uses the passed Manager to initialize default values for the // Reconciler and sets up the manager's scheme with the Reconciler's configured @@ -123,11 +127,13 @@ func (r *Reconciler) setupAnnotationMaps() { // // If an error occurs setting up the Reconciler with the manager, it is // returned. -func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { +func (r *Reconciler) SetupWithManager(mgr ctrl.Manager, opts SetupOpts) error { controllerName := fmt.Sprintf("%v-controller", strings.ToLower(r.gvk.Kind)) r.addDefaults(mgr, controllerName) - r.setupScheme(mgr) + if !opts.DisableSetupScheme { + r.setupScheme(mgr) + } c, err := controller.New(controllerName, mgr, controller.Options{Reconciler: r, MaxConcurrentReconciles: r.maxConcurrentReconciles}) if err != nil { diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 96768e2..2f2b9c3 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -397,7 +397,7 @@ var _ = Describe("Reconciler", func() { }), ) Expect(err).To(BeNil()) - Expect(r.SetupWithManager(mgr)).To(Succeed()) + Expect(r.SetupWithManager(mgr, SetupOpts{})).To(Succeed()) ac, err = r.actionClientGetter.ActionClientFor(obj) Expect(err).To(BeNil()) From d81ccf4c2405df29bc17f9df240820a089790ad4 Mon Sep 17 00:00:00 2001 From: Malte Isberner <2822367+misberner@users.noreply.github.com> Date: Tue, 15 Jun 2021 12:16:55 +0200 Subject: [PATCH 08/14] Allow adding reconciliation extensions (#9) --- pkg/extensions/types.go | 12 +++++++ pkg/reconciler/reconciler.go | 69 ++++++++++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 2 deletions(-) create mode 100644 pkg/extensions/types.go diff --git a/pkg/extensions/types.go b/pkg/extensions/types.go new file mode 100644 index 0000000..689f91d --- /dev/null +++ b/pkg/extensions/types.go @@ -0,0 +1,12 @@ +package extensions + +import ( + "context" + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +// ReconcileExtension is an arbitrary extension that can be implemented to run either before +// or after the main Helm reconciliation action. +// An error returned by a ReconcileExtension will cause the Reconcile to fail, unlike a hook error. +type ReconcileExtension func(context.Context, *unstructured.Unstructured, logr.Logger) error diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index aa3282b..aada8a3 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -46,6 +46,7 @@ import ( "github.com/joelanford/helm-operator/pkg/annotation" helmclient "github.com/joelanford/helm-operator/pkg/client" + "github.com/joelanford/helm-operator/pkg/extensions" "github.com/joelanford/helm-operator/pkg/hook" "github.com/joelanford/helm-operator/pkg/internal/sdk/controllerutil" "github.com/joelanford/helm-operator/pkg/reconciler/internal/conditions" @@ -67,6 +68,9 @@ type Reconciler struct { preHooks []hook.PreHook postHooks []hook.PostHook + preExtensions []extensions.ReconcileExtension + postExtensions []extensions.ReconcileExtension + log logr.Logger gvk *schema.GroupVersionKind chrt *chart.Chart @@ -360,6 +364,20 @@ func WithPreHook(h hook.PreHook) Option { } } +// WithPreExtension is an Option that configures the reconciler to run the given +// extension before performing any reconciliation steps (including values translation). +// An error returned from the extension will cause the reconciliation to fail. +// This should be preferred to WithPreHook in most cases, except for when the logic +// depends on the translated Helm values. +// The extension will be invoked with the raw object state; meaning it needs to be careful +// to check for existence of the deletionTimestamp field. +func WithPreExtension(e extensions.ReconcileExtension) Option { + return func(r *Reconciler) error { + r.preExtensions = append(r.preExtensions, e) + return nil + } +} + // 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 { @@ -369,6 +387,22 @@ func WithPostHook(h hook.PostHook) Option { } } +// WithPostExtension is an Option that configures the reconciler to run the given +// extension after performing any reconciliation steps (including uninstall of the release, +// but not removal of the finalizer). +// An error returned from the extension will cause the reconciliation to fail, which might +// prevent the finalizer from getting removed. +// This should be preferred to WithPostHook in most cases, except for when the logic +// depends on the translated Helm values. +// The extension will be invoked with the raw object state; meaning it needs to be careful +// to check for existence of the deletionTimestamp field. +func WithPostExtension(e extensions.ReconcileExtension) Option { + return func(r *Reconciler) error { + r.postExtensions = append(r.postExtensions, e) + return nil + } +} + // WithValueTranslator is an Option that configures a function that translates a // custom resource to the values passed to Helm. // Use this if you need to customize the logic that translates your custom resource to Helm values. @@ -472,6 +506,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } u.UpdateStatus(updater.EnsureCondition(conditions.Initialized(corev1.ConditionTrue, "", ""))) + for _, ext := range r.preExtensions { + if err := ext(ctx, obj, r.log); err != nil { + u.UpdateStatus( + updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), + updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), + ) + return ctrl.Result{}, err + } + } + if obj.GetDeletionTimestamp() != nil { err := r.handleDeletion(ctx, actionClient, obj, log) return ctrl.Result{}, err @@ -531,6 +575,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } } + for _, ext := range r.postExtensions { + if err := ext(ctx, obj, r.log); err != nil { + u.UpdateStatus( + updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), + updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), + ) + return ctrl.Result{}, err + } + } + ensureDeployedRelease(&u, rel) u.UpdateStatus( updater.EnsureCondition(conditions.ReleaseFailed(corev1.ConditionFalse, "", "")), @@ -586,7 +640,7 @@ func (r *Reconciler) handleDeletion(ctx context.Context, actionClient helmclient err = applyErr } }() - return r.doUninstall(actionClient, &uninstallUpdater, obj, log) + return r.doUninstall(ctx, actionClient, &uninstallUpdater, obj, log) }(); err != nil { return err } @@ -703,7 +757,7 @@ func (r *Reconciler) doReconcile(actionClient helmclient.ActionInterface, u *upd return nil } -func (r *Reconciler) doUninstall(actionClient helmclient.ActionInterface, u *updater.Updater, obj *unstructured.Unstructured, log logr.Logger) error { +func (r *Reconciler) doUninstall(ctx context.Context, actionClient helmclient.ActionInterface, u *updater.Updater, obj *unstructured.Unstructured, log logr.Logger) error { var opts []helmclient.UninstallOption for name, annot := range r.uninstallAnnotations { if v, ok := obj.GetAnnotations()[name]; ok { @@ -723,6 +777,17 @@ func (r *Reconciler) doUninstall(actionClient helmclient.ActionInterface, u *upd } else { log.Info("Release uninstalled", "name", resp.Release.Name, "version", resp.Release.Version) } + + for _, ext := range r.postExtensions { + if err := ext(ctx, obj, r.log); err != nil { + u.UpdateStatus( + updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), + updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), + ) + return err + } + } + u.Update(updater.RemoveFinalizer(uninstallFinalizer)) u.UpdateStatus( updater.EnsureCondition(conditions.ReleaseFailed(corev1.ConditionFalse, "", "")), From 5094baf0065dba3604145299f1bdeccc77598497 Mon Sep 17 00:00:00 2001 From: Malte Isberner <2822367+misberner@users.noreply.github.com> Date: Tue, 22 Jun 2021 18:10:33 +0200 Subject: [PATCH 09/14] Allow limiting the maximum release history on upgrades (#14) --- pkg/client/actionclient.go | 1 + pkg/reconciler/reconciler.go | 25 +++++++++++++++++++++++++ pkg/reconciler/reconciler_test.go | 13 +++++++++++++ 3 files changed, 39 insertions(+) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index e6315fe..71fd45a 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -163,6 +163,7 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m if rel != nil { rollback := action.NewRollback(c.conf) rollback.Force = true + rollback.MaxHistory = upgrade.MaxHistory // As of Helm 2.13, if Upgrade returns a non-nil release, that // means the release was also recorded in the release store. diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index aada8a3..7ee0e64 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -78,6 +78,7 @@ type Reconciler struct { skipDependentWatches bool maxConcurrentReconciles int reconcilePeriod time.Duration + maxHistory int annotSetupOnce sync.Once annotations map[string]struct{} @@ -291,6 +292,18 @@ func WithReconcilePeriod(rp time.Duration) Option { } } +// WithMaxReleaseHistory specifies the maximum size of the Helm release history maintained +// on upgrades/rollbacks. Zero (default) means unlimited. +func WithMaxReleaseHistory(maxHistory int) Option { + return func(r *Reconciler) error { + if maxHistory < 0 { + return errors.New("maximum Helm release history size must not be negative") + } + r.maxHistory = maxHistory + return nil + } +} + // WithInstallAnnotations is an Option that configures Install annotations // to enable custom action.Install fields to be set based on the value of // annotations found in the custom resource watched by this reconciler. @@ -666,6 +679,12 @@ func (r *Reconciler) getReleaseState(client helmclient.ActionInterface, obj meta } var opts []helmclient.UpgradeOption + if r.maxHistory > 0 { + opts = append(opts, func(u *action.Upgrade) error { + u.MaxHistory = r.maxHistory + return nil + }) + } for name, annot := range r.upgradeAnnotations { if v, ok := obj.GetAnnotations()[name]; ok { opts = append(opts, annot.UpgradeOption(v)) @@ -710,6 +729,12 @@ func (r *Reconciler) doInstall(actionClient helmclient.ActionInterface, u *updat func (r *Reconciler) doUpgrade(actionClient helmclient.ActionInterface, u *updater.Updater, obj *unstructured.Unstructured, vals map[string]interface{}, log logr.Logger) (*release.Release, error) { var opts []helmclient.UpgradeOption + if r.maxHistory > 0 { + opts = append(opts, func(u *action.Upgrade) error { + u.MaxHistory = r.maxHistory + return nil + }) + } for name, annot := range r.upgradeAnnotations { if v, ok := obj.GetAnnotations()[name]; ok { opts = append(opts, annot.UpgradeOption(v)) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 2f2b9c3..87421da 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -173,6 +173,19 @@ var _ = Describe("Reconciler", func() { Expect(WithReconcilePeriod(-time.Nanosecond)(r)).NotTo(Succeed()) }) }) + var _ = Describe("WithMaxReleaseHistory", func() { + It("should set the max history size", func() { + Expect(WithMaxReleaseHistory(10)(r)).To(Succeed()) + Expect(r.maxHistory).To(Equal(10)) + }) + It("should allow setting the history to unlimited", func() { + Expect(WithMaxReleaseHistory(0)(r)).To(Succeed()) + Expect(r.maxHistory).To(Equal(0)) + }) + It("should fail if value is less than 0", func() { + Expect(WithMaxReleaseHistory(-1)(r)).NotTo(Succeed()) + }) + }) var _ = Describe("WithInstallAnnotations", func() { It("should set multiple reconciler install annotations", func() { a1 := annotation.InstallDisableHooks{CustomName: "my.domain/custom-name1"} From c7be7f8f2c988cd811add48530c8793ac2f6b72b Mon Sep 17 00:00:00 2001 From: Malte Isberner <2822367+misberner@users.noreply.github.com> Date: Tue, 29 Jun 2021 15:16:01 +0200 Subject: [PATCH 10/14] Allow marking releases stuck in a pending state as failed (#16) --- pkg/client/actionclient.go | 9 ++++ .../internal/conditions/conditions.go | 1 + pkg/reconciler/internal/fake/actionclient.go | 34 +++++++++---- pkg/reconciler/reconciler.go | 51 +++++++++++++++++++ 4 files changed, 84 insertions(+), 11 deletions(-) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index 71fd45a..c7a5d9a 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -62,6 +62,7 @@ type ActionInterface interface { Get(name string, opts ...GetOption) (*release.Release, error) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...InstallOption) (*release.Release, error) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...UpgradeOption) (*release.Release, error) + MarkFailed(release *release.Release, reason string) error Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) Reconcile(rel *release.Release) error } @@ -180,6 +181,14 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return rel, nil } +func (c *actionClient) MarkFailed(rel *release.Release, reason string) error { + infoCopy := *rel.Info + releaseCopy := *rel + releaseCopy.Info = &infoCopy + releaseCopy.SetStatus(release.StatusFailed, reason) + return c.conf.Releases.Update(&releaseCopy) +} + func (c *actionClient) Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) { uninstall := action.NewUninstall(c.conf) for _, o := range opts { diff --git a/pkg/reconciler/internal/conditions/conditions.go b/pkg/reconciler/internal/conditions/conditions.go index 892aee3..a37735b 100644 --- a/pkg/reconciler/internal/conditions/conditions.go +++ b/pkg/reconciler/internal/conditions/conditions.go @@ -41,6 +41,7 @@ const ( ReasonUpgradeError = status.ConditionReason("UpgradeError") ReasonReconcileError = status.ConditionReason("ReconcileError") ReasonUninstallError = status.ConditionReason("UninstallError") + ReasonPendingError = status.ConditionReason("PendingError") ) func Initialized(stat corev1.ConditionStatus, reason status.ConditionReason, message interface{}) status.Condition { diff --git a/pkg/reconciler/internal/fake/actionclient.go b/pkg/reconciler/internal/fake/actionclient.go index 410c4de..2bd6022 100644 --- a/pkg/reconciler/internal/fake/actionclient.go +++ b/pkg/reconciler/internal/fake/actionclient.go @@ -48,17 +48,19 @@ func (hcg *fakeActionClientGetter) ActionClientFor(_ crclient.Object) (client.Ac } type ActionClient struct { - Gets []GetCall - Installs []InstallCall - Upgrades []UpgradeCall - Uninstalls []UninstallCall - Reconciles []ReconcileCall - - HandleGet func() (*release.Release, error) - HandleInstall func() (*release.Release, error) - HandleUpgrade func() (*release.Release, error) - HandleUninstall func() (*release.UninstallReleaseResponse, error) - HandleReconcile func() error + Gets []GetCall + Installs []InstallCall + Upgrades []UpgradeCall + MarkFaileds []MarkFailedCall + Uninstalls []UninstallCall + Reconciles []ReconcileCall + + HandleGet func() (*release.Release, error) + HandleInstall func() (*release.Release, error) + HandleUpgrade func() (*release.Release, error) + HandleMarkFailed func() error + HandleUninstall func() (*release.UninstallReleaseResponse, error) + HandleReconcile func() error } func NewActionClient() ActionClient { @@ -109,6 +111,11 @@ type UpgradeCall struct { Opts []client.UpgradeOption } +type MarkFailedCall struct { + Release *release.Release + Reason string +} + type UninstallCall struct { Name string Opts []client.UninstallOption @@ -133,6 +140,11 @@ func (c *ActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return c.HandleUpgrade() } +func (c *ActionClient) MarkFailed(rel *release.Release, reason string) error { + c.MarkFaileds = append(c.MarkFaileds, MarkFailedCall{rel, reason}) + return c.HandleMarkFailed() +} + func (c *ActionClient) Uninstall(name string, opts ...client.UninstallOption) (*release.UninstallReleaseResponse, error) { c.Uninstalls = append(c.Uninstalls, UninstallCall{name, opts}) return c.HandleUninstall() diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 7ee0e64..bedde99 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -78,6 +78,7 @@ type Reconciler struct { skipDependentWatches bool maxConcurrentReconciles int reconcilePeriod time.Duration + markFailedAfter time.Duration maxHistory int annotSetupOnce sync.Once @@ -304,6 +305,18 @@ func WithMaxReleaseHistory(maxHistory int) Option { } } +// WithMarkFailedAfter specifies the duration after which the reconciler will mark a release in a pending (locked) +// state as false in order to allow rolling forward. +func WithMarkFailedAfter(duration time.Duration) Option { + return func(r *Reconciler) error { + if duration < 0 { + return errors.New("auto-rollback after duration must not be negative") + } + r.markFailedAfter = duration + return nil + } +} + // WithInstallAnnotations is an Option that configures Install annotations // to enable custom action.Install fields to be set based on the value of // annotations found in the custom resource watched by this reconciler. @@ -553,6 +566,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. ) return ctrl.Result{}, err } + if state == statePending { + return r.handlePending(actionClient, rel, &u, log) + } + u.UpdateStatus(updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", ""))) for _, h := range r.preHooks { @@ -630,6 +647,7 @@ const ( stateNeedsInstall helmReleaseState = "needs install" stateNeedsUpgrade helmReleaseState = "needs upgrade" stateUnchanged helmReleaseState = "unchanged" + statePending helmReleaseState = "pending" stateError helmReleaseState = "error" ) @@ -678,6 +696,10 @@ func (r *Reconciler) getReleaseState(client helmclient.ActionInterface, obj meta return nil, stateNeedsInstall, nil } + if currentRelease.Info != nil && currentRelease.Info.Status.IsPending() { + return currentRelease, statePending, nil + } + var opts []helmclient.UpgradeOption if r.maxHistory > 0 { opts = append(opts, func(u *action.Upgrade) error { @@ -755,6 +777,35 @@ func (r *Reconciler) doUpgrade(actionClient helmclient.ActionInterface, u *updat return rel, nil } +func (r *Reconciler) handlePending(actionClient helmclient.ActionInterface, rel *release.Release, u *updater.Updater, log logr.Logger) (ctrl.Result, error) { + err := r.doHandlePending(actionClient, rel, log) + if err == nil { + err = errors.New("unknown error handling pending release") + } + u.UpdateStatus( + updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonPendingError, err))) + return ctrl.Result{}, err +} + +func (r *Reconciler) doHandlePending(actionClient helmclient.ActionInterface, rel *release.Release, log logr.Logger) error { + if r.markFailedAfter <= 0 { + return errors.New("Release is in a pending (locked) state and cannot be modified. User intervention is required.") + } + if rel.Info == nil || rel.Info.LastDeployed.IsZero() { + return errors.New("Release is in a pending (locked) state and lacks 'last deployed' timestamp. User intervention is required.") + } + if pendingSince := time.Since(rel.Info.LastDeployed.Time); pendingSince < r.markFailedAfter { + return fmt.Errorf("Release is in a pending (locked) state and cannot currently be modified. Release will be marked failed to allow a roll-forward in %v.", r.markFailedAfter-pendingSince) + } + + log.Info("Marking release as failed", "releaseName", rel.Name) + err := actionClient.MarkFailed(rel, fmt.Sprintf("operator marked pending (locked) release as failed after state did not change for %v", r.markFailedAfter)) + if err != nil { + return fmt.Errorf("Failed to mark pending (locked) release as failed: %w", err) + } + return fmt.Errorf("marked release %s as failed to allow upgrade to succeed in next reconcile attempt", rel.Name) +} + func (r *Reconciler) reportOverrideEvents(obj runtime.Object) { for k, v := range r.overrideValues { r.eventRecorder.Eventf(obj, "Warning", "ValueOverridden", From e0130a46e4bc423cdde4cbbe97bac26e8f3c50c6 Mon Sep 17 00:00:00 2001 From: Malte Isberner <2822367+misberner@users.noreply.github.com> Date: Thu, 1 Jul 2021 12:01:09 +0200 Subject: [PATCH 11/14] Allow stripping manifest from the CR status (#18) --- pkg/reconciler/reconciler.go | 26 +++++++++++++++++++++++--- pkg/reconciler/reconciler_test.go | 10 ++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index bedde99..25d45ad 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -81,6 +81,8 @@ type Reconciler struct { markFailedAfter time.Duration maxHistory int + stripManifestFromStatus bool + annotSetupOnce sync.Once annotations map[string]struct{} installAnnotations map[string]annotation.Install @@ -265,6 +267,17 @@ func SkipDependentWatches(skip bool) Option { } } +// StripManifestFromStatus is an Option that configures whether the manifest +// should be removed from the automatically populated status. +// This is recommended if the manifest might return sensitive data (i.e., +// secrets). +func StripManifestFromStatus(strip bool) Option { + return func(r *Reconciler) error { + r.stripManifestFromStatus = strip + return nil + } +} + // WithMaxConcurrentReconciles is an Option that configures the number of // concurrent reconciles that the controller will run. // @@ -528,7 +541,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. if errors.Is(err, driver.ErrReleaseNotFound) { u.UpdateStatus(updater.EnsureCondition(conditions.Deployed(corev1.ConditionFalse, "", ""))) } else if err == nil { - ensureDeployedRelease(&u, rel) + r.ensureDeployedRelease(&u, rel) } u.UpdateStatus(updater.EnsureCondition(conditions.Initialized(corev1.ConditionTrue, "", ""))) @@ -615,7 +628,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } } - ensureDeployedRelease(&u, rel) + r.ensureDeployedRelease(&u, rel) u.UpdateStatus( updater.EnsureCondition(conditions.ReleaseFailed(corev1.ConditionFalse, "", "")), updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", "")), @@ -943,7 +956,7 @@ func (r *Reconciler) setupWatches(mgr ctrl.Manager, c controller.Controller) err return nil } -func ensureDeployedRelease(u *updater.Updater, rel *release.Release) { +func (r *Reconciler) ensureDeployedRelease(u *updater.Updater, rel *release.Release) { reason := conditions.ReasonInstallSuccessful message := "release was successfully installed" if rel.Version > 1 { @@ -953,6 +966,13 @@ func ensureDeployedRelease(u *updater.Updater, rel *release.Release) { if rel.Info != nil && len(rel.Info.Notes) > 0 { message = rel.Info.Notes } + + if r.stripManifestFromStatus { + relCopy := *rel + relCopy.Manifest = "" + rel = &relCopy + } + u.Update(updater.EnsureFinalizer(uninstallFinalizer)) u.UpdateStatus( updater.EnsureCondition(conditions.Deployed(corev1.ConditionTrue, reason, message)), diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 87421da..f6ea2df 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -154,6 +154,16 @@ var _ = Describe("Reconciler", func() { Expect(r.skipDependentWatches).To(Equal(true)) }) }) + var _ = Describe("StripManifestFromStatus", func() { + It("should set to false", func() { + Expect(StripManifestFromStatus(false)(r)).To(Succeed()) + Expect(r.stripManifestFromStatus).To(Equal(false)) + }) + It("should set to true", func() { + Expect(StripManifestFromStatus(true)(r)).To(Succeed()) + Expect(r.stripManifestFromStatus).To(Equal(true)) + }) + }) var _ = Describe("WithMaxConcurrentReconciles", func() { It("should set the reconciler max concurrent reconciled", func() { Expect(WithMaxConcurrentReconciles(1)(r)).To(Succeed()) From 7857f66cf95e20c0a7bd7f92b466d34e1bb61a30 Mon Sep 17 00:00:00 2001 From: Malte Isberner <2822367+misberner@users.noreply.github.com> Date: Tue, 6 Jul 2021 02:52:54 +0200 Subject: [PATCH 12/14] ROX-7242: Make the operator preserve custom statuses, and allow updating custom status through extensions (#17) --- pkg/extensions/types.go | 7 +- pkg/reconciler/internal/updater/updater.go | 39 +++++++++-- .../internal/updater/updater_test.go | 70 ++++++++++++++++++- pkg/reconciler/reconciler.go | 6 +- 4 files changed, 111 insertions(+), 11 deletions(-) diff --git a/pkg/extensions/types.go b/pkg/extensions/types.go index 689f91d..fe449f8 100644 --- a/pkg/extensions/types.go +++ b/pkg/extensions/types.go @@ -2,11 +2,16 @@ package extensions import ( "context" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) +// UpdateStatusFunc is a function that updates an unstructured status. If the status has been modified, +// true must be returned, false otherwise. +type UpdateStatusFunc func(*unstructured.Unstructured) bool + // ReconcileExtension is an arbitrary extension that can be implemented to run either before // or after the main Helm reconciliation action. // An error returned by a ReconcileExtension will cause the Reconcile to fail, unlike a hook error. -type ReconcileExtension func(context.Context, *unstructured.Unstructured, logr.Logger) error +type ReconcileExtension func(context.Context, *unstructured.Unstructured, func(UpdateStatusFunc), logr.Logger) error diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index 590b5be..54ca71f 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -26,6 +26,7 @@ import ( "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/joelanford/helm-operator/pkg/extensions" "github.com/joelanford/helm-operator/pkg/internal/sdk/controllerutil" "github.com/joelanford/helm-operator/pkg/internal/sdk/status" ) @@ -53,6 +54,21 @@ func (u *Updater) UpdateStatus(fs ...UpdateStatusFunc) { u.updateStatusFuncs = append(u.updateStatusFuncs, fs...) } +func (u *Updater) UpdateStatusCustom(f extensions.UpdateStatusFunc) { + updateFn := func(status *helmAppStatus) bool { + status.updateStatusObject() + + unstructuredStatus := unstructured.Unstructured{Object: status.StatusObject} + if !f(&unstructuredStatus) { + return false + } + _ = runtime.DefaultUnstructuredConverter.FromUnstructured(unstructuredStatus.Object, status) + status.StatusObject = unstructuredStatus.Object + return true + } + u.UpdateStatus(updateFn) +} + func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) error { backoff := retry.DefaultRetry @@ -66,11 +82,8 @@ func (u *Updater) Apply(ctx context.Context, obj *unstructured.Unstructured) err needsStatusUpdate = f(st) || needsStatusUpdate } if needsStatusUpdate { - uSt, err := runtime.DefaultUnstructuredConverter.ToUnstructured(st) - if err != nil { - return err - } - obj.Object["status"] = uSt + st.updateStatusObject() + obj.Object["status"] = st.StatusObject return u.client.Status().Update(ctx, obj) } return nil @@ -149,10 +162,25 @@ func RemoveDeployedRelease() UpdateStatusFunc { } type helmAppStatus struct { + StatusObject map[string]interface{} `json:"-"` + Conditions status.Conditions `json:"conditions"` DeployedRelease *helmAppRelease `json:"deployedRelease,omitempty"` } +func (s *helmAppStatus) updateStatusObject() { + unstructuredHelmAppStatus, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(s) + if s.StatusObject == nil { + s.StatusObject = make(map[string]interface{}) + } + s.StatusObject["conditions"] = unstructuredHelmAppStatus["conditions"] + if deployedRelease := unstructuredHelmAppStatus["deployedRelease"]; deployedRelease != nil { + s.StatusObject["deployedRelease"] = deployedRelease + } else { + delete(s.StatusObject, "deployedRelease") + } +} + type helmAppRelease struct { Name string `json:"name,omitempty"` Manifest string `json:"manifest,omitempty"` @@ -175,6 +203,7 @@ func statusFor(obj *unstructured.Unstructured) *helmAppStatus { case map[string]interface{}: out := &helmAppStatus{} _ = runtime.DefaultUnstructuredConverter.FromUnstructured(s, out) + out.StatusObject = s return out default: return &helmAppStatus{} diff --git a/pkg/reconciler/internal/updater/updater_test.go b/pkg/reconciler/internal/updater/updater_test.go index 7df9d30..a52f2dc 100644 --- a/pkg/reconciler/internal/updater/updater_test.go +++ b/pkg/reconciler/internal/updater/updater_test.go @@ -86,6 +86,71 @@ var _ = Describe("Updater", func() { Expect((obj.Object["status"].(map[string]interface{}))["conditions"]).To(HaveLen(1)) Expect(obj.GetResourceVersion()).NotTo(Equal(resourceVersion)) }) + + It("should support a mix of standard and custom status updates", func() { + u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) + u.UpdateStatusCustom(func(uSt *unstructured.Unstructured) bool { + Expect(unstructured.SetNestedMap(uSt.Object, map[string]interface{}{"bar": "baz"}, "foo")).To(Succeed()) + return true + }) + u.UpdateStatus(EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", ""))) + u.UpdateStatusCustom(func(uSt *unstructured.Unstructured) bool { + Expect(unstructured.SetNestedField(uSt.Object, "quux", "foo", "qux")).To(Succeed()) + return true + }) + u.UpdateStatus(EnsureCondition(conditions.Initialized(corev1.ConditionTrue, "", ""))) + + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + Expect(client.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + Expect((obj.Object["status"].(map[string]interface{}))["conditions"]).To(HaveLen(3)) + _, found, err := unstructured.NestedFieldNoCopy(obj.Object, "status", "deployedRelease") + Expect(found).To(BeFalse()) + Expect(err).To(Not(HaveOccurred())) + + val, found, err := unstructured.NestedString(obj.Object, "status", "foo", "bar") + Expect(val).To(Equal("baz")) + Expect(found).To(BeTrue()) + Expect(err).To(Not(HaveOccurred())) + + val, found, err = unstructured.NestedString(obj.Object, "status", "foo", "qux") + Expect(val).To(Equal("quux")) + Expect(found).To(BeTrue()) + Expect(err).To(Not(HaveOccurred())) + }) + + It("should preserve any custom status across multiple apply calls", func() { + u.UpdateStatusCustom(func(uSt *unstructured.Unstructured) bool { + Expect(unstructured.SetNestedMap(uSt.Object, map[string]interface{}{"bar": "baz"}, "foo")).To(Succeed()) + return true + }) + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + + Expect(client.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + + _, found, err := unstructured.NestedFieldNoCopy(obj.Object, "status", "deployedRelease") + Expect(found).To(BeFalse()) + Expect(err).To(Not(HaveOccurred())) + + val, found, err := unstructured.NestedString(obj.Object, "status", "foo", "bar") + Expect(val).To(Equal("baz")) + Expect(found).To(BeTrue()) + Expect(err).To(Succeed()) + + u.UpdateStatus(EnsureCondition(conditions.Deployed(corev1.ConditionTrue, "", ""))) + Expect(u.Apply(context.TODO(), obj)).To(Succeed()) + + Expect(client.Get(context.TODO(), types.NamespacedName{Namespace: "testNamespace", Name: "testDeployment"}, obj)).To(Succeed()) + Expect((obj.Object["status"].(map[string]interface{}))["conditions"]).To(HaveLen(1)) + + _, found, err = unstructured.NestedFieldNoCopy(obj.Object, "status", "deployedRelease") + Expect(found).To(BeFalse()) + Expect(err).To(Not(HaveOccurred())) + + val, found, err = unstructured.NestedString(obj.Object, "status", "foo", "bar") + Expect(val).To(Equal("baz")) + Expect(found).To(BeTrue()) + Expect(err).To(Succeed()) + }) }) }) @@ -241,8 +306,9 @@ var _ = Describe("statusFor", func() { }) It("should handle map[string]interface{}", func() { - obj.Object["status"] = map[string]interface{}{} - Expect(statusFor(obj)).To(Equal(&helmAppStatus{})) + uSt := map[string]interface{}{} + obj.Object["status"] = uSt + Expect(statusFor(obj)).To(Equal(&helmAppStatus{StatusObject: uSt})) }) It("should handle arbitrary types", func() { diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 25d45ad..f8aa983 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -546,7 +546,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. u.UpdateStatus(updater.EnsureCondition(conditions.Initialized(corev1.ConditionTrue, "", ""))) for _, ext := range r.preExtensions { - if err := ext(ctx, obj, r.log); err != nil { + if err := ext(ctx, obj, u.UpdateStatusCustom, r.log); err != nil { u.UpdateStatus( updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), @@ -619,7 +619,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } for _, ext := range r.postExtensions { - if err := ext(ctx, obj, r.log); err != nil { + if err := ext(ctx, obj, u.UpdateStatusCustom, r.log); err != nil { u.UpdateStatus( updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), @@ -868,7 +868,7 @@ func (r *Reconciler) doUninstall(ctx context.Context, actionClient helmclient.Ac } for _, ext := range r.postExtensions { - if err := ext(ctx, obj, r.log); err != nil { + if err := ext(ctx, obj, u.UpdateStatusCustom, r.log); err != nil { u.UpdateStatus( updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonReconcileError, err)), updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), From c4258db5669f7d1d8cf39dc54f4e30f394d1b2bd Mon Sep 17 00:00:00 2001 From: Malte Isberner Date: Wed, 7 Jul 2021 18:25:11 +0200 Subject: [PATCH 13/14] X-Smart-Branch-Parent: main From e82aebad4b7b6241f8ae6335f8a81b402d31b8a0 Mon Sep 17 00:00:00 2001 From: Malte Isberner Date: Thu, 8 Jul 2021 03:05:03 +0200 Subject: [PATCH 14/14] allow custom dependent watches --- pkg/reconciler/internal/hook/hook.go | 49 ++++++++++++++++------- pkg/reconciler/internal/hook/hook_test.go | 18 ++++----- pkg/reconciler/reconciler.go | 15 ++++++- 3 files changed, 56 insertions(+), 26 deletions(-) diff --git a/pkg/reconciler/internal/hook/hook.go b/pkg/reconciler/internal/hook/hook.go index 8299739..9b2a305 100644 --- a/pkg/reconciler/internal/hook/hook.go +++ b/pkg/reconciler/internal/hook/hook.go @@ -37,18 +37,22 @@ import ( "github.com/joelanford/helm-operator/pkg/manifestutil" ) -func NewDependentResourceWatcher(c controller.Controller, rm meta.RESTMapper) hook.PostHook { +func NewDependentResourceWatcher(c controller.Controller, rm meta.RESTMapper, watchReleaseResources bool, extraWatches ...schema.GroupVersionKind) hook.PostHook { return &dependentResourceWatcher{ - controller: c, - restMapper: rm, - m: sync.Mutex{}, - watches: make(map[schema.GroupVersionKind]struct{}), + controller: c, + restMapper: rm, + watchReleaseResources: watchReleaseResources, + extraWatches: extraWatches, + m: sync.Mutex{}, + watches: make(map[schema.GroupVersionKind]struct{}), } } type dependentResourceWatcher struct { - controller controller.Controller - restMapper meta.RESTMapper + controller controller.Controller + restMapper meta.RESTMapper + watchReleaseResources bool + extraWatches []schema.GroupVersionKind m sync.Mutex watches map[schema.GroupVersionKind]struct{} @@ -58,16 +62,31 @@ func (d *dependentResourceWatcher) Exec(owner *unstructured.Unstructured, rel re // using predefined functions for filtering events dependentPredicate := predicate.DependentPredicateFuncs() - resources := releaseutil.SplitManifests(rel.Manifest) - d.m.Lock() - defer d.m.Unlock() - for _, r := range resources { - var obj unstructured.Unstructured - err := yaml.Unmarshal([]byte(r), &obj) - if err != nil { - return err + var allWatches []unstructured.Unstructured + if d.watchReleaseResources { + resources := releaseutil.SplitManifests(rel.Manifest) + for _, r := range resources { + var obj unstructured.Unstructured + err := yaml.Unmarshal([]byte(r), &obj) + if err != nil { + return err + } + + allWatches = append(allWatches, obj) } + } + // For extra watches, we only support same namespace + for _, extraWatchGVK := range d.extraWatches { + var obj unstructured.Unstructured + obj.SetGroupVersionKind(extraWatchGVK) + obj.SetNamespace(owner.GetNamespace()) + allWatches = append(allWatches, obj) + } + + d.m.Lock() + defer d.m.Unlock() + for _, obj := range allWatches { depGVK := obj.GroupVersionKind() if _, ok := d.watches[depGVK]; ok || depGVK.Empty() { continue diff --git a/pkg/reconciler/internal/hook/hook_test.go b/pkg/reconciler/internal/hook/hook_test.go index 6861e1c..dcda5f6 100644 --- a/pkg/reconciler/internal/hook/hook_test.go +++ b/pkg/reconciler/internal/hook/hook_test.go @@ -66,7 +66,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{rsOwnerNamespace}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) }) It("should fail with an invalid release manifest", func() { rel.Manifest = "---\nfoobar" @@ -110,7 +110,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{clusterRole, clusterRole, rsOwnerNamespace, rsOwnerNamespace}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -133,7 +133,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{rsOwnerNamespace, ssOtherNamespace}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -144,7 +144,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{clusterRole, clusterRoleBinding}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(2)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -154,7 +154,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{rsOwnerNamespaceWithKeep, ssOtherNamespaceWithKeep, clusterRoleWithKeep, clusterRoleBindingWithKeep}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(4)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) @@ -181,7 +181,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{rsOwnerNamespace}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(1)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&handler.EnqueueRequestForOwner{})) @@ -190,7 +190,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{clusterRole}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(1)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) @@ -199,7 +199,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{ssOtherNamespace}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(1)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) @@ -208,7 +208,7 @@ var _ = Describe("Hook", func() { rel = &release.Release{ Manifest: strings.Join([]string{rsOwnerNamespaceWithKeep, ssOtherNamespaceWithKeep, clusterRoleWithKeep}, "---\n"), } - drw = internalhook.NewDependentResourceWatcher(c, rm) + drw = internalhook.NewDependentResourceWatcher(c, rm, true) Expect(drw.Exec(owner, *rel, log)).To(Succeed()) Expect(c.WatchCalls).To(HaveLen(3)) Expect(c.WatchCalls[0].Handler).To(BeAssignableToTypeOf(&sdkhandler.EnqueueRequestForAnnotation{})) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index f8aa983..20c0110 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -76,6 +76,7 @@ type Reconciler struct { chrt *chart.Chart overrideValues map[string]string skipDependentWatches bool + extraDependentWatches []schema.GroupVersionKind maxConcurrentReconciles int reconcilePeriod time.Duration markFailedAfter time.Duration @@ -267,6 +268,16 @@ func SkipDependentWatches(skip bool) Option { } } +// WithExtraDependentWatches is an option that configures whether the reconciler +// will watch objects of the given kind. These objects will be watched even if +// SkipDependentWatches is set to true. +func WithExtraDependentWatches(gvks ...schema.GroupVersionKind) Option { + return func(r *Reconciler) error { + r.extraDependentWatches = append(r.extraDependentWatches, gvks...) + return nil + } +} + // StripManifestFromStatus is an Option that configures whether the manifest // should be removed from the automatically populated status. // This is recommended if the manifest might return sensitive data (i.e., @@ -950,8 +961,8 @@ func (r *Reconciler) setupWatches(mgr ctrl.Manager, c controller.Controller) err return err } - if !r.skipDependentWatches { - r.postHooks = append([]hook.PostHook{internalhook.NewDependentResourceWatcher(c, mgr.GetRESTMapper())}, r.postHooks...) + if !r.skipDependentWatches || len(r.extraDependentWatches) > 0 { + r.postHooks = append([]hook.PostHook{internalhook.NewDependentResourceWatcher(c, mgr.GetRESTMapper(), !r.skipDependentWatches, r.extraDependentWatches...)}, r.postHooks...) } return nil }