Return an error when a variable doesn't exist. Add new |? syntax.

changelog/fragments/1760622673-prevent-silently-removing-inputs-when-a-variable-doesnt-exist.yaml

@@ -0,0 +1,54 @@
+# REQUIRED
+# Kind can be one of:
+# - breaking-change: a change to previously-documented behavior
+# - deprecation: functionality that is being removed in a later release
+# - bug-fix: fixes a problem in a previous version
+# - enhancement: extends functionality but does not break or fix existing behavior
+# - feature: new functionality
+# - known-issue: problems that we are aware of in a given version
+# - security: impacts on the security of a product or a user’s deployment.
+# - upgrade: important information for someone upgrading from a prior version
+# - other: does not fit into any of the other categories
+kind: bug-fix
+
+# REQUIRED for all kinds
+# Change summary; a 80ish characters long description of the change.
+summary: Prevent silently removing inputs when a variable doesn't exist
+
+# REQUIRED for breaking-change, deprecation, known-issue
+# Long description; in case the summary is not enough to describe the change
+# this field accommodate a description without length limits.
+description: |
+    Previously, when a variable didn't exist, the entire input would be silently removed without any error or
+    log message. This change fixes that behavior by throwing an error when a variable cannot be substituted
+    in an input.
+
+    If you want the input to be silently removed when a variable doesn't exist, append the optional `|?` syntax to
+    the end of the variable chain (e.g., `${kubernetes.pod.name|?}`).
+
+# REQUIRED for breaking-change, deprecation, known-issue
+impact: |
+    Missing or invalid variables will no longer result in the whole input being silently removed. An error will occur
+    requiring the policy or configuration to be updated to fix the issue.
+
+# REQUIRED for breaking-change, deprecation, known-issue
+action: |
+    Add `|?` syntax to the end of the variable change to bring back previous behavior.
+
+# REQUIRED for all kinds
+# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc.
+component: elastic-agent
+
+# AUTOMATED
+# OPTIONAL to manually add other PR URLs
+# PR URL: A link the PR that added the changeset.
+# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added.
+# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number.
+# Please provide it if you are adding a fragment for a different PR.
+# pr: https://github.com/owner/repo/1234
+
+# AUTOMATED
+# OPTIONAL to manually add other issue URLs
+# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of).
+# If not present is automatically filled by the tooling with the issue linked to the PR number.
+# issue: https://github.com/owner/repo/1234

internal/pkg/agent/application/coordinator/coordinator_unit_test.go

@@ -1321,7 +1321,7 @@ outputs:
   default:
     type: elasticsearch
 inputs:
