Repository: kubernetes-sigs/node-feature-discovery
Pull request #668 (2021-11-24 11:02:32)

pkg/apis/nfd: stricter format checking for template labels
Require that the expanded LabelsTemplate has values. That is, the
(expanded) template must consist of key=value pairs separated by
newlines. No default value will be assigned and we now return an error
if a (non-empty) line not conforming with the key=value format is
encountered.

Commit c8d73666d described that the value defaults to "true" if not
specified. That was not the case and we defaulted to an empty string,
instead.

An example:

```
  - name: "my rule"
    labelsTemplate: |
      my.label.1=foo
      my.label.2=
```

Would create these labels:

```
  "my.label.1": "foo"
  "my.label.2": ""
```

Further, the following:

```
  - name: "my failing rule"
    labelsTemplate: |
      my.label.3
```

will cause an error in the rule processing.


k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/node-feature-discovery/pull/668#" title="Author self-approved">marquiz</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fnode-feature-discovery).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/node-feature-discovery/blob/master/OWNERS)~~ [marquiz]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
marquiz: Will change this 
/hold
marquiz: > An alternative to solution would be to ditch the default value ("true") and e.g. just error out if the value part is missing (i.e. the expanded template value does not have an equals sign).

Changed the PR to behave like this.

/unhold
k8s-ci-robot: @marquiz: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-node-feature-discovery-verify | a8057b1c44b720f3f7ac73f76135be016c863bd0 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_node-feature-discovery/668/pull-node-feature-discovery-verify/1463586322609344512) | true | `/test pull-node-feature-discovery-verify`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=node-feature-discovery&pr=668). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Amarquiz). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/node-feature-discovery/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @marquiz: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-node-feature-discovery-verify | 9758eceec887934ea9a6bfa05227b1d0c38d1c8a | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_node-feature-discovery/668/pull-node-feature-discovery-verify/1463588297019232256) | true | `/test pull-node-feature-discovery-verify`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=node-feature-discovery&pr=668). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Amarquiz). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/node-feature-discovery/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/pkg/apis/nfd/v1alpha1/rule.go b/pkg/apis/nfd/v1alpha1/rule.go
index 40aad3714..704b3d01d 100644
--- a/pkg/apis/nfd/v1alpha1/rule.go
+++ b/pkg/apis/nfd/v1alpha1/rule.go
@@ -85,14 +85,14 @@ func (r *Rule) executeLabelsTemplate(in matchedFeatures, out map[string]string)
 	if r.labelsTemplate == nil {
 		t, err := newTemplateHelper(r.LabelsTemplate)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to parse LabelsTemplate: %w", err)
 		}
 		r.labelsTemplate = t
 	}
 
 	labels, err := r.labelsTemplate.expandMap(in)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to expand LabelsTemplate: %w", err)
 	}
 	for k, v := range labels {
 		out[k] = v
@@ -212,7 +212,7 @@ func (h *templateHelper) expandMap(data interface{}) (map[string]string, error)
 		if trimmed := strings.TrimSpace(item); trimmed != "" {
 			split := strings.SplitN(trimmed, "=", 2)
 			if len(split) == 1 {
-				out[split[0]] = ""
+				return nil, fmt.Errorf("missing value in expanded template line %q, (format must be '<key>=<value>')", trimmed)
 			} else {
 				out[split[0]] = split[1]
 			}


Test patch:
diff --git a/pkg/apis/nfd/v1alpha1/rule_test.go b/pkg/apis/nfd/v1alpha1/rule_test.go
index 6ec28673d..0cadd0676 100644
--- a/pkg/apis/nfd/v1alpha1/rule_test.go
+++ b/pkg/apis/nfd/v1alpha1/rule_test.go
@@ -244,6 +244,8 @@ func TestTemplating(t *testing.T) {
 	r1 := Rule{
 		Labels: map[string]string{"label-1": "label-val-1"},
 		LabelsTemplate: `
+label-1=will-be-overridden
+label-2=
 {{range .domain_1.kf_1}}kf-{{.Name}}=present
 {{end}}
 {{range .domain_1.vf_1}}vf-{{.Name}}=vf-{{.Value}}
@@ -280,6 +282,7 @@ func TestTemplating(t *testing.T) {
 
 	expectedLabels := map[string]string{
 		"label-1": "label-val-1",
+		"label-2": "",
 		// From kf_1 template
 		"kf-key-a": "present",
 		"kf-key-c": "present",
@@ -295,4 +298,33 @@ func TestTemplating(t *testing.T) {
 	m, err := r1.Execute(f)
 	assert.Nilf(t, err, "unexpected error: %v", err)
 	assert.Equal(t, expectedLabels, m, "instances should have matched")
+
+	//
+	// Test error cases
+	//
+	r2 := Rule{
+		MatchFeatures: FeatureMatcher{
+			// We need at least one matcher to match to execute the template.
+			// Use a simple empty matchexpression set to match anything.
+			FeatureMatcherTerm{
+				Feature:          "domain_1.kf_1",
+				MatchExpressions: MatchExpressionSet{},
+			},
+		},
+	}
+
+	r2.LabelsTemplate = "foo=bar"
+	m, err = r2.Execute(f)
+	assert.Nil(t, err)
+	assert.Equal(t, map[string]string{"foo": "bar"}, m, "instances should have matched")
+
+	r2.labelsTemplate = nil
+	r2.LabelsTemplate = "foo"
+	_, err = r2.Execute(f)
+	assert.Error(t, err)
+
+	r2.labelsTemplate = nil
+	r2.LabelsTemplate = "{{"
+	_, err = r2.Execute(f)
+	assert.Error(t, err)
 }