Skip to content

Commit

Permalink
pkg: objstate: forbid merging data for different kinds
Browse files Browse the repository at this point in the history 
Verify that the objects to be merged are of same kinds otherwise return
an error, and add unit tests for this.

Signed-off-by: Shereen Haj <[email protected]>
  • Loading branch information
@shajmakh
shajmakh committed Feb 3, 2025
1 parent bf86404 commit a145443
Show file tree
Hide file tree
Showing 2 changed files with 134 additions and 10 deletions.
31 changes: 23 additions & 8 deletions pkg/objectstate/merge/merge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@ var (
func ServiceAccountForUpdate(current, updated client.Object) (client.Object, error) {
curSA, ok := current.(*corev1.ServiceAccount)
if !ok {
return updated, ErrWrongObjectType
return nil, ErrWrongObjectType
}
updSA, ok := updated.(*corev1.ServiceAccount)
if !ok {
return updated, ErrMismatchingObjects
return nil, ErrMismatchingObjects
}
updSA.Secrets = curSA.Secrets
return MetadataForUpdate(current, updated)
Expand All @@ -47,6 +47,9 @@ func ObjectForUpdate(current, updated client.Object) (client.Object, error) {
}

func MetadataForUpdate(current, updated client.Object) (client.Object, error) {
if !isSameKind(current, updated) {
return nil, ErrMismatchingObjects
}
updated.SetCreationTimestamp(current.GetCreationTimestamp())
updated.SetSelfLink(current.GetSelfLink())
updated.SetGeneration(current.GetGeneration())
Expand All @@ -55,13 +58,17 @@ func MetadataForUpdate(current, updated client.Object) (client.Object, error) {
updated.SetManagedFields(current.GetManagedFields())
updated.SetFinalizers(current.GetFinalizers())

_ = Annotations(current, updated)
_ = Labels(current, updated)
_, _ = Annotations(current, updated)
_, _ = Labels(current, updated)

return updated, nil
}

func Annotations(current, updated client.Object) client.Object {
func Annotations(current, updated client.Object) (client.Object, error) {
if !isSameKind(current, updated) {
return nil, ErrMismatchingObjects
}

updatedAnnotations := updated.GetAnnotations()
curAnnotations := current.GetAnnotations()

Expand All @@ -76,10 +83,14 @@ func Annotations(current, updated client.Object) client.Object {
if len(curAnnotations) != 0 {
updated.SetAnnotations(curAnnotations)
}
return updated
return updated, nil
}

func Labels(current, updated client.Object) client.Object {
func Labels(current, updated client.Object) (client.Object, error) {
if !isSameKind(current, updated) {
return nil, ErrMismatchingObjects
}

updatedLabels := updated.GetLabels()
curLabels := current.GetLabels()

Expand All @@ -94,5 +105,9 @@ func Labels(current, updated client.Object) client.Object {
if len(curLabels) != 0 {
updated.SetLabels(curLabels)
}
return updated
return updated, nil
}

func isSameKind(a, b client.Object) bool {
return a.GetObjectKind().GroupVersionKind().Kind == b.GetObjectKind().GroupVersionKind().Kind
}
113 changes: 111 additions & 2 deletions pkg/objectstate/merge/merge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ func TestLabels(t *testing.T) {
name string
input input
expected client.Object
err error
}{
{
name: "no labels shouldn't affect the current labels",
Expand Down Expand Up @@ -81,6 +82,9 @@ func TestLabels(t *testing.T) {
name: "empty labels values should be reflected",
input: input{
current: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
Expand All @@ -90,6 +94,9 @@ func TestLabels(t *testing.T) {
},
},
updated: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
Expand All @@ -99,6 +106,9 @@ func TestLabels(t *testing.T) {
},
},
expected: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
Expand All @@ -112,6 +122,9 @@ func TestLabels(t *testing.T) {
name: "new labels and values should be reflected",
input: input{
current: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
Expand All @@ -121,6 +134,9 @@ func TestLabels(t *testing.T) {
},
},
updated: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
Expand All @@ -131,6 +147,9 @@ func TestLabels(t *testing.T) {
},
},
expected: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
Expand All @@ -141,11 +160,42 @@ func TestLabels(t *testing.T) {
},
},
},
{
name: "merging labels for 2 different kinds is forbidden",
input: input{
current: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Labels: map[string]string{
"test1": "label1",
},
},
},
updated: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Labels: map[string]string{
"test2": "label2",
},
},
},
},
err: ErrMismatchingObjects,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := Labels(test.input.current, test.input.updated)
got, err := Labels(test.input.current, test.input.updated)
if err != test.err {
t.Errorf("got error: %v, while expected to fail due to: %v", err, test.err)
}
if !reflect.DeepEqual(got, test.expected) {
t.Errorf("expected: %v, got: %v", test.expected, got)
}
Expand All @@ -158,11 +208,15 @@ func TestAnnotations(t *testing.T) {
name string
input input
expected client.Object
err error
}{
{
name: "no annotations shouldn't affect the current ones",
input: input{
current: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -172,13 +226,19 @@ func TestAnnotations(t *testing.T) {
},
},
updated: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{},
},
},
},
expected: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -192,6 +252,9 @@ func TestAnnotations(t *testing.T) {
name: "empty annotation values should be reflected",
input: input{
current: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -201,6 +264,9 @@ func TestAnnotations(t *testing.T) {
},
},
updated: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -210,6 +276,9 @@ func TestAnnotations(t *testing.T) {
},
},
expected: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -223,6 +292,9 @@ func TestAnnotations(t *testing.T) {
name: "new annotations and values should be reflected",
input: input{
current: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -232,6 +304,9 @@ func TestAnnotations(t *testing.T) {
},
},
updated: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -242,6 +317,9 @@ func TestAnnotations(t *testing.T) {
},
},
expected: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
Expand All @@ -252,11 +330,42 @@ func TestAnnotations(t *testing.T) {
},
},
},
{
name: "merging annotation for 2 different kinds is forbidden",
input: input{
current: &nropv1.NUMAResourcesOperator{
TypeMeta: metav1.TypeMeta{
Kind: "numaresourcesoperator",
},
ObjectMeta: metav1.ObjectMeta{
Name: "nro",
Annotations: map[string]string{
"ann1": "val1",
},
},
},
updated: &corev1.ServiceAccount{
TypeMeta: metav1.TypeMeta{
Kind: "ServiceAccount",
},
ObjectMeta: metav1.ObjectMeta{
Name: "sa-1",
Annotations: map[string]string{
"ann2": "val2",
},
},
},
},
err: ErrMismatchingObjects,
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
got := Annotations(test.input.current, test.input.updated)
got, err := Annotations(test.input.current, test.input.updated)
if err != test.err {
t.Errorf("got error: %v, while expected to fail due to: %v", err, test.err)
}
if !reflect.DeepEqual(got, test.expected) {
t.Errorf("expected: %v, got: %v", test.expected, got)
}
Expand Down

0 comments on commit a145443

Please sign in to comment.