address feedback GoogleCloudPlatform#2

controller/common/manage_children.go

@@ -52,7 +52,9 @@ func ApplyUpdate(orig, update *unstructured.Unstructured) (*unstructured.Unstruc
 	if err := revertObjectMetaSystemFields(newObj, orig); err != nil {
 		return nil, fmt.Errorf("failed to revert ObjectMeta system fields: %v", err)
 	}
-	if err := revertStatus(newObj, orig); err != nil {
+	// Revert status because we don't currently support a parent changing status of
+	// its children, so we need to ensure no diffs on the children involve status.
+	if err := revertField(newObj, orig, "status"); err != nil {
 		return nil, fmt.Errorf("failed to revert .status: %v", err)
 	}
 	dynamicapply.SetLastApplied(newObj, update.UnstructuredContent())
@@ -77,43 +79,31 @@ var objectMetaSystemFields = []string{
 // If the field was unset before, we delete it if necessary.
 func revertObjectMetaSystemFields(newObj, orig *unstructured.Unstructured) error {
 	for _, fieldName := range objectMetaSystemFields {
-		val, found, err := unstructured.NestedFieldNoCopy(orig.UnstructuredContent(), "metadata", fieldName)
-		if err != nil {
-			return fmt.Errorf("can't traverse UnstructuredContent to look for metadata.%s: %v", fieldName, err)
-		}
-		if found {
-			// The original had this field set, so make sure it remains the same.
-			// SetNestedField will recursively ensure the field and all its parent
-			// fields exist, and then set the value.
-			if err := unstructured.SetNestedField(newObj.UnstructuredContent(), val, "metadata", fieldName); err != nil {
-				return fmt.Errorf("can't revert value of metadata.%s: %v", fieldName, err)
-			}
-		} else {
-			// The original had this field unset, so make sure it remains unset.
-			// RemoveNestedField is a no-op if the field or any of its parents
-			// don't exist.
-			unstructured.RemoveNestedField(newObj.UnstructuredContent(), "metadata", fieldName)
+		if err := revertField(newObj, orig, "metadata", fieldName); err != nil {
+			return err
 		}
 	}
 	return nil
 }
 
-// revertStatus overwrites the .status (i.e. all status fields) in newObj to match
-// what it was in orig by resetting .status directly without peeking into each field.
-func revertStatus(newObj, orig *unstructured.Unstructured) error {
-	status, found, err := unstructured.NestedFieldNoCopy(orig.UnstructuredContent(), "status")
+// revertField reverts field in newObj to match what it was in orig.
+func revertField(newObj, orig *unstructured.Unstructured, fieldPath ...string) error {
+	field, found, err := unstructured.NestedFieldNoCopy(orig.UnstructuredContent(), fieldPath...)
 	if err != nil {
-		return fmt.Errorf("can't traverse UnstructuredContent to look for status: %v", err)
+		return fmt.Errorf("can't traverse UnstructuredContent to look for field: %v", err)
 	}
 	if found {
-		// The original had the .status set, so make sure it remains the same.
-		if err := unstructured.SetNestedField(newObj.UnstructuredContent(), status, "status"); err != nil {
-			return fmt.Errorf("can't revert status: %v", err)
+		// The original had this field set, so make sure it remains the same.
+		// SetNestedField will recursively ensure the field and all its parent
+		// fields exist, and then set the value.
+		if err := unstructured.SetNestedField(newObj.UnstructuredContent(), field, fieldPath...); err != nil {
+			return fmt.Errorf("can't revert field: %v", err)
 		}
 	} else {
 		// The original had this field unset, so make sure it remains unset.
-		// RemoveNestedField is a no-op if the status already doesn't exist.
-		unstructured.RemoveNestedField(newObj.UnstructuredContent(), "status")
+		// RemoveNestedField is a no-op if the field or any of its parents
+		// don't exist.
+		unstructured.RemoveNestedField(newObj.UnstructuredContent(), fieldPath...)
 	}
 	return nil
 }

controller/composite/controller.go

@@ -244,9 +244,9 @@ func (pc *parentController) updateParentObject(old, cur interface{}) {
 	oldParent := old.(*unstructured.Unstructured)
 	curParent := cur.(*unstructured.Unstructured)
 	if curParent.GetResourceVersion() != oldParent.GetResourceVersion() {
-		oldGeneration := k8s.GetNestedField(oldParent.UnstructuredContent(), "metadata", "generation")
-		curGeneration := k8s.GetNestedField(curParent.UnstructuredContent(), "metadata", "generation")
-		if oldGeneration == curGeneration {
+		oldSpec := k8s.GetNestedField(oldParent.UnstructuredContent(), "spec")
+		curSpec := k8s.GetNestedField(curParent.UnstructuredContent(), "spec")
+		if reflect.DeepEqual(oldSpec, curSpec) {
 			return
 		}
 	}
@@ -561,7 +561,7 @@ func (pc *parentController) claimChildren(parent *unstructured.Unstructured) (co
 func (pc *parentController) updateParentStatus(parent *unstructured.Unstructured, status map[string]interface{}) (*unstructured.Unstructured, error) {
 	// Overwrite .status field of parent object without touching other parts.
 	// We can't use Patch() because we need to ensure that the UID matches.
-	return pc.parentClient.Namespace(parent.GetNamespace()).AtomicStatusUpdate(pc.resources, pc.parentResource, parent, func(obj *unstructured.Unstructured) bool {
+	return pc.parentClient.Namespace(parent.GetNamespace()).AtomicStatusUpdate(parent, func(obj *unstructured.Unstructured) bool {
 		oldStatus := k8s.GetNestedField(obj.UnstructuredContent(), "status")
 		if reflect.DeepEqual(oldStatus, status) {
 			// Nothing to do.

dynamic/clientset/clientset.go

@@ -177,9 +177,11 @@ func (rc *ResourceClient) RemoveFinalizer(orig *unstructured.Unstructured, name
 }
 
 // AtomicStatusUpdate is similar to AtomicUpdate, except that it updates status.
-func (rc *ResourceClient) AtomicStatusUpdate(resourceMap *dynamicdiscovery.ResourceMap, parentResource *dynamicdiscovery.APIResource, orig *unstructured.Unstructured, update func(obj *unstructured.Unstructured) bool) (result *unstructured.Unstructured, err error) {
+func (rc *ResourceClient) AtomicStatusUpdate(orig *unstructured.Unstructured, update func(obj *unstructured.Unstructured) bool) (result *unstructured.Unstructured, err error) {
 	name := orig.GetName()
 
+	// We should call GetStatus (if it HasSubresource) to respect subresource
+	// RBAC rules, but the dynamic client does not support this yet.
 	err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		current, err := rc.Get(name, metav1.GetOptions{})
 		if err != nil {
@@ -195,7 +197,7 @@ func (rc *ResourceClient) AtomicStatusUpdate(resourceMap *dynamicdiscovery.Resou
 			return nil
 		}
 
-		if resourceMap.HasSubresource(parentResource, "status") {
+		if rc.HasSubresource("status") {
 			result, err = rc.UpdateStatus(current)
 		} else {
 			result, err = rc.Update(current)