diff --git a/incubator/hnc/internal/forest/namespaceobjects.go b/incubator/hnc/internal/forest/namespaceobjects.go index cf02db49d..9c8da0160 100644 --- a/incubator/hnc/internal/forest/namespaceobjects.go +++ b/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 { diff --git a/incubator/hnc/internal/reconcilers/object.go b/incubator/hnc/internal/reconcilers/object.go index 550a41988..17720e6a8 100644 --- a/incubator/hnc/internal/reconcilers/object.go +++ b/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 } diff --git a/incubator/hnc/internal/reconcilers/object_test.go b/incubator/hnc/internal/reconcilers/object_test.go index 27c6e7551..510ba2b72 100644 --- a/incubator/hnc/internal/reconcilers/object_test.go +++ b/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) diff --git a/incubator/hnc/internal/validators/hierarchy.go b/incubator/hnc/internal/validators/hierarchy.go index f272eb633..e07ed257b 100644 --- a/incubator/hnc/internal/validators/hierarchy.go +++ b/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 ( diff --git a/incubator/hnc/internal/validators/hierarchy_test.go b/incubator/hnc/internal/validators/hierarchy_test.go index 797056d12..9153eab94 100644 --- a/incubator/hnc/internal/validators/hierarchy_test.go +++ b/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 diff --git a/incubator/hnc/internal/validators/object.go b/incubator/hnc/internal/validators/object.go index 02ec21198..f19f8e544 100644 --- a/incubator/hnc/internal/validators/object.go +++ b/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 diff --git a/incubator/hnc/internal/validators/object_test.go b/incubator/hnc/internal/validators/object_test.go index be5d94ac7..ea32bcd84 100644 --- a/incubator/hnc/internal/validators/object_test.go +++ b/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) +}