Fix permissions for config files used in tests #42914

AndersonQ · 2025-02-26T13:45:04Z

Proposed commit message

relax permissions checks for config files on test

set `strict.perms` flag to false on test that load config files committed to the
repo.

By default, beats require the config files to be writable only by the file 
owner, so tests reading config files committed do the repo might fail depending 
on how the host system is configured. For those tests the permission check is 
relaxed.

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

N/A

How to test this PR locally

tl;dr: no unit/system test should fail due to a configuration file with the wrong permissions.

set the system umask to 002
run the tests with the new test framework:

cd filebeat/testing/integration
go test -v -tags integration -run TestFilebeat

it should succeed, before it'd fail with:

[...]
        Exiting: error loading config file: config file ("/tmp/TestFilebeatFilebeat_starts_and_ingests_filesplain_text_files1677938519/002/filebeat.yml") can only be writable by the owner but the permissions are "-rwxrwxr-x" (to fix the permissions use: 'chmod go-w /tmp/TestFilebeatFilebeat_starts_and_ingests_filesplain_text_files1677938519/002/filebeat.yml')
[...]

try some of the other unit tests, but first ensure the configuration files have the wrong permissions

cd metricbeat/mb/testdata
chmod -R go+w ./
go test -run TestProcessorsForMetricSet_FromSource ./

it should succeed. Before it's fail with:

❯ go test -run TestProcessorsForMetricSet_FromSource ./              
--- FAIL: TestProcessorsForMetricSet_FromSource (0.00s)
    registry_test.go:245: 
        	Error Trace:	/home/ainsoph/devel/github.com/elastic/beats/metricbeat/mb/registry_test.go:245
        	Error:      	Received unexpected error:
        	            	reading processors for metricset 'withprocessors' in module 'unpack': loading light module 'unpack' definition: loading module configuration from 'testdata/lightmodules/unpack/module.yml': config file ("testdata/lightmodules/unpack/module.yml") can only be writable by the owner but the permissions are "-rw-rw-r--" (to fix the permissions use: 'chmod go-w /home/ainsoph/devel/github.com/elastic/beats/metricbeat/mb/testdata/lightmodules/unpack/module.yml')
        	Test:       	TestProcessorsForMetricSet_FromSource
FAIL
FAIL	github.com/elastic/beats/v7/metricbeat/mb	0.082s
FAIL

Related issues

Closes Test framework incorrectly creates beat config file #42779

mergify · 2025-02-26T13:45:42Z

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
backport-active-all is the label that automatically backports to all active branches.
backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

The tests which rely on reading config from disk did not correctly set the permissions. The beats require the config files must be writable only by the owner, now the tests explicitly set the permissions accordingly. A new `testcfg` package provides `NewFileWith644Perm` and `CopyDirectoryWithOwnerWriteOnly` as helper functions for tests to create a temporary copy of the files committed to the repo to use in the test. The test framework was also adjusted to save the config file with the correct permission.

elasticmachine · 2025-02-27T08:03:36Z

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

rdner

I'm not convinced we need an entire package with helpers. Especially copying files just to change permissions does not seem justified.

Instead I think we should fix permissions on the existing files use for testing. Or assert permissions as a part of the tests.

If we create files we have full control over permissions, so we just need to fix these occurrences.

CHANGELOG-developer.next.asciidoc

libbeat/testing/integration/run_beat.go

filebeat/config/config_test.go

rdner · 2025-03-03T11:06:09Z

filebeat/fileset/fileset_test.go

@@ -41,17 +42,24 @@ func makeTestInfo(version string) beat.Info {
 	}
 }

