Remove obsolete validation TODOs

I added some TODOs when improving the error messages from webhook validation. Most of these were suggesting to review, and eventually change, the status code returned. I realize now that this is probably not feasible, and apologize for adding the TODOs in the first place.

Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully.

Author: erikgb

internal/anchor/validator.go

@@ -97,13 +97,11 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 		// Can't create subnamespaces in unmanaged namespaces
 		if why := config.WhyUnmanaged(pnm); why != "" {
 			err := fmt.Errorf("cannot create a subnamespace in the unmanaged namespace %q (%s)", pnm, why)
-			// TODO(erikgb): Add to list of Invalid field errors?
 			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, pnm, err)
 		}
 		// Can't create subnamespaces using unmanaged namespace names
 		if why := config.WhyUnmanaged(cnm); why != "" {
 			err := fmt.Errorf("cannot create a subnamespace using the unmanaged namespace name %q (%s)", cnm, why)
-			// TODO(erikgb): Add to list of Invalid field errors?
 			return webhooks.DenyForbidden(api.SubnamespaceAnchorGR, cnm, err)
 		}
 
@@ -113,7 +111,6 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 			childIsMissingAnchor := (cns.Parent().Name() == pnm && cns.IsSub)
 			if !childIsMissingAnchor {
 				err := errors.New("cannot create a subnamespace using an existing namespace")
-				// TODO(erikgb): Add to list of Invalid field errors?
 				return webhooks.DenyConflict(api.SubnamespaceAnchorGR, cnm, err)
 			}
 		}

internal/hierarchyconfig/validator.go

@@ -123,12 +123,10 @@ func (v *Validator) handle(ctx context.Context, log logr.Logger, req *request) a
 
 	if why := config.WhyUnmanaged(req.hc.Namespace); why != "" {
 		err := fmt.Errorf("namespace %q is not managed by HNC (%s) and cannot be set as a child of another namespace", req.hc.Namespace, why)
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 	if why := config.WhyUnmanaged(req.hc.Spec.Parent); why != "" {
 		err := fmt.Errorf("namespace %q is not managed by HNC (%s) and cannot be set as the parent of another namespace", req.hc.Spec.Parent, why)
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 
@@ -190,7 +188,6 @@ func (v *Validator) checkNS(ns *forest.Namespace) admission.Response {
 	haltedRoot := ns.GetHaltedRoot()
 	if haltedRoot != "" && haltedRoot != ns.Name() {
 		err := fmt.Errorf("ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration", haltedRoot, ns.Name())
-		// TODO(erikgb): InternalError better?
 		return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 
@@ -201,7 +198,6 @@ func (v *Validator) checkNS(ns *forest.Namespace) admission.Response {
 func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) admission.Response {
 	if ns.IsExternal() && newParent != nil {
 		err := fmt.Errorf("namespace %q is managed by %q, not HNC, so it cannot have a parent in HNC", ns.Name(), ns.Manager)
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 
@@ -212,14 +208,12 @@ func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) admi
 	// Prevent changing parent of a subnamespace
 	if ns.IsSub {
 		err := fmt.Errorf("illegal parent: Cannot set the parent of %q to %q because it's a subnamespace of %q", ns.Name(), newParent.Name(), curParent.Name())
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyConflict(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 
 	// non existence of parent namespace -> not allowed
 	if newParent != nil && !newParent.Exists() {
 		err := fmt.Errorf("requested parent %q does not exist", newParent.Name())
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyForbidden(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 
@@ -230,7 +224,6 @@ func (v *Validator) checkParent(ns, curParent, newParent *forest.Namespace) admi
 	// parent conflicts with something in the _existing_ hierarchy.
 	if reason := ns.CanSetParent(newParent); reason != "" {
 		err := fmt.Errorf("illegal parent: %s", reason)
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyConflict(api.HierarchyConfigurationGR, api.Singleton, err)
 	}
 

internal/hncconfig/validator.go

@@ -49,7 +49,6 @@ func (v *Validator) Handle(ctx context.Context, req admission.Request) admission
 	if req.Operation == k8sadm.Delete {
 		if req.Name == api.HNCConfigSingleton {
 			err := errors.New("deleting the 'config' object is forbidden")
-			// TODO(erikgb): MethodNotSupported error better?
 			return webhooks.DenyForbidden(api.HNCConfigurationGR, api.HNCConfigSingleton, err)
 		} else {
 			// We allow deleting other objects. We should never enter this case with the CRD validation. We introduced

internal/namespace/validator.go

@@ -136,7 +136,6 @@ func (v *Validator) illegalTreeLabel(req *nsRequest) admission.Response {
 		// Check if new HNC label tree key isn't being added
 		if oldLabels[key] != val {
 			err := fmt.Errorf("cannot set or modify tree label %q in namespace %q; these can only be managed by HNC", key, req.ns.Name)
-			// TODO(erikgb): Invalid field error list better?
 			return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err)
 		}
 	}
@@ -146,7 +145,6 @@ func (v *Validator) illegalTreeLabel(req *nsRequest) admission.Response {
 		if strings.Contains(key, api.LabelTreeDepthSuffix) {
 			if _, ok := req.ns.Labels[key]; !ok {
 				err := fmt.Errorf("cannot remove tree label %q in namespace %q; these can only be managed by HNC", key, req.ns.Name)
-				// TODO(erikgb): Invalid field error list better?
 				return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err)
 			}
 		}
@@ -175,7 +173,6 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp
 		err := fmt.Errorf("you cannot enforce webhook rules on this unmanaged namespace using the %q label. "+
 			"See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label "+
 			"for detail", api.LabelIncludedNamespace)
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err)
 	}
 
@@ -187,7 +184,6 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp
 		err := fmt.Errorf("you cannot change the value of the %q label. It has to be set as true on all managed namespaces. "+
 			"See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label "+
 			"for detail", api.LabelIncludedNamespace)
-		// TODO(erikgb): Invalid field error better?
 		return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err)
 	}
 
@@ -220,7 +216,6 @@ func (v *Validator) conflictBetweenParentAndExternalManager(req *nsRequest, ns *
 	if mgr != "" && mgr != api.MetaGroup && ns.Parent() != nil {
 		err := fmt.Errorf("is a child of %q. Namespaces with parents defined by HNC cannot also be managed externally. "+
 			"To manage this namespace with %q, first make it a root in HNC", ns.Parent().Name(), mgr)
-		// TODO(erikgb): Conflict error better?
 		return webhooks.DenyForbidden(namespaceGR, req.ns.Name, err)
 	}
 	return webhooks.Allow("")