filestream: validate input id on startup #41731

AndersonQ · 2024-11-21T14:43:57Z

During startup filebeat now validates the filestream inputs and fails to start if there are inputs without ID or with duplicated IDs. Duplicated IDs might cause data duplication therefore now it's mandatory to have unique and non-empty ID for each filestream input.

Proposed commit message

filestream: require unique input id on filebeat startup

During startup filebeat now validates the filestream inputs and fails to start if there are with duplicated IDs. Duplicated IDs might cause data duplication therefore now it's mandatory to have unique for each filestream input. 

Disruptive User Impact
Impact: Users who have not configured unique IDs for their filestream inputs will need to update their configurations to include unique IDs for each input. Failure to do so will prevent Filebeat from starting.

Previously: Filebeat would only log an error if filestream would find inputs with duplicated IDs, potentially leading to data duplication.

Now: Filebeat will fail to start if any filestream input has a duplicated ID.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~[ ] I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Impact: Users who have not configured unique IDs for their filestream inputs will need to update their configurations to include unique IDs for each input. Failure to do so will prevent Filebeat from starting.

Previously: Filebeat would only log an error if filestream would find inputs had missing or duplicated IDs, potentially leading to data duplication.

Now: Filebeat will fail to start if any filestream input lacks an ID or has a duplicated ID.

How to test this PR locally

build filebeat

cd filebeat
go build .

use the following as filebeat.yml. Change as necessary to simulate" only unique IDs, valid empty ID, duplicated IDs, duplicated empty ID.

