From ce4ddbea8a7a2a7889feb3511f994b6f31834a67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Reme=C5=A1?= Date: Wed, 19 Apr 2023 10:12:59 +0200 Subject: [PATCH 1/2] DVO-105: extends the logic of matching "app" labels --- pkg/controller/generic_reconciler.go | 72 +++--- pkg/controller/generic_reconciler_test.go | 287 +++++++++++++++------- pkg/utils/app_label.go | 100 ++++++++ pkg/utils/app_label_test.go | 106 ++++++++ 4 files changed, 452 insertions(+), 113 deletions(-) create mode 100644 pkg/utils/app_label.go create mode 100644 pkg/utils/app_label_test.go diff --git a/pkg/controller/generic_reconciler.go b/pkg/controller/generic_reconciler.go index 183d1f2d..57e88e48 100644 --- a/pkg/controller/generic_reconciler.go +++ b/pkg/controller/generic_reconciler.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "sort" "strconv" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -13,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" + "github.com/app-sre/deployment-validation-operator/pkg/utils" "github.com/app-sre/deployment-validation-operator/pkg/validations" "github.com/go-logr/logr" @@ -138,32 +140,21 @@ func (gr *GenericReconciler) reconcileEverything(ctx context.Context) error { return nil } -// getAppLabel tries to read "app" label from the provided unstructured object -func getAppLabel(object *unstructured.Unstructured) (string, error) { - appLabel, found, err := unstructured.NestedString(object.Object, "metadata", "labels", "app") - // if not found try another path - e.g for PDB resource - if !found { - appLabel, found, err = unstructured.NestedString(object.Object, - "spec", "selector", "matchLabels", "app") - if err != nil { - return "", err - } - if !found { - return "", fmt.Errorf("can't find any 'app' label for %s resource from %s namespace", - object.GetName(), object.GetNamespace()) - } - } - if err != nil { - return "", err - } - return appLabel, nil -} - // groupAppObjects iterates over provided GroupVersionKind in given namespace // and returns map of objects grouped by their "app" label func (gr *GenericReconciler) groupAppObjects(ctx context.Context, namespace string, gvks []schema.GroupVersionKind) (map[string][]*unstructured.Unstructured, error) { relatedObjects := make(map[string][]*unstructured.Unstructured) + + // sorting GVKs is very important for getting the consistent results + // when trying to match the 'app' label values. We must be sure that + // resources from the group apps/v1 are processed between first. + sort.Slice(gvks, func(i, j int) bool { + f := gvks[i] + s := gvks[j] + return f.Group < s.Group + }) + for _, gvk := range gvks { list := unstructured.UnstructuredList{} listOptions := &client.ListOptions{ @@ -179,12 +170,7 @@ func (gr *GenericReconciler) groupAppObjects(ctx context.Context, for i := range list.Items { obj := &list.Items[i] - appLabel, err := getAppLabel(obj) - if err != nil { - // swallow the error here. it will be too noisy to log - continue - } - relatedObjects[appLabel] = append(relatedObjects[appLabel], obj) + processObjectLabelSelectors(obj, relatedObjects) } listContinue := list.GetContinue() @@ -193,11 +179,40 @@ func (gr *GenericReconciler) groupAppObjects(ctx context.Context, } listOptions.Continue = listContinue } - } return relatedObjects, nil } +// processObjectLabelSelectors gets 'app' label selectors from the respective object and parses them and stores +// them in the 'relatedObjects' map. +func processObjectLabelSelectors(obj *unstructured.Unstructured, relatedObjects map[string][]*unstructured.Unstructured) { + appSelectors, err := utils.GetAppSelectors(obj) + if err != nil { + // swallow the error here. it will be too noisy to log + return + } + for _, as := range appSelectors { + switch as.Operator { + case metav1.LabelSelectorOpExists: + for k := range relatedObjects { + relatedObjects[k] = append(relatedObjects[k], obj) + } + case metav1.LabelSelectorOpIn: + for v := range as.Values { + relatedObjects[v] = append(relatedObjects[v], obj) + } + case metav1.LabelSelectorOpNotIn: + for selectorVal := range as.Values { + for appLabelVal := range relatedObjects { + if appLabelVal != selectorVal { + relatedObjects[appLabelVal] = append(relatedObjects[appLabelVal], obj) + } + } + } + } + } +} + func (gr *GenericReconciler) processNamespacedResources( ctx context.Context, gvks []schema.GroupVersionKind, namespaces *[]namespace) error { @@ -319,6 +334,5 @@ func (gr GenericReconciler) getNamespacedResourcesGVK(resources []metav1.APIReso namespacedResources = append(namespacedResources, gvk) } } - return namespacedResources } diff --git a/pkg/controller/generic_reconciler_test.go b/pkg/controller/generic_reconciler_test.go index afd6ff4f..8751fc5f 100644 --- a/pkg/controller/generic_reconciler_test.go +++ b/pkg/controller/generic_reconciler_test.go @@ -84,88 +84,6 @@ func TestHelperFunctions(t *testing.T) { }) } -func TestGetAppLabel(t *testing.T) { - tests := []struct { - testName string - object runtime.Object - expectedLabel string - expectedError error - }{ - { - testName: "Pod with defined label", - object: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "app-A", - Labels: map[string]string{ - "app": "app-A", - }, - }, - }, - expectedLabel: "app-A", - expectedError: nil, - }, - { - testName: "Pod with undefined label", - object: &v1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "empty-app", - Namespace: "test", - }, - }, - expectedLabel: "", - expectedError: fmt. - Errorf("can't find any 'app' label for empty-app resource from test namespace"), - }, - { - testName: "PDB with defined selector label", - object: &policyv1.PodDisruptionBudget{ - ObjectMeta: metav1.ObjectMeta{ - Name: "pdb-A", - Namespace: "test", - }, - Spec: policyv1.PodDisruptionBudgetSpec{ - Selector: &metav1.LabelSelector{ - MatchLabels: map[string]string{ - "app": "app-with-pdb", - }, - }, - }, - }, - expectedLabel: "app-with-pdb", - expectedError: nil, - }, - { - testName: "PDB with empty selector", - object: &policyv1.PodDisruptionBudget{ - ObjectMeta: metav1.ObjectMeta{ - Name: "empty-app", - Namespace: "test", - }, - }, - expectedLabel: "", - expectedError: fmt. - Errorf("can't find any 'app' label for empty-app resource from test namespace"), - }, - } - - for _, tt := range tests { - t.Run(tt.testName, func(t *testing.T) { - o, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tt.object) - assert.NoError(t, err) - u := &unstructured.Unstructured{ - Object: o, - } - label, err := getAppLabel(u) - if tt.expectedError != nil { - assert.Error(t, err, tt.expectedError) - } else { - assert.NoError(t, err) - } - assert.Equal(t, tt.expectedLabel, label) - }) - } -} - func TestGroupAppObjects(t *testing.T) { tests := []struct { name string @@ -288,6 +206,207 @@ func TestGroupAppObjects(t *testing.T) { "B": {"test-B-pod", "test-B-deployment"}, }, }, + { + name: "Four deployments with multiple various matching PDBs", + namespace: "test", + objs: []client.Object{ + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment-A", + Namespace: "test", + Labels: map[string]string{ + "app": "A", + }, + }, + }, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment-B", + Namespace: "test", + Labels: map[string]string{ + "app": "B", + }, + }, + }, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment-C", + Namespace: "test", + Labels: map[string]string{ + "app": "C", + }, + }, + }, + &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-deployment-D", + Namespace: "test", + Labels: map[string]string{ + "app": "D", + }, + }, + }, + &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pdb-A-B-C", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpIn, + Values: []string{"A", "B", "C"}, + }, + }, + }, + }, + }, + &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pdb-not-in-C", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"C"}, + }, + }, + }, + }, + }, + &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pdb-exists", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpExists, + }, + }, + }, + }, + }, + &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pdb-not-in-D", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"D"}, + }, + }, + }, + }, + }, + }, + gvks: []schema.GroupVersionKind{ + { + Group: "policy", + Kind: "PodDisruptionBudget", + Version: "v1", + }, + { + Group: "apps", + Kind: "Deployment", + Version: "v1", + }, + }, + expectedNames: map[string][]string{ + "A": {"test-pdb-A-B-C", "test-pdb-not-in-C", "test-pdb-exists", "test-deployment-A", "test-pdb-not-in-D"}, + "B": {"test-pdb-A-B-C", "test-pdb-not-in-C", "test-pdb-exists", "test-deployment-B", "test-pdb-not-in-D"}, + "C": {"test-pdb-A-B-C", "test-pdb-exists", "test-deployment-C", "test-pdb-not-in-D"}, + "D": {"test-deployment-D", "test-pdb-exists", "test-pdb-not-in-C"}, + }, + }, + { + name: "Two StatefulSets with some matching PDBs", + namespace: "test", + objs: []client.Object{ + &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pdb-not-in-A", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"A"}, + }, + }, + }, + }, + }, + &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pdb-not-in-B", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "app", + Operator: metav1.LabelSelectorOpNotIn, + Values: []string{"B"}, + }, + }, + }, + }, + }, + &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "statefulset-A", + Namespace: "test", + Labels: map[string]string{ + "app": "A", + }, + }, + }, + &appsv1.StatefulSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "statefulset-B", + Namespace: "test", + Labels: map[string]string{ + "app": "B", + }, + }, + }, + }, + gvks: []schema.GroupVersionKind{ + { + Group: "apps", + Kind: "StatefulSet", + Version: "v1", + }, + { + Group: "policy", + Kind: "PodDisruptionBudget", + Version: "v1", + }, + }, + expectedNames: map[string][]string{ + "A": {"statefulset-A", "pdb-not-in-B"}, + "B": {"statefulset-B", "pdb-not-in-A"}, + }, + }, } for _, tt := range tests { @@ -301,8 +420,8 @@ func TestGroupAppObjects(t *testing.T) { objects, ok := groupMap[expectedLabel] assert.True(t, ok, "can't find label %s", expectedLabel) actualNames := unstructuredToNames(objects) - for _, exoectedName := range expectedNames { - assert.Contains(t, actualNames, exoectedName) + for _, expectedName := range expectedNames { + assert.Contains(t, actualNames, expectedName, "can't find %s for label value %s", expectedName, expectedLabel) } } }) diff --git a/pkg/utils/app_label.go b/pkg/utils/app_label.go new file mode 100644 index 00000000..861e769c --- /dev/null +++ b/pkg/utils/app_label.go @@ -0,0 +1,100 @@ +package utils + +import ( + "fmt" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/sets" +) + +// AppSelector is a helper type which +// consists of LabelSelectorOperator type and +// string values +type AppSelector struct { + Operator metav1.LabelSelectorOperator + Values sets.Set[string] +} + +// GetAppSelectors tries to get values (there can be more) of the "app" label. +// First it tries to read "metadata.labels.app" path (e.g for Deployments) if not found, +// then it tries to read "spec.selector.matchLabels.app" path (e.g for PodDisruptionBudget) if not found, +// then it tries to read "spec.selector.matchExpressions" path. +func GetAppSelectors(object *unstructured.Unstructured) ([]AppSelector, error) { + appLabel, found, err := unstructured.NestedString(object.Object, "metadata", "labels", "app") + if err != nil { + return nil, err + } + if found { + return []AppSelector{ + { + Operator: metav1.LabelSelectorOpIn, + Values: sets.New(appLabel), + }, + }, nil + } + // if not found try spec.selector.matchLabels path - e.g for PDB resource + appLabel, found, err = unstructured.NestedString(object.Object, + "spec", "selector", "matchLabels", "app") + if err != nil { + return nil, err + } + if found { + return []AppSelector{ + { + Operator: metav1.LabelSelectorOpIn, + Values: sets.New(appLabel), + }, + }, nil + } + // if not found try spec.selector.matchExpressions path + expressions, found, err := unstructured.NestedSlice(object.Object, "spec", "selector", "matchExpressions") + if err != nil { + return nil, err + } + if !found { + return nil, fmt.Errorf("can't find any 'app' label for %s resource from %s namespace", + object.GetName(), object.GetNamespace()) + } + appSelectors := parseMatchExpressions(expressions) + return appSelectors, nil +} + +// parseMatchExpressions tries to parse provided untyped slice of expressions +// and return a slice of appSelectors. Any expression key with a value other than "app" is skipped. +// Label selector operator "DoesNotExist" is skipped too. +func parseMatchExpressions(expressions []interface{}) []AppSelector { + appSelectors := []AppSelector{} + for _, exp := range expressions { + expAsMap, ok := exp.(map[string]interface{}) + if !ok { + continue + } + if expAsMap["key"] != "app" { + continue + } + appSelector := AppSelector{} + switch expAsMap["operator"] { + case "In": + values, _, err := unstructured.NestedStringSlice(expAsMap, "values") + if err != nil { + continue + } + appSelector.Operator = metav1.LabelSelectorOpIn + appSelector.Values = sets.New(values...) + case "NotIn": + values, _, err := unstructured.NestedStringSlice(expAsMap, "values") + if err != nil { + continue + } + appSelector.Operator = metav1.LabelSelectorOpNotIn + appSelector.Values = sets.New(values...) + case "Exists": + appSelector.Operator = metav1.LabelSelectorOpExists + case "DoesNotExist": + continue + } + appSelectors = append(appSelectors, appSelector) + } + return appSelectors +} diff --git a/pkg/utils/app_label_test.go b/pkg/utils/app_label_test.go new file mode 100644 index 00000000..13d896f7 --- /dev/null +++ b/pkg/utils/app_label_test.go @@ -0,0 +1,106 @@ +package utils + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + policyv1 "k8s.io/api/policy/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" +) + +func TestGetAppSelectors(t *testing.T) { + tests := []struct { + testName string + object runtime.Object + expectedLabelSelectors []AppSelector + expectedError error + }{ + { + testName: "Pod with defined label", + object: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "app-A", + Labels: map[string]string{ + "app": "app-A", + }, + }, + }, + expectedLabelSelectors: []AppSelector{ + { + Operator: metav1.LabelSelectorOpIn, + Values: sets.New("app-A"), + }, + }, + expectedError: nil, + }, + { + testName: "Pod with undefined label", + object: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-app", + Namespace: "test", + }, + }, + expectedLabelSelectors: nil, + expectedError: fmt. + Errorf("can't find any 'app' label for empty-app resource from test namespace"), + }, + { + testName: "PDB with defined selector label", + object: &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pdb-A", + Namespace: "test", + }, + Spec: policyv1.PodDisruptionBudgetSpec{ + Selector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "app": "app-with-pdb", + }, + }, + }, + }, + expectedLabelSelectors: []AppSelector{ + { + Operator: metav1.LabelSelectorOpIn, + Values: sets.New("app-with-pdb"), + }, + }, + expectedError: nil, + }, + { + testName: "PDB with empty selector", + object: &policyv1.PodDisruptionBudget{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-app", + Namespace: "test", + }, + }, + expectedLabelSelectors: nil, + expectedError: fmt. + Errorf("can't find any 'app' label for empty-app resource from test namespace"), + }, + } + + for _, tt := range tests { + t.Run(tt.testName, func(t *testing.T) { + o, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tt.object) + assert.NoError(t, err) + u := &unstructured.Unstructured{ + Object: o, + } + appSelectors, err := GetAppSelectors(u) + if tt.expectedError != nil { + assert.Error(t, err, tt.expectedError) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.expectedLabelSelectors, appSelectors) + }) + } +} From 2a5b7aef392494940d4841cec9ef776170ff42d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Reme=C5=A1?= Date: Mon, 29 May 2023 16:02:45 +0200 Subject: [PATCH 2/2] fix lint --- pkg/controller/generic_reconciler.go | 3 ++- pkg/controller/generic_reconciler_test.go | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/pkg/controller/generic_reconciler.go b/pkg/controller/generic_reconciler.go index 57e88e48..53ec3981 100644 --- a/pkg/controller/generic_reconciler.go +++ b/pkg/controller/generic_reconciler.go @@ -185,7 +185,8 @@ func (gr *GenericReconciler) groupAppObjects(ctx context.Context, // processObjectLabelSelectors gets 'app' label selectors from the respective object and parses them and stores // them in the 'relatedObjects' map. -func processObjectLabelSelectors(obj *unstructured.Unstructured, relatedObjects map[string][]*unstructured.Unstructured) { +func processObjectLabelSelectors(obj *unstructured.Unstructured, + relatedObjects map[string][]*unstructured.Unstructured) { appSelectors, err := utils.GetAppSelectors(obj) if err != nil { // swallow the error here. it will be too noisy to log diff --git a/pkg/controller/generic_reconciler_test.go b/pkg/controller/generic_reconciler_test.go index 8751fc5f..570d0968 100644 --- a/pkg/controller/generic_reconciler_test.go +++ b/pkg/controller/generic_reconciler_test.go @@ -327,8 +327,8 @@ func TestGroupAppObjects(t *testing.T) { }, }, expectedNames: map[string][]string{ - "A": {"test-pdb-A-B-C", "test-pdb-not-in-C", "test-pdb-exists", "test-deployment-A", "test-pdb-not-in-D"}, - "B": {"test-pdb-A-B-C", "test-pdb-not-in-C", "test-pdb-exists", "test-deployment-B", "test-pdb-not-in-D"}, + "A": {"test-pdb-A-B-C", "test-pdb-not-in-C", "test-pdb-exists", "test-deployment-A", "test-pdb-not-in-D"}, //nolint:lll + "B": {"test-pdb-A-B-C", "test-pdb-not-in-C", "test-pdb-exists", "test-deployment-B", "test-pdb-not-in-D"}, //nolint:lll "C": {"test-pdb-A-B-C", "test-pdb-exists", "test-deployment-C", "test-pdb-not-in-D"}, "D": {"test-deployment-D", "test-pdb-exists", "test-pdb-not-in-C"}, }, @@ -421,7 +421,8 @@ func TestGroupAppObjects(t *testing.T) { assert.True(t, ok, "can't find label %s", expectedLabel) actualNames := unstructuredToNames(objects) for _, expectedName := range expectedNames { - assert.Contains(t, actualNames, expectedName, "can't find %s for label value %s", expectedName, expectedLabel) + assert.Contains(t, actualNames, expectedName, + "can't find %s for label value %s", expectedName, expectedLabel) } } })