Skip to content

Commit e61bf1a

Browse files
ryanzhang-ossryanzhang-oss
authored
fix: initialize the annotation map before assigning a value (#944)
1 parent 3cb2de9 commit e61bf1a

File tree

2 files changed

+236
-13
lines changed

2 files changed

+236
-13
lines changed

‎pkg/controllers/workgenerator/controller.go‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -619,6 +619,9 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre
619619
}
620620
work := workList.Items[0]
621621
work.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = resourceSnapshot.Labels[fleetv1beta1.ResourceIndexLabel]
622+
if work.Annotations == nil {
623+
work.Annotations = make(map[string]string)
624+
}
622625
work.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = resourceBinding.Spec.ResourceSnapshotName
623626
work.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = resourceOverrideSnapshotHash
624627
work.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = clusterResourceOverrideSnapshotHash
@@ -681,23 +684,29 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee
681684
// check if we need to update the existing work object
682685
workResourceIndex, err := labels.ExtractResourceSnapshotIndexFromWork(existingWork)
683686
if err != nil {
684-
klog.ErrorS(err, "work has invalid parent resource index", "work", workObj)
685-
return false, controller.NewUnexpectedBehaviorError(err)
686-
}
687-
// we already checked the label in fetchAllResourceSnapShots function so no need to check again
688-
resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot)
689-
if workResourceIndex == resourceIndex {
690-
// no need to do anything if the work is generated from the same snapshots
691-
if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] &&
692-
existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] {
693-
klog.V(2).InfoS("Work is not associated with the desired override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation],
694-
"existingCROHash", existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation], "work", workObj)
695-
return false, nil
687+
klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "work has invalid parent resource index", "work", workObj)
688+
} else {
689+
// we already checked the label in fetchAllResourceSnapShots function so no need to check again
690+
resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot)
691+
if workResourceIndex == resourceIndex {
692+
// no need to do anything if the work is generated from the same resource/override snapshots
693+
if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] &&
694+
existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] {
695+
klog.V(2).InfoS("Work is associated with the desired resource/override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation],
696+
"existingCROHash", existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation], "work", workObj)
697+
return false, nil
698+
}
699+
klog.V(2).InfoS("Work is already associated with the desired resourceSnapshot but still not having the right override snapshots", "resourceIndex", resourceIndex, "work", workObj, "resourceSnapshot", resourceSnapshotObj)
696700
}
697-
klog.V(2).InfoS("Work is already associated with the desired resourceSnapshot but still not having the right override snapshots", "resourceIndex", resourceIndex, "work", workObj, "resourceSnapshot", resourceSnapshotObj)
698701
}
699702
// need to copy the new work to the existing work, only 5 possible changes:
703+
if existingWork.Labels == nil {
704+
existingWork.Labels = make(map[string]string)
705+
}
700706
existingWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
707+
if existingWork.Annotations == nil {
708+
existingWork.Annotations = make(map[string]string)
709+
}
701710
existingWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation]
702711
existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation]
703712
existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation]

