Watch and sync changes to related resources#149
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
7253588 to
14bea92
Compare
|
/test pull-api-syncagent-verify |
1 similar comment
|
/test pull-api-syncagent-verify |
|
/retest pull-api-syncagent-verify |
|
@iakmc: The Use DetailsIn response to this:
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-sigs/prow repository. |
|
/test pull-api-syncagent-verify |
|
/test pull-api-syncagent-test-e2e-kcp-0.28 pull-api-syncagent-validate-prow-yaml |
change copyright year of related handlers On-behalf-of: SAP <iskren.pertov@sap.com> Signed-off-by: Iskren Petrov <iskren@kubermatic.com> Watch and sync changes to related resources
14bea92 to
f929711
Compare
|
/test pull-api-syncagent-test-e2e-kcp-0.28 |
|
/test pull-api-syncagent-validate-prow-yaml |
|
/retest |
|
|
||
| // RelatedResourceWatch configures how the watch handler maps a changed related resource | ||
| // back to its owning primary object. | ||
| // Exactly one of ByOwner or ByLabel must be set. |
There was a problem hiding this comment.
Please add an XValidation rule to ensure this. There are some examples in this file already.
| // each value is a Go template expression evaluated with the changed object available as | ||
| // .watchObject (with fields .name, .namespace, .labels). | ||
| // +optional | ||
| ByLabel map[string]string `json:"byLabel,omitempty"` |
There was a problem hiding this comment.
Can we support a full-blown metav1.LabelSelector here?
| // +optional | ||
| ByOwner *RelatedResourceWatchByOwner `json:"byOwner,omitempty"` | ||
|
|
||
| // ByLabel configures the watch handler to list primary objects matching a label selector |
There was a problem hiding this comment.
Can we rename this to "ByLabels" or "BySelector"? "ByLabel" is singular and I think this feature supports a selector with multiple key-value pairs, right?
| // RelatedResourceWatchByOwner configures reverse lookup via OwnerReferences. | ||
| type RelatedResourceWatchByOwner struct { | ||
| // Kind is the Kind to look for in the OwnerReferences of the changed related object. | ||
| Kind string `json:"kind"` |
There was a problem hiding this comment.
I think we should take a full GVK here and not just compare based on Kinds.
There was a problem hiding this comment.
Actually, I think the opposite now. This is not required at all, is it?
We already know the GVK of the primary object on both sides (origin and destination). We also already know the GVK/GVR of every related resource (like ConfigMaps). So what the agent needs to do is to watch all the GVR of related resources on the origin side (ConfigMaps, for example) and check if they have an ownerRef to the GVK of the primary object (a Cluster object, or whatever). And if so, cool, enqueue the primary object (i.e. the owner).
We don't need any further configuration for the ByOwner functionality here, I think.
| // of the owning primary object. Only related resources with a Watch config are covered. | ||
| watchedGVKs := sets.New[schema.GroupVersionKind]() | ||
| for _, relRes := range pubRes.Spec.Related { | ||
| if relRes.Origin != syncagentv1alpha1.RelatedResourceOriginKcp || relRes.Watch == nil { |
There was a problem hiding this comment.
I can't quite remember why we would only want to do this on the kcp side.. Couldn't we technically also have related objects on the service cluster side? It feels like this watching behaviour should work regardless of the origin side, no? 🤔
There was a problem hiding this comment.
I.e. it should always watch on the origin side, regardless where that side actually is.
| } | ||
|
|
||
| // Use the local REST mapper to determine the Kind. | ||
| gvk, err := localManager.GetRESTMapper().KindFor(gvr) |
There was a problem hiding this comment.
If the related resource originates in kcp (as per the first if statement in this loop), then why are we using the local (= service cluster) REST mapper to resolve it?
Related resources support projection (i.e. changing their GVK when syncing from one side to another), so a GVK that exists in kcp is not necessarily the same as it is on the service cluster.
Ideally this should use a restmapper of the origin side (whereever that might be).
| // Use the local REST mapper to determine the Kind. | ||
| gvk, err := localManager.GetRESTMapper().KindFor(gvr) | ||
| if err != nil { | ||
| log.Warnw("Failed to determine Kind for origin:kcp related resource, skipping watch", "gvr", gvr, "error", err) |
There was a problem hiding this comment.
I fear this log message will never be seen by anyone and during runtime, this state will also not fix itself (the agent won't try to re-establish this watch at a later time, unless you restart the entire agent).
To catch misconfigurations, wouldn't it make more sense to error out here?
| } | ||
|
|
||
| default: | ||
| log.Warnw("origin:kcp related resource has Watch set but neither byOwner nor byLabel configured, skipping", "gvk", gvk) |
There was a problem hiding this comment.
This should definitely be an error. Misconfigured PublishedResources should IMHO scream loudly. If we have the XValidation rule such errors will be much less likely, but I would still error out if the PublishedResource is broken.
|
|
||
| func (h *byOwnerEventHandler) enqueue(obj *unstructured.Unstructured, q workqueue.TypedRateLimitingInterface[mcreconcile.Request]) { | ||
| for _, ref := range obj.GetOwnerReferences() { | ||
| if ref.Kind == h.ownerKind { |
There was a problem hiding this comment.
This should compare G, V and K, I think.
| "namespace": obj.GetNamespace(), | ||
| "labels": obj.GetLabels(), | ||
| }, | ||
| } |
There was a problem hiding this comment.
Please create an explicit struct for this templating context in https://github.com/kcp-dev/api-syncagent/blob/main/internal/sync/templating/related.go, which helps in documenting the available template variables. Please also add a corresponding New...() func, to ensure that the newly introduced struct always gets built correctly.
You can then extend https://github.com/kcp-dev/api-syncagent/blob/main/docs/content/publish-resources/templating.md, which is trivial once the struct is setup.
Summary
Previously, changes to related resources with
origin: kcp(e.g. aConfigMap created by a user in a kcp workspace) were ignored — only
changes to the primary object triggered reconciliation.
This adds a
watchfield toRelatedResourceSpecthat allows serviceproviders to configure how the agent identifies the owning primary object
when a related resource changes. Two strategies are supported:
byOwner: inspects the OwnerReferences of the changed object to findthe primary object by Kind.
byLabel: evaluates Go template expressions against the changed objectto build a label selector, then lists primary objects matching that
selector.
When
watchis configured on anorigin: kcprelated resource, theagent sets up a
MultiClusterWatchfor that resource type. When a changeis detected, the configured strategy is used to enqueue the owning
primary object for reconciliation, which then syncs the updated related
object to the service cluster.
What Type of PR Is This?
/kind feature
/kind api-change
Related Issue(s)
Fixes #118
Release Notes