-  - id: ${TEST_VAR}
+  - id: ${TEST_VAR|?}
     type: filestream
     use_output: default
 `)

internal/pkg/agent/transpiler/inputs.go

@@ -7,6 +7,7 @@ package transpiler
 import (
 	"errors"
 	"fmt"
+	"slices"
 
 	"github.com/cespare/xxhash/v2"
 )
@@ -27,8 +28,10 @@ func RenderInputs(inputs Node, varsArray []*Vars) (Node, error) {
 	var nodes []varIDMap
 	nodesMap := map[uint64]*Dict{}
 	hasher := xxhash.New()
+	inputApplied := make(map[int]bool)
+	inputNoMatchErr := make(map[int]error)
 	for _, vars := range varsArray {
-		for _, node := range l.Value().([]Node) {
+		for inputIdx, node := range l.Value().([]Node) {
 			dict, ok := node.(*Dict)
 			if !ok {
 				continue
@@ -39,8 +42,17 @@ func RenderInputs(inputs Node, varsArray []*Vars) (Node, error) {
 			}
 			// Apply creates a new Node with a deep copy of all the values
 			n, err := dict.Apply(vars)
+			if errors.Is(err, errNoMatchAllowed) {
+				// has an optional variable that didn't match, so we ignore it
+				continue
+			}
 			if errors.Is(err, ErrNoMatch) {
-				// has a variable that didn't exist, so we ignore it
+				// has a required variable that didn't exist
+				if _, exists := inputNoMatchErr[inputIdx]; !exists {
+					// store it; only if it never gets a match will it be an error
+					inputNoMatchErr[inputIdx] = err
+				}
+				// try other vars
 				continue
 			}
 			if err != nil {
@@ -67,8 +79,29 @@ func RenderInputs(inputs Node, varsArray []*Vars) (Node, error) {
 				nodesMap[hash] = dict
 				nodes = append(nodes, varIDMap{vars.ID(), dict})
 			}
+			// input successfully applied
+			inputApplied[inputIdx] = true
 		}
 	}
+
+	// check if any inputs had ErrNoMatch but were never successfully applied
+	var errStrs []string
+	var err error
+	for inputIdx, inputErr := range inputNoMatchErr {
+		if !inputApplied[inputIdx] {
+			// not applied
+			// only add unique errors that way the same variable is not repeated
+			// multiple times cause the error message to be un-readable.
+			if !slices.Contains(errStrs, inputErr.Error()) {
+				errStrs = append(errStrs, inputErr.Error())
+				err = errors.Join(err, inputErr)
+			}
+		}
+	}
+	if err != nil {
+		return nil, err
+	}
+
 	var nInputs []Node
 	for _, node := range nodes {
 		if node.id != "" {

internal/pkg/agent/transpiler/inputs_test.go

@@ -110,7 +110,7 @@ func TestRenderInputs(t *testing.T) {
 					NewKey("key", NewStrVal("${var1.missing|var1.diff}")),
 				}),
 				NewDict([]Node{
-					NewKey("key", NewStrVal("${var1.removed}")),
+					NewKey("key", NewStrVal("${var1.removed|?}")),
 				}),
 			})),
 			expected: NewList([]Node{
@@ -296,7 +296,7 @@ func TestRenderInputs(t *testing.T) {
 				}),
 				mustMakeVars(map[string]interface{}{
 					"var1": map[string]interface{}{
-						"missing": "other",
+						"name": "value4",
 					},
 				}),
 			},
@@ -788,6 +788,53 @@ func TestRenderInputs(t *testing.T) {
 					nil),
 			},
 		},
+		"required var missing causes error": {
+			input: NewKey("inputs", NewList([]Node{
+				NewDict([]Node{
+					NewKey("key", NewStrVal("${var1.missing}")),
+				}),
+			})),
+			err: true,
+			varsArray: []*Vars{
+				mustMakeVars(map[string]interface{}{
+					"var1": map[string]interface{}{
+						"name": "value1",
+					},
+				}),
+			},
+		},
+		"input fails on first vars but succeeds with following vars": {
+			input: NewKey("inputs", NewList([]Node{
+				NewDict([]Node{
+					NewKey("key", NewStrVal("${var1.name}")),
+				}),
+			})),
+			expected: NewList([]Node{
+				NewDict([]Node{
+					NewKey("key", NewStrVal("value1")),
+				}),
+				NewDict([]Node{
+					NewKey("key", NewStrVal("value2")),
+				}),
+			}),
+			varsArray: []*Vars{
+				mustMakeVars(map[string]interface{}{
+					"var1": map[string]interface{}{
+						"other": "value3",
+					},
+				}),
+				mustMakeVars(map[string]interface{}{
+					"var1": map[string]interface{}{
+						"name": "value1",
+					},
+				}),
+				mustMakeVars(map[string]interface{}{
+					"var1": map[string]interface{}{
+						"name": "value2",
+					},
+				}),
+			},
+		},
 	}
 
 	for name, test := range testcases {

internal/pkg/agent/transpiler/outputs.go

@@ -5,6 +5,7 @@
 package transpiler
 
 import (
+	"errors"
 	"fmt"
 )
 
@@ -50,10 +51,12 @@ func RenderOutputs(outputs Node, varsArray []*Vars) (Node, error) {
 		}
 		// Apply creates a new Node with a deep copy of all the values
 		value, err := dict.Apply(vars)
-		// inputs allows a variable not to match and it will be removed
-		// outputs are not that way, if an ErrNoMatch is returned we
-		// return it back to the caller
 		if err != nil {
+			if errors.Is(err, errNoMatchAllowed) {
+				// optional `|?` syntax; remove the output as no match was provided
+				continue
+			}
+			// no match and not optional
 			return nil, fmt.Errorf("rendering output %q failed: %w", key.name, err)
 		}
 		keys[i] = &Key{

internal/pkg/agent/transpiler/outputs_test.go

@@ -100,6 +100,19 @@ func TestRenderOutputs(t *testing.T) {
 				},
 			}, "var1"),
 		},
+		"optional single var": {
+			input: NewKey("outputs", NewDict([]Node{
+				NewKey("default", NewDict([]Node{
+					NewKey("key", NewStrVal("${var1.name|?}")),
+				})),
+			})),
+			expected: NewDict([]Node{}),
+			vars: mustMakeVars(map[string]interface{}{
+				"var1": map[string]interface{}{
+					"other": "value1",
+				},
+			}),
+		},
 	}
 
 	for name, test := range testcases {

internal/pkg/agent/transpiler/vars.go

@@ -17,11 +17,16 @@ import (
 
 const varsSeparator = "."
 
-var varsRegex = regexp.MustCompile(`\$\$?{([\p{L}\d\s\\\-_|.'":\/]*)}`)
+var varsRegex = regexp.MustCompile(`\$\$?{([\p{L}\d\s\\\-_|.'":\/\?]*)}`)
 
 // ErrNoMatch is return when the replace didn't fail, just that no vars match to perform the replace.
 var ErrNoMatch = errors.New("no matching vars")
 
+// errNoMatchAllowed is returned when the replace didn't fail, no vars match to perform the replace, but the variable was marked as optional with |?.
+// This is kept private because it should not be used outside of this module. Only ErrNoMatch will ever be returned
+// outside of the module.
+var errNoMatchAllowed = errors.New("no matching vars allowed")
+
 // Vars is a context of variables that also contain a list of processors that go with the mapping.
 type Vars struct {
 	id                    string
@@ -124,7 +129,7 @@ func replaceVars(value string, replacer func(variable string) (Node, Processors,
 				continue
 			}
 			// match on a non-escaped var
-			vars, err := extractVars(value[r[i+2]:r[i+3]], defaultProvider)
+			vars, optional, err := extractVars(value[r[i+2]:r[i+3]], defaultProvider)
 			if err != nil {
 				return nil, fmt.Errorf(`error parsing variable "%s": %w`, value[r[i]:r[i+1]], err)
 			}
@@ -155,15 +160,18 @@ func replaceVars(value string, replacer func(variable string) (Node, Processors,
 				}
 			}
 			if !set && reqMatch {
-				return NewStrVal(""), fmt.Errorf("%w: %s", ErrNoMatch, toRepresentation(vars))
+				if optional {
+					return NewStrVal(""), fmt.Errorf("%w: %s", errNoMatchAllowed, toRepresentation(vars, optional))
+				}
+				return NewStrVal(""), fmt.Errorf("%w: %s", ErrNoMatch, toRepresentation(vars, optional))
 			}
 			lastIndex = r[1]
 		}
 	}
 	return NewStrValWithProcessors(result+value[lastIndex:], processors), nil
 }
 
-func toRepresentation(vars []varI) string {
+func toRepresentation(vars []varI, optional bool) string {
 	var sb strings.Builder
 	sb.WriteString("${")
 	for i, val := range vars {
@@ -179,6 +187,9 @@ func toRepresentation(vars []varI) string {
 			}
 		}
 	}
+	if optional {
+		sb.WriteString("|?")
+	}
 	sb.WriteString("}")
 	return sb.String()
 }
@@ -230,28 +241,29 @@ func (v *constString) Value() string {
 	return v.value
 }
 
-func extractVars(i string, defaultProvider string) ([]varI, error) {
+func extractVars(i string, defaultProvider string) ([]varI, bool, error) {
 	const out = rune(0)
 
 	quote := out
 	constant := false
 	escape := false
+	optional := false
 	is := make([]rune, 0, len(i))
 	res := make([]varI, 0)
 	for _, r := range i {
 		if r == '|' || r == ':' {
 			if escape {
 				if r == '|' {
-					return nil, fmt.Errorf(`variable pipe cannot be escaped; remove '\' before '|'`)
+					return nil, false, fmt.Errorf(`variable pipe cannot be escaped; remove '\' before '|'`)
 				}
-				return nil, fmt.Errorf(`default pipe cannot be escaped; remove '\' before ':'`)
+				return nil, false, fmt.Errorf(`default pipe cannot be escaped; remove '\' before ':'`)
 			}
 			if quote == out {
 				if constant {
 					res = append(res, &constString{string(is)})
 				} else if len(is) > 0 {
 					if is[len(is)-1] == '.' {
-						return nil, fmt.Errorf("variable cannot end with '.'")
+						return nil, false, fmt.Errorf("variable cannot end with '.'")
 					}
 					res = append(res, &varString{maybeAddDefaultProvider(string(is), defaultProvider)})
 				}
@@ -287,17 +299,20 @@ func extractVars(i string, defaultProvider string) ([]varI, error) {
 		}
 	}
 	if quote != out {
-		return nil, fmt.Errorf(`starting %s is missing ending %s`, string(quote), string(quote))
+		return nil, false, fmt.Errorf(`starting %s is missing ending %s`, string(quote), string(quote))
 	}
-	if constant {
+	// Check if the final segment is just "?" which marks the variable as optional
+	if !constant && len(is) == 1 && is[0] == '?' {
+		optional = true
+	} else if constant {
 		res = append(res, &constString{string(is)})
 	} else if len(is) > 0 {
 		if is[len(is)-1] == '.' {
-			return nil, fmt.Errorf("variable cannot end with '.'")
+			return nil, false, fmt.Errorf("variable cannot end with '.'")
 		}
 		res = append(res, &varString{maybeAddDefaultProvider(string(is), defaultProvider)})
 	}
-	return res, nil
+	return res, optional, nil
 }
 
 func varPrefixMatched(val string, key string) bool {