Skip to content

Commit

Permalink
fix: initialize the annotation map before assigning a value (#944)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ryanzhang-oss
ryanzhang-oss authored Nov 8, 2024
1 parent 3cb2de9 commit e61bf1a
Show file tree
Hide file tree
Showing 2 changed files with 236 additions and 13 deletions.
35 changes: 22 additions & 13 deletions pkg/controllers/workgenerator/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,9 @@ func (r *Reconciler) getConfigMapEnvelopWorkObj(ctx context.Context, workNamePre
}
work := workList.Items[0]
work.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = resourceSnapshot.Labels[fleetv1beta1.ResourceIndexLabel]
if work.Annotations == nil {
work.Annotations = make(map[string]string)
}
work.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = resourceBinding.Spec.ResourceSnapshotName
work.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = resourceOverrideSnapshotHash
work.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = clusterResourceOverrideSnapshotHash
Expand Down Expand Up @@ -681,23 +684,29 @@ func (r *Reconciler) upsertWork(ctx context.Context, newWork, existingWork *flee
// check if we need to update the existing work object
workResourceIndex, err := labels.ExtractResourceSnapshotIndexFromWork(existingWork)
if err != nil {
klog.ErrorS(err, "work has invalid parent resource index", "work", workObj)
return false, controller.NewUnexpectedBehaviorError(err)
}
// we already checked the label in fetchAllResourceSnapShots function so no need to check again
resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot)
if workResourceIndex == resourceIndex {
// no need to do anything if the work is generated from the same snapshots
if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] &&
existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] {
klog.V(2).InfoS("Work is not associated with the desired override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation],
"existingCROHash", existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation], "work", workObj)
return false, nil
klog.ErrorS(controller.NewUnexpectedBehaviorError(err), "work has invalid parent resource index", "work", workObj)
} else {
// we already checked the label in fetchAllResourceSnapShots function so no need to check again
resourceIndex, _ := labels.ExtractResourceIndexFromClusterResourceSnapshot(resourceSnapshot)
if workResourceIndex == resourceIndex {
// no need to do anything if the work is generated from the same resource/override snapshots
if existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] &&
existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] == newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] {
klog.V(2).InfoS("Work is associated with the desired resource/override snapshots", "existingROHash", existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation],
"existingCROHash", existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation], "work", workObj)
return false, nil
}
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)
}
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)
}
// need to copy the new work to the existing work, only 5 possible changes:
if existingWork.Labels == nil {
existingWork.Labels = make(map[string]string)
}
existingWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel] = newWork.Labels[fleetv1beta1.ParentResourceSnapshotIndexLabel]
if existingWork.Annotations == nil {
existingWork.Annotations = make(map[string]string)
}
existingWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceSnapshotNameAnnotation]
existingWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation]
existingWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation] = newWork.Annotations[fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation]
Expand Down
214 changes: 214 additions & 0 deletions pkg/controllers/workgenerator/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ import (
"testing"
"time"

appsv1 "k8s.io/api/apps/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"

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

fleetv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
"go.goms.io/fleet/pkg/controllers/work"
"go.goms.io/fleet/pkg/utils"
"go.goms.io/fleet/pkg/utils/condition"
"go.goms.io/fleet/pkg/utils/controller"
"go.goms.io/fleet/test/utils/informer"
Expand Down Expand Up @@ -143,6 +147,216 @@ func TestGetWorkNamePrefixFromSnapshotName(t *testing.T) {
}
}

func TestUpsertWork(t *testing.T) {
workName := "work"
namespace := "default"

var cmpOptions = []cmp.Option{
// ignore the message as we may change the message in the future
cmpopts.IgnoreFields(fleetv1beta1.Work{}, "Status"),
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "CreationTimestamp"),
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ResourceVersion"),
cmpopts.IgnoreFields(metav1.ObjectMeta{}, "ManagedFields"),
cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
cmpopts.IgnoreFields(fleetv1beta1.WorkloadTemplate{}, "Manifests"),
}

testDeployment := appsv1.Deployment{
TypeMeta: metav1.TypeMeta{
Kind: utils.DeploymentKind,
APIVersion: utils.DeploymentGVK.GroupVersion().String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: "testDeployment",
},
Spec: appsv1.DeploymentSpec{
Replicas: ptr.To(int32(2)),
MinReadySeconds: 5,
},
}
newWork := &fleetv1beta1.Work{
ObjectMeta: metav1.ObjectMeta{
Name: workName,
Namespace: namespace,
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
},
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "hash1",
fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: "hash2",
},
},
Spec: fleetv1beta1.WorkSpec{
Workload: fleetv1beta1.WorkloadTemplate{
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Object: &testDeployment}}},
},
},
}

