Skip to content

Commit 0eb5aea

Browse files
committed
store original remote name as annotation, use hashed values for labels
On-behalf-of: @SAP christoph.mewes@sap.com
1 parent 02f4a27 commit 0eb5aea

File tree

9 files changed

+373
-168
lines changed

9 files changed

+373
-168
lines changed

internal/sync/meta.go

Lines changed: 23 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ import (
2222
"github.com/kcp-dev/logicalcluster/v3"
2323
"go.uber.org/zap"
2424

25+
"github.com/kcp-dev/api-syncagent/internal/crypto"
26+
2527
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2628
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2729
"k8s.io/apimachinery/pkg/labels"
@@ -56,7 +58,7 @@ func ensureAnnotations(obj metav1.Object, desiredAnnotations map[string]string)
5658
}
5759

5860
func ensureFinalizer(ctx context.Context, log *zap.SugaredLogger, client ctrlruntimeclient.Client, obj *unstructured.Unstructured, finalizer string) (updated bool, err error) {
59-
finalizers := sets.New[string](obj.GetFinalizers()...)
61+
finalizers := sets.New(obj.GetFinalizers()...)
6062
if finalizers.Has(deletionFinalizer) {
6163
return false, nil
6264
}
@@ -75,7 +77,7 @@ func ensureFinalizer(ctx context.Context, log *zap.SugaredLogger, client ctrlrun
7577
}
7678

7779
func removeFinalizer(ctx context.Context, log *zap.SugaredLogger, client ctrlruntimeclient.Client, obj *unstructured.Unstructured, finalizer string) (updated bool, err error) {
78-
finalizers := sets.New[string](obj.GetFinalizers()...)
80+
finalizers := sets.New(obj.GetFinalizers()...)
7981
if !finalizers.Has(deletionFinalizer) {
8082
return false, nil
8183
}
@@ -122,27 +124,32 @@ func (k objectKey) String() string {
122124
}
123125

124126
func (k objectKey) Key() string {
125-
result := k.Name
126-
if k.Namespace != "" {
127-
result = k.Namespace + "_" + result
128-
}
129-
if k.ClusterName != "" {
130-
result = string(k.ClusterName) + "_" + result
131-
}
132-
133-
return result
127+
return crypto.Hash(k)
134128
}
135129

136130
func (k objectKey) Labels() labels.Set {
137-
return labels.Set{
138-
remoteObjectClusterLabel: string(k.ClusterName),
139-
remoteObjectNamespaceLabel: k.Namespace,
140-
remoteObjectNameLabel: k.Name,
131+
// Name and namespace can be more than 63 characters long, so we must hash them
132+
// to turn them into valid label values. The full, original value is kept as an annotation.
133+
s := labels.Set{
134+
remoteObjectClusterLabel: string(k.ClusterName),
135+
remoteObjectNameHashLabel: crypto.Hash(k.Name),
141136
}
137+
138+
if k.Namespace != "" {
139+
s[remoteObjectNamespaceHashLabel] = crypto.Hash(k.Namespace)
140+
}
141+
142+
return s
142143
}
143144

144145
func (k objectKey) Annotations() labels.Set {
145-
s := labels.Set{}
146+
s := labels.Set{
147+
remoteObjectNameAnnotation: k.Name,
148+
}
149+
150+
if k.Namespace != "" {
151+
s[remoteObjectNamespaceAnnotation] = k.Namespace
152+
}
146153

147154
if !k.WorkspacePath.Empty() {
148155
s[remoteObjectWorkspacePathAnnotation] = k.WorkspacePath.String()

internal/sync/metadata.go

Lines changed: 106 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package sync
1818

1919
import (
20+
"fmt"
2021
"maps"
2122
"strings"
2223

@@ -29,7 +30,7 @@ import (
2930
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3031
)
3132

32-
func stripMetadata(obj *unstructured.Unstructured) *unstructured.Unstructured {
33+
func stripMetadata(obj *unstructured.Unstructured) error {
3334
obj.SetCreationTimestamp(metav1.Time{})
3435
obj.SetFinalizers(nil)
3536
obj.SetGeneration(0)
@@ -39,70 +40,100 @@ func stripMetadata(obj *unstructured.Unstructured) *unstructured.Unstructured {
3940
obj.SetUID("")
4041
obj.SetSelfLink("")
4142

42-
stripAnnotations(obj)
43-
stripLabels(obj)
43+
if err := stripAnnotations(obj); err != nil {
44+
return fmt.Errorf("failed to strip annotations: %w", err)
45+
}
46+
if err := stripLabels(obj); err != nil {
47+
return fmt.Errorf("failed to strip labels: %w", err)
48+
}
4449

45-
return obj
50+
return nil
4651
}
4752

48-
func stripAnnotations(obj *unstructured.Unstructured) *unstructured.Unstructured {
49-
annotations := obj.GetAnnotations()
50-
if annotations == nil {
51-
return obj
53+
func setNestedMapOmitempty(obj *unstructured.Unstructured, value map[string]string, path ...string) error {
54+
if len(value) == 0 {
55+
unstructured.RemoveNestedField(obj.Object, path...)
56+
return nil
5257
}
5358

54-
delete(annotations, "kcp.io/cluster")
55-
delete(annotations, "kubectl.kubernetes.io/last-applied-configuration")
59+
return unstructured.SetNestedStringMap(obj.Object, value, path...)
60+
}
5661

57-
maps.DeleteFunc(annotations, func(annotation string, _ string) bool {
58-
return strings.HasPrefix(annotation, relatedObjectAnnotationPrefix)
59-
})
62+
func stripAnnotations(obj *unstructured.Unstructured) error {
63+
annotations := obj.GetAnnotations()
64+
if annotations == nil {
65+
return nil
66+
}
6067

61-
obj.SetAnnotations(annotations)
68+
if err := setNestedMapOmitempty(obj, filterUnsyncableAnnotations(annotations), "metadata", "annotations"); err != nil {
69+
return err
70+
}
6271

63-
return obj
72+
return nil
6473
}
6574

66-
func stripLabels(obj *unstructured.Unstructured) *unstructured.Unstructured {
75+
func stripLabels(obj *unstructured.Unstructured) error {
6776
labels := obj.GetLabels()
6877
if labels == nil {
69-
return obj
78+
return nil
7079
}
7180

72-
for _, label := range ignoredRemoteLabels.UnsortedList() {
73-
delete(labels, label)
81+
if err := setNestedMapOmitempty(obj, filterUnsyncableLabels(labels), "metadata", "labels"); err != nil {
82+
return err
7483
}
7584

76-
obj.SetLabels(labels)
77-
78-
return obj
85+
return nil
7986
}
8087

81-
// ignoredRemoteLabels are labels we never want to copy from the remote to local objects.
82-
var ignoredRemoteLabels = sets.New[string](
88+
// unsyncableLabels are labels we never want to copy from the remote to local objects.
89+
var unsyncableLabels = sets.New(
8390
remoteObjectClusterLabel,
84-
remoteObjectNamespaceLabel,
85-
remoteObjectNameLabel,
91+
remoteObjectNamespaceHashLabel,
92+
remoteObjectNameHashLabel,
8693
)
8794

88-
// filterRemoteLabels removes all unwanted remote labels and returns a new label set.
89-
func filterRemoteLabels(remoteLabels labels.Set) labels.Set {
90-
filteredLabels := labels.Set{}
95+
// filterUnsyncableLabels removes all unwanted remote labels and returns a new label set.
96+
func filterUnsyncableLabels(original labels.Set) labels.Set {
97+
return filterLabels(original, unsyncableLabels)
98+
}
99+
100+
// unsyncableAnnotations are annotations we never want to copy from the remote to local objects.
101+
var unsyncableAnnotations = sets.New(
102+
"kcp.io/cluster",
103+
"kubectl.kubernetes.io/last-applied-configuration",
104+
remoteObjectNamespaceAnnotation,
105+
remoteObjectNameAnnotation,
106+
remoteObjectWorkspacePathAnnotation,
107+
)
91108

92-
for k, v := range remoteLabels {
93-
if !ignoredRemoteLabels.Has(k) {
94-
filteredLabels[k] = v
109+
// filterUnsyncableAnnotations removes all unwanted remote annotations and returns a new label set.
110+
func filterUnsyncableAnnotations(original labels.Set) labels.Set {
111+
filtered := filterLabels(original, unsyncableAnnotations)
112+
113+
maps.DeleteFunc(filtered, func(annotation string, _ string) bool {
114+
return strings.HasPrefix(annotation, relatedObjectAnnotationPrefix)
115+
})
116+
117+
return filtered
118+
}
119+
120+
func filterLabels(original labels.Set, forbidList sets.Set[string]) labels.Set {
121+
filtered := labels.Set{}
122+
for k, v := range original {
123+
if !forbidList.Has(k) {
124+
filtered[k] = v
95125
}
96126
}
97127

98-
return filteredLabels
128+
return filtered
99129
}
100130

101131
func RemoteNameForLocalObject(localObj ctrlruntimeclient.Object) *reconcile.Request {
102132
labels := localObj.GetLabels()
133+
annotations := localObj.GetAnnotations()
103134
clusterName := labels[remoteObjectClusterLabel]
104-
namespace := labels[remoteObjectNamespaceLabel]
105-
name := labels[remoteObjectNameLabel]
135+
namespace := annotations[remoteObjectNamespaceAnnotation]
136+
name := annotations[remoteObjectNameAnnotation]
106137

107138
// reject/ignore invalid/badly labelled object
108139
if clusterName == "" || name == "" {
@@ -117,3 +148,43 @@ func RemoteNameForLocalObject(localObj ctrlruntimeclient.Object) *reconcile.Requ
117148
},
118149
}
119150
}
151+
152+
// threeWayDiffMetadata is used when updating an object. Since the lastKnownState for any object
153+
// does not contain syncer-related metadata, this function determines whether labels/annotations are
154+
// missing by comparing the desired* sets with the current state on the destObj.
155+
// If a label/annotation is found to be missing or wrong, this function will set it on the sourceObj.
156+
// This is confusing at first, but the source object here is just a DeepCopy from the actual source
157+
// object and the caller is not meant to persist changes on the source object. The reason the changes
158+
// are performed on the source object is so that when creating the patch later on (which is done by
159+
// comparing the source object with the lastKnownState), the patch will contain the necessary changes.
160+
func threeWayDiffMetadata(sourceObj, destObj *unstructured.Unstructured, desiredLabels, desiredAnnotations labels.Set) {
161+
destLabels := destObj.GetLabels()
162+
sourceLabels := sourceObj.GetLabels()
163+
164+
for label, value := range desiredLabels {
165+
if destValue, ok := destLabels[label]; !ok || destValue != value {
166+
if sourceLabels == nil {
167+
sourceLabels = map[string]string{}
168+
}
169+
170+
sourceLabels[label] = value
171+
}
172+
}
173+
174+
sourceObj.SetLabels(sourceLabels)
175+
176+
destAnnotations := destObj.GetAnnotations()
177+
sourceAnnotations := sourceObj.GetAnnotations()
178+
179+
for label, value := range desiredAnnotations {
180+
if destValue, ok := destAnnotations[label]; !ok || destValue != value {
181+
if sourceAnnotations == nil {
182+
sourceAnnotations = map[string]string{}
183+
}
184+
185+
sourceAnnotations[label] = value
186+
}
187+
}
188+
189+
sourceObj.SetAnnotations(sourceAnnotations)
190+
}

internal/sync/object_syncer.go

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ type objectSyncer struct {
5151
syncStatusBack bool
5252
// whether or not to add/expect a finalizer on the source
5353
blockSourceDeletion bool
54+
// whether or not to place sync-related metadata on the destination object
55+
metadataOnDestination bool
5456
// optional mutations for both directions of the sync
5557
mutator mutation.Mutator
5658
// stateStore is capable of remembering the state of a Kubernetes object
@@ -177,7 +179,9 @@ func (s *objectSyncer) syncObjectSpec(log *zap.SugaredLogger, source, dest syncS
177179
}
178180

179181
sourceObjCopy := source.object.DeepCopy()
180-
stripMetadata(sourceObjCopy)
182+
if err = stripMetadata(sourceObjCopy); err != nil {
183+
return false, fmt.Errorf("failed to strip metadata from source object: %w", err)
184+
}
181185

182186
log = log.With("dest-object", newObjectKey(dest.object, dest.clusterName, logicalcluster.None))
183187

@@ -187,10 +191,21 @@ func (s *objectSyncer) syncObjectSpec(log *zap.SugaredLogger, source, dest syncS
187191
lastKnownSourceState.SetAPIVersion(sourceObjCopy.GetAPIVersion())
188192
lastKnownSourceState.SetKind(sourceObjCopy.GetKind())
189193

190-
// update annotations (this is important if the admin later flipped the enableWorkspacePaths
191-
// option in the PublishedResource)
192-
sourceKey := newObjectKey(source.object, source.clusterName, source.workspacePath)
193-
ensureAnnotations(sourceObjCopy, sourceKey.Annotations())
194+
// We want to now restore/fix broken labels or annotations on the destination object. Not all
195+
// of the sync-related metadata is relevant to _finding_ the destination object, so there is
196+
// a chance that a user has fiddled with the metadata and would have broken some other part
197+
// of the syncing.
198+
// The lastKnownState is based on the source object, so just from looking at it we could not
199+
// determine whether a label/annotation is missing/broken. However we do need to know this,
200+
// because we later have to distinguish between empty and non-empty patches, so that we know
201+
// when to requeue or stop syncing. So we cannot just blindly call ensureLabels() on the
202+
// sourceObjCopy, as that could create meaningless patches.
203+
// To achieve this, we individually check the labels/annotations on the destination object,
204+
// which we thankfully already fetched earlier.
205+
if s.metadataOnDestination {
206+
sourceKey := newObjectKey(source.object, source.clusterName, source.workspacePath)
207+
threeWayDiffMetadata(sourceObjCopy, dest.object, sourceKey.Labels(), sourceKey.Annotations())
208+
}
194209

195210
// now we can diff the two versions and create a patch
196211
rawPatch, err := s.createMergePatch(lastKnownSourceState, sourceObjCopy)
@@ -221,8 +236,8 @@ func (s *objectSyncer) syncObjectSpec(log *zap.SugaredLogger, source, dest syncS
221236
}
222237

223238
// update selected metadata fields
224-
ensureLabels(dest.object, filterRemoteLabels(sourceObjCopy.GetLabels()))
225-
ensureAnnotations(dest.object, sourceObjCopy.GetAnnotations())
239+
ensureLabels(dest.object, filterUnsyncableLabels(sourceObjCopy.GetLabels()))
240+
ensureAnnotations(dest.object, filterUnsyncableAnnotations(sourceObjCopy.GetAnnotations()))
226241

227242
// TODO: Check if anything has changed and skip the .Update() call if source and dest
228243
// are identical w.r.t. the fields we have copied (spec, annotations, labels, ..).
@@ -236,7 +251,8 @@ func (s *objectSyncer) syncObjectSpec(log *zap.SugaredLogger, source, dest syncS
236251
}
237252

238253
if requeue {
239-
// remember this object state for the next reconciliation
254+
// remember this object state for the next reconciliation (this will strip any syncer-related
255+
// metadata the 3-way diff may have added above)
240256
if err := s.stateStore.Put(sourceObjCopy, source.clusterName, s.subresources); err != nil {
241257
return true, fmt.Errorf("failed to update sync state: %w", err)
242258
}
@@ -278,19 +294,20 @@ func (s *objectSyncer) ensureDestinationObject(log *zap.SugaredLogger, source, d
278294
return fmt.Errorf("failed to ensure destination namespace: %w", err)
279295
}
280296

281-
// remove source metadata (like UID and generation) to allow destination object creation to succeed
282-
stripMetadata(destObj)
297+
// remove source metadata (like UID and generation, but also labels and annotations belonging to
298+
// the sync-agent) to allow destination object creation to succeed
299+
if err := stripMetadata(destObj); err != nil {
300+
return fmt.Errorf("failed to strip metadata from destination object: %w", err)
301+
}
283302

284303
// remember the connection between the source and destination object
285304
sourceObjKey := newObjectKey(source.object, source.clusterName, source.workspacePath)
286305
ensureLabels(destObj, sourceObjKey.Labels())
306+
ensureAnnotations(destObj, sourceObjKey.Annotations())
287307

288308
// remember what agent synced this object
289309
s.labelWithAgent(destObj)
290310

291-
// put optional additional annotations on the new object
292-
ensureAnnotations(destObj, sourceObjKey.Annotations())
293-
294311
// finally, we can create the destination object
295312
objectLog := log.With("dest-object", newObjectKey(destObj, dest.clusterName, logicalcluster.None))
296313
objectLog.Debugw("Creating destination object…")
@@ -333,9 +350,10 @@ func (s *objectSyncer) adoptExistingDestinationObject(log *zap.SugaredLogger, de
333350
// the destination object from another source object, which would then lead to the two source objects
334351
// "fighting" about the one destination object.
335352
ensureLabels(existingDestObj, sourceKey.Labels())
336-
s.labelWithAgent(existingDestObj)
337353
ensureAnnotations(existingDestObj, sourceKey.Annotations())
338354

355+
s.labelWithAgent(existingDestObj)
356+
339357
if err := dest.client.Update(dest.ctx, existingDestObj); err != nil {
340358
return fmt.Errorf("failed to upsert current destination object labels: %w", err)
341359
}

internal/sync/state_store.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,10 @@ import (
2020
"fmt"
2121
"strings"
2222

23-
"github.com/kcp-dev/api-syncagent/internal/crypto"
2423
"github.com/kcp-dev/logicalcluster/v3"
2524

25+
"github.com/kcp-dev/api-syncagent/internal/crypto"
26+
2627
corev1 "k8s.io/api/core/v1"
2728
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2829
"k8s.io/apimachinery/pkg/labels"
@@ -82,7 +83,9 @@ func (op *objectStateStore) Put(obj *unstructured.Unstructured, clusterName logi
8283

8384
func (op *objectStateStore) snapshotObject(obj *unstructured.Unstructured, subresources []string) (string, error) {
8485
obj = obj.DeepCopy()
85-
obj = stripMetadata(obj)
86+
if err := stripMetadata(obj); err != nil {
87+
return "", err
88+
}
8689

8790
// besides metadata, we also do not care about the object's subresources
8891
data := obj.UnstructuredContent()

0 commit comments

Comments
 (0)