Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KEP 1645: add labels and annotations export #4922

Merged
merged 1 commit into from
Dec 11, 2024
Merged

KEP 1645: add labels and annotations export #4922

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 52 additions & 4 deletions keps/sig-multicluster/1645-multi-cluster-services-api/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ tags, and then generate with `hack/update-toc.sh`.
- [Service Port](#service-port)
- [Headlessness](#headlessness)
- [Session Affinity](#session-affinity)
- [Labels and Annotations](#labels-and-annotations)
- [Test Plan](#test-plan)
- [Graduation Criteria](#graduation-criteria)
- [Alpha -> Beta Graduation](#alpha---beta-graduation)
Expand All @@ -119,6 +120,7 @@ tags, and then generate with `hack/update-toc.sh`.
- [Export services via label selector](#export-services-via-label-selector)
- [Export via annotation](#export-via-annotation)
- [Other conflict resolution algorithms](#other-conflict-resolution-algorithms)
- [Exporting labels/annotations from the Service/ServiceExport objects](#exporting-labelsannotations-from-the-serviceserviceexport-objects)
- [Infrastructure Needed](#infrastructure-needed)
<!-- /toc -->

Expand Down Expand Up @@ -413,9 +415,19 @@ type ServiceExport struct {
// +optional
metav1.ObjectMeta `json:"metadata,omitempty"`
// +optional
Spec ServiceExportSpec `json:"spec,omitempty"`
// +optional
Status ServiceExportStatus `json:"status,omitempty"`
}

// ServiceExportSpec describes an exported service and extra exported information
type ServiceExportSpec struct {
// +optional
ExportedLabels map[string]string `json:"exportedLabels"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ExportedLabels map[string]string `json:"exportedLabels"`
ExportedLabels map[string]string `json:"exportedLabels,omitempty"`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yep nice catch! It's already like that in the associated PR for the CRD but the KEP should be amended indeed

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@MrFreezeex MrFreezeex Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Member Author

@MrFreezeex MrFreezeex Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@MrFreezeex MrFreezeex Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We explicitly state this here:

This does not apply for labels and
annotations which are stored in ServiceExport directly in spec.exportedLabels and spec.exportedAnnotations. Exporting labels and annotations is optionally supported by MCS-API implementations. If supported, annotations or labels must not be exported from the metadata of the Service or ServiceExport resources.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// +optional
ExportedAnnotations map[string]string `json:"exportedAnnotations"`
MrFreezeex marked this conversation as resolved.
Show resolved Hide resolved
}

// ServiceExportStatus contains the current status of an export.
type ServiceExportStatus struct {
// +optional
Expand Down Expand Up @@ -497,9 +509,13 @@ single authority across all clusters. It is that authority’s responsibility to
ensure that a name is shared by multiple services within the namespace if and
only if they are instances of the same service.

All information about the service, including ports, backends and topology, will
continue to be stored in the `Service` objects, which are each name mapped to a
`ServiceExport`.
Most information about the service, including ports, backends, topology and
session affinity, will continue to be stored in the `Service` objects, which
MrFreezeex marked this conversation as resolved.
Show resolved Hide resolved
are each name mapped to a `ServiceExport`. This does not apply for labels and
annotations which are stored in `ServiceExport` directly in `spec.exportedLabels`
and `spec.exportedAnnotations`. Exporting labels and annotations is optionally
supported by MCS-API implementations. If supported, annotations or labels must
not be exported from the `metadata` of the `Service` or `ServiceExport` resources.
MrFreezeex marked this conversation as resolved.
Show resolved Hide resolved

Deleting a `ServiceExport` will stop exporting the name-mapped `Service`.

Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this would revert @sftim's prior suggested change in #4922 (comment)

for a given consumer. 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.

### Test Plan

E2E tests can use [kind](https://kind.sigs.k8s.io/) to create multiple clusters
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is about ServiceExport spec so I would say it should still be right?

Copy link
Member

@mikemorris mikemorris Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

replaced with an annotation, e.g. `multicluster.kubernetes.io/export`. When a
service is found with the annotation, it would be considered marked for export
to the clusterset. The controller would then create `EndpointSlices` and an
Expand Down Expand Up @@ -1258,6 +1281,31 @@ more confusing for users. Having just one simple deciding factor based on
ServiceExport oldness makes resolving conflicts straightforward, and this
alternative conflict resolution algorithm could hinder this ease of use.

### Exporting labels/annotations from the Service/ServiceExport objects

`Service` and `ServiceExport` have labels and annotations which could be used during
export and propagated to the `ServiceImport`. However various tools such as kubectl or
ArgoCD add some labels and annotations which would then need to be actively
filtered to avoid any conflict. Filtering those labels and annotations is not
something easy and we chose to avoid this problem entirely by not using the metadata
object and adding dedicated fields in the spec of the `ServiceExport` resource.

Also if we were using the labels and annotations from the metadata of either the
`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
Comment on lines +1294 to +1295
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`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.

implementation merge the labels/annotations from both objects? Should it favor one?
Should it takes only from the `Service` object? With dedicated fields for labels
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.
Comment on lines +1298 to +1301
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.


We also favored dedicated fields on the `ServiceExport` resource to allow for better
flexibility, as it will allow to export labels and annotations fully decorrelated
from the `Service` and `ServiceExport` metadata. More flexibility could also be
achieved with CEL expression on the `ServiceExport` at the cost of greater
MrFreezeex marked this conversation as resolved.
Show resolved Hide resolved
complexity (managing CEL expressions on potentially many `ServiceExport` across clusters).

## Infrastructure Needed
<!--
Use this section if you need things from the project/SIG. Examples include a
Expand Down