pkg/helm: only inject ownerrefs into namespace-scoped resources

CHANGELOG.md

@@ -10,6 +10,7 @@
 
 ### Bug Fixes
 - Configure the repo path correctly in `operator-sdk add crd` and prevent the command from running outside of an operator project. ([#1660](https://github.com/operator-framework/operator-sdk/pull/1660))
+- In the Helm operator, skip owner reference injection for cluster-scoped resources in release manifests. The Helm operator only supports namespace-scoped CRs, and namespaced resources cannot own cluster-scoped resources. ([#1817](https://github.com/operator-framework/operator-sdk/pull/1817))
 
 ## v0.10.0
 

pkg/helm/engine/ownerref.go

@@ -20,6 +20,7 @@ import (
 	"sort"
 	"strings"
 
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -32,7 +33,9 @@ import (
 // OwnerRefEngine wraps a tiller Render engine, adding ownerrefs to rendered assets
 type OwnerRefEngine struct {
 	environment.Engine
-	refs []metav1.OwnerReference
+
+	restMapper meta.RESTMapper
+	refs       []metav1.OwnerReference
 }
 
 // assert interface
@@ -89,9 +92,28 @@ func (o *OwnerRefEngine) addOwnerRefs(fileContents string) (string, error) {
 		}
 
 		unstructured := &unstructured.Unstructured{Object: unst}
-		unstructured.SetOwnerReferences(o.refs)
 
-		// Write the document with owner ref to the buffer
+		// Get the REST mapping for the GVK to determine its scope.
+		gvk := unstructured.GroupVersionKind()
+		mapping, err := o.restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+		if err != nil {
+			return "", err
+		}
+
+		// If the GVK is namespace-scoped, add owner references to this
+		// resource. We don't do this for cluster-scoped manifest resources
+		// since a namespace-scoped resource (the CR) cannot own a
+		// cluster-scoped resource.
+		//
+		// NOTE: the helm-operator that uses this engine ALSO uses a finalizer
+		// that ensures that the release is uninstalled via the built-in
+		// tiller, so manual cleanup of cluster-scoped dependent resources
+		// should not be necessary.
+		if mapping.Scope == meta.RESTScopeNamespace {
+			unstructured.SetOwnerReferences(o.refs)
+		}
+
+		// Write the document with to the buffer
 		// Also add document start marker
 		_, err = outBuf.WriteString(documentSeparator + chartutil.ToYaml(unstructured.Object))
 		if err != nil {
@@ -103,10 +125,12 @@ func (o *OwnerRefEngine) addOwnerRefs(fileContents string) (string, error) {
 }
 
 // NewOwnerRefEngine creates a new OwnerRef engine with a set of metav1.OwnerReferences to be added to assets
-func NewOwnerRefEngine(baseEngine environment.Engine, refs []metav1.OwnerReference) environment.Engine {
+func NewOwnerRefEngine(baseEngine environment.Engine, restMapper meta.RESTMapper, refs []metav1.OwnerReference) environment.Engine {
 	return &OwnerRefEngine{
 		Engine: baseEngine,
-		refs:   refs,
+
+		restMapper: restMapper,
+		refs:       refs,
 	}
 }
 

pkg/helm/engine/ownerref_test.go

@@ -15,10 +15,14 @@
 package engine
 
 import (
+	"fmt"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/helm/pkg/chartutil"
 	"k8s.io/helm/pkg/proto/hapi/chart"
 )
@@ -31,53 +35,36 @@ func (e *mockEngine) Render(chrt *chart.Chart, v chartutil.Values) (map[string]s
 	return e.out, nil
 }
 
-func TestOwnerRefEngine(t *testing.T) {
-	ownerRefs := []metav1.OwnerReference{
-		{
-			APIVersion: "v1",
-			Kind:       "Test",
-			Name:       "test",
-			UID:        "123",
-		},
-	}
-
-	baseOut := `apiVersion: stable.nicolerenee.io/v1
-kind: Character
-metadata:
-  name: nemo
-spec:
-  Name: Nemo
-`
+type testCase struct {
+	input    map[string]string
+	expected map[string]string
+}
 
-	expectedOut := `---
-apiVersion: stable.nicolerenee.io/v1
-kind: Character
-metadata:
-  name: nemo
-  ownerReferences:
-  - apiVersion: v1
-    kind: Test
-    name: test
-    uid: "123"
-spec:
-  Name: Nemo
-`
-	expected := map[string]string{"template.yaml": expectedOut, "template2.yaml": expectedOut}
+type restMapping struct {
+	gvk   schema.GroupVersionKind
+	scope meta.RESTScope
+}
 
-	baseEngineOutput := map[string]string{
-		"template.yaml":  baseOut,
-		"template2.yaml": baseOut,
-		"empty.yaml":     "",
-		"comment.yaml":   "# This is empty",
+func genTemplate(resourceCount int, withLeadingSep bool, gvk schema.GroupVersionKind, ownerRefs []metav1.OwnerReference) string {
+	sb := &strings.Builder{}
+
+	for i := 0; i < resourceCount; i++ {
+		if withLeadingSep || i > 0 {
+			sb.WriteString("---\n")
+		}
+		sb.WriteString(fmt.Sprintf("apiVersion: %s/%s\nkind: %s\nmetadata:\n  name: example-%s-%d\n", gvk.Group, gvk.Version, gvk.Kind, strings.ToLower(gvk.Kind), i))
+		if len(ownerRefs) > 0 {
+			sb.WriteString("  ownerReferences:\n")
+		}
+		for _, or := range ownerRefs {
+			sb.WriteString(fmt.Sprintf("  - apiVersion: %s\n    kind: %s\n    name: %s\n    uid: %q\n", or.APIVersion, or.Kind, or.Name, or.UID))
+		}
+		sb.WriteString(fmt.Sprintf("spec:\n  value: value-%d\n", i))
 	}
-
-	engine := NewOwnerRefEngine(&mockEngine{out: baseEngineOutput}, ownerRefs)
-	out, err := engine.Render(&chart.Chart{}, map[string]interface{}{})
-	require.NoError(t, err)
-	require.EqualValues(t, expected, out)
+	return sb.String()
 }
 
-func TestOwnerRefEngine_MultiDocumentYaml(t *testing.T) {
+func TestOwnerRefEngine(t *testing.T) {
 	ownerRefs := []metav1.OwnerReference{
 		{
 			APIVersion: "v1",
@@ -87,52 +74,69 @@ func TestOwnerRefEngine_MultiDocumentYaml(t *testing.T) {
 		},
 	}
 
-	baseOut := `kind: ConfigMap
-apiVersion: v1
-metadata:
-  name: eighth
-  data:
-    name: value
----
-apiVersion: v1
-kind: Pod
-metadata:
-  name: example-test
-`
-
-	expectedOut := `---
-apiVersion: v1
-kind: ConfigMap
-metadata:
-  data:
-    name: value
-  name: eighth
-  ownerReferences:
-  - apiVersion: v1
-    kind: Test
-    name: test
-    uid: "123"
----
-apiVersion: v1
-kind: Pod
-metadata:
-  name: example-test
-  ownerReferences:
-  - apiVersion: v1
-    kind: Test
-    name: test
-    uid: "123"
-`
-
-	expected := map[string]string{"template.yaml": expectedOut}
-
-	baseEngineOutput := map[string]string{
-		"template.yaml": baseOut,
+	ns := restMapping{
+		gvk: schema.GroupVersionKind{
+			Group:   "app.example.com",
+			Version: "v1",
+			Kind:    "App",
+		},
+		scope: meta.RESTScopeNamespace,
 	}
+	cs := restMapping{
+		gvk: schema.GroupVersionKind{
+			Group:   "app.example.com",
+			Version: "v1",
+			Kind:    "ClusterApp",
+		},
+		scope: meta.RESTScopeRoot,
+	}
+	restMapper := meta.NewDefaultRESTMapper(nil)
+	restMapper.Add(ns.gvk, ns.scope)
+	restMapper.Add(cs.gvk, cs.scope)
 
-	engine := NewOwnerRefEngine(&mockEngine{out: baseEngineOutput}, ownerRefs)
-	out, err := engine.Render(&chart.Chart{}, map[string]interface{}{})
+	testCases := []testCase{
+		{
+			input: map[string]string{
+				"template1.yaml": genTemplate(1, false, ns.gvk, nil),
+				"template2.yaml": genTemplate(1, false, cs.gvk, nil),
+				"template3.yaml": genTemplate(2, true, ns.gvk, nil),
+				"template4.yaml": genTemplate(2, true, cs.gvk, nil),
+				"template5.yaml": fmt.Sprintf("%s%s",
+					genTemplate(1, true, ns.gvk, nil),
+					genTemplate(1, true, cs.gvk, nil),
+				),
+				"template6.yaml": fmt.Sprintf("%s%s",
+					genTemplate(1, true, cs.gvk, nil),
+					genTemplate(1, true, ns.gvk, nil),
+				),
+				"empty.yaml":   "",
+				"comment.yaml": "# This is empty",
+			},
+			expected: map[string]string{
+				"template1.yaml": genTemplate(1, true, ns.gvk, ownerRefs),
+				"template2.yaml": genTemplate(1, true, cs.gvk, nil),
+				"template3.yaml": genTemplate(2, true, ns.gvk, ownerRefs),
+				"template4.yaml": genTemplate(2, true, cs.gvk, nil),
+				"template5.yaml": fmt.Sprintf("%s%s",
+					genTemplate(1, true, ns.gvk, ownerRefs),
+					genTemplate(1, true, cs.gvk, nil),
+				),
+				"template6.yaml": fmt.Sprintf("%s%s",
+					genTemplate(1, true, cs.gvk, nil),
+					genTemplate(1, true, ns.gvk, ownerRefs),
+				),
+			},
+		},
+	}
 
-	require.NoError(t, err)
-	require.Equal(t, expected, out)
+	for _, tc := range testCases {
+		engine := NewOwnerRefEngine(&mockEngine{out: tc.input}, restMapper, ownerRefs)
+		out, err := engine.Render(&chart.Chart{}, map[string]interface{}{})
+		require.NoError(t, err)
+		for expectedKey, expectedValue := range tc.expected {
+			actualValue, actualKeyExists := out[expectedKey]
+			require.True(t, actualKeyExists, "Did not find expected template %q in output", expectedKey)
+			require.EqualValues(t, expectedValue, actualValue)
+		}
+	}
 }

pkg/helm/release/manager_factory.go

@@ -24,7 +24,7 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	apitypes "k8s.io/apimachinery/pkg/types"
 	clientset "k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/kubernetes/typed/core/v1"
+	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
 	helmengine "k8s.io/helm/pkg/engine"
 	"k8s.io/helm/pkg/kube"
 	"k8s.io/helm/pkg/storage"
@@ -92,7 +92,11 @@ func getReleaseServer(cr *unstructured.Unstructured, storageBackend *storage.Sto
 		*controllerRef,
 	}
 	baseEngine := helmengine.New()
-	e := engine.NewOwnerRefEngine(baseEngine, ownerRefs)
+	restMapper, err := tillerKubeClient.Factory.ToRESTMapper()
+	if err != nil {
+		return nil, err
+	}
+	e := engine.NewOwnerRefEngine(baseEngine, restMapper, ownerRefs)
 	var ey environment.EngineYard = map[string]environment.Engine{
 		environment.GoTplEngine: e,
 	}