Replace WithValueTranslator implementation with upstream one (#23)

* Revert Translator commits.

Revert "Pass context to Translate(). (#8)"
This reverts commit c3df552.

Revert "Add a WithValueTranslator option to Reconciller. (#6)"
This reverts commit 88508a2.

* Add a WithValueTranslator option to Reconciller (cherry-picked from upstream PR).

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.

Author: porridge

pkg/reconciler/internal/values/values.go

@@ -19,20 +19,35 @@ 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/joelanford/helm-operator/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")
 	}
@@ -44,36 +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 })
-
-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
-	}
-	return internalValues.Map(), err
-})

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/joelanford/helm-operator/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)))
+	})
+})

pkg/reconciler/reconciler.go

@@ -64,7 +64,7 @@ type Reconciler struct {
 	client             client.Client
 	actionClientGetter helmclient.ActionClientGetter
 	valueTranslator    values.Translator
-	valueMapper        values.Mapper
+	valueMapper        values.Mapper // nolint:staticcheck
 	eventRecorder      record.EventRecorder
 	preHooks           []hook.PreHook
 	postHooks          []hook.PostHook
@@ -253,8 +253,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
 		}
 
@@ -453,6 +453,19 @@ func WithPostExtension(e extensions.ReconcileExtension) 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
@@ -464,6 +477,9 @@ func WithValueTranslator(t values.Translator) Option {
 // 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
@@ -665,15 +681,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl.
 }
 
 func (r *Reconciler) getValues(ctx context.Context, obj *unstructured.Unstructured) (chartutil.Values, error) {
-	vals, err := r.valueTranslator.Translate(ctx, obj)
-	if err != nil {
+	if err := internalvalues.ApplyOverrides(r.overrideValues, obj); err != nil {
 		return chartutil.Values{}, err
 	}
-	crVals := internalvalues.New(vals)
-	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

pkg/reconciler/reconciler_test.go

@@ -382,6 +382,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() {
@@ -560,6 +570,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"))
 						})
 
@@ -805,6 +816,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"))
 						})
 
@@ -834,6 +846,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() {
@@ -1231,14 +1282,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) {