resourceSnapshot := &fleetv1beta1.ClusterResourceSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: "snapshot-1",
Labels: map[string]string{
fleetv1beta1.ResourceIndexLabel: "1",
},
},
}

tests := []struct {
name string
existingWork *fleetv1beta1.Work
expectChanged bool
}{
{
name: "Create new work when existing work is nil",
existingWork: nil,
expectChanged: true,
},
{
name: "Update existing work with new annotations",
existingWork: &fleetv1beta1.Work{
ObjectMeta: metav1.ObjectMeta{
Name: workName,
Namespace: namespace,
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
},
},
Spec: fleetv1beta1.WorkSpec{
Workload: fleetv1beta1.WorkloadTemplate{
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
},
},
},
expectChanged: true,
},
{
name: "Update existing work even if it does not have the resource snapshot label",
existingWork: &fleetv1beta1.Work{
ObjectMeta: metav1.ObjectMeta{
Name: workName,
Namespace: namespace,
},
Spec: fleetv1beta1.WorkSpec{
Workload: fleetv1beta1.WorkloadTemplate{
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
},
},
},
expectChanged: true,
},
{
name: "Update existing work if it misses annotations even if the resource snapshot label is correct",
existingWork: &fleetv1beta1.Work{
ObjectMeta: metav1.ObjectMeta{
Name: workName,
Namespace: namespace,
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
},
},
Spec: fleetv1beta1.WorkSpec{
Workload: fleetv1beta1.WorkloadTemplate{
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
},
},
},
expectChanged: true,
},
{
name: "Update existing work if it does not have correct override snapshot hash",
existingWork: &fleetv1beta1.Work{
ObjectMeta: metav1.ObjectMeta{
Name: workName,
Namespace: namespace,
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
},
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "wrong-hash"},
},
Spec: fleetv1beta1.WorkSpec{
Workload: fleetv1beta1.WorkloadTemplate{
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
},
},
},
expectChanged: true,
},
{
name: "Do not update the existing work if it already points to the same resource and override snapshots",
existingWork: &fleetv1beta1.Work{
ObjectMeta: metav1.ObjectMeta{
Name: workName,
Namespace: namespace,
Labels: map[string]string{
fleetv1beta1.ParentResourceSnapshotIndexLabel: "1",
},
Annotations: map[string]string{
fleetv1beta1.ParentResourceSnapshotNameAnnotation: "snapshot-1",
fleetv1beta1.ParentClusterResourceOverrideSnapshotHashAnnotation: "hash1",
fleetv1beta1.ParentResourceOverrideSnapshotHashAnnotation: "hash2",
},
},
Spec: fleetv1beta1.WorkSpec{
Workload: fleetv1beta1.WorkloadTemplate{
Manifests: []fleetv1beta1.Manifest{{RawExtension: runtime.RawExtension{Raw: []byte("{}")}}},
},
},
},
expectChanged: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
scheme := serviceScheme(t)
objects := []client.Object{resourceSnapshot}
if tt.existingWork != nil {
objects = append(objects, tt.existingWork)
}
fakeClient := fake.NewClientBuilder().
WithStatusSubresource(objects...).
WithScheme(scheme).
WithObjects(objects...).
Build()
// Create reconciler with custom client
reconciler := &Reconciler{
Client: fakeClient,
recorder: record.NewFakeRecorder(10),
InformerManager: &informer.FakeManager{},
}
changed, _ := reconciler.upsertWork(ctx, newWork, tt.existingWork, resourceSnapshot)
if changed != tt.expectChanged {
t.Fatalf("expected changed: %v, got: %v", tt.expectChanged, changed)
}
upsertedWork := &fleetv1beta1.Work{}
if fakeClient.Get(ctx, client.ObjectKeyFromObject(newWork), upsertedWork) != nil {
t.Fatalf("failed to get upserted work")
}
if diff := cmp.Diff(newWork, upsertedWork, cmpOptions...); diff != "" {
t.Errorf("upsertWork didn't update the work, mismatch (-want +got):\n%s", diff)
}
if tt.expectChanged {
// check if the deployment is applied
var u unstructured.Unstructured
if err := u.UnmarshalJSON(upsertedWork.Spec.Workload.Manifests[0].Raw); err != nil {
t.Fatalf("Failed to unmarshal the result: %v, want nil", err)
}
var deployment appsv1.Deployment
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, &deployment); err != nil {
t.Fatalf("Failed to convert the result to deployment: %v, want nil", err)
}
if diff := cmp.Diff(testDeployment, deployment); diff != "" {
t.Errorf("The new Deployment mismatch (-want, +got):\n%s", diff)
}
}
})
}
}

func TestBuildAllWorkAppliedCondition(t *testing.T) {
tests := map[string]struct {
works map[string]*fleetv1beta1.Work
Expand Down

0 comments on commit e61bf1a

Please sign in to comment.