Add unit-tests and fix a trailing comma bug in generateExcludeNamespacesFieldSelector

Signed-off-by: Richard Wall <[email protected]>

Author: wallrj

pkg/datagatherer/k8s/dynamic.go

@@ -83,10 +83,13 @@ func (c *ConfigDynamic) validate() error {
 		errors = append(errors, "invalid configuration: GroupVersionResource.Resource cannot be empty")
 	}
 
-	for _, selectorString := range c.FieldSelectors {
+	for i, selectorString := range c.FieldSelectors {
+		if selectorString == "" {
+			errors = append(errors, fmt.Sprintf("invalid field selector %d: must not be empty", i))
+		}
 		_, err := fields.ParseSelector(selectorString)
 		if err != nil {
-			errors = append(errors, fmt.Sprintf("invalid field selector %q: %s", selectorString, err))
+			errors = append(errors, fmt.Sprintf("invalid field selector %d: %s", i, err))
 		}
 	}
 
@@ -161,9 +164,11 @@ func (c *ConfigDynamic) newDataGathererWithClient(ctx context.Context, cl dynami
 		return nil, err
 	}
 	// init shared informer for selected namespaces
-	fieldSelector := generateFieldSelector(c.ExcludeNamespaces)
+	fieldSelector := generateExcludedNamespacesFieldSelector(c.ExcludeNamespaces)
 
-	// add any custom field selectors to the namespace selector
+	// Add any custom field selectors to the excluded namespaces selector
+	// The selectors have already been validated, so it is safe to use
+	// ParseSelectorOrDie here.
 	for _, selectorString := range c.FieldSelectors {
 		fieldSelector = fields.AndSelectors(fieldSelector, fields.ParseSelectorOrDie(selectorString))
 	}
@@ -437,17 +442,17 @@ func namespaceResourceInterface(iface dynamic.NamespaceableResourceInterface, na
 	return iface.Namespace(namespace)
 }
 
-// generateFieldSelector creates a field selector string from a list of
-// namespaces to exclude.
-func generateFieldSelector(excludeNamespaces []string) fields.Selector {
-	fieldSelector := fields.Nothing()
+// generateExcludedNamespacesFieldSelector creates a field selector string from
+// a list of namespaces to exclude.
+func generateExcludedNamespacesFieldSelector(excludeNamespaces []string) fields.Selector {
+	var selectors []fields.Selector
 	for _, excludeNamespace := range excludeNamespaces {
 		if excludeNamespace == "" {
 			continue
 		}
-		fieldSelector = fields.AndSelectors(fields.OneTermNotEqualSelector("metadata.namespace", excludeNamespace), fieldSelector)
+		selectors = append(selectors, fields.OneTermNotEqualSelector("metadata.namespace", excludeNamespace))
 	}
-	return fieldSelector
+	return fields.AndSelectors(selectors...)
 }
 
 func isIncludedNamespace(namespace string, namespaces []string) bool {

pkg/datagatherer/k8s/dynamic_test.go

@@ -105,8 +105,12 @@ func sortGatheredResources(list []*api.GatheredResource) {
 func TestNewDataGathererWithClientAndDynamicInformer(t *testing.T) {
 	ctx := context.Background()
 	config := ConfigDynamic{
-		IncludeNamespaces:    []string{"a"},
+		ExcludeNamespaces:    []string{"kube-system"},
 		GroupVersionResource: schema.GroupVersionResource{Group: "foobar", Version: "v1", Resource: "foos"},
+		FieldSelectors: []string{
+			"type!=kubernetes.io/service-account-token",
+			"type!=kubernetes.io/dockercfg",
+		},
 	}
 	cl := fake.NewSimpleDynamicClient(runtime.NewScheme())
 	dg, err := config.newDataGathererWithClient(ctx, cl, nil)
@@ -121,7 +125,8 @@ func TestNewDataGathererWithClientAndDynamicInformer(t *testing.T) {
 		groupVersionResource: config.GroupVersionResource,
 		// it's important that the namespaces are set as the IncludeNamespaces
 		// during initialization
-		namespaces: config.IncludeNamespaces,
+		namespaces:    config.IncludeNamespaces,
+		fieldSelector: "metadata.namespace!=kube-system,type!=kubernetes.io/service-account-token,type!=kubernetes.io/dockercfg",
 	}
 
 	gatherer := dg.(*DataGathererDynamic)
@@ -150,6 +155,9 @@ func TestNewDataGathererWithClientAndDynamicInformer(t *testing.T) {
 	if gatherer.nativeSharedInformer != nil {
 		t.Errorf("unexpected nativeSharedInformer value: %v. should be nil", gatherer.nativeSharedInformer)
 	}
+	if !reflect.DeepEqual(gatherer.fieldSelector, expected.fieldSelector) {
+		t.Errorf("expected %v, got %v", expected.fieldSelector, gatherer.fieldSelector)
+	}
 }
 
 func TestNewDataGathererWithClientAndSharedIndexInformer(t *testing.T) {
@@ -216,6 +224,8 @@ exclude-namespaces:
 # from the config file
 include-namespaces:
 - default
+field-selectors:
+- type!=kubernetes.io/service-account-token
 `
 
 	expectedGVR := schema.GroupVersionResource{
@@ -231,6 +241,10 @@ include-namespaces:
 
 	expectedIncludeNamespaces := []string{"default"}
 
+	expectedFieldSelectors := []string{
+		"type!=kubernetes.io/service-account-token",
+	}
+
 	cfg := ConfigDynamic{}
 	err := yaml.Unmarshal([]byte(textCfg), &cfg)
 	if err != nil {
@@ -251,6 +265,9 @@ include-namespaces:
 	if got, want := cfg.IncludeNamespaces, expectedIncludeNamespaces; !reflect.DeepEqual(got, want) {
 		t.Errorf("IncludeNamespaces does not match: got=%+v want=%+v", got, want)
 	}
+	if got, want := cfg.FieldSelectors, expectedFieldSelectors; !reflect.DeepEqual(got, want) {
+		t.Errorf("FieldSelectors does not match: got=%+v want=%+v", got, want)
+	}
 }
 
 func TestConfigDynamicValidate(t *testing.T) {
@@ -275,17 +292,42 @@ func TestConfigDynamicValidate(t *testing.T) {
 			},
 			ExpectedError: "cannot set excluded and included namespaces",
 		},
+		{
+			Config: ConfigDynamic{
+				GroupVersionResource: schema.GroupVersionResource{
+					Group:    "",
+					Version:  "v1",
+					Resource: "secrets",
+				},
+				FieldSelectors: []string{""},
+			},
+			ExpectedError: "invalid field selector 0: must not be empty",
+		},
+		{
+			Config: ConfigDynamic{
+				GroupVersionResource: schema.GroupVersionResource{
+					Group:    "",
+					Version:  "v1",
+					Resource: "secrets",
+				},
+				FieldSelectors: []string{"foo"},
+			},
+			ExpectedError: "invalid field selector 0: invalid selector: 'foo'; can't understand 'foo'",
+		},
 	}
 
 	for _, test := range tests {
 		err := test.Config.validate()
-		if !strings.Contains(err.Error(), test.ExpectedError) {
+		if err == nil && test.ExpectedError != "" {
+			t.Errorf("expected error: %q, got: nil", test.ExpectedError)
+		}
+		if err != nil && !strings.Contains(err.Error(), test.ExpectedError) {
 			t.Errorf("expected %s, got %s", test.ExpectedError, err.Error())
 		}
 	}
 }
 
-func TestGenerateFieldSelector(t *testing.T) {
+func TestGenerateExcludedNamespacesFieldSelector(t *testing.T) {
 	tests := []struct {
 		ExcludeNamespaces     []string
 		ExpectedFieldSelector string
@@ -300,19 +342,19 @@ func TestGenerateFieldSelector(t *testing.T) {
 			ExcludeNamespaces: []string{
 				"kube-system",
 			},
-			ExpectedFieldSelector: "metadata.namespace!=kube-system,",
+			ExpectedFieldSelector: "metadata.namespace!=kube-system",
 		},
 		{
 			ExcludeNamespaces: []string{
 				"kube-system",
 				"my-namespace",
 			},
-			ExpectedFieldSelector: "metadata.namespace!=my-namespace,metadata.namespace!=kube-system,",
+			ExpectedFieldSelector: "metadata.namespace!=kube-system,metadata.namespace!=my-namespace",
 		},
 	}
 
 	for _, test := range tests {
-		fieldSelector := generateFieldSelector(test.ExcludeNamespaces).String()
+		fieldSelector := generateExcludedNamespacesFieldSelector(test.ExcludeNamespaces).String()
 		if fieldSelector != test.ExpectedFieldSelector {
 			t.Errorf("ExpectedFieldSelector does not match: got=%+v want=%+v", fieldSelector, test.ExpectedFieldSelector)
 		}