diff --git a/incubator/hnc/api/v1alpha1/hierarchy_types.go b/incubator/hnc/api/v1alpha1/hierarchy_types.go index 8b96797d7..67f37c48b 100644 --- a/incubator/hnc/api/v1alpha1/hierarchy_types.go +++ b/incubator/hnc/api/v1alpha1/hierarchy_types.go @@ -33,12 +33,12 @@ const ( // Condition codes. *All* codes must also be documented in the comment to Condition.Code. 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..73d99f639 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. + 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 in-momery hierarchyConfig instance of the hierarchical namespace. - // The hierarchyConfig reconciler will create the namespace and hierarchyConfig - // instances on apiserver accordingly. + // Enqueue the not-yet existent self-serve subnamespace. The HierarchyConfig reconciler will + // create the namespace and the 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 + + 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 - // TODO sync with forest to report conflicts in hns.Status.State. See issue: - // https://github.com/kubernetes-sigs/multi-tenancy/issues/487 + reason := "new/updated hierarchical namespace" + r.hcr.enqueueAffected(log, reason, 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, "otherParent", 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..1b4872555 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 set the hns.status.state to Ok if the subnamespace is created succussfully", 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())