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

✨ Upsync Endpoints of synced services #2829

Merged
merged 8 commits into from
Mar 9, 2023

Conversation

davidfestal
Copy link
Member

@davidfestal davidfestal commented Feb 21, 2023

Summary

Upsync Endpoints of synced services, so that controllers that rely of their existence to check application readiness (like KNative does) can work.

Related issue(s)

Fixes #2817

@openshift-ci openshift-ci bot requested review from jmprusi and s-urbaniak February 21, 2023 20:56
@openshift-ci openshift-ci bot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 22, 2023
@davidfestal davidfestal changed the title ✨ [WIP] Upsync Endpoints of synced services ✨ Upsync Endpoints of synced services Feb 23, 2023
@davidfestal davidfestal force-pushed the upsync-endpoints branch 2 times, most recently from e8b928f to 1e39201 Compare February 24, 2023 10:05
@davidfestal davidfestal requested a review from sttts February 24, 2023 10:41
@davidfestal davidfestal force-pushed the upsync-endpoints branch 4 times, most recently from 3396666 to 0138f52 Compare March 7, 2023 08:41
... on the service from which the Endpoints resource is derived.

Signed-off-by: David Festal <[email protected]>
//
// For now, only endpoints can be upsynced on demand by the syncer with this mechanism,
// but the list of such resources will increase in the future.
UpsyncDerivedResourcesAnnotationKey = "workload.kcp.io/upsync-derived-resources"
Copy link
Member

Choose a reason for hiding this comment

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

is this experimental and going to move into the SyncTarget spec?

Copy link
Member

Choose a reason for hiding this comment

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

and is this annotation user-facing in the first place? Who should be able to change it? (now it is protected through admission).

Copy link
Member Author

Choose a reason for hiding this comment

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

No it's not experimental.
And it's not expected to be added to the SyncTarget (obviously it is a copy/paste erorr in the comment).
The real comment should be:

	// UpsyncDerivedResourcesAnnotationKey is an annotation set on synced resources that contains a
	// command-separated list of stringified GroupResource (<resource>.<group>) for the derived resources
	// that are expected to be upsynced.
	// To allow upsyncing an Endpoints resource related to a synced service, the Service instance should be annotated with:
	//
	//   workload.kcp.io/upsync-derived-resources: endpoints
	//
	// For now, only endpoints can be upsynced on demand by the syncer with this mechanism,
	// but the list of such resources will increase in the future

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 fixed the comment in commit 5ba3c35

Copy link
Member Author

@davidfestal davidfestal Mar 8, 2023

Choose a reason for hiding this comment

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

and is this annotation user-facing in the first place? Who should be able to change it?

Yes, it is user-facing. For now the use should be able to change it. Mid-term it could be done by a dedicated coordination controller.

(now it is protected through admission).

Are all the workload.kcp.io/xxx annotation protected ?

Copy link
Member

Choose a reason for hiding this comment

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

Are all the workload.kcp.io/xxx annotation protected ?

yes. You have to opt-out in the admission plugin for that.

Yes, it is user-facing. For now the use should be able to change it. Mid-term it could be done by a dedicated coordination controller.

So it is experimental?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it a security problem? I can steal objects from the compute cluster, e.g. a service account.

Copy link
Member Author

Choose a reason for hiding this comment

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

The presence of this annotation isn't sufficient to trigger the Upsync, but it will be used as an additional condition by the logic that exists downstream and that marks resources for Upsync (by setting the RequestState label to Upsync).
In the case of Endpoints, the controller in the Syncer implements the logic that identifies which Endpoints should be Upsynced, and part of this logic is to look at this annotation on the related Service.

In any case, there is also a protection on the Upsyncer virtual workspace side + Syncer about the resource types that can be upsynced: for now the list is fixed: only PVs, Pods and Endpoints can be upsynced.

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally made the annotation experimental and added more comments on the annotation definition.

@@ -11,6 +11,7 @@ spec:
- v124.services.core
- v124.deployments.apps
- v124.pods.core
- v124.endpoints.core
Copy link
Member

Choose a reason for hiding this comment

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

what about EndpointSlices? Does one need them? Does one need both or do endpoints always exist?

Copy link
Member Author

Choose a reason for hiding this comment

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

For now we didn't have any use-case requiring upsyncing the EndpointSlices.
This PR tackles minimal requirements spotted by the KNative support scenario.
Additional resources will be added in the future as required.

It would still be possible to import EndpointSlices from the physical cluster if needed, and sync them as any other resource if necessary.

@davidfestal
Copy link
Member Author

/test e2e-sharded

Signed-off-by: David Festal <[email protected]>
... and complete comments

Signed-off-by: David Festal <[email protected]>
@davidfestal
Copy link
Member Author

davidfestal commented Mar 8, 2023

@sttts Is it OK for you now with the annotation made experimental and the added comments on it ?

// It is experimental since the provided user-experience is unsatisfactory,
// and further work should be done to define such (up)syncing strategies at a more appropriate level
// (SyncTarget, KCP namespace, KCP workspace ?).
ExperimentalUpsyncDerivedResourcesAnnotationKey = "experimental.workload.kcp.io/upsync-derived-resources"
Copy link
Member

Choose a reason for hiding this comment

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

better 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

would you approve ? ;-)

@jmprusi
Copy link
Member

jmprusi commented Mar 9, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2023
@sttts
Copy link
Member

sttts commented Mar 9, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 9, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 9, 2023
@openshift-merge-robot openshift-merge-robot merged commit dc49b99 into kcp-dev:main Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Upsync Endpoints
4 participants