From 09a6eb4de4e698e0896379edda6458b3e6c72c39 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Tue, 25 May 2021 11:25:25 +0200 Subject: [PATCH] Add a WithValueTranslator option to Reconciller. A Translator is a way to produces helm values based on the fetched custom resource itself (unlike `Mapper` which can only see `Values`). This way the code which converts the custom resource to Helm values can first convert an `Unstructured` into a regular struct, and then rely on Go type safety rather than work with a tree of maps from `string` to `interface{}`. Thanks to having access to a `Context`, the code can also safely access the network, for example in order to retrieve other resources from the k8s cluster, when they are referenced by the custom resource. --- pkg/reconciler/internal/values/values.go | 51 +++++------ pkg/reconciler/internal/values/values_test.go | 91 +++++++++---------- pkg/reconciler/reconciler.go | 52 +++++++++-- pkg/reconciler/reconciler_test.go | 77 +++++++++++++++- pkg/values/values.go | 22 +++++ 5 files changed, 205 insertions(+), 88 deletions(-) diff --git a/pkg/reconciler/internal/values/values.go b/pkg/reconciler/internal/values/values.go index dd6e51fd..77e8bdd6 100644 --- a/pkg/reconciler/internal/values/values.go +++ b/pkg/reconciler/internal/values/values.go @@ -17,21 +17,37 @@ limitations under the License. package values import ( + "context" "fmt" - "os" - "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/strvals" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "os" "github.com/operator-framework/helm-operator-plugins/pkg/values" ) -type Values struct { - m map[string]interface{} +var DefaultMapper = values.MapperFunc(func(v chartutil.Values) chartutil.Values { return v }) + +var DefaultTranslator = values.TranslatorFunc(func(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) { + return getSpecMap(u) +}) + +func ApplyOverrides(overrideValues map[string]string, obj *unstructured.Unstructured) error { + specMap, err := getSpecMap(obj) + if err != nil { + return err + } + for inK, inV := range overrideValues { + val := fmt.Sprintf("%s=%s", inK, os.ExpandEnv(inV)) + if err := strvals.ParseInto(val, specMap); err != nil { + return err + } + } + return nil } -func FromUnstructured(obj *unstructured.Unstructured) (*Values, error) { +func getSpecMap(obj *unstructured.Unstructured) (map[string]interface{}, error) { if obj == nil || obj.Object == nil { return nil, fmt.Errorf("nil object") } @@ -43,28 +59,5 @@ func FromUnstructured(obj *unstructured.Unstructured) (*Values, error) { if !ok { return nil, fmt.Errorf("spec must be a map") } - return New(specMap), nil -} - -func New(m map[string]interface{}) *Values { - return &Values{m: m} -} - -func (v *Values) Map() map[string]interface{} { - if v == nil { - return nil - } - return v.m + return specMap, nil } - -func (v *Values) ApplyOverrides(in map[string]string) error { - for inK, inV := range in { - val := fmt.Sprintf("%s=%s", inK, os.ExpandEnv(inV)) - if err := strvals.ParseInto(val, v.m); err != nil { - return err - } - } - return nil -} - -var DefaultMapper = values.MapperFunc(func(v chartutil.Values) chartutil.Values { return v }) diff --git a/pkg/reconciler/internal/values/values_test.go b/pkg/reconciler/internal/values/values_test.go index d9c2d049..29dfce39 100644 --- a/pkg/reconciler/internal/values/values_test.go +++ b/pkg/reconciler/internal/values/values_test.go @@ -17,6 +17,7 @@ limitations under the License. package values_test import ( + "context" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "helm.sh/helm/v3/pkg/chartutil" @@ -25,73 +26,50 @@ import ( . "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/values" ) -var _ = Describe("Values", func() { - var _ = Describe("FromUnstructured", func() { - It("should error with nil object", func() { - u := &unstructured.Unstructured{} - v, err := FromUnstructured(u) - Expect(v).To(BeNil()) - Expect(err).NotTo(BeNil()) - }) +var _ = Describe("ApplyOverrides", func() { + var u *unstructured.Unstructured - It("should error with missing spec", func() { - u := &unstructured.Unstructured{Object: map[string]interface{}{}} - v, err := FromUnstructured(u) - Expect(v).To(BeNil()) - Expect(err).NotTo(BeNil()) + When("Unstructured object is invalid", func() { + It("should error with nil unstructured", func() { + u = nil + Expect(ApplyOverrides(nil, u)).NotTo(BeNil()) }) - It("should error with non-map spec", func() { - u := &unstructured.Unstructured{Object: map[string]interface{}{"spec": 0}} - v, err := FromUnstructured(u) - Expect(v).To(BeNil()) - Expect(err).NotTo(BeNil()) + It("should error with nil object", func() { + u = &unstructured.Unstructured{} + Expect(ApplyOverrides(nil, u)).NotTo(BeNil()) }) - It("should succeed with valid spec", func() { - values := New(map[string]interface{}{"foo": "bar"}) - u := &unstructured.Unstructured{Object: map[string]interface{}{"spec": values.Map()}} - Expect(FromUnstructured(u)).To(Equal(values)) + It("should error with missing spec", func() { + u = &unstructured.Unstructured{Object: map[string]interface{}{}} + Expect(ApplyOverrides(nil, u)).NotTo(BeNil()) }) - }) - var _ = Describe("New", func() { - It("should return new values", func() { - m := map[string]interface{}{"foo": "bar"} - v := New(m) - Expect(v.Map()).To(Equal(m)) + It("should error with non-map spec", func() { + u = &unstructured.Unstructured{Object: map[string]interface{}{"spec": 0}} + Expect(ApplyOverrides(nil, u)).NotTo(BeNil()) }) }) - var _ = Describe("Map", func() { - It("should return nil with nil values", func() { - var v *Values - Expect(v.Map()).To(BeNil()) - }) + When("Unstructured object is valid", func() { - It("should return values as a map", func() { - m := map[string]interface{}{"foo": "bar"} - v := New(m) - Expect(v.Map()).To(Equal(m)) + BeforeEach(func() { + u = &unstructured.Unstructured{Object: map[string]interface{}{"spec": map[string]interface{}{}}} }) - }) - var _ = Describe("ApplyOverrides", func() { It("should succeed with empty values", func() { - v := New(map[string]interface{}{}) - Expect(v.ApplyOverrides(map[string]string{"foo": "bar"})).To(Succeed()) - Expect(v.Map()).To(Equal(map[string]interface{}{"foo": "bar"})) + Expect(ApplyOverrides(map[string]string{"foo": "bar"}, u)).To(Succeed()) + Expect(u.Object).To(Equal(map[string]interface{}{"spec": map[string]interface{}{"foo": "bar"}})) }) - It("should succeed with empty values", func() { - v := New(map[string]interface{}{"foo": "bar"}) - Expect(v.ApplyOverrides(map[string]string{"foo": "baz"})).To(Succeed()) - Expect(v.Map()).To(Equal(map[string]interface{}{"foo": "baz"})) + It("should succeed with non-empty values", func() { + u.Object["spec"].(map[string]interface{})["foo"] = "bar" + Expect(ApplyOverrides(map[string]string{"foo": "baz"}, u)).To(Succeed()) + Expect(u.Object).To(Equal(map[string]interface{}{"spec": map[string]interface{}{"foo": "baz"}})) }) It("should fail with invalid overrides", func() { - v := New(map[string]interface{}{"foo": "bar"}) - Expect(v.ApplyOverrides(map[string]string{"foo[": "test"})).ToNot(BeNil()) + Expect(ApplyOverrides(map[string]string{"foo[": "test"}, u)).ToNot(BeNil()) }) }) }) @@ -103,3 +81,20 @@ var _ = Describe("DefaultMapper", func() { Expect(out).To(Equal(in)) }) }) + +var _ = Describe("DefaultTranslator", func() { + var m map[string]interface{} + + It("returns empty spec untouched", func() { + m = map[string]interface{}{} + }) + + It("returns filled spec untouched", func() { + m = map[string]interface{}{"something": 0} + }) + + AfterEach(func() { + u := &unstructured.Unstructured{Object: map[string]interface{}{"spec": m}} + Expect(DefaultTranslator.Translate(context.Background(), u)).To(Equal(chartutil.Values(m))) + }) +}) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 533cfff5..df0f941e 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -61,7 +61,8 @@ const uninstallFinalizer = "uninstall-helm-release" type Reconciler struct { client client.Client actionClientGetter helmclient.ActionClientGetter - valueMapper values.Mapper + valueTranslator values.Translator + valueMapper values.Mapper // nolint:staticcheck eventRecorder record.EventRecorder preHooks []hook.PreHook postHooks []hook.PostHook @@ -231,8 +232,8 @@ func WithOverrideValues(overrides map[string]string) Option { // Validate that overrides can be parsed and applied // so that we fail fast during operator setup rather // than during the first reconciliation. - m := internalvalues.New(map[string]interface{}{}) - if err := m.ApplyOverrides(overrides); err != nil { + obj := &unstructured.Unstructured{Object: map[string]interface{}{"spec": map[string]interface{}{}}} + if err := internalvalues.ApplyOverrides(overrides, obj); err != nil { return err } @@ -375,8 +376,36 @@ 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. +// If you wish to, you can convert the Unstructured that is passed to your Translator to your own +// Custom Resource struct like this: +// +// import "k8s.io/apimachinery/pkg/runtime" +// foo := your.Foo{} +// if err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &foo); err != nil { +// return nil, err +// } +// // work with the type-safe foo +// +// Alternatively, your translator can also work similarly to a Mapper, by accessing the spec with: +// +// u.Object["spec"].(map[string]interface{}) +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. +// +// Deprecated: Use WithValueTranslator instead. +// WithValueMapper will be removed in a future release. func WithValueMapper(m values.Mapper) Option { return func(r *Reconciler) error { r.valueMapper = m @@ -471,7 +500,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,15 +563,15 @@ 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) { - crVals, err := internalvalues.FromUnstructured(obj) - if err != nil { +func (r *Reconciler) getValues(ctx context.Context, obj *unstructured.Unstructured) (chartutil.Values, error) { + if err := internalvalues.ApplyOverrides(r.overrideValues, obj); err != nil { return chartutil.Values{}, err } - if err := crVals.ApplyOverrides(r.overrideValues); err != nil { + vals, err := r.valueTranslator.Translate(ctx, obj) + if err != nil { return chartutil.Values{}, err } - vals := r.valueMapper.Map(crVals.Map()) + vals = r.valueMapper.Map(vals) vals, err = chartutil.CoalesceValues(r.chrt, vals) if err != nil { return chartutil.Values{}, err @@ -761,6 +790,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/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 290a5f39..7c6d982d 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -372,6 +372,16 @@ var _ = Describe("Reconciler", func() { Expect(r.valueMapper.Map(chartutil.Values{})).To(Equal(chartutil.Values{"mapped": true})) }) }) + var _ = Describe("WithValueTranslator", func() { + It("should set the reconciler value translator", func() { + translator := values.TranslatorFunc(func(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) { + return chartutil.Values{"translated": true}, nil + }) + Expect(WithValueTranslator(translator)(r)).To(Succeed()) + Expect(r.valueTranslator).NotTo(BeNil()) + Expect(r.valueTranslator.Translate(context.Background(), &unstructured.Unstructured{})).To(Equal(chartutil.Values{"translated": true})) + }) + }) }) var _ = Describe("Reconcile", func() { @@ -550,6 +560,7 @@ var _ = Describe("Reconciler", func() { By("reconciling unsuccessfully", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) + Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("error parsing index")) }) @@ -795,6 +806,7 @@ var _ = Describe("Reconciler", func() { By("reconciling unsuccessfully", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) + Expect(err).ToNot(BeNil()) Expect(err.Error()).To(ContainSubstring("error parsing index")) }) @@ -824,6 +836,45 @@ var _ = Describe("Reconciler", func() { }) }) }) + When("value translator fails", func() { + BeforeEach(func() { + r.valueTranslator = values.TranslatorFunc(func(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) { + return nil, errors.New("translation failure") + }) + }) + It("returns an error", func() { + By("reconciling unsuccessfully", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err.Error()).To(ContainSubstring("translation failure")) + }) + + By("getting the CR", func() { + Expect(mgr.GetAPIReader().Get(ctx, objKey, obj)).To(Succeed()) + }) + + By("verifying the CR status", func() { + objStat := &objStatus{} + Expect(runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, objStat)).To(Succeed()) + Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeInitialized)).To(BeTrue()) + Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeIrreconcilable)).To(BeTrue()) + Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypeDeployed)).To(BeTrue()) + Expect(objStat.Status.Conditions.IsUnknownFor(conditions.TypeReleaseFailed)).To(BeTrue()) + + c := objStat.Status.Conditions.GetCondition(conditions.TypeIrreconcilable) + Expect(c).NotTo(BeNil()) + Expect(c.Reason).To(Equal(conditions.ReasonErrorGettingValues)) + Expect(c.Message).To(ContainSubstring("translation failure")) + + Expect(objStat.Status.DeployedRelease.Name).To(Equal(currentRelease.Name)) + Expect(objStat.Status.DeployedRelease.Manifest).To(Equal(currentRelease.Manifest)) + }) + + By("verifying the uninstall finalizer is not present on the CR", func() { + Expect(controllerutil.ContainsFinalizer(obj, uninstallFinalizer)).To(BeTrue()) + }) + }) + }) When("requested CR release is not deployed", func() { var actionConf *action.Configuration BeforeEach(func() { @@ -1221,14 +1272,38 @@ func verifyRelease(ctx context.Context, cl client.Reader, ns string, rel *releas } }) + var objs []client.Object + By("verifying the release resources exist", func() { - objs := manifestToObjects(rel.Manifest) + objs = manifestToObjects(rel.Manifest) for _, obj := range objs { key := client.ObjectKeyFromObject(obj) err := cl.Get(ctx, key, obj) Expect(err).To(BeNil()) } }) + + By("verifying that deployment image was overridden", func() { + for _, obj := range objs { + if obj.GetName() == "test-test-chart" && obj.GetObjectKind().GroupVersionKind().Kind == "Deployment" { + expectDeploymentImagePrefix(obj, "custom-nginx:") + return + } + } + Fail("expected deployment not found") + }) +} + +func expectDeploymentImagePrefix(obj client.Object, prefix string) { + u := obj.(*unstructured.Unstructured) + containers, ok, err := unstructured.NestedSlice(u.Object, "spec", "template", "spec", "containers") + Expect(ok).To(BeTrue()) + Expect(err).To(BeNil()) + container := containers[0].(map[string]interface{}) + val, ok, err := unstructured.NestedString(container, "image") + Expect(ok).To(BeTrue()) + Expect(err).To(BeNil()) + Expect(val).To(HavePrefix(prefix)) } func verifyNoRelease(ctx context.Context, cl client.Client, ns string, name string, rel *release.Release) { diff --git a/pkg/values/values.go b/pkg/values/values.go index 46e19412..16ad3a2a 100644 --- a/pkg/values/values.go +++ b/pkg/values/values.go @@ -17,9 +17,14 @@ limitations under the License. package values import ( + "context" "helm.sh/helm/v3/pkg/chartutil" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) +// Mapper is an interface expected by the reconciler.WithValueMapper option. +// +// Deprecated: use Translator instead. type Mapper interface { Map(chartutil.Values) chartutil.Values } @@ -29,3 +34,20 @@ type MapperFunc func(chartutil.Values) chartutil.Values func (m MapperFunc) Map(v chartutil.Values) chartutil.Values { return m(v) } + +// Translator is an interface expected by the reconciler.WithValueTranslator option. +// +// Translate should return helm values based on the content of the unstructured object +// which is being reconciled. +// +// See also the option documentation. +type Translator interface { + Translate(ctx context.Context, unstructured *unstructured.Unstructured) (chartutil.Values, error) +} + +// TranslatorFunc is a helper type for passing a function as a Translator. +type TranslatorFunc func(context.Context, *unstructured.Unstructured) (chartutil.Values, error) + +func (t TranslatorFunc) Translate(ctx context.Context, u *unstructured.Unstructured) (chartutil.Values, error) { + return t(ctx, u) +}