Filter unknown custom resources in Delete- and ToggleTask (#1552)

Filter the rendered templates - If one of them is of an unknown type, we shouldn't try to delete it as it can't be there anyway.

This PR splits the convert functionality from the enhancer and skips enhancing for the DeleteTask, then filters the resources for unknown types

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

pkg/engine/renderer/enhancer.go

@@ -19,7 +19,7 @@ import (
 // and annotations
 // it also takes care of setting an owner of all the resources to the provided object
 type Enhancer interface {
-	Apply(templates map[string]string, metadata Metadata) ([]runtime.Object, error)
+	Apply(objs []runtime.Object, metadata Metadata) ([]runtime.Object, error)
 }
 
 // DefaultEnhancer is implementation of Enhancer that applies the defined conventions by directly editing runtime.Objects (Unstructured).
@@ -31,47 +31,41 @@ type DefaultEnhancer struct {
 
 // Apply accepts templates to be rendered in kubernetes and enhances them with our own KUDO conventions
 // These include the way we name our objects and what labels we apply to them
-func (de *DefaultEnhancer) Apply(templates map[string]string, metadata Metadata) (objsToAdd []runtime.Object, err error) {
-	unstructuredObjs := make([]*unstructured.Unstructured, 0, len(templates))
+func (de *DefaultEnhancer) Apply(sourceObjs []runtime.Object, metadata Metadata) ([]runtime.Object, error) {
+	unstructuredObjs := make([]*unstructured.Unstructured, 0, len(sourceObjs))
 
-	for name, v := range templates {
-		parsed, err := YamlToObject(v)
+	for _, obj := range sourceObjs {
+		unstructMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
 		if err != nil {
-			return nil, fmt.Errorf("%wparsing YAML from %s: %v", engine.ErrFatalExecution, name, err)
+			return nil, fmt.Errorf("%wconverting to unstructured failed: %v", engine.ErrFatalExecution, err)
 		}
-		for _, obj := range parsed {
-			unstructMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
-			if err != nil {
-				return nil, fmt.Errorf("%wconverting to unstructured failed: %v", engine.ErrFatalExecution, err)
-			}
 
-			objUnstructured := &unstructured.Unstructured{Object: unstructMap}
+		objUnstructured := &unstructured.Unstructured{Object: unstructMap}
 
-			if err = addLabels(objUnstructured, metadata); err != nil {
-				return nil, fmt.Errorf("%wadding labels on parsed object: %v", engine.ErrFatalExecution, err)
-			}
-			if err = addAnnotations(objUnstructured, metadata); err != nil {
-				return nil, fmt.Errorf("%wadding annotations on parsed object %s: %v", engine.ErrFatalExecution, obj.GetObjectKind(), err)
-			}
+		if err = addLabels(objUnstructured, metadata); err != nil {
+			return nil, fmt.Errorf("%wadding labels on parsed object: %v", engine.ErrFatalExecution, err)
+		}
+		if err = addAnnotations(objUnstructured, metadata); err != nil {
+			return nil, fmt.Errorf("%wadding annotations on parsed object %s: %v", engine.ErrFatalExecution, obj.GetObjectKind(), err)
+		}
 
-			isNamespaced, err := resource.IsNamespacedObject(obj, de.Discovery)
-			if err != nil {
-				return nil, fmt.Errorf("failed to determine if object %s is namespaced: %v", obj.GetObjectKind(), err)
-			}
+		isNamespaced, err := resource.IsNamespacedObject(obj, de.Discovery)
+		if err != nil {
+			return nil, fmt.Errorf("failed to determine if object %s is namespaced: %v", obj.GetObjectKind(), err)
+		}
 
-			// Note: Cross-namespace owner references are disallowed by design. This means:
-			// 1) Namespace-scoped dependents can only specify owners in the same namespace, and owners that are cluster-scoped.
-			// 2) Cluster-scoped dependents can only specify cluster-scoped owners, but not namespace-scoped owners.
-			// More: https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/
-			if isNamespaced {
-				objUnstructured.SetNamespace(metadata.InstanceNamespace)
-				if err := controllerutil.SetControllerReference(metadata.ResourcesOwner, objUnstructured, de.Scheme); err != nil {
-					return nil, fmt.Errorf("%wsetting controller reference on parsed object %s: %v", engine.ErrFatalExecution, obj.GetObjectKind(), err)
-				}
+		// Note: Cross-namespace owner references are disallowed by design. This means:
+		// 1) Namespace-scoped dependents can only specify owners in the same namespace, and owners that are cluster-scoped.
+		// 2) Cluster-scoped dependents can only specify cluster-scoped owners, but not namespace-scoped owners.
+		// More: https://kubernetes.io/docs/concepts/workloads/controllers/garbage-collection/
+		if isNamespaced {
+			objUnstructured.SetNamespace(metadata.InstanceNamespace)
+			if err := controllerutil.SetControllerReference(metadata.ResourcesOwner, objUnstructured, de.Scheme); err != nil {
+				return nil, fmt.Errorf("%wsetting controller reference on parsed object %s: %v", engine.ErrFatalExecution, obj.GetObjectKind(), err)
 			}
-
-			unstructuredObjs = append(unstructuredObjs, objUnstructured)
 		}
+
+		unstructuredObjs = append(unstructuredObjs, objUnstructured)
 	}
 
 	if err := de.addDependenciesHashes(unstructuredObjs); err != nil {

pkg/engine/renderer/enhancer_test.go

@@ -16,7 +16,6 @@ import (
 	"k8s.io/apimachinery/pkg/util/uuid"
 	"k8s.io/kubectl/pkg/scheme"
 	clientfake "sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"sigs.k8s.io/yaml"
 
 	"github.com/kudobuilder/kuttl/pkg/test/utils"
 
@@ -27,8 +26,8 @@ import (
 
 func TestEnhancerApply_embeddedMetadataStatefulSet(t *testing.T) {
 
-	tpls := map[string]string{
-		"deployment": resourceAsString(statefulSet("sfs1", "default")),
+	tpls := []runtime.Object{
+		statefulSet("sfs1", "default"),
 	}
 
 	meta := metadata()
@@ -72,8 +71,8 @@ func TestEnhancerApply_embeddedMetadataStatefulSet(t *testing.T) {
 
 func TestEnhancerApply_embeddedMetadataCronjob(t *testing.T) {
 
-	tpls := map[string]string{
-		"cron": resourceAsString(cronjob("cronjob", "default")),
+	tpls := []runtime.Object{
+		cronjob("cronjob", "default"),
 	}
 
 	meta := metadata()
@@ -113,9 +112,9 @@ func TestEnhancerApply_embeddedMetadataCronjob(t *testing.T) {
 
 func TestEnhancerApply_noAdditionalMetadata(t *testing.T) {
 
-	tpls := map[string]string{
-		"pod": resourceAsString(pod("pod", "default")),
-		"crd": resourceAsString(unstructuredCrd("crd", "default")),
+	tpls := []runtime.Object{
+		pod("pod", "default"),
+		unstructuredCrd("crd", "default"),
 	}
 
 	meta := metadata()
@@ -157,9 +156,7 @@ func TestEnhancerApply_noAdditionalMetadata(t *testing.T) {
 func TestEnhancerApply_dependencyHash_noDependencies(t *testing.T) {
 	ss := statefulSet("statefulset", "default")
 
-	tpls := map[string]string{
-		"statefulset": resourceAsString(ss),
-	}
+	tpls := []runtime.Object{ss}
 
 	meta := metadata()
 	meta.PlanUID = uuid.NewUUID()
@@ -202,9 +199,7 @@ func TestEnhancerApply_dependencyHash_unavailableResource(t *testing.T) {
 		},
 	})
 
-	tpls := map[string]string{
-		"statefulset": resourceAsString(ss),
-	}
+	tpls := []runtime.Object{ss}
 
 	meta := metadata()
 	meta.PlanUID = uuid.NewUUID()
@@ -250,9 +245,7 @@ func TestEnhancerApply_dependencyHash_calculatedOnResourceWithoutLastAppliedConf
 		},
 	})
 
-	tpls := map[string]string{
-		"statefulset": resourceAsString(ss),
-	}
+	tpls := []runtime.Object{ss}
 
 	meta := metadata()
 	meta.PlanUID = uuid.NewUUID()
@@ -295,10 +288,7 @@ func TestEnhancerApply_dependencyHash_changes(t *testing.T) {
 		},
 	})
 
-	tpls := map[string]string{
-		"statefulset": resourceAsString(ss),
-		"configmap":   resourceAsString(cm),
-	}
+	tpls := []runtime.Object{ss, cm}
 
 	meta := metadata()
 	meta.PlanUID = uuid.NewUUID()
@@ -327,10 +317,7 @@ func TestEnhancerApply_dependencyHash_changes(t *testing.T) {
 	assert.NotNil(t, hash, "Pod template spec annotations contains no dependency hash field")
 
 	cm.Data["newkey"] = "newvalue"
-	tpls = map[string]string{
-		"statefulset": resourceAsString(ss),
-		"configmap":   resourceAsString(cm),
-	}
+	tpls = []runtime.Object{ss, cm}
 
 	objs, err = e.Apply(tpls, meta)
 	assert.Nil(t, err)
@@ -380,11 +367,6 @@ func owner() *corev1.Pod {
 	}
 }
 
-func resourceAsString(resource runtime.Object) string {
-	bytes, _ := yaml.Marshal(resource)
-	return string(bytes)
-}
-
 func configMap(name string, namespace string) *corev1.ConfigMap {
 	configMap := &corev1.ConfigMap{
 		TypeMeta: metav1.TypeMeta{

pkg/engine/resource/object_key.go

@@ -38,33 +38,59 @@ func IsNamespacedObject(r runtime.Object, di discovery.CachedDiscoveryInterface)
 	return isNamespaced(gvk, di)
 }
 
+func IsKnownObjectType(r runtime.Object, di discovery.CachedDiscoveryInterface) (bool, error) {
+	gvk := r.GetObjectKind().GroupVersionKind()
+	return isKnownType(gvk, di)
+}
+
 // isNamespaced method return true if given runtime.Object is a namespaced (not cluster-scoped) resource. It uses the
 // discovery client to fetch all API resources (with Groups and Versions), searches for a resource with the passed GVK
 // and returns true if it's namespaced. Method returns an error if passed GVK wasn't found in the discovered resource list.
 func isNamespaced(gvk schema.GroupVersionKind, di discovery.CachedDiscoveryInterface) (bool, error) {
-	// Fetch namespaced API resources
+	apiResource, err := getUncachedAPIResource(gvk, di)
+	if err != nil {
+		return false, err
+	}
+	if apiResource != nil {
+		return apiResource.Namespaced, nil
+	}
+	return false, fmt.Errorf("a resource with GVK %v seems to be missing in API resource list", gvk)
+}
+
+func isKnownType(gvk schema.GroupVersionKind, di discovery.CachedDiscoveryInterface) (bool, error) {
+	apiResource, err := getUncachedAPIResource(gvk, di)
+	if err != nil {
+		return false, err
+	}
+	if apiResource != nil {
+		return true, nil
+	}
+	return false, nil
+}
 
+// getUncachedAPIResource tries to invalidate the cache and requery the discovery interface to make sure no stale data is returned
+func getUncachedAPIResource(gvk schema.GroupVersionKind, di discovery.CachedDiscoveryInterface) (*metav1.APIResource, error) {
 	// First try, this may return nil because of the cache
 	apiResource, err := getAPIResource(gvk, di)
 	if err != nil {
-		return false, err
+		return nil, err
 	}
 	if apiResource != nil {
-		return apiResource.Namespaced, nil
+		return apiResource, nil
 	}
 
 	// Second try, now with invalidated cache. If we still get nil, we know it's not there.
 	log.Printf("Failed to get APIResource for %v, retry with invalidated cache.", gvk)
 	di.Invalidate()
 	apiResource, err = getAPIResource(gvk, di)
 	if err != nil {
-		return false, err
+		return nil, err
 	}
 	if apiResource != nil {
-		return apiResource.Namespaced, nil
+		return apiResource, nil
 	}
 
-	return false, fmt.Errorf("a resource with GVK %v seems to be missing in API resource list", gvk)
+	return nil, nil
 }
 
 // getAPIResource returns a specific APIResource from the DiscoveryInterface or nil if no resource was found.

pkg/engine/task/render.go

@@ -38,10 +38,25 @@ func render(resourceNames []string, ctx Context) (map[string]string, error) {
 	return resources, nil
 }
 
-// enhance method takes a slice of rendered templates, applies conventions using Enhancer and
-// returns a slice of k8s objects.
-func enhance(rendered map[string]string, meta renderer.Metadata, enhancer renderer.Enhancer) ([]runtime.Object, error) {
-	enhanced, err := enhancer.Apply(rendered, meta)
+// convert takes a map of rendered yaml templates and converts them to k8s objects
+func convert(rendered map[string]string) ([]runtime.Object, error) {
+	objs := make([]runtime.Object, 0, len(rendered))
+
+	for name, v := range rendered {
+		parsed, err := renderer.YamlToObject(v)
+		if err != nil {
+			return nil, fmt.Errorf("%wparsing YAML from %s: %v", engine.ErrFatalExecution, name, err)
+		}
+		objs = append(objs, parsed...)
+	}
+
+	return objs, nil
+}
+
+// enhance method takes a slice of rendered k8s objects, applies conventions using Enhancer and
+// returns a slice of enhanced k8s objects.
+func enhance(objs []runtime.Object, meta renderer.Metadata, enhancer renderer.Enhancer) ([]runtime.Object, error) {
+	enhanced, err := enhancer.Apply(objs, meta)
 
 	switch {
 	case errors.Is(err, engine.ErrFatalExecution):

pkg/engine/task/task_apply.go

@@ -45,8 +45,14 @@ func (at ApplyTask) Run(ctx Context) (bool, error) {
 		return false, fatalExecutionError(err, taskRenderingError, ctx.Meta)
 	}
 
+	// 2. - Convert to objects
+	objs, err := convert(rendered)
+	if err != nil {
+		return false, fatalExecutionError(err, taskRenderingError, ctx.Meta)
+	}
+
 	// 2. - Enhance them with metadata -
-	enhanced, err := enhance(rendered, ctx.Meta, ctx.Enhancer)
+	enhanced, err := enhance(objs, ctx.Meta, ctx.Enhancer)
 	if err != nil {
 		return false, err
 	}

pkg/engine/task/task_apply_test.go

@@ -198,26 +198,18 @@ func resourceAsString(resource metav1.Object) string {
 
 type testEnhancer struct{}
 
-func (k *testEnhancer) Apply(templates map[string]string, metadata renderer.Metadata) ([]runtime.Object, error) {
-	result := make([]runtime.Object, 0)
-	for _, t := range templates {
-		objsToAdd, err := renderer.YamlToObject(t)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing kubernetes objects after applying enhance: %w", err)
-		}
-		result = append(result, objsToAdd[0])
-	}
-	return result, nil
+func (k *testEnhancer) Apply(objs []runtime.Object, metadata renderer.Metadata) ([]runtime.Object, error) {
+	return objs, nil
 }
 
 type fatalErrorEnhancer struct{}
 
-func (k *fatalErrorEnhancer) Apply(templates map[string]string, metadata renderer.Metadata) ([]runtime.Object, error) {
+func (k *fatalErrorEnhancer) Apply(objs []runtime.Object, metadata renderer.Metadata) ([]runtime.Object, error) {
 	return nil, fmt.Errorf("%wsomething fatally bad happens every time", engine.ErrFatalExecution)
 }
 
 type transientErrorEnhancer struct{}
 
-func (k *transientErrorEnhancer) Apply(templates map[string]string, metadata renderer.Metadata) ([]runtime.Object, error) {
+func (k *transientErrorEnhancer) Apply(objs []runtime.Object, metadata renderer.Metadata) ([]runtime.Object, error) {
 	return nil, fmt.Errorf("something transiently bad happens every time")
 }