Index metadata.name for cluster-name label

[vincepri]

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:

#728 introduced a bug where the metadata.name field cannot be queried because the client uses a cached-index implementation to retrieve results. This bug prevents providers to successfully spin up clusters when using the Cluster resource.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
The branch has been successfully tested on the AWS and vSphere providers.

Release note:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vincepri]

/assign @detiber

[detiber]

Does it matter that IndexerFunc is meant to be unique across namespaces?

cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go

Lines 91 to 93 in 0617f27

	// IndexerFunc knows how to take an object and turn it into a series
	// of (non-namespaced) keys for that object.
	type IndexerFunc func(runtime.Object) []string

[vincepri]

I believe that's taken care of in this wrapper

cluster-api/vendor/sigs.k8s.io/controller-runtime/pkg/cache/informer_cache.go

Lines 133 to 178 in 0617f27

	func (ip *informerCache) IndexField(obj runtime.Object, field string, extractValue client.IndexerFunc) error {
	informer, err := ip.GetInformer(obj)
	if err != nil {
	return err
	}
	return indexByField(informer.GetIndexer(), field, extractValue)
	}
	func indexByField(indexer cache.Indexer, field string, extractor client.IndexerFunc) error {
	indexFunc := func(objRaw interface{}) ([]string, error) {
	// TODO(directxman12): check if this is the correct type?
	obj, isObj := objRaw.(runtime.Object)
	if !isObj {
	return nil, fmt.Errorf("object of type %T is not an Object", objRaw)
	}
	meta, err := apimeta.Accessor(obj)
	if err != nil {
	return nil, err
	}
	ns := meta.GetNamespace()
	rawVals := extractor(obj)
	var vals []string
	if ns == "" {
	// if we're not doubling the keys for the namespaced case, just re-use what was returned to us
	vals = rawVals
	} else {
	// if we need to add non-namespaced versions too, double the length
	vals = make([]string, len(rawVals)*2)
	}
	for i, rawVal := range rawVals {
	// save a namespaced variant, so that we can ask
	// "what are all the object matching a given index *in a given namespace*"
	vals[i] = internal.KeyToNamespacedKey(ns, rawVal)
	if ns != "" {
	// if we have a namespace, also inject a special index key for listing
	// regardless of the object namespace
	vals[i+len(rawVals)] = internal.KeyToNamespacedKey("", rawVal)
	}
	}
	return vals, nil
	}
	return indexer.AddIndexers(cache.Indexers{internal.FieldIndexName(field): indexFunc})
	}

[detiber]

@vincepri From the looks of it, that field indexes both the namespaced and non-namespaced keys for each value, so wouldn't there be a collision for the non-namespaced value of two clusters with the same name in different namespaces?

[davidewatson]

Modulo @detiber's comments, plus one from me. I'd like if the Cluster object were the user interface for managing clusters.

[figo]

there is another option to filter cluster object by using LabelSelector,
it does require adding a new label to cluster object.

// ListOptions contains options for limitting or filtering results.
// It's generally a subset of metav1.ListOptions, with support for
// pre-parsed selectors (since generally, selectors will be executed
// against the cache).
type ListOptions struct {
	// LabelSelector filters results by label.  Use SetLabelSelector to
	// set from raw string form.
	LabelSelector labels.Selector
	// FieldSelector filters results by a particular field.  In order
	// to use this with cache-based implementations, restrict usage to
	// a single field-value pair that's been added to the indexers.
	FieldSelector fields.Selector

have you considered this?
i guess my point is to avoid this extra indexing logic.

[vincepri]

The problem that I see using labels is that it doesn't guarantee uniqueness and if we duplicate the name there we'd need to keep it in sync, which isn't ideal

[figo]

/lgtm

[k8s-ci-robot]

@figo: changing LGTM is restricted to assignees, and only kubernetes-sigs/cluster-api repo collaborators may be assigned issues.

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[detiber]

@figo since you are already a member of the kubernetes org, you can request to be added to kubernetes-sigs as well, which will give you the ability to be assigned issues in this repo: https://github.com/kubernetes/community/blob/master/community-membership.md#kubernetes-ecosystem

[detiber]

/lgtm