add integration test for clean un-install use case (#24)

This commit has fixes and enhancements that were required to
implement clean un-install use case. This use case talks about
deletion of CRDs and custom resources belonging to a particular
workload when that workload's namespace is deleted. This use case
takes care of cases where these custom resources might have
finalizers set against them.

While this works, there is certainly more amount of work that needs
to get into Metac.

Among other things Metac's GenericController needs to be improved
further to default to 2-way merge or 3-way merge (kubectl like apply)
depending on scenarios. In this particular case, 2-way merge seemed
to be a good choice over 3-way merge. Since, Metac does not support
2-way merge, this example had to make use of sync as well as finalize
hook to achieve the end goal. Ideal implementation would have been to
just make use of finalize hook to solve this requirement.

Signed-off-by: AmitKumarDas <[email protected]>

Makefile

@@ -63,4 +63,4 @@ integration-dependencies:
 # This can be run on one's laptop or Travis like CI environments.
 .PHONY: integration-test
 integration-test: generated_files integration-dependencies
-	@PATH="$(PWD)/hack/bin:$(PATH)" go test ./test/integration/... -v -timeout 5m -args --logtostderr -v=4
+	@PATH="$(PWD)/hack/bin:$(PATH)" go test ./test/integration/... -v -timeout 5m -args --logtostderr -v=1

TODO

@@ -1,11 +1,17 @@
 ### TODO:
+- test metac with changes to rbac
+
 - gctl add integration tests:
     - check if finalizer works
     - check if attachments not created by gctl are not updated
     - check if attachments not created by gctl are not deleted
     - check if ReadOnly works
     - check if UpdateAny works
+    - check if DeleteAny works
     - check if same watch can update an attachment via multiple gctl specs
+    - check for 2-way merge as well as 3-way merge
+        - default to 2-way merge if only finalize hook is present
+        - default to 3-way merge if both sync & finalize hooks are present
 
 - gctl - read & review
     - https://github.com/GoogleCloudPlatform/metacontroller/issues/98

apis/metacontroller/v1alpha1/types_generic.go

@@ -77,7 +77,10 @@ type GenericControllerSpec struct {
 	//
 	// NOTE:
 	//	This is optional. However this should not be set to true if
-	// UpdateAny is set to true.
+	// UpdateAny or DeleteAny is set to true.
+	//
+	// NOTE:
+	// 	ReadOnly overrides UpdateAny and DeleteAny tunables
 	ReadOnly *bool `json:"readOnly,omitempty"`
 
 	// UpdateAny enables this controller to execute update operations
@@ -94,6 +97,20 @@ type GenericControllerSpec struct {
 	// ReadOnly is set to true.
 	UpdateAny *bool `json:"updateAny,omitempty"`
 
+	// DeleteAny enables this controller to execute delete operations
+	// against any attachments.
+	//
+	// NOTE:
+	//	This tunable changes the default working mode of GenericController.
+	// When set to true, the controller instance is granted with the
+	// permission to delete any attachments even if these attachments
+	// were not created by this controller instance.
+	//
+	// NOTE:
+	//	This is optional. However this should not be set to true if
+	// ReadOnly is set to true.
+	DeleteAny *bool `json:"deleteAny,omitempty"`
+
 	// Parameters represent a set of key value pairs that can be used by
 	// the sync hook implementation logic.
 	//

controller/common/common.go

@@ -44,9 +44,41 @@ var (
 // instances in a way that is easy to find / filter later.
 type AnyUnstructRegistry map[string]map[string]*unstructured.Unstructured
 
+// String implements Stringer interface
+func (m AnyUnstructRegistry) String() string {
+	var message []string
+	title := "Resource Instances:-"
+	for vk, list := range m {
+		for nsname, obj := range list {
+			message = append(message, fmt.Sprintf("\t%s:%s %s", vk, nsname, obj.GetUID()))
+		}
+	}
+	return fmt.Sprintf("%s\n%s\n", title, strings.Join(message, "\n"))
+}
+
 // IsEmpty returns true if this registry is empty
 func (m AnyUnstructRegistry) IsEmpty() bool {
-	return len(m) == 0
+	for _, list := range m {
+		for _, obj := range list {
+			if obj != nil {
+				return false
+			}
+		}
+	}
+	return true
+}
+
+// Len returns count of not nil items in this registry
+func (m AnyUnstructRegistry) Len() int {
+	var count int
+	for _, list := range m {
+		for _, obj := range list {
+			if obj != nil {
+				count++
+			}
+		}
+	}
+	return count
 }
 
 // InitGroupByVK initialises (or re-initializes) a group within the

controller/common/hooks.go

@@ -29,7 +29,7 @@ import (
 // caller logic based on the provided schema
 //
 // NOTE:
-//	This logic is expected to have multiple if conditions to support
+//	This logic is expected to have multiple **if conditions** to support
 // different hook types e.g. Webhook, gRPCHook, GoTemplateHook etc
 // when they are supported in future
 func SetCallFnFromSchema(schema *v1alpha1.Hook) hooks.HookCallerOption {

controller/common/manage_attachments.go

@@ -22,10 +22,11 @@ import (
 	"strings"
 
 	"github.com/golang/glog"
+	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/util/diff"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	"openebs.io/metac/apis/metacontroller/v1alpha1"
@@ -64,11 +65,17 @@ type AttachmentExecuteBase struct {
 	// should be claimed by the watch.
 	IsWatchOwner *bool
 
-	// UpdateAny follows the GenericController specifications. When set
+	// UpdateAny follows the GenericController's spec.UpdateAny. When set
 	// to true it grants this executor to update any attachments even if
 	// these attachments were not created due to the watch set in this
 	// executor.
 	UpdateAny *bool
+
+	// DeleteAny follows the GenericController's spec.DeleteAny. When set
+	// to true it grants this executor to delete any attachments even if
+	// these attachments were not created due to the watch set in this
+	// executor.
+	DeleteAny *bool
 }
 
 // String implements Stringer interface
@@ -213,9 +220,9 @@ func (m *AttachmentManager) preApply() {
 		)
 	}
 	if m.IsWatchOwner == nil {
-		// defaults to set this watch as the owner
+		// defaults to set this watch as not the owner
 		// of the attachments
-		m.IsWatchOwner = kubernetes.BoolPtr(true)
+		m.IsWatchOwner = kubernetes.BoolPtr(false)
 	}
 }
 
@@ -321,7 +328,7 @@ func (e *AttachmentResourcesExecutor) Update(oObj, dObj *unstructured.Unstructur
 		// it gets deleted by someone else i.e. we won't delete it
 		// ourselves
 		glog.V(4).Infof(
-			"%s: Can't update %s: It's OnDelete update strategy",
+			"%s: Can't update %s: It's OnDelete update strategy or resource not available",
 			e, DescObjectAsKey(dObj),
 		)
 		return nil
@@ -367,7 +374,7 @@ func (e *AttachmentResourcesExecutor) Update(oObj, dObj *unstructured.Unstructur
 		glog.Infof(
 			"%s: Diff: a=observed, b=desired:\n%s",
 			e,
-			diff.ObjectReflectDiff(
+			cmp.Diff(
 				oObj.UnstructuredContent(),
 				uObj.UnstructuredContent(),
 			),
@@ -509,6 +516,12 @@ func (e *AttachmentResourcesExecutor) CreateOrUpdate() error {
 func (e *AttachmentResourcesExecutor) Delete() error {
 	var errs []error
 
+	// check if controller has rights to delete any attachments
+	deleteAny := false
+	if e.DeleteAny != nil {
+		deleteAny = *e.DeleteAny
+	}
+
 	for name, obj := range e.Observed {
 		if obj.GetDeletionTimestamp() != nil {
 			// Skip objects that are already pending deletion.
@@ -518,28 +531,32 @@ func (e *AttachmentResourcesExecutor) Delete() error {
 			continue
 		}
 
+		// check which watch created this resource in the first place
 		ann := obj.GetAnnotations()
 		wantWatch := string(e.Watch.GetUID())
 		gotWatch := ""
 		if ann != nil {
 			gotWatch = ann[attachmentCreateAnnotationKey]
 		}
-		if gotWatch != wantWatch {
+
+		if gotWatch != wantWatch && !deleteAny {
 			// Skip objects that was not created due to this watch
-			glog.V(4).Infof("%s: Can't delete %s: Annotation %s has %q want %q",
+			glog.V(4).Infof(
+				"%s: Can't delete %s: Annotation %s has %q want %q: DeleteAny %t",
 				e,
 				DescObjectAsKey(obj),
 				attachmentCreateAnnotationKey,
 				gotWatch,
 				wantWatch,
+				deleteAny,
 			)
 			continue
 		}
 
 		if e.Desired == nil || e.Desired[name] == nil {
 			// This observed object wasn't listed as desired.
 			// Hence, this is the right candidate to be deleted.
-			glog.Infof("%s: Deleting %s", e, DescObjectAsKey(obj))
+			glog.V(4).Infof("%s: Deleting %s", e, DescObjectAsKey(obj))
 
 			uid := obj.GetUID()
 			// Explicitly request deletion propagation, which is what
@@ -554,6 +571,12 @@ func (e *AttachmentResourcesExecutor) Delete() error {
 				},
 			)
 			if err != nil {
+				if apierrors.IsNotFound(err) {
+					glog.V(4).Infof(
+						"%s: Can't delete %s: Is not found: %v",
+						e, DescObjectAsKey(obj), err)
+					continue
+				}
 				errs = append(
 					errs,
 					errors.Wrapf(err, "%s: Failed to delete %s", e, DescObjectAsKey(obj)),

controller/common/manage_children.go

@@ -21,6 +21,7 @@ import (
 	"reflect"
 
 	"github.com/golang/glog"
+	"github.com/pkg/errors"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -47,6 +48,13 @@ type Apply struct {
 	// SetLastAppliedFn sets the last applied configuration against
 	// the provided unstruct instance
 	SetLastAppliedFn func(obj *unstructured.Unstructured, lastApplied map[string]interface{}) error
+
+	// SanitizeLastAppliedFn sanitizes the last applied configuration
+	// to avoid recursive diff while executing apply logic
+	//
+	// NOTE:
+	//	This is typically invoked before calling SetLastAppliedFn
+	SanitizeLastAppliedFn func(lastApplied map[string]interface{})
 }
 
 // NewApplyFromAnnKey returns a new instance of Apply based on the provided
@@ -59,6 +67,15 @@ func NewApplyFromAnnKey(key string) *Apply {
 		SetLastAppliedFn: func(o *unstructured.Unstructured, last map[string]interface{}) error {
 			return dynamicapply.SetLastAppliedByAnnKey(o, last, key)
 		},
+		SanitizeLastAppliedFn: func(last map[string]interface{}) {
+			// delete the key that stores the last applied configuration
+			// from the last applied configuration itself. This is needed
+			// to break the chain of last applied states. In other words
+			// this avoids last applied config storing details about previous
+			// last applied state that in turn stores the details of its
+			// previous last applied state & so on.
+			dynamicapply.SanitizeLastAppliedByAnnKey(last, key)
+		},
 	}
 }
 
@@ -68,14 +85,17 @@ func (a *Apply) Merge(
 	orig, update *unstructured.Unstructured,
 ) (*unstructured.Unstructured, error) {
 
+	// defaults to old way
 	if a.GetLastAppliedFn == nil {
-		// defaults to old way
 		a.GetLastAppliedFn = dynamicapply.GetLastApplied
 	}
 	if a.SetLastAppliedFn == nil {
-		// defaults to old way
 		a.SetLastAppliedFn = dynamicapply.SetLastApplied
 	}
+	if a.SanitizeLastAppliedFn == nil {
+		// a no-op
+		a.SanitizeLastAppliedFn = func(l map[string]interface{}) {}
+	}
 
 	return a.merge(orig, update)
 }
@@ -113,17 +133,21 @@ func (a *Apply) merge(
 	//
 	// See: https://github.com/GoogleCloudPlatform/metacontroller/issues/76
 	if err := revertObjectMetaSystemFields(newObj, orig); err != nil {
-		return nil, fmt.Errorf("failed to revert ObjectMeta system fields: %v", err)
+		return nil, errors.Wrapf(err, "Failed to revert ObjectMeta system fields")
 	}
 
 	// 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)
+		return nil, errors.Wrapf(err, "Failed to revert .status")
 	}
 
+	// sanitize the last applied state before storing the same
+	// in the newly formed object
+	a.SanitizeLastAppliedFn(update.UnstructuredContent())
 	a.SetLastAppliedFn(newObj, update.UnstructuredContent())
+
 	return newObj, nil
 }
 
@@ -154,16 +178,22 @@ func revertObjectMetaSystemFields(newObj, orig *unstructured.Unstructured) error
 
 // 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...)
+	// check the field in original
+	fieldVal, found, err :=
+		unstructured.NestedFieldNoCopy(orig.UnstructuredContent(), fieldPath...)
 	if err != nil {
-		return fmt.Errorf("can't traverse UnstructuredContent to look for field %v: %v", fieldPath, err)
+		return errors.Wrapf(
+			err,
+			"Can't traverse UnstructuredContent to look for field %v", fieldPath,
+		)
 	}
 	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(), field, fieldPath...); err != nil {
-			return fmt.Errorf("can't revert field %v: %v", fieldPath, err)
+		err := unstructured.SetNestedField(newObj.UnstructuredContent(), fieldVal, fieldPath...)
+		if err != nil {
+			return errors.Wrapf(err, "Can't revert field %v", fieldPath)
 		}
 	} else {
 		// The original had this field unset, so make sure it remains unset.