‎pkg/controllers/workgenerator/controller_test.go‎

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,10 @@ import (
1111
"testing"
1212
"time"
1313

14+
appsv1 "k8s.io/api/apps/v1"
1415
k8serrors "k8s.io/apimachinery/pkg/api/errors"
16+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
17+
"k8s.io/apimachinery/pkg/runtime"
1518
"k8s.io/apimachinery/pkg/runtime/schema"
1619

1720
"github.com/google/go-cmp/cmp"
@@ -24,6 +27,7 @@ import (
2427

2528
fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
2629
"go.goms.io/fleet/pkg/controllers/work"
30+
"go.goms.io/fleet/pkg/utils"
2731
"go.goms.io/fleet/pkg/utils/condition"
2832
"go.goms.io/fleet/pkg/utils/controller"
2933
"go.goms.io/fleet/test/utils/informer"
@@ -143,6 +147,216 @@ func TestGetWorkNamePrefixFromSnapshotName(t *testing.T) {
143147
}
144148
}
145149

150+
func TestUpsertWork(t *testing.T) {
151+
workName := "work"
152+
namespace := "default"
153+
154+
var cmpOptions = []cmp.Option{
155+
// ignore the message as we may change the message in the future
156+
cmpopts.IgnoreFields(fleetv1beta1.Work{}, "Status"),
157+
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "CreationTimestamp"),
158+
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"),
159+
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields"),
160+
cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
161+
cmpopts.IgnoreFields(fleetv1beta1.WorkloadTemplate{}, "Manifests"),
162+
}
163+
164+
testDeployment := appsv1.Deployment{
165+
TypeMeta: metav1.TypeMeta{
166+
Kind: utils.DeploymentKind,
167+
APIVersion: utils.DeploymentGVK.GroupVersion().String(),
168+
},
169+
ObjectMeta: metav1.ObjectMeta{
170+
Name: "testDeployment",
171+
},
172+
Spec: appsv1.DeploymentSpec{
173+
Replicas: ptr.To(int32(2)),
174+
MinReadySeconds: 5,
175+
},
176+
}
177+
newWork := &fleetv1beta1.Work{
178+
ObjectMeta: metav1.ObjectMeta{
179+
Name: workName,
180+
Namespace: namespace,
181+
Labels: map[string]string{
182+
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
183+
},
184+
Annotations: map[string]string{
185+
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
186+
fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "hash1",
187+
fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: "hash2",
188+
},
189+
},
190+
Spec: fleetv1beta1.WorkSpec{
191+
Workload: fleetv1beta1.WorkloadTemplate{
192+
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Object: &testDeployment}}},
193+
},
194+
},
195+
}
196+
197+
resourceSnapshot := &fleetv1beta1.ClusterResourceSnapshot{
198+
ObjectMeta: metav1.ObjectMeta{
199+
Name: "snapshot-1",
200+
Labels: map[string]string{
201+
fleetv1beta1.ResourceIndexLabel: "1",
202+
},
203+
},
204+
}
205+
206+
tests := []struct {
207+
name string
208+
existingWork *fleetv1beta1.Work
209+
expectChanged bool
210+
}{
211+
{
212+
name: "Create new work when existing work is nil",
213+
existingWork: nil,
214+
expectChanged: true,
215+
},
216+
{
217+
name: "Update existing work with new annotations",
218+
existingWork: &fleetv1beta1.Work{
219+
ObjectMeta: metav1.ObjectMeta{
220+
Name: workName,
221+
Namespace: namespace,
222+
Labels: map[string]string{
223+
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
224+
},
225+
},
226+
Spec: fleetv1beta1.WorkSpec{
227+
Workload: fleetv1beta1.WorkloadTemplate{
228+
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
229+
},
230+
},
231+
},
232+
expectChanged: true,
233+
},
234+
{
235+
name: "Update existing work even if it does not have the resource snapshot label",
236+
existingWork: &fleetv1beta1.Work{
237+
ObjectMeta: metav1.ObjectMeta{
238+
Name: workName,
239+
Namespace: namespace,
240+
},
241+
Spec: fleetv1beta1.WorkSpec{
242+
Workload: fleetv1beta1.WorkloadTemplate{
243+
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
244+
},
245+
},
246+
},
247+
expectChanged: true,
248+
},
249+
{
250+
name: "Update existing work if it misses annotations even if the resource snapshot label is correct",
251+
existingWork: &fleetv1beta1.Work{
252+
ObjectMeta: metav1.ObjectMeta{
253+
Name: workName,
254+
Namespace: namespace,
255+
Labels: map[string]string{
256+
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
257+
},
258+
},
259+
Spec: fleetv1beta1.WorkSpec{
260+
Workload: fleetv1beta1.WorkloadTemplate{
261+
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
262+
},
263+
},
264+
},
265+
expectChanged: true,
266+
},
267+
{
268+
name: "Update existing work if it does not have correct override snapshot hash",
269+
existingWork: &fleetv1beta1.Work{
270+
ObjectMeta: metav1.ObjectMeta{
271+
Name: workName,
272+
Namespace: namespace,
273+
Labels: map[string]string{
274+
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
275+
},
276+
Annotations: map[string]string{
277+
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
278+
fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "wrong-hash"},
279+
},
280+
Spec: fleetv1beta1.WorkSpec{
281+
Workload: fleetv1beta1.WorkloadTemplate{
282+
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
283+
},
284+
},
285+
},
286+
expectChanged: true,
287+
},
288+
{
289+
name: "Do not update the existing work if it already points to the same resource and override snapshots",
290+
existingWork: &fleetv1beta1.Work{
291+
ObjectMeta: metav1.ObjectMeta{
292+
Name: workName,
293+
Namespace: namespace,
294+
Labels: map[string]string{
295+
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
296+
},
297+
Annotations: map[string]string{
298+
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
299+
fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "hash1",
300+
fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: "hash2",
301+
},
302+
},
303+
Spec: fleetv1beta1.WorkSpec{
304+
Workload: fleetv1beta1.WorkloadTemplate{
305+
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
306+
},
307+
},
308+
},
309+
expectChanged: false,
310+
},
311+
}
312+
313+
for _, tt := range tests {
314+
t.Run(tt.name, func(t *testing.T) {
315+
scheme := serviceScheme(t)
316+
objects := []client.Object{resourceSnapshot}
317+
if tt.existingWork != nil {
318+
objects = append(objects, tt.existingWork)
319+
}
320+
fakeClient := fake.NewClientBuilder().
321+
WithStatusSubresource(objects...).
322+
WithScheme(scheme).
323+
WithObjects(objects...).
324+
Build()
325+
// Create reconciler with custom client
326+
reconciler := &Reconciler{
327+
Client: fakeClient,
328+
recorder: record.NewFakeRecorder(10),
329+
InformerManager: &informer.FakeManager{},
330+
}
331+
changed, _ := reconciler.upsertWork(ctx, newWork, tt.existingWork, resourceSnapshot)
332+
if changed != tt.expectChanged {
333+
t.Fatalf("expected changed: %v, got: %v", tt.expectChanged, changed)
334+
}
335+
upsertedWork := &fleetv1beta1.Work{}
336+
if fakeClient.Get(ctx, client.ObjectKeyFromObject(newWork), upsertedWork) != nil {
337+
t.Fatalf("failed to get upserted work")
338+
}
339+
if diff := cmp.Diff(newWork, upsertedWork, cmpOptions...); diff != "" {
340+
t.Errorf("upsertWork didn't update the work, mismatch (-want +got):\n%s", diff)
341+
}
342+
if tt.expectChanged {
343+
// check if the deployment is applied
344+
var u unstructured.Unstructured
345+
if err := u.UnmarshalJSON(upsertedWork.Spec.Workload.Manifests[0].Raw); err != nil {
346+
t.Fatalf("Failed to unmarshal the result: %v, want nil", err)
347+
}
348+
var deployment appsv1.Deployment
349+
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &deployment); err != nil {
350+
t.Fatalf("Failed to convert the result to deployment: %v, want nil", err)
351+
}
352+
if diff := cmp.Diff(testDeployment, deployment); diff != "" {
353+
t.Errorf("The new Deployment mismatch (-want, +got):\n%s", diff)
354+
}
355+
}
356+
})
357+
}
358+
}
359+
146360
func TestBuildAllWorkAppliedCondition(t *testing.T) {
147361
tests := map[string]struct {
148362
works map[string]*fleetv1beta1.Work

0 commit comments

Comments
 (0)