Always pass in obj with latest generation (#1187)

There were several cases where we weren't passing the correct object to the health module which caused object being determined as healthy prematurely.

pkg/engine/renderer/parser_test.go

@@ -1,6 +1,10 @@
 package renderer
 
-import "testing"
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
 
 func TestParseKubernetesObjects_UnknownType(t *testing.T) {
 	_, err := YamlToObject(`apiVersion: monitoring.coreos.com/v1
@@ -22,3 +26,34 @@ spec:
 		t.Errorf("Expecting no error but got %s", err)
 	}
 }
+
+func TestParseKubernetesObjects_KnownType(t *testing.T) {
+	obj, err := YamlToObject(`apiVersion: apps/v1
+kind: Deployment
+metadata:
+name: nginx
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+     app: nginx
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+      - name: nginx
+        image: nginx:1.7.9
+        ports:
+        - containerPort: 80
+        env:
+        - name: PARAM_ENV
+          value: 1`)
+
+	if err != nil {
+		t.Errorf("Expecting no error but got %s", err)
+	}
+
+	assert.Equal(t, "Deployment", obj[0].GetObjectKind().GroupVersionKind().Kind)
+}

pkg/engine/task/task_apply.go

@@ -69,18 +69,25 @@ func apply(ro []runtime.Object, c client.Client) ([]runtime.Object, error) {
 		switch {
 		case apierrors.IsNotFound(err): // create resource if it doesn't exist
 			err = c.Create(context.TODO(), r)
+			// c.Create always overrides the input, in this case, the object that had previously set GVK loses it (at least for integration tests)
+			// and this was causing problems in health module
+			// with error failed to convert *unstructured.Unstructured to *v1.Deployment: Object 'Kind' is missing in 'unstructured object has no kind'
+			// so re-setting the GVK here to be sure
+			// https://github.com/kubernetes/kubernetes/issues/80609
+			r.GetObjectKind().SetGroupVersionKind(existing.GetObjectKind().GroupVersionKind())
 			if err != nil {
 				return nil, err
 			}
+			applied = append(applied, r)
 		case err != nil: // raise any error other than StatusReasonNotFound
 			return nil, err
 		default: // update existing resource
-			err := patch(r, existing, c)
+			err := patch(r, c)
 			if err != nil {
 				return nil, err
 			}
+			applied = append(applied, r)
 		}
-		applied = append(applied, existing)
 	}
 
 	return applied, nil
@@ -92,7 +99,8 @@ func apply(ro []runtime.Object, c client.Client) ([]runtime.Object, error) {
 // kubernetes native objects might be a problem because we cannot just compare the spec as the spec might have extra fields
 // and those extra fields are set by some kubernetes component
 // because of that for now we just try to apply the patch every time
-func patch(newObj runtime.Object, existingObj runtime.Object, c client.Client) error {
+// it mutates the object passed in to be consistent with the kubernetes client behavior
+func patch(newObj runtime.Object, c client.Client) error {
 	newObjJSON, _ := apijson.Marshal(newObj)
 	key, _ := client.ObjectKeyFromObject(newObj)
 	_, isUnstructured := newObj.(runtime.Unstructured)
@@ -102,14 +110,15 @@ func patch(newObj runtime.Object, existingObj runtime.Object, c client.Client) e
 		// strategic merge patch is not supported for these types, falling back to merge patch
 		err := c.Patch(context.TODO(), newObj, client.ConstantPatch(types.MergePatchType, newObjJSON))
 		if err != nil {
-			return fmt.Errorf("failed to apply merge patch to object %s/%s: %w", key.Name, key.Name, err)
+			return fmt.Errorf("failed to apply merge patch to object %s/%s: %w", key.Namespace, key.Name, err)
 		}
 	} else {
-		err := c.Patch(context.TODO(), existingObj, client.ConstantPatch(types.StrategicMergePatchType, newObjJSON))
+		err := c.Patch(context.TODO(), newObj, client.ConstantPatch(types.StrategicMergePatchType, newObjJSON))
 		if err != nil {
 			return fmt.Errorf("failed to apply StrategicMergePatch to object %s/%s: %w", key.Namespace, key.Name, err)
 		}
 	}
+
 	return nil
 }
 

test/integration/create-operator-in-operator/00-dream.yaml

@@ -25,14 +25,14 @@ spec:
       metadata:
         name: inner-instance
         labels:
-          kudo.dev/operator: test-operator
+          kudo.dev/operator: configmap-operator
       spec:
         operatorVersion:
           kind: OperatorVersion
-          name: test-operator-1.0
+          name: configmap-operator-1.0
           namespace: default
         parameters:
-          REPLICAS: "0"
+          PARAM: "xxx"
   tasks:
     - name: operator
       kind: Apply

test/integration/create-operator-in-operator/01-assert.yaml

@@ -14,9 +14,9 @@ status:
   aggregatedStatus:
     status: COMPLETE
 ---
-apiVersion: apps/v1
-kind: Deployment
+apiVersion: v1
+kind: ConfigMap
 metadata:
-  name: nginx
-spec:
-  replicas: 0
+  name: test-configmap
+data:
+  foo: bar-xxx

test/manifests/configmap-operator.yaml

@@ -0,0 +1,48 @@
+apiVersion: kudo.dev/v1beta1
+kind: Operator
+metadata:
+  name: configmap-operator
+  namespace: default
+spec:
+  maintainers:
+    - name: KUDO
+      email: kudobuilder@gmail.com
+---
+apiVersion: kudo.dev/v1beta1
+kind: OperatorVersion
+metadata:
+  name: configmap-operator-1.0
+  namespace: default
+spec:
+  operator:
+    name: configmap-operator
+    kind: Operator
+  version: "1.0"
+  parameters:
+  - name: PARAM
+    description: "Sample parameter"
+    default: "before"
+  templates:
+    configmap.yaml: |
+      apiVersion: v1
+      kind: ConfigMap
+      metadata:
+        name: test-configmap
+      data:
+        foo: bar-{{ .Params.PARAM }}
+  tasks:
+    - name: deploy
+      kind: Apply
+      spec:
+        resources:
+          - configmap.yaml
+  plans:
+    deploy:
+      strategy: serial
+      phases:
+      - name: deploy
+        strategy: parallel
+        steps:
+        - name: deploy
+          tasks:
+          - deploy