KEP 1645: add labels and annotations export #4922
Conversation
MrFreezeex
commented
Oct 16, 2024
•
- One-line PR description: add labels and annotations export
- Issue link: Multi-Cluster Services API #1645
- Other comments: Add new fields in the ServiceExport resources to sync labels and annotations. This approach is inspired by the GatewayAPI (https://gateway-api.sigs.k8s.io/geps/gep-1762/#labels-and-annotations) one to have dedicated fields for that and there's some explanation inline in the KEP why we might favor that.
|
Skipping CI for Draft Pull Request. |
939b732 to
2ae4c68
Compare
6b7282b to
a849f28
Compare
keps/sig-multicluster/1645-multi-cluster-services-api/README.md
Outdated
Show resolved
Hide resolved
keps/sig-multicluster/1645-multi-cluster-services-api/README.md
Outdated
Show resolved
Hide resolved
a849f28 to
4960c5b
Compare
keps/sig-multicluster/1645-multi-cluster-services-api/README.md
Outdated
Show resolved
Hide resolved
b33c1eb to
83d1af6
Compare
keps/sig-multicluster/1645-multi-cluster-services-api/README.md
Outdated
Show resolved
Hide resolved
Signed-off-by: Arthur Outhenin-Chalandre <arthur@cri.epita.fr>
83d1af6 to
271740f
Compare
|
/approve @mikemorris, @JeremyOT, @lauralorenz could you take a look (after Thanksgiving week-end I imagine 😉)? |
|
Will prioritize reviewing this and kubernetes-sigs/mcs-api/pull/85 this week. |
There was a problem hiding this comment.
/approve
This feels good to me - it's both simpler and more flexible than the "copy/filter" strategy to start, and with the tweak I suggested from "annotations or labels must not be exported from the metadata" to "values specified in spec.exportedLabels or spec.exportedAnnotations MUST take precedence" I think we have future design space if we determine the alternative UX is needed in the future.
| @@ -1013,6 +1029,13 @@ Session affinity affects a service as a whole for a given consumer. The derived | |||
| service's session affinity will be decided according to the conflict resolution | |||
| policy. | |||
|
|
|||
| #### Labels and Annotations | |||
|
|
|||
| If supported, exporting labels and annotations would affect a `Service` as a whole | |||
There was a problem hiding this comment.
| If supported, exporting labels and annotations would affect a `Service` as a whole | |
| If supported, exporting labels and annotations would affect a service as a whole |
For consistency with the above sections which do not explicitly mention the Service resource - my reading is that a derived Service resource is an optional implementation detail which is one component of the "service as a whole".
There was a problem hiding this comment.
I see this would revert @sftim's prior suggested change in #4922 (comment)
| @@ -1229,7 +1252,7 @@ retain the flexibility of selectors. | |||
|
|
|||
| ### Export via annotation | |||
|
|
|||
| `ServiceExport` as described has no spec and seems like it could just be | |||
| `ServiceExport` initially had no spec and seemed like it could just be | |||
There was a problem hiding this comment.
Out of scope for this change, but I think the original rationale here is actually outdated - the Service resource does have a status.conditions field now https://kubernetes.io/docs/reference/kubernetes-api/service-resources/service-v1/#ServiceStatus
There was a problem hiding this comment.
This is about ServiceExport spec so I would say it should still be right?
There was a problem hiding this comment.
This section is describing why a ServiceExport resource with no spec was originally proposed at all - the ServiceExport status was intended to fill a UX gap from Service lacking a status.conditions field to message if an annotation-based API on Service to indicate an intent to export would have been successful or conflicted. It's not directly applicable to the focus of this PR though, I may update the language for historical purposes in a separate PR.
| `ServiceExport` or `Service` resources, it may be more confusing for users as it | ||
| would be the only fields present in both resources. For instance, should an |
There was a problem hiding this comment.
| `ServiceExport` or `Service` resources, it may be more confusing for users as it | |
| would be the only fields present in both resources. For instance, should an | |
| `ServiceExport` or `Service` resources, it may be more confusing for users. | |
| For instance, should an |
I didn't quite understand this, and think the point is clear without this phrase.
| and annotations in the spec of the `ServiceExport` resource, it may becomes more | ||
| straightforward that each resource have their own labels and annotations in their | ||
| metadata and that the exported labels and annotations are from the dedicated | ||
| fields in the `ServiceExport` spec. |
There was a problem hiding this comment.
| and annotations in the spec of the `ServiceExport` resource, it may becomes more | |
| straightforward that each resource have their own labels and annotations in their | |
| metadata and that the exported labels and annotations are from the dedicated | |
| fields in the `ServiceExport` spec. | |
| and annotations in the spec of the `ServiceExport` resource, we expect it will be | |
| straightforward that while each resource has their own labels and annotations in their | |
| metadata, the exported labels and annotations are from the dedicated fields in the | |
| `ServiceExport` spec. |
Minor phrasing nit.
|
This looks good to me, thank you! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JeremyOT, mikemorris, MrFreezeex, skitt 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:
Approvers can indicate their approval by writing |
| // ServiceExportSpec describes an exported service and extra exported information | ||
| type ServiceExportSpec struct { | ||
| // +optional | ||
| ExportedLabels map[string]string `json:"exportedLabels"` |
There was a problem hiding this comment.
| ExportedLabels map[string]string `json:"exportedLabels"` | |
| ExportedLabels map[string]string `json:"exportedLabels,omitempty"` |
There was a problem hiding this comment.
Ah yep nice catch! It's already like that in the associated PR for the CRD but the KEP should be amended indeed
| // ServiceExportSpec describes an exported service and extra exported information | ||
| type ServiceExportSpec struct { | ||
| // +optional | ||
| ExportedLabels map[string]string `json:"exportedLabels"` |
There was a problem hiding this comment.
could you please explain which case should use exportedLabels/exportedAnnotations? provide an example?
it will be really helpful to provide some guidance on what information should be in the annotation/labels on the serviceExport or exportedLabels/exportedAnnotations on the spec of the serviceExport.
There was a problem hiding this comment.
This is to add any labels or annotations the user wants to the ServiceImport (+ the associated derived service depending if the MCS-API implementation uses that). The use case can vary a lot depending of the context and the environments.
For instance, if there is an operator selecting ServiceImport/Service based on a label selector you would need some kind of labels there. Prometheus operator is one example of that, it has a service selector and if the implementation uses a derived service you could supposedly add labels through that and uses a ServiceMonitor to monitor a service deployed on remote clusters too.
For annotations, it also depends a lot, that would be probably things that would change the behavior of Service or ServiceImport based on some annotation. As an example Cilium have many annotation that updates the Service behavior (and not all are specific to Cilium clustermesh and might be useful for any MCS-API implementation deployed on a cluster with Cilium as the CNI), here is a couple example that I know OTOH: https://docs.cilium.io/en/stable/network/servicemesh/envoy-load-balancing/#add-proxy-load-balancing-annotations-to-the-services & https://docs.cilium.io/en/latest/network/clustermesh/services/#synchronizing-kubernetes-endpointslice-beta & https://docs.cilium.io/en/stable/network/clustermesh/affinity/#enabling-global-service-affinity, and there is even the topology mode for a regular installation with kube-proxy although that is replaced by the dedicated "trafficDistribution" field but still.
There was a problem hiding this comment.
I see, these labels/annotations will be added in the derived-service, so it's not the label/annotation for the service or serviceExport itself.
Could you please explain how these exportedLabels/exportedAnnotations can be used in the serviceImport?
based on the serviceImport definition, "servcieImport will be introduced to act as the in-cluster representation of a multi-cluster service in each importing cluster.", exportedLabels/exportedAnnotations is per cluster, how can we aggregate these information on the label of serviceImport?
There was a problem hiding this comment.
My concern is, the field name itself reveals some implementation details, especially label/annotation. if i understand it correctly, they're extra information about how to export the service itself. Should we use some generic name? eg, "properties"
There was a problem hiding this comment.
Could you please explain how these exportedLabels/exportedAnnotations can be used in the serviceImport?
The KEP currently states this:
The derived service's labels and annotations will be decided
according to the conflict resolution if the set of name/value pairs are not identical
between the constituent clusters.
Basically if an implementation chose to add supports for this they will have to make sure all the maps of exported{Annotations,Labels} are identical or take only the oldest ServiceExport maps and raise a conflict like we do for all the other properties. Those will then be transposed to each ServiceImport .metadata.{annotations,labels} and possibly its derived service(s) if the implementation has that.
My concern is, the field name itself reveals some implementation details, especially label/annotation. if i understand it correctly, they're extra information about how to export the service itself. Should we use some generic name? eg, "properties"
Labels/annotations is a generic thing present on every kubernetes objects, so I am not sure I understand your point unfortunately. This is only about having the possibility of getting some labels and annotations up to the ServiceImport resources which can then be used by anything that interact with either ServiceImports or the derived Services including any third parties controllers that might have nothing to do with the specific MCS-API implementation.
There was a problem hiding this comment.
if i understand it correctly, it's mainly about how to solve the conflicts for the labels/annotations on the original Service itself, not about the new fields in the ServiceExport.
No the labels/annotations of the original services should be ignored during the export and should not influence the labels/annotations of the ServiceImport (or related derived service)
About the labels/annotations, from this comment , all the examples given are related to the implementation and then decide where these extra information should be put, either annotation or label. For example, Prometheus operator wants the label from the derived service, and as a result, the information should be in the label.
I am not sure if this is your point but there is no known usage of labels/annotations on the ServiceImport resources currently which is mainly why this is an optional feature as it's not very useful right now for implementation not using derived services.
There was a problem hiding this comment.
No the labels/annotations of the original services should be ignored during the export and should not influence the labels/annotations of the ServiceImport (or related derived service) then, need to clarify it in the KEP. There are three types of labels/annotations now: on service itself, on serviceExport itself, servcieExport fields.
I am not sure if this is your point but there is no known usage of labels/annotations on the ServiceImport resources currently which is mainly why this is an optional feature as it's not very useful right now for implementation not using derived services.
My concern is more about naming. I would like to put extra information to the servceExport itself. But about where these information will be used, it could be one of label, annotation or even a field in a CR, which depends on the implementation.
There was a problem hiding this comment.
We explicitly state this here:
This does not apply for labels and
annotations which are stored inServiceExportdirectly inspec.exportedLabelsandspec.exportedAnnotations. Exporting labels and annotations is optionally supported by MCS-API implementations. If supported, annotations or labels must not be exported from themetadataof theServiceorServiceExportresources.
And also we are talking about this in the alternative section as well.
My concern is more about naming. I would like to put extra information to the servceExport itself. But about where these information will be used, it could be one of label, annotation or even a field in a CR, which depends on the implementation.
Well this not a a huge amount of context, but I guess you could add stuff in annotations yes or propose some new fields depending on what is it.
There was a problem hiding this comment.
No the labels/annotations of the original services should be ignored during the export and should not influence the labels/annotations of the ServiceImport (or related derived service)
It seems that https://github.com/kubernetes/enhancements/blob/master/keps/sig-multicluster/1645-multi-cluster-services-api/README.md#labels-and-annotations was talking about the original service? Does the new spec change the original statement? I wonder what happens if the export and the original service both have a label "x"?
There was a problem hiding this comment.
It seems that https://github.com/kubernetes/enhancements/blob/master/keps/sig-multicluster/1645-multi-cluster-services-api/README.md#labels-and-annotations was talking about the original service? Does the new spec change the original statement?
AFAIU this is talking about the derived service and was added together with the new fields btw.
I wonder what happens if the export and the original service both have a label "x"?
Original Service labels are meant to be completely ignored to not have to skip some of the labels/annotation as we would need to figure out a filtering approach to skip some of the annotations/labels in this case.