filebeat.inputs:
  - type: filestream
    enabled: true
    paths:
      - /tmp/noID-1/*.log
  - type: filestream
    enabled: true
    paths:
      - /tmp/noID-2/*.log
  - type: filestream
    id: duplicated-id
    enabled: true
    paths:
      - /tmp/duplicated-id-1/*.log
  - type: filestream
    id: duplicated-id
    enabled: true
    paths:
      - /tmp/duplicated-id-2/*.log

output.discard.enabled: true
logging:
  level: info
  metrics:
    enabled: false

run filebeat:

./filebeat -e 2>&1 | grep message

verify filebeat:

will exit with error
will produce a log with all the invalid input configuration:

{"log.level":"error","@timestamp":"2024-11-25T10:44:19.062+0100","log.logger":"filestream","log.origin":{"function":"github.com/elastic/beats/v7/filebeat/input/filestream.ValidateInputIDs","file.name":"filestream/config.go","file.line":174},"message":"filestream inputs with duplicated IDs","service.name":"filebeat","inputs":[{"enabled":true,"paths":["/tmp/noID-1/*.log"],"type":"filestream"},{"enabled":true,"paths":["/tmp/noID-2/*.log"],"type":"filestream"},{"enabled":true,"id":"duplicated-id","paths":["/tmp/duplicated-id-1/*.log"],"type":"filestream"},{"enabled":true,"id":"duplicated-id","paths":["/tmp/duplicated-id-2/*.log"],"type":"filestream"}],"ecs.version":"1.6.0"}

will produce a log pointing out the issues:

{"log.level":"error","@timestamp":"2024-11-25T10:44:19.062+0100","log.origin":{"function":"github.com/elastic/beats/v7/filebeat/beater.(*Filebeat).Run","file.name":"beater/filebeat.go","file.line":297},"message":"invalid filestream configuration: filestream inputs validation error: filestream inputs with duplicated IDs: \"\",\"duplicated-id\"","service.name":"filebeat","ecs.version":"1.6.0"}
Exiting: filestream inputs validation error: filestream inputs with duplicated IDs: "","duplicated-id"

Related issues

Relates Filebeat should fail to start when multiple filestream inputs have the same input ID #40540

Logs

{"log.level":"info","@timestamp":"2024-11-25T10:44:19.062+0100","log.origin":{"function":"github.com/elastic/beats/v7/libbeat/cmd/instance.(*Beat).launch","file.name":"instance/beat.go","file.line":770},"message":"filebeat start running.","service.name":"filebeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2024-11-25T10:44:19.062+0100","log.logger":"filestream","log.origin":{"function":"github.com/elastic/beats/v7/filebeat/input/filestream.ValidateInputIDs","file.name":"filestream/config.go","file.line":174},"message":"filestream inputs with duplicated IDs","service.name":"filebeat","inputs":[{"enabled":true,"paths":["/tmp/noID-1/*.log"],"type":"filestream"},{"enabled":true,"paths":["/tmp/noID-2/*.log"],"type":"filestream"},{"enabled":true,"id":"duplicated-id","paths":["/tmp/duplicated-id-1/*.log"],"type":"filestream"},{"enabled":true,"id":"duplicated-id","paths":["/tmp/duplicated-id-2/*.log"],"type":"filestream"}],"ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2024-11-25T10:44:19.062+0100","log.origin":{"function":"github.com/elastic/beats/v7/filebeat/beater.(*Filebeat).Run","file.name":"beater/filebeat.go","file.line":297},"message":"invalid filestream configuration: filestream inputs validation error: filestream inputs with duplicated IDs: \"\",\"duplicated-id\"","service.name":"filebeat","ecs.version":"1.6.0"}
{"log.level":"info","@timestamp":"2024-11-25T10:44:19.062+0100","log.origin":{"function":"github.com/elastic/beats/v7/libbeat/cmd/instance.(*Beat).launch","file.name":"instance/beat.go","file.line":779},"message":"filebeat stopped.","service.name":"filebeat","ecs.version":"1.6.0"}
{"log.level":"error","@timestamp":"2024-11-25T10:44:19.062+0100","log.origin":{"function":"github.com/elastic/beats/v7/libbeat/cmd/instance.handleError","file.name":"instance/beat.go","file.line":1594},"message":"Exiting: filestream inputs validation error: filestream inputs with duplicated IDs: \"\",\"duplicated-id\"","service.name":"filebeat","ecs.version":"1.6.0"}
Exiting: filestream inputs validation error: filestream inputs with duplicated IDs: "","duplicated-id"

mergify · 2024-11-21T14:44:40Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @AndersonQ? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-11-21T14:44:40Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

During startup filebeat now validates the filestream inputs and fails to start if there are inputs without ID or with duplicated IDs

elasticmachine · 2024-11-21T14:57:00Z

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

mauri870 · 2024-11-22T14:19:43Z

filebeat/input/filestream/config.go

+			toJson := map[string]interface{}{}
+			err := cfg.Unpack(&toJson)
+			if err != nil {
+				toJson["emptyID"] = fmt.Sprintf("failes to umpack config: %v", err)


Suggested change

toJson["emptyID"] = fmt.Sprintf("failes to umpack config: %v", err)

toJson["emptyID"] = fmt.Sprintf("failed to unpack config: %v", err)

mauri870 · 2024-11-22T14:20:08Z

filebeat/input/filestream/config.go

+			toJson := map[string]interface{}{}
+			err := dupcfgs.Unpack(&toJson)
+			if err != nil {
+				toJson[id] = fmt.Sprintf("failes to umpack config: %v", err)


mauri870

Codewise looks good, left some comments about some minor spelling issues.

I tested locally following the issue description and confirmed that it validates the ids properly.

LGTM.

filebeat/input/filestream/config.go

filebeat/input/filestream/config_test.go

cmacknz · 2024-11-22T21:41:27Z

if there are inputs without ID

An empty ID is fine, the problem is if there are two inputs with an empty ID. Agent allows the empty ID as a valid ID. This avoids breaking the configuration unnecessarily where there is only a single filestream input with no ID.

rdner

All my comments have been addressed.
We might want to treat an empty ID as a regular ID value as @cmacknz pointed out. So, we can apply the duplication check to that as well.

AndersonQ · 2024-11-25T09:49:26Z

I removed the bit forbidding an empty ID. It was already checking for multiple empty IDs and including them in the list of invalid inputs.

rdner · 2024-11-25T15:31:03Z

@AndersonQ looks like the tests need adjusting?

=== FAIL: filebeat/tests/integration TestFilestreamValidationPreventsFilebeatStart/empty_ID (10.12s)
    filestream_test.go:180:
        	Error Trace:	/opt/buildkite-agent/builds/bk-agent-prod-gcp-1732528923926044677/elastic/filebeat/libbeat/tests/integration/framework.go:540
        	            				/opt/buildkite-agent/builds/bk-agent-prod-gcp-1732528923926044677/elastic/filebeat/filebeat/tests/integration/filestream_test.go:180
        	Error:      	Condition never satisfied
        	Test:       	TestFilestreamValidationPreventsFilebeatStart/empty_ID
        	Messages:   	Filebeat did log a validation error

belimawr · 2024-11-25T16:55:12Z

filebeat/beater/filebeat.go

@@ -291,6 +292,11 @@ func (fb *Filebeat) Run(b *beat.Beat) error {
 	}
 	defer stateStore.Close()

+	err = filestream.ValidateInputIDs(config.Inputs, logp.NewLogger("filestream"))


The filestream input actually uses input.filestream as the logger name, so I suggest keeping the pattern.

Suggested change

err = filestream.ValidateInputIDs(config.Inputs, logp.NewLogger("filestream"))

err = filestream.ValidateInputIDs(config.Inputs, logp.NewLogger("input.filestream"))

filebeat/input/filestream/config.go

belimawr · 2024-11-25T17:01:48Z

filebeat/tests/integration/filestream_test.go

+			filebeat.WaitForLogs(
+				"filestream inputs validation error",
+				10*time.Second,
+				"Filebeat did log a validation error")


should it be "Filebeat did not log a validation error"?

Suggested change

"Filebeat did log a validation error")

"Filebeat did not log a validation error")

belimawr · 2024-11-25T17:02:34Z

filebeat/tests/integration/filestream_test.go

+	filebeat.WaitForLogs(
+		"Input 'filestream' starting",
+		10*time.Second,
+		"Filebeat did log a validation error")


Maybe something else, like "filebeat did not start the Filestream input"

Suggested change

"Filebeat did log a validation error")

"Filebeat did not start the Filestream input")

belimawr · 2024-11-25T17:05:02Z

if there are inputs without ID

An empty ID is fine, the problem is if there are two inputs with an empty ID. Agent allows the empty ID as a valid ID. This avoids breaking the configuration unnecessarily where there is only a single filestream input with no ID.

Honestly, as we're already doing a breaking change to enforce ID validation, I rather not allow empty IDs, it is a corner case and Elastic-Agent/Fleet already adds IDs to all inputs, so I don't see a real need for it. It only makes for an extra corner case to maintain.

belimawr · 2024-11-25T17:08:50Z

@AndersonQ, @cmacknz, Should we also cover the case of dynamic inputs like:

Input reload is enabled and inputs are read from inputs.d + filebeat.yml
Filebeat autodiscover
Filebeat running under Elastic-Agent

I like the way it's implemented here that gives this quick and precise feedback to the use, however it does not cover all cases.

I believe we should, at least, also cover the inputs.d if Filebeat is setup to read inputs from it, even if it's just at startup.

AndersonQ · 2024-11-25T17:23:08Z

Honestly, as we're already doing a breaking change to enforce ID validation, I rather not allow empty IDs

Agreed, but also what was approved in the breaking change committee was indeed to enforce the uniqueness of IDs. Also considering the agent already allows a single empty ID, I agree with Craig it's fine to have an empty ID as long as it's just one.

@AndersonQ, @cmacknz, Should we also cover the case of dynamic inputs like:

Input reload is enabled and inputs are read from inputs.d + filebeat.yml

Filebeat autodiscover

Filebeat running under Elastic-Agent

I like the way it's implemented here that gives this quick and precise feedback to the use, however it does not cover all cases.

Those are different things. Here we're tacking the start up and early feedback.

In order to completely prevent an input with duplicated ID, we'd need to change it to return an error instead of just logging it. Which happens in a completely different part of the code. Having it on another issue and PR if we really want to do that is a better option.

…ids' into 40540-filestream-require-unique-ids

cmacknz · 2024-11-25T18:59:50Z

Filebeat running under Elastic-Agent

Since 8.6.0 Elastic Agent has required that all inputs have a unique ID, and it considers an empty ID a unique ID. The constraint is that two inputs can't have the same ID, not that the ID has to be non-empty.

https://github.com/elastic/elastic-agent/blob/434986ed9fa6fbed6257557a4c6773d519742f66/pkg/component/component.go#L734-L745

func hasDuplicate(outputsMap map[string]outputI, id string) bool {
	for _, o := range outputsMap {
		for _, i := range o.inputs {
			for _, j := range i {
				if j.id == id {
					return true
				}
			}
		}
	}
	return false
}

If we were designing this from scratch we'd probably forbid empty IDs, but allowing empty IDs solves the underlying functional problem without unnecessarily breaking people who started from our default configurations that didn't specify IDs a while ago. We could log a warning that IDs should be explicit if you want.

Input reload is enabled and inputs are read from inputs.d + filebeat.yml
Filebeat autodiscover

+1 to handling these as well, autodiscovery with the ID composed from variable substitutions with non-unique values is a common way people were ending up with duplicated logs on K8s. The original issue doesn't put a constraint on the execution environment :)

This doesn't need to be fixed in this PR though, it can be done as a separate PR, but I don't think we can close #40540 until it's done, otherwise we have implemented "sometimes require unique IDs" and not "always require unique IDs".

belimawr · 2024-11-25T22:11:39Z

filebeat/input/filestream/config.go

+// containing the offending input configurations and returns an error containing
+// the duplicated IDs.
+func ValidateInputIDs(inputs []*conf.C, logger *logp.Logger) error {
+	ids := make(map[string][]*conf.C)


[Suggestion]
Rename this map to something that better describe its contents. It is a map of inputs (or configs) grouped by ID, so ids does not seem to be the better description to it.

mauri870 · 2024-12-02T17:43:02Z

Reviewed again, LGTM.

AndersonQ · 2024-12-03T09:08:01Z

@cmacknz, regarding

This doesn't need to be fixed in this PR though, it can be done as a separate PR, but I don't think we can close #40540 until it's done, otherwise we have implemented "sometimes require unique IDs" and not "always require unique IDs".

Whereas I agree this PR only enforces unique IDs at startup, it's exactly what #40540. Perhaps it'd be better to create another issue, or change the whole issue to be indeed about fielstream requiring unique IDs and add 2 sub issues, one with the current content of #40540 and another for enforce uniqueness at runtime.

Also I believe when requiring uniqueness at runtime we should not crash filebeat, I believe it's better for the users to log an error and not start the offending input. Thinking about a standalone filebeat, which might have different modules running at the same time, I don't think it'd be good for the users to have filebeat crashing because a possible data duplication. Better to have just the offending issue not running.

rdner

2 suggestions for code readability, the new changes look good to me.

filebeat/input/filestream/config.go

…ids' into 40540-filestream-require-unique-ids

cmacknz · 2024-12-03T20:50:01Z

Also I believe when requiring uniqueness at runtime we should not crash filebeat, I believe it's better for the users to log an error and not start the offending input. Thinking about a standalone filebeat, which might have different modules running at the same time, I don't think it'd be good for the users to have filebeat crashing because a possible data duplication. Better to have just the offending issue not running.

My guide for how this should be have is to test what Elastic Agent does and then follow its behavior because this problem is completely solved there.

When dynamic variable substitution is involved, we don't need to fail at runtime because templated inputs with the same ID end up only creating a single input in agent. I will open an issue for you to either confirm Filebeat also does this or implement that behavior based on what you find Filebeat does in that situation.

AndersonQ added breaking change Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team labels Nov 21, 2024

AndersonQ self-assigned this Nov 21, 2024

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Nov 21, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Nov 21, 2024

filestream: validate input ids on startup

b6ecc4e

During startup filebeat now validates the filestream inputs and fails to start if there are inputs without ID or with duplicated IDs

AndersonQ force-pushed the 40540-filestream-require-unique-ids branch from 247727b to b6ecc4e Compare November 21, 2024 14:45

AndersonQ marked this pull request as ready for review November 21, 2024 14:56

AndersonQ requested a review from a team as a code owner November 21, 2024 14:56

AndersonQ requested review from mauri870 and khushijain21 November 21, 2024 14:56

AndersonQ added backport-skip Skip notification from the automated backport with mergify and removed backport-8.x Automated backport to the 8.x branch with mergify labels Nov 21, 2024

pierrehilbert requested review from belimawr and rdner and removed request for khushijain21 November 21, 2024 16:29

mauri870 reviewed Nov 22, 2024

View reviewed changes

mauri870 approved these changes Nov 22, 2024

View reviewed changes

fix typos

70c3569

AndersonQ enabled auto-merge (squash) November 22, 2024 14:42

rdner reviewed Nov 22, 2024

View reviewed changes

filebeat/input/filestream/config.go Outdated Show resolved Hide resolved

filebeat/input/filestream/config.go Show resolved Hide resolved

filebeat/input/filestream/config_test.go Show resolved Hide resolved

pierrehilbert disabled auto-merge November 22, 2024 16:00

AndersonQ added 2 commits November 22, 2024 18:40

pr changes

0bc1392

pr changes

e05c888

AndersonQ requested a review from rdner November 22, 2024 21:37

rdner approved these changes Nov 23, 2024

View reviewed changes

AndersonQ added 2 commits November 25, 2024 10:47

allow empty IDs

885799e

Merge branch 'main' into 40540-filestream-require-unique-ids

5b70c55

AndersonQ requested a review from rdner November 25, 2024 09:49

belimawr reviewed Nov 25, 2024

View reviewed changes

AndersonQ added 2 commits November 25, 2024 18:41

adjust integration tests

100e0f5

Merge remote-tracking branch 'origin/40540-filestream-require-unique-…

ca65d39

…ids' into 40540-filestream-require-unique-ids

AndersonQ requested a review from belimawr November 25, 2024 17:45

Merge branch 'main' into 40540-filestream-require-unique-ids

c8505e0

belimawr reviewed Nov 25, 2024

View reviewed changes

rdner approved these changes Dec 3, 2024

View reviewed changes

filebeat/input/filestream/config.go Outdated Show resolved Hide resolved

filebeat/input/filestream/config.go Outdated Show resolved Hide resolved

AndersonQ added 2 commits December 3, 2024 11:44

wee refactor on docs and names

cec09f3

Merge remote-tracking branch 'origin/40540-filestream-require-unique-…

f599e74

…ids' into 40540-filestream-require-unique-ids

AndersonQ merged commit 8e20316 into elastic:main Dec 3, 2024
31 checks passed

AndersonQ deleted the 40540-filestream-require-unique-ids branch December 3, 2024 12:59

This was referenced Dec 3, 2024

Investigate if it is possible to prevent inputs with non-unique IDs from being templated by autodiscovery #41881

Open

Filebeat should fail to start when multiple filestream inputs have the same input ID #40540

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filestream: validate input id on startup #41731

filestream: validate input id on startup #41731

AndersonQ commented Nov 21, 2024 •

edited

Loading

mergify bot commented Nov 21, 2024

mergify bot commented Nov 21, 2024

elasticmachine commented Nov 21, 2024

mauri870 Nov 22, 2024

mauri870 Nov 22, 2024

mauri870 left a comment

cmacknz commented Nov 22, 2024

rdner left a comment

AndersonQ commented Nov 25, 2024

rdner commented Nov 25, 2024

belimawr Nov 25, 2024

belimawr Nov 25, 2024

belimawr Nov 25, 2024

belimawr commented Nov 25, 2024

belimawr commented Nov 25, 2024

AndersonQ commented Nov 25, 2024

cmacknz commented Nov 25, 2024 •

edited

Loading

belimawr Nov 25, 2024

mauri870 commented Dec 2, 2024

AndersonQ commented Dec 3, 2024

rdner left a comment

cmacknz commented Dec 3, 2024

	toJson["emptyID"] = fmt.Sprintf("failes to umpack config: %v", err)
	toJson["emptyID"] = fmt.Sprintf("failed to unpack config: %v", err)

	err = filestream.ValidateInputIDs(config.Inputs, logp.NewLogger("filestream"))
	err = filestream.ValidateInputIDs(config.Inputs, logp.NewLogger("input.filestream"))

	"Filebeat did log a validation error")
	"Filebeat did not log a validation error")

	"Filebeat did log a validation error")
	"Filebeat did not start the Filestream input")

filestream: validate input id on startup #41731

filestream: validate input id on startup #41731

Conversation

AndersonQ commented Nov 21, 2024 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

How to test this PR locally

verify filebeat:

Related issues

Logs

mergify bot commented Nov 21, 2024

mergify bot commented Nov 21, 2024

elasticmachine commented Nov 21, 2024

mauri870 Nov 22, 2024

Choose a reason for hiding this comment

mauri870 Nov 22, 2024

Choose a reason for hiding this comment

mauri870 left a comment

Choose a reason for hiding this comment

cmacknz commented Nov 22, 2024

rdner left a comment

Choose a reason for hiding this comment

AndersonQ commented Nov 25, 2024

rdner commented Nov 25, 2024

belimawr Nov 25, 2024

Choose a reason for hiding this comment

belimawr Nov 25, 2024

Choose a reason for hiding this comment

belimawr Nov 25, 2024

Choose a reason for hiding this comment

belimawr commented Nov 25, 2024

belimawr commented Nov 25, 2024

AndersonQ commented Nov 25, 2024

cmacknz commented Nov 25, 2024 • edited Loading

belimawr Nov 25, 2024

Choose a reason for hiding this comment

mauri870 commented Dec 2, 2024

AndersonQ commented Dec 3, 2024

rdner left a comment

Choose a reason for hiding this comment

cmacknz commented Dec 3, 2024

AndersonQ commented Nov 21, 2024 •

edited

Loading

cmacknz commented Nov 25, 2024 •

edited

Loading