Cluster-api: Commas not accepted in labels.

[maelk]

What steps did you take and what happened:
I tried to set the contract label for two versions according to https://cluster-api.sigs.k8s.io/developer/providers/v1alpha2-to-v1alpha3.html#apply-the-contract-version-label-clusterx-k8sioversion-version1version2version3-to-your-crds and got the following error :

Error from server (Invalid): error when creating "examples/_out/provider-components.yaml": CustomResourceDefinition.apiextensions.k8s.io "metal3clusters.infrastructure.cluster.x-k8s.io" is invalid: metadata.labels: Invalid value: "v1alpha3,v1alpha4": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue',  or 'my_value',  or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?')

The separator needs to be changed here https://github.com/kubernetes-sigs/cluster-api/blob/master/util/conversion/conversion.go#L57 and in the contract (inc. book)

What did you expect to happen:
The separator should be one of [ ".", "_", "-"]

/assign

/kind bug

[vincepri]

/milestone v0.3.3
/priority important-soon
/area api

[maelk]

@vincepri How would this work in the case of multi-tenancy ? Let's say that the provider v1alpha3 and v1alpha4 fulfill the contract. Clusterctl will deploy it so that the CRD list both, v1alpha4 is the storage version. Now if clusterctl is used to deploy a v1alpha3 provider in a separate namespace, we would hit an issue, since https://github.com/kubernetes-sigs/cluster-api/blob/master/util/conversion/conversion.go#L57 would return v1alpha4, while the provider works with v1alpha3, hence https://github.com/kubernetes-sigs/cluster-api/blob/master/util/util.go#L310 would be called with a v1alpha3 schema. So they will never match, and the infra object will never be reconciled when the machine object is updated. Is this right ? Should we fix this too at the same time ?

[maelk]

A solution could be to match on group kind and not version in https://github.com/kubernetes-sigs/cluster-api/blob/master/util/util.go#L318

[vincepri]

while the provider works with v1alpha3

which provider are you talking about here?

[vincepri]

For the map function you linked (https://github.com/kubernetes-sigs/cluster-api/blob/master/util/util.go#L310), this should only be looking at the groupkind, that's correct. Let's fix this in a separate PR though with a ⚠️ title so we can put it in the release notes

[maelk]

ok! @jan-est will take care of the actual implementation though

[bboreham]

Forgive my lack of background on this point, but I wonder if it suggests that this concept does not map onto a label?
Labels are indexed, so you can use them in selectors. I don't see how a multi-valued field could work that way. Is it more like an annotation?

[jan-est]

I open first PR for fixing the separator problem. I will fix the other issue with separate PR tomorrow.

[JoelSpeed]

Labels are indexed, so you can use them in selectors. I don't see how a multi-valued field could work that way. Is it more like an annotation?

One would assume annotations also have the same restriction on delimitation right?

Forgive my lack of background on this point, but I wonder if it suggests that this concept does not map onto a label?

I agree with this sentiment, it suggests to me that maybe multiple labels/annotations would be preferable for this long term, one per version in the mapping

[vincepri]

Labels are indexed and we can ask the client to get us only the ones we're interested in, unfortunately we can't do the same with annotations and we'd have to list all the CRDs, which is a very expensive operation.

If we feel strongly about it and we want to change it, it should probably go in v0.4 given that is non backward compatible change. The contract versions applied to CRDs are also a temporary solution, we'll need to figure out a better way that doesn't require listing CRDs or applying labels, but that's also for later.

[bboreham]

One would assume annotations also have the same restriction on delimitation right?

Absolutely not. Compare labels and annotations

[bboreham]

we can ask the client to get us only the ones we're interested in

This is the bit I don't get: how do we know we are interested in v1alphaX,v1beta1 ? Doesn't the set of possible combinations extend off to infinity?

[vincepri]

We use HasLabels to filter only on the CRDs that have the key, and we don't look at the values until we have to sort them and pick the highest version. https://github.com/kubernetes-sigs/cluster-api/blob/a406781c0f44f8252424bb9a32a27dbd5c9c52eb/util/util.go#L445

[maelk]

The second bug discussed is actually in two functions : MachineToInfrastructureMapFunc and in ClusterToInfrastructureMapFunc

[vincepri]

Fixed in #2780 and #2786

/close

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

Fixed in #2780

/close

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.