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.

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
Changes from all commits
Commits
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
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

// +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.
Comment on lines +517 to +518
Copy link
Member

@mikemorris mikemorris Dec 11, 2024

Choose a reason for hiding this comment

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

Suggested change
supported by MCS-API implementations. If supported, annotations or labels must
not be exported from the `metadata` of the `Service` or `ServiceExport` resources.
supported by MCS-API implementations. If supported, values specified in
`spec.exportedLabels` or `spec.exportedAnnotations` MUST take precedence over any
labels or annotations which may be synced or copied from the `metadata` of the
`Service` or `ServiceExport` resources.

I think this tweak would give us design space to implement a "copy filter" strategy either globally in implementation configuration or locally via additional fields on ServiceExport if we find a need to add that in the future but without adding additional complexity now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not sure I agree with this because it is intentionally vague about what implementation should do (does precedence means merging or completely overriding?), if we find the need to change that behavior I would just edit those sentence rather than having something somewhat unclear to begin with


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

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
Comment on lines +1305 to +1306
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
from the `Service` and `ServiceExport` metadata. More flexibility could also be
achieved with CEL expression on the `ServiceExport` at the cost of greater
from the `Service` and `ServiceExport` metadata.
This implementation does not preclude the possibility of additionally adopting a
"copy/filter" strategy (either as global configuration for a particular implementation, or
granularly as additional `ServiceExport` fields) if the need arises. Such an strategy could
select specific label or annotation keys to sync from the `Service` by exact name, by
prefix, or further flexibility could be achieved with CEL expressions at the cost of
greater

Feel free to reject this suggestion - from the long thread above this felt like this possibility could be worth mentioning but perhaps the "exportedLabels and exportedAnnotations MUST take precedence" snippet I suggested adding above is a sufficent oblique reference without introducing confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit similar to the previous thread I would recommend to keep the kep consistent for all implementations regarding this for now and if the need arises we should just update it IMO or make it more loose

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