From 52a1559f920c7d460289e32114f9d7964d1e907f Mon Sep 17 00:00:00 2001 From: Yiqi Gao Date: Thu, 5 Mar 2020 17:16:40 -0500 Subject: [PATCH] Update transient & report invalid HNS states Update transient HNS states and report invalid states if bypassing the admission controller. Add a channel in the HNS reconciler to enqueue HNS for reconciliation. Add HNS reconciler and HC reconciler into each other so that they can enqueue the other resource. Sync the HNS instances in HNS reconciler, instead of syncing the spec.requiredChildren in "syncChildren()" func in HC reconciler. Add integration tests to test all different states. Tested with integration tests ("make test" & "make test HNS=1") and on GKE cluster. To test manually, I added the "enable-hns-reconciler" flag and commented out the webhook "Prevent changing parent of a required child" part, since it uses the "RequiredChildOf" field in the forest. I tested deleting the subnamespace, changing the annotation, change the parent of the subnamespace. They all worked as expected. --- incubator/hnc/api/v1alpha1/hierarchy_types.go | 14 +- .../pkg/reconcilers/hierarchical_namespace.go | 182 ++++++++++++++++-- .../hierarchical_namespace_test.go | 117 +++++++++-- .../hnc/pkg/reconcilers/hierarchy_config.go | 128 +++++++----- .../pkg/reconcilers/hierarchy_config_test.go | 8 +- incubator/hnc/pkg/reconcilers/object.go | 2 +- incubator/hnc/pkg/reconcilers/setup.go | 23 ++- 7 files changed, 370 insertions(+), 104 deletions(-) diff --git a/incubator/hnc/api/v1alpha1/hierarchy_types.go b/incubator/hnc/api/v1alpha1/hierarchy_types.go index 8b96797d7..fbec7b826 100644 --- a/incubator/hnc/api/v1alpha1/hierarchy_types.go +++ b/incubator/hnc/api/v1alpha1/hierarchy_types.go @@ -32,13 +32,15 @@ const ( ) // Condition codes. *All* codes must also be documented in the comment to Condition.Code. +// TODO change condition codes to CamelCase strings. See issue: +// https://github.com/kubernetes-sigs/multi-tenancy/issues/500 const ( - CritParentMissing Code = "CRIT_PARENT_MISSING" - CritParentInvalid Code = "CRIT_PARENT_INVALID" - CritAncestor Code = "CRIT_ANCESTOR" - RequiredChildConflict Code = "REQUIRED_CHILD_CONFLICT" - CannotUpdate Code = "CANNOT_UPDATE_OBJECT" - CannotPropagate Code = "CANNOT_PROPAGATE_OBJECT" + CritParentMissing Code = "CRIT_PARENT_MISSING" + CritParentInvalid Code = "CRIT_PARENT_INVALID" + CritAncestor Code = "CRIT_ANCESTOR" + SubnamespaceConflict Code = "SUBNAMESPACE_CONFLICT" + CannotUpdate Code = "CANNOT_UPDATE_OBJECT" + CannotPropagate Code = "CANNOT_PROPAGATE_OBJECT" ) // EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! diff --git a/incubator/hnc/pkg/reconcilers/hierarchical_namespace.go b/incubator/hnc/pkg/reconcilers/hierarchical_namespace.go index dae175524..6d1e61e2d 100644 --- a/incubator/hnc/pkg/reconcilers/hierarchical_namespace.go +++ b/incubator/hnc/pkg/reconcilers/hierarchical_namespace.go @@ -16,11 +16,19 @@ limitations under the License. package reconcilers import ( + "context" + "github.com/go-logr/logr" api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1" "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/forest" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/source" ) // HierarchicalNamespaceReconciler reconciles HierarchicalNamespace CRs to make sure @@ -31,44 +39,182 @@ type HierarchicalNamespaceReconciler struct { forest *forest.Forest hcr *HierarchyConfigReconciler + + // Affected is a channel of event.GenericEvent (see "Watching Channels" in + // https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) that is used to + // enqueue additional objects that need updating. + Affected chan event.GenericEvent } // Reconcile sets up some basic variables and then calls the business logic. It currently // only handles the creation of the namespaces but no deletion or state reporting yet. func (r *HierarchicalNamespaceReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { - // TODO report error state if the webhook is bypassed - see issue - // https://github.com/kubernetes-sigs/multi-tenancy/issues/459 - if ex[req.Namespace] { - return ctrl.Result{}, nil - } - + ctx := context.Background() log := r.Log.WithValues("trigger", req.NamespacedName) log.Info("Reconciling HNS") - // Names of the hierarchical namespace and the current namespace. + // Get names of the hierarchical namespace and the current namespace. nm := req.Name pnm := req.Namespace - // Set RequiredChildOf (Owner) in the forest of the hierarchical namespace to the - // current namespace. + // Get instance from apiserver. If the instance doesn't exist, we don't want to reconcile + // it since it may trigger the HC reconciler to recreate the namespace that was just deleted. + // TODO expand on this to check the owner's hc.spec.allowCascadingDelete. If it's set to + // true, we still want to reconcile the hns instance. See issue: + // https://github.com/kubernetes-sigs/multi-tenancy/issues/501 + inst, err := r.getInstance(ctx, pnm, nm) + if err != nil { + if errors.IsNotFound(err) { + // If the instance doesn't exist, return nil to prevent a retry. + return ctrl.Result{}, nil + } + return ctrl.Result{}, err + } + + // Report "Forbidden" state and early exist if the namespace is not allowed to self-serve + // namespaces but has bypassed the webhook and successfully created the hns instance. + // TODO refactor/split the EX map for 1) reconciler exclusion and 2) self-serve not allowed + // purposes. See issue: https://github.com/kubernetes-sigs/multi-tenancy/issues/495 + if EX[pnm] { + inst.Status.State = api.Forbidden + return ctrl.Result{}, r.writeInstance(ctx, log, inst) + } + + // Get the self-serve namespace's hierarchyConfig and namespace instances. + hcInst, nsInst, err := r.hcr.GetInstances(ctx, log, nm) + if err != nil { + return ctrl.Result{}, err + } + + r.syncWithForest(log, inst, nsInst, hcInst) + + // TODO report the "SubnamespaceConflict" in the HNS reconciliation. See issue: + // https://github.com/kubernetes-sigs/multi-tenancy/issues/490 + + return ctrl.Result{}, r.writeInstance(ctx, log, inst) +} + +// HierarchicalNamespace (HNS) is synced with the in-memory forest, the HierarchyConfig and +// the namespace instances to update its HNS state. This will be the only place the "Owner" +// field in the forest is set. Therefore, the "Owner" field can be used in the forest to get +// all the HNS objects of a namespace. +func (r *HierarchicalNamespaceReconciler) syncWithForest(log logr.Logger, inst *api.HierarchicalNamespace, nsInst *corev1.Namespace, hcInst *api.HierarchyConfiguration) { + r.forest.Lock() + defer r.forest.Unlock() + + // Names of the hierarchical namespace and the current namespace. + nm := inst.Name + pnm := inst.Namespace + + // Get the namespace instance in memory. + ns := r.forest.Get(nm) + + switch { + case nsInst.Name == "": + // If the real namespace instance doesn't exist yet, update forest and enqueue the namespace + // to HierarchyConfig reconciler. + log.Info("The self-serve subnamespace does not exist", "namespace", nm) + r.syncMissing(log, inst, ns) + case nsInst.Annotations[api.AnnotationOwner] != pnm: + // If the namespace is not a self-serve namespace or it's a self-serve namespace of another + // namespace. Report the conflict. + log.Info("The owner annotation of the subnamespace doesn't match the owner", "annotation", nsInst.Annotations[api.AnnotationOwner]) + inst.Status.State = api.Conflict + default: + log.Info("The subnamespace has the correct owner annotation", "annotation", nsInst.Annotations[api.AnnotationOwner]) + r.syncExisting(log, inst, ns, hcInst, nsInst) + } +} + +func (r *HierarchicalNamespaceReconciler) syncMissing(log logr.Logger, inst *api.HierarchicalNamespace, ns *forest.Namespace) { + pnm := inst.Namespace + nm := inst.Name + + // Set the HNS state to "Missing" because the subnamespace doesn't exist. + inst.Status.State = api.Missing + + // Set the "Owner" in the forest of the hierarchical namespace to the current namespace. + log.Info("Setting the subnamespace's owner in the forest", "owner", pnm, "namespace", nm) // TODO rename RequiredChildOf to Owner in the forest. See issue: // https://github.com/kubernetes-sigs/multi-tenancy/issues/469 - r.forest.Get(nm).RequiredChildOf = pnm + ns.RequiredChildOf = pnm + + // Enqueue the not-yet existent self-serve subnamespace. The HierarchyConfig reconciler will + // create the namespace and the HierarchyConfig instances on apiserver accordingly. + r.hcr.enqueueAffected(log, "new subnamespace", nm) +} - // Enqueue the in-momery hierarchyConfig instance of the hierarchical namespace. - // The hierarchyConfig reconciler will create the namespace and hierarchyConfig - // instances on apiserver accordingly. - reason := "new/updated hierarchical namespace" - r.hcr.enqueueAffected(log, reason, nm) +// syncExisting syncs the existing subnamespace with its owner namespace. It updates the HNS state +// to "Ok" or "Conflict" according to the hierarchy. +func (r *HierarchicalNamespaceReconciler) syncExisting(log logr.Logger, inst *api.HierarchicalNamespace, ns *forest.Namespace, hcInst *api.HierarchyConfiguration, nsInst *corev1.Namespace) { + pnm := inst.Namespace + nm := inst.Name - // TODO sync with forest to report conflicts in hns.Status.State. See issue: - // https://github.com/kubernetes-sigs/multi-tenancy/issues/487 + switch hcInst.Spec.Parent { + case "": + log.Info("Parent is not set", "namespace", nm) + // This case is rare. It means the namespace is created with the right annotation but + // no HC. This could be a transient state before HC reconciler finishes creating the HC + // or a human manually created the namespace with the right annotation but no HC. + // Both cases meant to create this namespace as HNS. + log.Info("Setting the subnamespace's owner in the forest", "owner", pnm, "namespace", nm) + ns.RequiredChildOf = pnm + r.hcr.enqueueAffected(log, "updated subnamespace", nm) - return ctrl.Result{}, nil + // We will set it as "Conflict" though it's just a short transient state. Once the hc is + // reconciled, this HNS will be enqueued and then set the state to "Ok". + inst.Status.State = api.Conflict + case pnm: + log.Info("Setting the HierarchicalNamespace state to Ok") + inst.Status.State = api.Ok + default: + log.Info("Self-serve subnamespace is already owned by another parent", "child", nm, "intendedParent", pnm, "actualParent", hcInst.Spec.Parent) + inst.Status.State = api.Conflict + } +} + +// It enqueues a hierarchicalNamespace instance for later reconciliation. This occurs in a goroutine +// so the caller doesn't block; since the reconciler is never garbage-collected, this is safe. +func (r *HierarchicalNamespaceReconciler) enqueue(log logr.Logger, nm, pnm, reason string) { + go func() { + // The watch handler doesn't care about anything except the metadata. + inst := &api.HierarchicalNamespace{} + inst.ObjectMeta.Name = nm + inst.ObjectMeta.Namespace = pnm + log.Info("Enqueuing for reconciliation", "affected", pnm+"/"+nm, "reason", reason) + r.Affected <- event.GenericEvent{Meta: inst} + }() +} + +func (r *HierarchicalNamespaceReconciler) getInstance(ctx context.Context, pnm, nm string) (*api.HierarchicalNamespace, error) { + nsn := types.NamespacedName{Namespace: pnm, Name: nm} + inst := &api.HierarchicalNamespace{} + if err := r.Get(ctx, nsn, inst); err != nil { + return nil, err + } + return inst, nil +} + +func (r *HierarchicalNamespaceReconciler) writeInstance(ctx context.Context, log logr.Logger, inst *api.HierarchicalNamespace) error { + if inst.CreationTimestamp.IsZero() { + log.Info("Creating instance on apiserver") + if err := r.Create(ctx, inst); err != nil { + log.Error(err, "while creating on apiserver") + return err + } + } else { + log.Info("Updating instance on apiserver") + if err := r.Update(ctx, inst); err != nil { + log.Error(err, "while updating on apiserver") + return err + } + } + return nil } func (r *HierarchicalNamespaceReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&api.HierarchicalNamespace{}). + Watches(&source.Channel{Source: r.Affected}, &handler.EnqueueRequestForObject{}). Complete(r) } diff --git a/incubator/hnc/pkg/reconcilers/hierarchical_namespace_test.go b/incubator/hnc/pkg/reconcilers/hierarchical_namespace_test.go index b53d4954f..706e87b89 100644 --- a/incubator/hnc/pkg/reconcilers/hierarchical_namespace_test.go +++ b/incubator/hnc/pkg/reconcilers/hierarchical_namespace_test.go @@ -4,13 +4,14 @@ import ( "context" api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1" + "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/reconcilers" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" ) -var _ = Describe("Hierarchy", func() { +var _ = Describe("HierarchicalNamespace", func() { ctx := context.Background() var ( @@ -25,39 +26,104 @@ var _ = Describe("Hierarchy", func() { fooName = createNS(ctx, "foo") barName = createNSName("bar") + }) - // Create "bar" hns in "foo" namespace + It("should create the subnamespace and update the hierarchy according to the HNS instance", func() { + // Create "bar" hns in "foo" namespace. foo_hns_bar := newHierarchicalNamespace(barName, fooName) updateHierarchicalNamespace(ctx, foo_hns_bar) - }) - It("should set the self-serve subnamespace as a child on the current namespace", func() { + // It should set the self-serve subnamespace "bar" as a child on the owner + // namespace "foo". Eventually(func() []string { fooHier := getHierarchy(ctx, fooName) return fooHier.Status.Children }).Should(Equal([]string{barName})) - }) - It("should set the current namespace as the parent of the self-serve subnamespace", func() { + // It should set the owner namespace "foo" as the parent of the self-serve + // subnamespace "bar". Eventually(func() string { barHier := getHierarchy(ctx, barName) return barHier.Spec.Parent }).Should(Equal(fooName)) - }) - It("should create the self-serve subnamespace", func() { + // It should create the self-serve subnamespace "bar". nnm := types.NamespacedName{Name: barName} ns := &corev1.Namespace{} Eventually(func() error { return k8sClient.Get(ctx, nnm, ns) }).Should(Succeed()) + + // It should set the self-serve subnamespace's owner annotation to the owner + // namespace (should set bar's owner annotation to "foo"). + Eventually(getNamespaceAnnotation(ctx, barName, api.AnnotationOwner)).Should(Equal(fooName)) + + // It should set the hns.status.state to Ok if the above sub-tests all pass. + Eventually(getHNSState(ctx, fooName, barName)).Should(Equal(api.Ok)) }) - It("should set the self-serve subnamespace's owner annotation to the current namespace", func() { + It("should set the hns.status.state to Forbidden if the parent namespace is not allowed to self-serve subnamespaces", func() { + kube_system_hns_bar := newHierarchicalNamespace(barName, "kube-system") + updateHierarchicalNamespace(ctx, kube_system_hns_bar) + Eventually(getHNSState(ctx, "kube-system", barName)).Should(Equal(api.Forbidden)) + }) + + It("should set the hns.status.state to Missing if the self-serve subnamespace doesn't exist", func() { + // This is a trick to disable hc reconciler on "bar" namespace by having "bar" in the excluded namespace list. + // Therefore the "bar" namespace won't be created even if the HNS reconciler enqueues the not-yet existing + // "bar" namespace for hc reconciler to reconcile and create. + reconcilers.EX[barName] = true + + // Create "bar" hns in "foo" namespace after the HC reconciler is "disabled" (only for "bar" namespace). + foo_hns_bar := newHierarchicalNamespace(barName, fooName) + updateHierarchicalNamespace(ctx, foo_hns_bar) + + // We should then have the HNS state stays at "Missing" + Eventually(getHNSState(ctx, fooName, barName)).Should(Equal(api.Missing)) + }) + + It("should set the hns.status.state to Conflict if the namespace's owner annotation is wrong", func() { + // Create "bar" hns in "foo" namespace. + foo_hns_bar := newHierarchicalNamespace(barName, fooName) + updateHierarchicalNamespace(ctx, foo_hns_bar) + + // It should set the self-serve subnamespace's owner annotation to the owner + // namespace (should set bar's owner annotation to "foo"). Eventually(getNamespaceAnnotation(ctx, barName, api.AnnotationOwner)).Should(Equal(fooName)) + + // Change the owner annotation to a different value and the HNS state should + // be set to "Conflict". + setWrongNamespaceOwnerAnnotation(ctx, barName) + Eventually(getHNSState(ctx, fooName, barName)).Should(Equal(api.Conflict)) + }) + + It("should set the hns.status.state to Conflict if the namespace's parent is not the owner", func() { + // Create "bar" hns in "foo" namespace. + foo_hns_bar := newHierarchicalNamespace(barName, fooName) + updateHierarchicalNamespace(ctx, foo_hns_bar) + + // Change the bar's parent. The HNS should be set to "Conflict". + barHier := getHierarchy(ctx, barName) + barHier.Spec.Parent = "other" + updateHierarchy(ctx, barHier) + Eventually(getHNSState(ctx, fooName, barName)).Should(Equal(api.Conflict)) }) }) +func getHNSState(ctx context.Context, pnm, nm string) func() api.HNSState { + return func() api.HNSState { + return getHierarchicalNamespace(ctx, pnm, nm).Status.State + } +} + +func getNamespaceAnnotation(ctx context.Context, nnm, annotation string) func() string { + return func() string { + ns := getNamespace(ctx, nnm) + val, _ := ns.GetAnnotations()[annotation] + return val + } +} + func newHierarchicalNamespace(hnsnm, nm string) *api.HierarchicalNamespace { hns := &api.HierarchicalNamespace{} hns.ObjectMeta.Namespace = nm @@ -65,6 +131,19 @@ func newHierarchicalNamespace(hnsnm, nm string) *api.HierarchicalNamespace { return hns } +func getHierarchicalNamespace(ctx context.Context, pnm, nm string) *api.HierarchicalNamespace { + return getHierarchicalNamespaceWithOffset(1, ctx, pnm, nm) +} + +func getHierarchicalNamespaceWithOffset(offset int, ctx context.Context, pnm, nm string) *api.HierarchicalNamespace { + nsn := types.NamespacedName{Name: nm, Namespace: pnm} + hns := &api.HierarchicalNamespace{} + EventuallyWithOffset(offset+1, func() error { + return k8sClient.Get(ctx, nsn, hns) + }).Should(Succeed()) + return hns +} + func updateHierarchicalNamespace(ctx context.Context, hns *api.HierarchicalNamespace) { if hns.CreationTimestamp.IsZero() { ExpectWithOffset(1, k8sClient.Create(ctx, hns)).Should(Succeed()) @@ -73,10 +152,18 @@ func updateHierarchicalNamespace(ctx context.Context, hns *api.HierarchicalNames } } -func getNamespaceAnnotation(ctx context.Context, nnm, annotation string) func() string { - return func() string { - ns := getNamespace(ctx, nnm) - val, _ := ns.GetAnnotations()[annotation] - return val - } +func setWrongNamespaceOwnerAnnotation(ctx context.Context, nnm string) { + a := make(map[string]string) + a[api.AnnotationOwner] = "wrong" + ns := getNamespace(ctx, nnm) + ns.SetAnnotations(a) + updateNamespace(ctx, nnm) +} + +func updateNamespace(ctx context.Context, nm string) { + ns := &corev1.Namespace{} + ns.ObjectMeta.Name = nm + EventuallyWithOffset(1, func() error { + return k8sClient.Update(ctx, ns) + }).Should(Succeed()) } diff --git a/incubator/hnc/pkg/reconcilers/hierarchy_config.go b/incubator/hnc/pkg/reconcilers/hierarchy_config.go index 33d31b694..209c96d3f 100644 --- a/incubator/hnc/pkg/reconcilers/hierarchy_config.go +++ b/incubator/hnc/pkg/reconcilers/hierarchy_config.go @@ -69,6 +69,8 @@ type HierarchyConfigReconciler struct { // the GitHub issue "Implement self-service namespace" is resolved // (https://github.com/kubernetes-sigs/multi-tenancy/issues/457) HNSReconcilerEnabled bool + + hnsr *HierarchicalNamespaceReconciler } // +kubebuilder:rbac:groups=hnc.x-k8s.io,resources=hierarchies,verbs=get;list;watch;create;update;patch;delete @@ -77,7 +79,7 @@ type HierarchyConfigReconciler struct { // Reconcile sets up some basic variables and then calls the business logic. func (r *HierarchyConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { - if ex[req.Namespace] { + if EX[req.Namespace] { return ctrl.Result{}, nil } @@ -100,7 +102,7 @@ func (r *HierarchyConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, er func (r *HierarchyConfigReconciler) reconcile(ctx context.Context, log logr.Logger, nm string) error { // Get the singleton and namespace. - inst, nsInst, err := r.getInstances(ctx, log, nm) + inst, nsInst, err := r.GetInstances(ctx, log, nm) if err != nil { return err } @@ -196,6 +198,12 @@ func (r *HierarchyConfigReconciler) onMissingNamespace(log logr.Logger, ns *fore // Remove it from the forest and notify its relatives r.enqueueAffected(log, "relative of deleted namespace", ns.RelativesNames()...) + // Enqueue the HNS if the self-serve subnamespace is deleted. + if r.HNSReconcilerEnabled { + if ns.RequiredChildOf != "" { + r.hnsr.enqueue(log, ns.Name(), ns.RequiredChildOf, "hns for the deleted self-serve subnamespace") + } + } ns.UnsetExists() log.Info("Removed namespace") return true @@ -227,10 +235,17 @@ func (r *HierarchyConfigReconciler) syncRequiredChildOf(log logr.Logger, inst *a inst.Spec.Parent = ns.RequiredChildOf case ns.RequiredChildOf: // ok + if r.HNSReconcilerEnabled { + // Enqueue HNS to update the state to "Ok". + r.hnsr.enqueue(log, ns.Name(), ns.RequiredChildOf, "the HNS state should be updated to ok") + } default: if r.HNSReconcilerEnabled { - // TODO report the conflict in hierarchicalnamespace.Status.State and enqueue the owner namespace. - // See issue: https://github.com/kubernetes-sigs/multi-tenancy/issues/487 + // Enqueue the HNS to report the conflict in hierarchicalnamespace.Status.State and enqueue the + // owner namespace to report the "SubnamespaceConflict" condition. + log.Info("Self-serve subnamespace: conflict with parent", "owner", ns.RequiredChildOf, "parent", inst.Spec.Parent) + r.hnsr.enqueue(log, ns.Name(), ns.RequiredChildOf, "self-serve subnamespace has a parent but it's not the owner") + r.enqueueAffected(log, "required child already has a parent", ns.RequiredChildOf) } else { // This should never happen unless there's some crazy race condition. log.Info("Required subnamespace: conflict with existing parent", "requiredChildOf", ns.RequiredChildOf, "actual", inst.Spec.Parent) @@ -311,8 +326,14 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier // Update the most recent list of children from the forest inst.Status.Children = ns.ChildNames() - // We'll reset any of these conditions if they occur. - ns.ClearAllConditions(api.RequiredChildConflict) + // Currently when HNS reconciler is not enabled, the "SubnamespaceConflict" condition is cleared and + // updated when a parent namespace syncChildren(). + // TODO report the "SubnamespaceConflict" in the HNS reconciliation. See issue: + // https://github.com/kubernetes-sigs/multi-tenancy/issues/490 + if !r.HNSReconcilerEnabled { + // We'll reset any of these conditions if they occur. + ns.ClearAllConditions(api.SubnamespaceConflict) + } // Make a set to make it easy to look up if a child is required or not isRequired := map[string]bool{} @@ -320,7 +341,7 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier // TODO Remove the spec.requiredChildren field when the hns reconciler is in use. // See issue: https://github.com/kubernetes-sigs/multi-tenancy/issues/457 // TODO Sync the hns instances in HNS reconciler, instead of syncing the spec.requiredChildren - // here. Replace hc "RequiredChildConflict" condition with hns "conflict" state. See issue: + // here. Update hns states in hns reconciliation. See issue: // https://github.com/kubernetes-sigs/multi-tenancy/issues/487 // Use the old spec.requiredChildren field to get the list of the self-serve subnamespaces. if !r.HNSReconcilerEnabled { @@ -343,49 +364,59 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier // The forest will be reset to the correct value, below. log.Error(forest.OutOfSync, "While syncing children", "child", cn, "requiredChildOf", cns.RequiredChildOf) r.enqueueAffected(log, "forest out-of-sync: requiredChildOf != parent", cns.RequiredChildOf) + if r.HNSReconcilerEnabled { + r.hnsr.enqueue(log, cn, cns.RequiredChildOf, "forest out-of-sync: owner != parent") + } } - if isRequired[cn] { - // This child is actually required. Remove it from the set so we know we found it. Also, the - // forest is almost certainly already in sync, but just set it again in case something went - // wrong (eg, the error shown above). - delete(isRequired, cn) - cns.RequiredChildOf = ns.Name() + if r.HNSReconcilerEnabled { + if isRequired[cn] { + delete(isRequired, cn) + } } else { - // This isn't a required child, but it looks like it *used* to be a required child of this - // namespace. Clear the RequiredChildOf field from the forest to bring our state in line with - // what's on the apiserver. - cns.RequiredChildOf = "" + if isRequired[cn] { + // This child is actually required. Remove it from the set so we know we found it. Also, the + // forest is almost certainly already in sync, but just set it again in case something went + // wrong (eg, the error shown above). + delete(isRequired, cn) + cns.RequiredChildOf = ns.Name() + } else { + // This isn't a required child, but it looks like it *used* to be a required child of this + // namespace. Clear the RequiredChildOf field from the forest to bring our state in line with + // what's on the apiserver. + cns.RequiredChildOf = "" + } } } - // Anything that's still in isRequired at this point is a required child according to our own - // spec, but it doesn't exist as a child of this namespace. There could be one of two reasons: - // either the namespace hasn't been created (yet), in which case we just need to enqueue it for - // reconciliation, or else it *also* is claimed by another namespace. - for cn := range isRequired { - log.Info("Required child is missing", "child", cn) - cns := r.Forest.Get(cn) - - // We'll always set a condition just in case the required namespace can't be created/configured, - // but if all is working well, the condition will be resolved as soon as the child namespace is - // configured correctly (see the call to ClearAllConditions, above, in this function). This is - // the default message, we'll override it if the namespace doesn't exist. - msg := fmt.Sprintf("required subnamespace %s exists but cannot be set as a child of this namespace", cn) - - // If this child isn't claimed by another parent, claim it and make sure it gets reconciled - // (which is when it will be created). - if cns.Parent() == nil && (cns.RequiredChildOf == "" || cns.RequiredChildOf == ns.Name()) { - cns.RequiredChildOf = ns.Name() - r.enqueueAffected(log, "required child is missing", cn) - if !cns.Exists() { - msg = fmt.Sprintf("required subnamespace %s does not exist", cn) - } - } else { - // TODO Sync the hns instances in HNS reconciler, instead of syncing the spec.requiredChildren - // here. Replace hc "RequiredChildConflict" condition with hns "conflict" state. See issue: - // https://github.com/kubernetes-sigs/multi-tenancy/issues/487 - if !r.HNSReconcilerEnabled { + if r.HNSReconcilerEnabled { + for cn := range isRequired { + r.hnsr.enqueue(log, cn, ns.Name(), "parent of the self-serve subnamespace is not the owner") + } + } else { + // Anything that's still in isRequired at this point is a required child according to our own + // spec, but it doesn't exist as a child of this namespace. There could be one of two reasons: + // either the namespace hasn't been created (yet), in which case we just need to enqueue it for + // reconciliation, or else it *also* is claimed by another namespace. + for cn := range isRequired { + log.Info("Required child is missing", "child", cn) + cns := r.Forest.Get(cn) + + // We'll always set a condition just in case the required namespace can't be created/configured, + // but if all is working well, the condition will be resolved as soon as the child namespace is + // configured correctly (see the call to ClearAllConditions, above, in this function). This is + // the default message, we'll override it if the namespace doesn't exist. + msg := fmt.Sprintf("required subnamespace %s exists but cannot be set as a child of this namespace", cn) + + // If this child isn't claimed by another parent, claim it and make sure it gets reconciled + // (which is when it will be created). + if cns.Parent() == nil && (cns.RequiredChildOf == "" || cns.RequiredChildOf == ns.Name()) { + cns.RequiredChildOf = ns.Name() + r.enqueueAffected(log, "required child is missing", cn) + if !cns.Exists() { + msg = fmt.Sprintf("required subnamespace %s does not exist", cn) + } + } else { // Someone else got it first. This should never happen if the validator is working correctly. other := cns.RequiredChildOf if other == "" { @@ -393,15 +424,10 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier } log.Info("Required child is already owned/claimed by another parent", "child", cn, "otherParent", other) } - } - // TODO Sync the hns instances in HNS reconciler, instead of syncing the spec.requiredChildren - // here. Replace hc "RequiredChildConflict" condition with hns "conflict" state. See issue: - // https://github.com/kubernetes-sigs/multi-tenancy/issues/487 - if !r.HNSReconcilerEnabled { // Set the condition that the required child isn't an actual child. As mentioned above, if we // just need to create it, this condition will be removed shortly. - ns.SetCondition(cn, api.RequiredChildConflict, msg) + ns.SetCondition(cn, api.SubnamespaceConflict, msg) } } } @@ -468,7 +494,7 @@ func (r *HierarchyConfigReconciler) enqueueAffected(log logr.Logger, reason stri }() } -func (r *HierarchyConfigReconciler) getInstances(ctx context.Context, log logr.Logger, nm string) (inst *api.HierarchyConfiguration, ns *corev1.Namespace, err error) { +func (r *HierarchyConfigReconciler) GetInstances(ctx context.Context, log logr.Logger, nm string) (inst *api.HierarchyConfiguration, ns *corev1.Namespace, err error) { inst, err = r.getSingleton(ctx, nm) if err != nil { log.Error(err, "Couldn't read singleton") diff --git a/incubator/hnc/pkg/reconcilers/hierarchy_config_test.go b/incubator/hnc/pkg/reconcilers/hierarchy_config_test.go index 4f2bb3da1..8cae95bcf 100644 --- a/incubator/hnc/pkg/reconcilers/hierarchy_config_test.go +++ b/incubator/hnc/pkg/reconcilers/hierarchy_config_test.go @@ -154,7 +154,7 @@ var _ = Describe("Hierarchy", func() { }).Should(Equal([]string{barName})) }) - It("should set RequiredChildConflict condition if a required child cannot be set", func() { + It("should set SubnamespaceConflict condition if a required child cannot be set", func() { if enableHNSReconciler { return } @@ -177,13 +177,13 @@ var _ = Describe("Hierarchy", func() { Eventually(hasCondition(ctx, fooName, "")).Should(Equal(false)) Eventually(hasCondition(ctx, bazName, "")).Should(Equal(false)) want := &api.Condition{ - Code: api.RequiredChildConflict, + Code: api.SubnamespaceConflict, Affects: []api.AffectedObject{{Version: "v1", Kind: "Namespace", Name: bazName}}, } - Eventually(getCondition(ctx, barName, api.RequiredChildConflict)).Should(Equal(want)) + Eventually(getCondition(ctx, barName, api.SubnamespaceConflict)).Should(Equal(want)) }) - It("should clear RequiredChildConflict condition if the parent removes the required child", func() { + It("should clear SubnamespaceConflict condition if the parent removes the required child", func() { if enableHNSReconciler { return } diff --git a/incubator/hnc/pkg/reconcilers/object.go b/incubator/hnc/pkg/reconcilers/object.go index c6b245800..6df46aba2 100644 --- a/incubator/hnc/pkg/reconcilers/object.go +++ b/incubator/hnc/pkg/reconcilers/object.go @@ -97,7 +97,7 @@ func (r *ObjectReconciler) GetGVK() schema.GroupVersionKind { } func (r *ObjectReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { - if ex[req.Namespace] { + if EX[req.Namespace] { return ctrl.Result{}, nil } diff --git a/incubator/hnc/pkg/reconcilers/setup.go b/incubator/hnc/pkg/reconcilers/setup.go index 2f0a37436..29b80250e 100644 --- a/incubator/hnc/pkg/reconcilers/setup.go +++ b/incubator/hnc/pkg/reconcilers/setup.go @@ -9,10 +9,10 @@ import ( "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/forest" ) -// The ex map is used by reconcilers to exclude namespaces to reconcile. We explicitly +// The EX map is used by reconcilers to exclude namespaces to reconcile. We explicitly // exclude some default namespaces with constantly changing objects. // TODO make the exclusion configurable - https://github.com/kubernetes-sigs/multi-tenancy/issues/374 -var ex = map[string]bool{ +var EX = map[string]bool{ "kube-system": true, "hnc-system": true, "cert-manager": true, @@ -23,6 +23,7 @@ var ex = map[string]bool{ // This function is called both from main.go as well as from the integ tests. func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int, enableHNSReconciler bool) error { hcChan := make(chan event.GenericEvent) + hnsChan := make(chan event.GenericEvent) // Create different reconcilers based on if the enableHNSReconciler flag is set or not. if enableHNSReconciler { @@ -34,16 +35,20 @@ func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int, enableHNSReco Affected: hcChan, HNSReconcilerEnabled: true, } - if err := hcr.SetupWithManager(mgr, maxReconciles); err != nil { - return fmt.Errorf("cannot create Hierarchy reconciler: %s", err.Error()) - } // Create HierarchicalNamespaceReconciler. hnsr := &HierarchicalNamespaceReconciler{ - Client: mgr.GetClient(), - Log: ctrl.Log.WithName("reconcilers").WithName("HierarchicalNamespace"), - forest: f, - hcr: hcr, + Client: mgr.GetClient(), + Log: ctrl.Log.WithName("reconcilers").WithName("HierarchicalNamespace"), + forest: f, + hcr: hcr, + Affected: hnsChan, + } + + // Set hcr.hnsr after the HNS reconciler is created. + hcr.hnsr = hnsr + if err := hcr.SetupWithManager(mgr, maxReconciles); err != nil { + return fmt.Errorf("cannot create Hierarchy reconciler: %s", err.Error()) } if err := hnsr.SetupWithManager(mgr); err != nil { return fmt.Errorf("cannot create HierarchicalNamespace reconciler: %s", err.Error())