Refactor nameref for readability.

api/filters/nameref/nameref.go

@@ -69,9 +69,8 @@ func (f Filter) run(node *yaml.RNode) (*yaml.RNode, error) {
 	return node, nil
 }
 
-// This function is called at many nodes in the YAML doc tree.
+// This function is called on the node found at FieldSpec.Path.
-// Only on first entry can one expect the argument to match the
+// It's some node in the Referrer.
-// top-level node backing the Referrer.
 func (f Filter) set(node *yaml.RNode) error {
 	if yaml.IsMissingOrNull(node) {
 		return nil
@@ -91,7 +90,12 @@ func (f Filter) set(node *yaml.RNode) error {
 	}
 }
 
-// Replace name field within a map RNode and leverage the namespace field.
+// This method used when NameFieldToUpdate doesn't lead to
+// one scalar field (typically called 'name'), but rather
+// leads to a map field (called anything). In this case we
+// must complete the field path, looking for both  a 'name'
+// and a 'namespace' field to help select the proper
+// ReferralTarget to read the name and namespace from.
 func (f Filter) setMapping(node *yaml.RNode) error {
 	if node.YNode().Kind != yaml.MappingNode {
 		return fmt.Errorf("expect a mapping node")
@@ -101,27 +105,16 @@ func (f Filter) setMapping(node *yaml.RNode) error {
 		return errors.Wrap(err, "trying to match 'name' field")
 	}
 	if nameNode == nil {
+		// This is a _configuration_ error; the field path
+		// specified in NameFieldToUpdate.Path doesn't resolve
+		// to a map with a 'name' field, so we have no idea what
+		// field to update with a new name.
 		return fmt.Errorf("path config error; no 'name' field in node")
 	}
-	namespaceNode, err := node.Pipe(yaml.FieldMatcher{Name: "namespace"})
+	candidates, err := f.filterMapCandidatesByNamespace(node)
 	if err != nil {
-		return errors.Wrap(err, "trying to match 'namespace' field")
+		return err
 	}
-
-	// name will not be updated if the namespace doesn't match
-	candidates := f.ReferralCandidates.Resources()
-	if namespaceNode != nil {
-		namespace := namespaceNode.YNode().Value
-		bynamespace := f.ReferralCandidates.GroupedByOriginalNamespace()
-		if _, ok := bynamespace[namespace]; !ok {
-			bynamespace = f.ReferralCandidates.GroupedByCurrentNamespace()
-			if _, ok := bynamespace[namespace]; !ok {
-				return nil
-			}
-		}
-		candidates = bynamespace[namespace]
-	}
-
 	oldName := nameNode.YNode().Value
 	referral, err := f.selectReferral(oldName, candidates)
 	if err != nil || referral == nil {
@@ -133,23 +126,42 @@ func (f Filter) setMapping(node *yaml.RNode) error {
 		// The name has not changed, nothing to do.
 		return nil
 	}
-	err = node.PipeE(yaml.FieldSetter{
+	if err = node.PipeE(yaml.FieldSetter{
 		Name:        "name",
 		StringValue: referral.GetName(),
-	})
+	}); err != nil {
-	if err != nil {
 		return err
 	}
-	if referral.GetNamespace() != "" {
+	if referral.GetNamespace() == "" {
-		// We don't want value "" to replace value "default" since
+		// Don't write an empty string into the namespace field, as
-		// the empty string is handled as a wild card here not default namespace
+		// it should not replace the value "default".  The empty
-		// by kubernetes.
+		// string is handled as a wild card here, not as an implicit
-		err = node.PipeE(yaml.FieldSetter{
+		// specification of the "default" k8s namespace.
+		return nil
+	}
+	return node.PipeE(yaml.FieldSetter{
 		Name:        "namespace",
 		StringValue: referral.GetNamespace(),
 	})
 }
-	return err
+
+func (f Filter) filterMapCandidatesByNamespace(
+	node *yaml.RNode) ([]*resource.Resource, error) {
+	namespaceNode, err := node.Pipe(yaml.FieldMatcher{Name: "namespace"})
+	if err != nil {
+		return nil, errors.Wrap(err, "trying to match 'namespace' field")
+	}
+	if namespaceNode == nil {
+		return f.ReferralCandidates.Resources(), nil
+	}
+	namespace := namespaceNode.YNode().Value
+	nsMap := f.ReferralCandidates.GroupedByOriginalNamespace()
+	if candidates, ok := nsMap[namespace]; ok {
+		return candidates, nil
+	}
+	nsMap = f.ReferralCandidates.GroupedByCurrentNamespace()
+	// This could be nil, or an empty list.
+	return nsMap[namespace], nil
 }
 
 func (f Filter) setScalar(node *yaml.RNode) error {
@@ -172,10 +184,6 @@ func (f Filter) recordTheReferral(referral *resource.Resource) {
 	referral.AppendRefBy(f.Referrer.CurId())
 }
 
-func (f Filter) isRoleRef() bool {
-	return strings.HasSuffix(f.NameFieldToUpdate.Path, "roleRef/name")
-}
-
 // getRoleRefGvk returns a Gvk in the roleRef field. Return error
 // if the roleRef, roleRef/apiGroup or roleRef/kind is missing.
 func getRoleRefGvk(res json.Marshaler) (*resid.Gvk, error) {
@@ -212,75 +220,136 @@ func getRoleRefGvk(res json.Marshaler) (*resid.Gvk, error) {
 	}, nil
 }
 
-func (f Filter) filterReferralCandidates(
+// sieveFunc returns true if the resource argument satisfies some criteria.
-	matches []*resource.Resource) []*resource.Resource {
+type sieveFunc func(*resource.Resource) bool
-	var ret []*resource.Resource
+
-	for _, m := range matches {
+// doSieve uses a function to accept or ignore resources from a list.
-		// If target kind is not ServiceAccount, we shouldn't consider condidates which
+// If list is nil, returns immediately.
-		// doesn't have same namespace.
+// It's a filter obviously, but that term is overloaded here.
-		if f.ReferralTarget.Kind != "ServiceAccount" &&
+func doSieve(list []*resource.Resource, fn sieveFunc) (s []*resource.Resource) {
-			m.GetNamespace() != f.Referrer.GetNamespace() {
+	for _, r := range list {
-			continue
+		if fn(r) {
+			s = append(s, r)
 		}
-		if !f.Referrer.PrefixesSuffixesEquals(m) {
-			continue
 	}
-		ret = append(ret, m)
+	return
-	}
-	return ret
 }
 
-// selectReferral picks the referral among a subset of candidates.
+func acceptAll(r *resource.Resource) bool {
-// The content of the candidateSubset slice is most of the time
+	return true
-// identical to the ReferralCandidates ResMap. Still in some cases, such
+}
-// as ClusterRoleBinding, the subset only contains the resources of a specific
+
-// namespace.
+func originalNameMatches(name string) sieveFunc {
-func (f Filter) selectReferral(
+	return func(r *resource.Resource) bool {
-	oldName string,
+		return r.GetOriginalName() == name
-	referralCandidates []*resource.Resource) (*resource.Resource, error) {
+	}
-	var roleRefGvk *resid.Gvk
+}
-	if f.isRoleRef() {
+
-		var err error
+func originalIdSelectedByGvk(gvk *resid.Gvk) sieveFunc {
-		roleRefGvk, err = getRoleRefGvk(f.Referrer)
+	return func(r *resource.Resource) bool {
+		return r.OrgId().IsSelected(gvk)
+	}
+}
+
+// If the we are updating a 'roleRef/name' field, the 'apiGroup' and 'kind'
+// fields in the same 'roleRef' map must be considered.
+// If either object is cluster-scoped (!IsNamespaceableKind), there
+// can be a referral.
+// E.g. a RoleBinding (which exists in a namespace) can refer
+// to a ClusterRole (cluster-scoped) object.
+// https://kubernetes.io/docs/reference/access-authn-authz/rbac/#role-and-clusterrole
+// Likewise, a ClusterRole can refer to a Secret (in a namespace).
+// Objects in different namespaces generally cannot refer to other
+// with some exceptions (e.g. RoleBinding and ServiceAccount are both
+// namespaceable, but the former can refer to accounts in other namespaces).
+func (f Filter) roleRefFilter() sieveFunc {
+	if !strings.HasSuffix(f.NameFieldToUpdate.Path, "roleRef/name") {
+		return acceptAll
+	}
+	roleRefGvk, err := getRoleRefGvk(f.Referrer)
 	if err != nil {
-			return nil, err
+		return acceptAll
+	}
+	return func(r *resource.Resource) bool {
+		return r.OrgId().IsSelected(roleRefGvk)
 	}
 }
-	for _, candidate := range referralCandidates {
+
-		if candidate.GetOriginalName() != oldName {
+func prefixSuffixEquals(other resource.ResCtx) sieveFunc {
-			continue
+	return func(r *resource.Resource) bool {
-		}
+		return r.PrefixesSuffixesEquals(other)
-		id := candidate.OrgId()
-		if !id.IsSelected(&f.ReferralTarget) {
-			continue
-		}
-		// If the we are processing a roleRef, the apiGroup and Kind in the
-		// roleRef are needed to be considered.
-		if f.isRoleRef() && !id.IsSelected(roleRefGvk) {
-			continue
-		}
-		matches := f.ReferralCandidates.GetMatchingResourcesByOriginalId(id.Equals)
-		// If there's more than one match,
-		// filter the matches by prefix and suffix
-		if len(matches) > 1 {
-			filteredMatches := f.filterReferralCandidates(matches)
-			if len(filteredMatches) > 1 {
-				return nil, fmt.Errorf(
-					"cannot fix name in '%s' field of referrer '%s';"+
-						" found multiple possible referrals: %v",
-					f.NameFieldToUpdate.Path,
-					f.Referrer.CurId(),
-					getIds(filteredMatches))
-			}
-			// Check is the match the resource we are working on
-			if len(filteredMatches) == 0 || candidate != filteredMatches[0] {
-				continue
 	}
 }
-		return candidate, nil
+
+func (f Filter) sameCurrentNamespaceAsReferrer() sieveFunc {
+	referrerCurId := f.Referrer.CurId()
+	if !referrerCurId.IsNamespaceableKind() {
+		// If the referrer is cluster-scoped, let anything through.
+		return acceptAll
 	}
+	return func(r *resource.Resource) bool {
+		if !r.CurId().IsNamespaceableKind() {
+			// Allow cluster-scoped through.
+			return true
+		}
+		if r.GetKind() == "ServiceAccount" {
+			// Allow service accounts through, even though they
+			// are in a namespace.  A RoleBinding in another namespace
+			// can reference them.
+			return true
+		}
+		return referrerCurId.IsNsEquals(r.CurId())
+	}
+}
+
+// selectReferral picks the best referral from a list of candidates.
+func (f Filter) selectReferral(
+	// The name referral that may need to be updated.
+	oldName string,
+	candidates []*resource.Resource) (*resource.Resource, error) {
+	candidates = doSieve(candidates, originalNameMatches(oldName))
+	candidates = doSieve(candidates, originalIdSelectedByGvk(&f.ReferralTarget))
+	candidates = doSieve(candidates, f.roleRefFilter())
+	candidates = doSieve(candidates, f.sameCurrentNamespaceAsReferrer())
+	if len(candidates) == 1 {
+		return candidates[0], nil
+	}
+	candidates = doSieve(candidates, prefixSuffixEquals(f.Referrer))
+	if len(candidates) == 1 {
+		return candidates[0], nil
+	}
+	if len(candidates) == 0 {
 		return nil, nil
 	}
+	if allNamesAreTheSame(candidates) {
+		// Just take the first one.
+		return candidates[0], nil
+	}
+	ids := getIds(candidates)
+	f.failureDetails(candidates)
+	return nil, fmt.Errorf(" found multiple possible referrals: %s", ids)
+}
+
+func (f Filter) failureDetails(resources []*resource.Resource) {
+	fmt.Printf(
+		"\n**** Too many possible referral targets to referrer:\n%s\n",
+		f.Referrer.MustYaml())
+	for i, r := range resources {
+		fmt.Printf(
+			"--- possible referral %d:\n%s", i, r.MustYaml())
+		fmt.Println("------")
+	}
+}
+
+func allNamesAreTheSame(resources []*resource.Resource) bool {
+	name := resources[0].GetName()
+	for i := 1; i < len(resources); i++ {
+		if name != resources[i].GetName() {
+			return false
+		}
+	}
+	return true
+}
 
 func getIds(rs []*resource.Resource) string {
 	var result []string