-func getModuleForTesting(t *testing.T, module, fileset string) *Fileset {
+func getModuleForTesting(t *testing.T, module, fileset string, filesetCfg *FilesetConfig) *Fileset {


Why filesetCfg *FilesetConfig was introduced if it's always nil in all usages? Can't we just always pass nil to the New function?

it's not always nil:

https://github.com/AndersonQ/beats/blob/dc6126016cfa68a6c8b6b86e1506b6b803d7477f/filebeat/fileset/fileset_test.go#L107

https://github.com/AndersonQ/beats/blob/dc6126016cfa68a6c8b6b86e1506b6b803d7477f/filebeat/fileset/fileset_test.go#L243

that's why it was introduced as a parameter. getModuleForTesting ensures the configuration files will have the correct permissions, thus I changed TestEvaluateVarsNginxOverride and TestGetInputConfigNginxOverrides to call it instead of calling New directly

rdner · 2025-03-03T11:09:28Z

filebeat/fileset/modules_test.go

+	fixedModulesPath := testcfg.CopyDirectoryWithOwnerWriteOnly(t, modulesPath, "")
+	reg, err := newModuleRegistry(fixedModulesPath, configs, nil, beat.Info{Version: "5.2.0"}, FilesetOverrides{})


Why do we need copy? Why can't we assert permissions and change them on the existing files?

I don't think tests should be doing permanent changes on the repo, that's why I took this approach of copying the files as it isn't possible to commit the full file permission to git.

I don't think tests should be doing permanent changes on the repo, that's why I took this approach of copying the files as it isn't possible to commit the full file permission to git.

This is not entirely true, git stores permissions but only 2 possible values 644 and 755 which you can set/switch with git update-index --chmod=+x.

So, unless you switch the executable flag, git would not detect permission changes and for git the repository remains untouched.

I still don't think that copying files is a good idea, it increases the load on CI for no reason and complicates our tests.

We should create files with right permissions and assert files to be with right permissions without copying files. The only exception is when the test data is supposed to be changed by the test.

I don't think tests should be doing permanent changes on the repo, that's why I took this approach of copying the files as it isn't possible to commit the full file permission to git.

I don't see it as a permanent change, it's a part of the test setup to make sure that the permissions on the test files are correct. We would not commit these changes to the repository, so I'm confused about "permanent" here.

"permanent" as in after the test run the original state of the files won't be restored.

it increases the load on CI for no reason and complicates our tests.

I don't think there is concrete evidence this overhead will be significant. We probably have other things that can be improved if we want to make CI performance better.

I still don't think that copying files is a good idea

I don't see why it's a problem, given as I said above I don't believe the overhead is any issue. Much more problematic is the unit tests making permanent changes to files. That's why I don't see as a good solution a "single function Ensure644 that walks the directory, and sets 644 permission to every file".

Rather than writing a bunch of code to solve this, my thought would be to just relax the permissions checks in the beats during these tests.

It turns out we have had this problem before and there is already a flag that does this, --strict-perms. It is also available as the BEAT_STRICT_PERMS environment variable:

beats/libbeat/common/config.go

Lines 37 to 44 in f1e42fc

// IsStrictPerms returns true if strict permission checking on config files is

// enabled.

func IsStrictPerms() bool {

if !*flagStrictPerms || os.Getenv("BEAT_STRICT_PERMS") == "false" {

return false

}

return true

}

It turns off exactly the error we are seeing:

beats/libbeat/common/config.go

Lines 79 to 84 in f1e42fc

func LoadFile(path string) (*config.C, error) {

if IsStrictPerms() {

if err := OwnerHasExclusiveWritePerms(path); err != nil {

return nil, err

}

}

Pretty much every metricbeat module test already sets this. It is very widely used in our other tests. https://github.com/search?q=repo%3Aelastic%2Fbeats%20BEAT_STRICT_PERMS&type=code

Can we make this problem go away by using --strict.perms=false or BEAT_STRICT_PERMS=false in the new framework instead?

@AndersonQ FYI: we already set permissions to a Filebeat configuration file in another test

beats/libbeat/cmd/instance/beat_test.go

Lines 91 to 97 in ca4ad40

const configPath = "../test/filebeat_test.yml"

// Ensure that the config has owner-exclusive write permissions.

// This is necessary on some systems which have a default umask

// of 0o002, meaning that files are checked out by git with mode

// 0o664. This would cause cfgfile.Load to fail.

err = os.Chmod(configPath, 0o644)

to make it work. What I describe is not new.

It turns out we have had this problem before and there is already a flag that does this, --strict-perms. It is also available as the BEAT_STRICT_PERMS environment variable:

that's interesting. I'll set it them

I confess I wonder why we have this strict permission requirement in place.

AndersonQ · 2025-03-03T11:48:46Z

Instead I think we should fix permissions on the existing files use for testing.

Git does not save the full set of permissions, it only saves if the file is executable or not. So we can't just fix the permissions on the git repo.

Also having the test permanently changing the files does not seems a good approach.

If we create files we have full control over permissions, so we just need to fix these occurrences.

If we do it in runtime, yes, I just believe the current approach is better than changing all the affected tests to create the files in runtime.

I'm not convinced we need an entire package with helpers. Especially copying files just to change permissions does not seem justified.

as I said above, the alternative would be to make all the tests to create the files in runtime. It's an option, but I think the current approach is less intrusive.

rdner · 2025-03-06T09:42:52Z

@AndersonQ can we just create a single function Ensure644 that walks the directory, and sets 644 permission to every file?

As I explained #42914 (comment) git should not detect changes unless the executable flag is flipped.

pierrehilbert · 2025-03-07T07:55:39Z

Considering this is only fixing tests, shouldn't we backport this to all active branches?

set `strict.perms` flag to false on test that load config files committed to the repo. By default beats require the config files to be writable only by the file owner, so tests reading config files committed do the repo might fail depending on how the host system is configured. For those tests the permission check is relaxed.

AndersonQ · 2025-03-07T14:34:20Z

Considering this is only fixing tests, shouldn't we backport this to all active branches?

done

AndersonQ · 2025-03-07T14:36:21Z

@AndersonQ can we just create a single function Ensure644 that walks the directory, and sets 644 permission to every file?

As I explained #42914 (comment) git should not detect changes unless the executable flag is flipped.

I followed Craig's suggestion and relaxed the permission check instead.

AndersonQ self-assigned this Feb 26, 2025

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Feb 26, 2025

AndersonQ added Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team backport-8.x Automated backport to the 8.x branch with mergify labels Feb 27, 2025

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Feb 27, 2025

AndersonQ force-pushed the 42779-fix-test-config-creation branch from 0151d2e to dc61260 Compare February 27, 2025 08:03

AndersonQ marked this pull request as ready for review February 27, 2025 08:03

AndersonQ requested a review from a team as a code owner February 27, 2025 08:03

AndersonQ requested review from faec and khushijain21 February 27, 2025 08:03

AndersonQ changed the title ~~[WIP] Fix permissions for config files used in tests~~ Fix permissions for config files used in tests Feb 27, 2025

rdner reviewed Mar 3, 2025

View reviewed changes

PR changes

9cfe7de

AndersonQ requested a review from rdner March 5, 2025 13:35

AndersonQ added backport-active-all Automated backport with mergify to all the active branches and removed backport-8.x Automated backport to the 8.x branch with mergify labels Mar 7, 2025

add license headers

71c4e45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix permissions for config files used in tests #42914

Fix permissions for config files used in tests #42914

AndersonQ commented Feb 26, 2025 •

edited

Loading

mergify bot commented Feb 26, 2025

elasticmachine commented Feb 27, 2025

rdner left a comment

rdner Mar 3, 2025

AndersonQ Mar 3, 2025

rdner Mar 3, 2025

AndersonQ Mar 3, 2025

rdner Mar 6, 2025

rdner Mar 6, 2025

AndersonQ Mar 6, 2025

cmacknz Mar 6, 2025

rdner Mar 7, 2025

AndersonQ Mar 7, 2025

AndersonQ commented Mar 3, 2025

rdner commented Mar 6, 2025

pierrehilbert commented Mar 7, 2025

AndersonQ commented Mar 7, 2025

AndersonQ commented Mar 7, 2025

		fixedModulesPath := testcfg.CopyDirectoryWithOwnerWriteOnly(t, modulesPath, "")
		reg, err := newModuleRegistry(fixedModulesPath, configs, nil, beat.Info{Version: "5.2.0"}, FilesetOverrides{})

	// IsStrictPerms returns true if strict permission checking on config files is
	// enabled.
	func IsStrictPerms() bool {
	if !*flagStrictPerms \|\| os.Getenv("BEAT_STRICT_PERMS") == "false" {
	return false
	}
	return true
	}

	func LoadFile(path string) (*config.C, error) {
	if IsStrictPerms() {
	if err := OwnerHasExclusiveWritePerms(path); err != nil {
	return nil, err
	}
	}

	const configPath = "../test/filebeat_test.yml"

	// Ensure that the config has owner-exclusive write permissions.
	// This is necessary on some systems which have a default umask
	// of 0o002, meaning that files are checked out by git with mode
	// 0o664. This would cause cfgfile.Load to fail.
	err = os.Chmod(configPath, 0o644)

Fix permissions for config files used in tests #42914

Are you sure you want to change the base?

Fix permissions for config files used in tests #42914

Conversation

AndersonQ commented Feb 26, 2025 • edited Loading

Proposed commit message

Checklist

Disruptive User Impact

How to test this PR locally

Related issues

mergify bot commented Feb 26, 2025

elasticmachine commented Feb 27, 2025

rdner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndersonQ commented Mar 3, 2025

rdner commented Mar 6, 2025

pierrehilbert commented Mar 7, 2025

AndersonQ commented Mar 7, 2025

AndersonQ commented Mar 7, 2025

AndersonQ commented Feb 26, 2025 •

edited

Loading