Merge pull request #1115 from yiqigao217/v0.5

Cherrypick: Handle source object overwriting conflict

Author: k8s-ci-robot

incubator/hnc/internal/forest/namespaceobjects.go

@@ -63,6 +63,14 @@ func (ns *Namespace) GetPropagatedObjects(gvk schema.GroupVersionKind) []*unstru
 	return o
 }
 
+// GetPropagatingObjects returns all the source objects to be propagated into the
+// descendants of this namespace.
+// TODO update this function to reflect the changes introduced by the future HNC
+//  exceptions implementation.
+func (ns *Namespace) GetPropagatingObjects(gvk schema.GroupVersionKind) []*unstructured.Unstructured {
+	return append(ns.GetPropagatedObjects(gvk), ns.GetOriginalObjects(gvk)...)
+}
+
 // GetSource returns the original copy in the ancestors if it exists.
 // Otherwise, return nil.
 func (ns *Namespace) GetSource(gvk schema.GroupVersionKind, name string) *unstructured.Unstructured {

incubator/hnc/internal/reconcilers/object.go

@@ -325,21 +325,33 @@ func (r *ObjectReconciler) syncObject(ctx context.Context, log logr.Logger, inst
 		return actionNop, nil
 	}
 
-	// This object is the source if it doesn't have the "api.LabelInheritedFrom" label.
-	if !hasPropagatedLabel(inst) {
+	// This object is a propagated copy if it has "api.LabelInheritedFrom" label.
+	if hasPropagatedLabel(inst) {
+		return r.syncPropagated(ctx, log, inst)
+	}
+
+	// Find the source object of the same name in the ancestors from top down to
+	// see if there's a conflicting source.
+	srcInst := r.Forest.Get(inst.GetNamespace()).GetSource(r.GVK, inst.GetName())
+
+	// The object is a source without conflict if a copy of the source is not
+	// found in the forest or itself is found.
+	if srcInst == nil || srcInst.GetNamespace() == inst.GetNamespace() {
 		r.syncSource(ctx, log, inst)
 		// No action needs to take on source objects.
 		return actionNop, nil
 	}
 
-	// This object is a propagated copy.
+	// Since there's a conflict that another source with the same name is found in
+	// the ancestors, this instance will be treated as propagated objects and will
+	// be overwritten by the source in the ancestor.
 	return r.syncPropagated(ctx, log, inst)
 }
 
 // syncPropagated will determine whether to delete the obsolete copy or overwrite it with the source.
 // Or do nothing if it remains the same as the source object.
 func (r *ObjectReconciler) syncPropagated(ctx context.Context, log logr.Logger, inst *unstructured.Unstructured) (syncAction, *unstructured.Unstructured) {
-	// Find a source object of the same name in any of the ancestores.
+	// Find the source object of the same name in the ancestors from top down.
 	srcInst := r.Forest.Get(inst.GetNamespace()).GetSource(r.GVK, inst.GetName())
 
 	// If no source object exists, delete this object. This can happen when the source was deleted by
@@ -354,7 +366,9 @@ func (r *ObjectReconciler) syncPropagated(ctx context.Context, log logr.Logger,
 	// If the copy does not exist, or is different from the source, return the write action and the
 	// source instance. Note that DeepEqual could return `true` even if the object doesn't exist if
 	// the source object is trivial (e.g. a completely empty ConfigMap).
-	if !exists || !reflect.DeepEqual(object.Canonical(inst), object.Canonical(srcInst)) {
+	if !exists ||
+		!reflect.DeepEqual(object.Canonical(inst), object.Canonical(srcInst)) ||
+		inst.GetLabels()[api.LabelInheritedFrom] != srcInst.GetNamespace() {
 		metadata.SetLabel(inst, api.LabelInheritedFrom, srcInst.GetNamespace())
 		return actionWrite, srcInst
 	}

incubator/hnc/internal/reconcilers/object_test.go

@@ -134,6 +134,25 @@ var _ = Describe("Secret", func() {
 		Eventually(isModified(ctx, bazName, "foo-role")).Should(BeTrue())
 	})
 
+	It("should overwrite the conflicting source in the descedants", func() {
+		setParent(ctx, barName, fooName)
+		setParent(ctx, bazName, barName)
+		Eventually(hasObject(ctx, "Role", barName, "bar-role")).Should(BeTrue())
+		Eventually(hasObject(ctx, "Role", bazName, "bar-role")).Should(BeTrue())
+		Expect(objectInheritedFrom(ctx, "Role", bazName, "bar-role")).Should(Equal(barName))
+
+		makeObject(ctx, "Role", fooName, "bar-role")
+		// Add a 500-millisecond gap here to allow updating the cached bar-roles in bar
+		// and baz namespaces. Without this, even having 20 seconds in the "Eventually()"
+		// funcs below, the test failed with timeout. Guess the reason is that it's
+		// constantly getting the cached object.
+		time.Sleep(500 * time.Millisecond)
+		Eventually(hasObject(ctx, "Role", bazName, "bar-role")).Should(BeTrue())
+		Eventually(objectInheritedFrom(ctx, "Role", bazName, "bar-role")).Should(Equal(fooName))
+		Eventually(hasObject(ctx, "Role", barName, "bar-role")).Should(BeTrue())
+		Eventually(objectInheritedFrom(ctx, "Role", barName, "bar-role")).Should(Equal(fooName))
+	})
+
 	It("should have deletions propagated after crit conditions are removed", func() {
 		// Create tree: bar -> foo (root) and make sure foo-role is propagated
 		setParent(ctx, barName, fooName)

incubator/hnc/internal/validators/hierarchy.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"os"
+	"strings"
 
 	"github.com/go-logr/logr"
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
@@ -12,6 +13,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -210,9 +212,59 @@ func (v *Hierarchy) checkParent(ns, curParent, newParent *forest.Namespace) admi
 		return deny(metav1.StatusReasonConflict, "Illegal parent: "+reason)
 	}
 
+	// Prevent overwriting source objects in the descendants after the hierarchy change.
+	if co := v.getConflictingObjects(newParent, ns); len(co) != 0 {
+		msg := "Cannot update hierarchy because it would overwrite the following object(s):\n"
+		msg += "  * " + strings.Join(co, "\n  * ") + "\n"
+		msg += "To fix this, please rename or remove the conflicting objects first."
+		return deny(metav1.StatusReasonConflict, msg)
+	}
+
 	return allow("")
 }
 
+// getConflictingObjects returns a list of namespaced objects if there's any conflict.
+func (v *Hierarchy) getConflictingObjects(newParent, ns *forest.Namespace) []string {
+	// If the new parent is nil,  early exit since it's impossible to introduce
+	// new naming conflicts.
+	if newParent == nil {
+		return nil
+	}
+	// Traverse all the types with 'Propagate' mode to find any conflicts.
+	conflicts := []string{}
+	for _, t := range v.Forest.GetTypeSyncers() {
+		if t.GetMode() == api.Propagate {
+			conflicts = append(conflicts, v.getConflictingObjectsOfType(t.GetGVK(), newParent, ns)...)
+		}
+	}
+	return conflicts
+}
+
+// getConflictingObjectsOfType returns a list of namespaced objects if there's
+// any conflict between the new ancestors and the descendants.
+func (v *Hierarchy) getConflictingObjectsOfType(gvk schema.GroupVersionKind, newParent, ns *forest.Namespace) []string {
+	// Get all the source objects in the new ancestors that would be propagated
+	// into the descendants.
+	newAnsSrcObjs := make(map[string]bool)
+	for _, o := range newParent.GetPropagatingObjects(gvk) {
+		newAnsSrcObjs[o.GetName()] = true
+	}
+
+	// Look in the descendants to find if there's any conflict.
+	cos := []string{}
+	dnses := append(ns.DescendantNames(), ns.Name())
+	for _, dns := range dnses {
+		for _, o := range v.Forest.Get(dns).GetOriginalObjects(gvk) {
+			if newAnsSrcObjs[o.GetName()] {
+				co := fmt.Sprintf("Namespace %q: %s (%v)", dns, o.GetName(), gvk)
+				cos = append(cos, co)
+			}
+		}
+	}
+
+	return cos
+}
+
 type serverCheckType int
 
 const (

incubator/hnc/internal/validators/hierarchy_test.go

@@ -7,7 +7,9 @@ import (
 	. "github.com/onsi/gomega"
 	authn "k8s.io/api/authentication/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/reconcilers"
 
 	api "sigs.k8s.io/multi-tenancy/incubator/hnc/api/v1alpha1"
 	"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/foresttest"
@@ -96,6 +98,57 @@ func TestChangeParentOnManagedBy(t *testing.T) {
 	}
 }
 
+func TestChangeParentWithConflict(t *testing.T) {
+	f := foresttest.Create("-a-c") // a <- b; c <- d
+
+	// Set secret to "Propagate" mode. (Use Secret in this test because the test
+	// forest doesn't have Role or RoleBinding by default either. We can also create
+	// secret by existing `createSecret()` function.)
+	or := &reconcilers.ObjectReconciler{
+		GVK:  schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"},
+		Mode: api.Propagate,
+	}
+	f.AddTypeSyncer(or)
+
+	// Create secrets with the same name in namespace 'a' and 'd'.
+	createSecret("conflict", "a", f)
+	createSecret("conflict", "d", f)
+
+	h := &Hierarchy{Forest: f}
+	l := zap.Logger(false)
+
+	tests := []struct {
+		name string
+		nnm  string
+		pnm  string
+		fail bool
+	}{
+		{name: "conflict in itself and the new parent", nnm: "a", pnm: "d", fail: true},
+		{name: "conflict in itself and a new ancestor (not the parent)", nnm: "d", pnm: "b", fail: true},
+		{name: "ok: no conflict in ancestors", nnm: "a", pnm: "c"},
+		{name: "conflict in subtree leaf and the new parent", nnm: "c", pnm: "a", fail: true},
+		{name: "conflict in subtree leaf and a new ancestor (not the parent)", nnm: "c", pnm: "b", fail: true},
+		{name: "ok: set a namespace as root", nnm: "d", pnm: ""},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup
+			g := NewGomegaWithT(t)
+			hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.pnm}}
+			hc.ObjectMeta.Name = api.Singleton
+			hc.ObjectMeta.Namespace = tc.nnm
+			req := &request{hc: hc}
+
+			// Test
+			got := h.handle(context.Background(), l, req)
+
+			// Report
+			logResult(t, got.AdmissionResponse.Result)
+			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+		})
+	}
+}
+
 func TestAuthz(t *testing.T) {
 	tests := []struct {
 		name   string

incubator/hnc/internal/validators/object.go

@@ -2,7 +2,9 @@ package validators
 
 import (
 	"context"
+	"fmt"
 	"reflect"
+	"strings"
 
 	"github.com/go-logr/logr"
 	admissionv1beta1 "k8s.io/api/admission/v1beta1"
@@ -106,8 +108,14 @@ func (o *Object) handle(ctx context.Context, log logr.Logger, op admissionv1beta
 	oldSource, oldInherited := metadata.GetLabel(oldInst, api.LabelInheritedFrom)
 	newSource, newInherited := metadata.GetLabel(inst, api.LabelInheritedFrom)
 
-	// If the object wasn't and isn't inherited, it's none of our business.
+	// If the object wasn't and isn't inherited, we will check to see if the
+	// source can be created without causing any conflict.
 	if !oldInherited && !newInherited {
+		if yes, dnses := o.hasConflict(inst); yes {
+			dnsesStr := strings.Join(dnses, "\n  * ")
+			msg := fmt.Sprintf("\nCannot create %q (%s) in namespace %q because it would overwrite objects in the following descendant namespace(s):\n  * %s\nTo fix this, choose a different name for the object, or remove the conflicting objects from the above namespaces.", inst.GetName(), inst.GroupVersionKind(), inst.GetNamespace(), dnsesStr)
+			return deny(metav1.StatusReasonConflict, msg)
+		}
 		return allow("source object")
 	}
 
@@ -146,6 +154,33 @@ func (o *Object) handle(ctx context.Context, log logr.Logger, op admissionv1beta
 	return deny(metav1.StatusReasonInternalError, "unknown operation: "+string(op))
 }
 
+// hasConflict checks if there's any conflicting objects in the descendants. Returns
+// true and a list of conflicting descendants, if yes.
+func (o *Object) hasConflict(inst *unstructured.Unstructured) (bool, []string) {
+	o.Forest.Lock()
+	defer o.Forest.Unlock()
+
+	// If the instance is empty (for a delete operation) or it's not namespace-scoped,
+	// there must be no conflict.
+	if inst == nil || inst.GetNamespace() == "" {
+		return false, nil
+	}
+
+	nm := inst.GetName()
+	gvk := inst.GroupVersionKind()
+	descs := o.Forest.Get(inst.GetNamespace()).DescendantNames()
+	conflicts := []string{}
+
+	// Get a list of conflicting descendants if there's any.
+	for _, desc := range descs {
+		if o.Forest.Get(desc).HasOriginalObject(gvk, nm) {
+			conflicts = append(conflicts, desc)
+		}
+	}
+
+	return len(conflicts) != 0, conflicts
+}
+
 func (o *Object) InjectClient(c client.Client) error {
 	o.client = c
 	return nil

incubator/hnc/internal/validators/object_test.go

@@ -11,6 +11,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/foresttest"
 
 	api "sigs.k8s.io/multi-tenancy/incubator/hnc/api/v1alpha1"
 	"sigs.k8s.io/multi-tenancy/incubator/hnc/internal/forest"
@@ -332,3 +333,70 @@ func TestUserChanges(t *testing.T) {
 		})
 	}
 }
+
+func TestCreatingConflictSource(t *testing.T) {
+	tests := []struct {
+		name              string
+		forest            string
+		conflictInstName  string
+		conflictNamespace string
+		newInstName       string
+		newInstNamespace  string
+		fail              bool
+	}{{
+		name:              "Deny creation of source objects with conflict in child",
+		forest:            "-a",
+		conflictInstName:  "secret-b",
+		conflictNamespace: "b",
+		newInstName:       "secret-b",
+		newInstNamespace:  "a",
+		fail:              true,
+	}, {
+		name:              "Deny creation of source objects with conflict in grandchild",
+		forest:            "-ab",
+		conflictInstName:  "secret-c",
+		conflictNamespace: "c",
+		newInstName:       "secret-c",
+		newInstNamespace:  "a",
+		fail:              true,
+	}, {
+		name:             "Allow creation of source objects with no conflict",
+		newInstName:      "secret-a",
+		newInstNamespace: "a",
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup
+			g := NewGomegaWithT(t)
+			f := foresttest.Create(tc.forest)
+			createSecret(tc.conflictInstName, tc.conflictNamespace, f)
+			o := &Object{Forest: f}
+			l := zap.Logger(false)
+			op := admissionv1beta1.Create
+			inst := &unstructured.Unstructured{}
+			inst.SetName(tc.newInstName)
+			inst.SetNamespace(tc.newInstNamespace)
+			inst.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"})
+			// Test
+			got := o.handle(context.Background(), l, op, inst, &unstructured.Unstructured{})
+			// Report
+			code := got.AdmissionResponse.Result.Code
+			reason := got.AdmissionResponse.Result.Reason
+			msg := got.AdmissionResponse.Result.Message
+			t.Logf("Got code %d, reason %q, message %q", code, reason, msg)
+			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+		})
+	}
+}
+
+func createSecret(nm, nsn string, f *forest.Forest) {
+	if nm == "" || nsn == "" {
+		return
+	}
+	inst := &unstructured.Unstructured{}
+	inst.SetName(nm)
+	inst.SetNamespace(nsn)
+	inst.SetGroupVersionKind(schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"})
+	f.Get(nsn).SetOriginalObject(inst)
+}