Skip to content

Conversation

@slobodanadamovic
Copy link
Contributor

@slobodanadamovic slobodanadamovic commented Feb 3, 2025

This PR disables the "queryable built-in roles" feature for the CoreWithSecurityClientYamlTestSuiteIT and DataStreamsClientYamlTestSuiteIT YAML test suites.
The feature was enabled by default in the #120323 PR, which asynchronously creates the .security index after cluster formation and indexes all built-in roles. The asynchronous creation of the .security index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle .security in existing tests. These tests would have to exclude the .security index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the .security index. This simply adds overhead without much gain. The feature is already test covered by XPackRestIT and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves #121536
Resolves #121513
Resolves #121484
Resolves #121478
Resolves #121290
Resolves #121246
Resolves #121242
Resolves #121238
Resolves #121186
Resolves #121131
Resolves #121130
Resolves #121128
Resolves #121014
Resolves #120965
Resolves #120920
Resolves #120890

@slobodanadamovic slobodanadamovic self-assigned this Feb 3, 2025
@slobodanadamovic slobodanadamovic added >test Issues or PRs that are addressing/adding tests :Security/Security Security issues without another label Team:Security Meta label for security team auto-backport Automatically create backport pull requests when merged v9.0.0 v8.18.1 v8.19.0 v9.0.1 labels Feb 3, 2025
…le-queryable-built-in-feature-for-yaml-tests

# Conflicts:
#	muted-tests.yml
…le-queryable-built-in-feature-for-yaml-tests

# Conflicts:
#	muted-tests.yml
@slobodanadamovic slobodanadamovic requested a review from a team February 3, 2025 22:46
@slobodanadamovic slobodanadamovic marked this pull request as ready for review February 3, 2025 22:46
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@jfreden
Copy link
Contributor

jfreden commented Feb 4, 2025

Is there a plan to get rid of es.queryable_built_in_roles_enabled=false in these tests in the future? Even if they're not directly related to that feature, the feature being integration tested will run with es.queryable_built_in_roles_enabled=true in production which make me a little hesitant to just turning it off.

…le-queryable-built-in-feature-for-yaml-tests
@jfreden
Copy link
Contributor

jfreden commented Feb 4, 2025

It would be nice if the long term plan is to add it back somehow? Even if there is no relation between the security index creation and those tests today, there might be in the future. If the property is going to stick around for a while it would be nice to rename it to something related to the security index to help with debugging if something goes wrong.

@slobodanadamovic
Copy link
Contributor Author

slobodanadamovic commented Feb 4, 2025

Is there a plan to get rid of es.queryable_built_in_roles_enabled=false in these tests in the future? Even if they're not directly related to that feature, the feature being integration tested will run with es.queryable_built_in_roles_enabled=true in production which make me a little hesitant to just turning it off.

No. I was a bit too optimistic that I can make this work for all tests, but the reality is that I need to keep a flag/setting in order to be able to disable it due to the asynchronous nature of .security index creation.
This is only temporary solution which I’m planning to make a permanent by keeping the feature flag in some way (e.g by converting it to a setting or renaming system property to something like xpack.security.queryable_built_in_roles.enabled or xpack.security.auto_built_in_roles_indexing.enabled). This will be then used to disable the queryable built-in role feature in tests (similar to geoip downloader).

I think it's safe to turn it off for these tests. If you have a concrete concern, I think we should address that by adding more test coverage. But I don't think that this would reduce the confidence in feature working as intended. The problem is that the async nature of .security index creation is introducing too much flakiness in YAML tests which don't have nice mechanisms for ensuring consistent test executions. FWIW We have better control over it in integration and rest tests and we leverage them to have higher coverage.

Copy link
Contributor

@jfreden jfreden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@slobodanadamovic slobodanadamovic merged commit d1beb01 into elastic:main Feb 4, 2025
22 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.0 Commit could not be cherrypicked due to conflicts
8.18 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 121541

slobodanadamovic added a commit to slobodanadamovic/elasticsearch that referenced this pull request Feb 4, 2025
… tests (elastic#121541)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites.
The feature was enabled by default in the elastic#120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves elastic#121536
Resolves elastic#121513
Resolves elastic#121484
Resolves elastic#121478
Resolves elastic#121290
Resolves elastic#121246
Resolves elastic#121242
Resolves elastic#121238
Resolves elastic#121186
Resolves elastic#121131
Resolves elastic#121130
Resolves elastic#121128
Resolves elastic#121014
Resolves elastic#120965
Resolves elastic#120920
Resolves elastic#120890

(cherry picked from commit d1beb01)

# Conflicts:
#	muted-tests.yml
slobodanadamovic added a commit to slobodanadamovic/elasticsearch that referenced this pull request Feb 4, 2025
… tests (elastic#121541)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites.
The feature was enabled by default in the elastic#120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves elastic#121536
Resolves elastic#121513
Resolves elastic#121484
Resolves elastic#121478
Resolves elastic#121290
Resolves elastic#121246
Resolves elastic#121242
Resolves elastic#121238
Resolves elastic#121186
Resolves elastic#121131
Resolves elastic#121130
Resolves elastic#121128
Resolves elastic#121014
Resolves elastic#120965
Resolves elastic#120920
Resolves elastic#120890

(cherry picked from commit d1beb01)

# Conflicts:
#	muted-tests.yml
@slobodanadamovic
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x
9.0
8.18

Questions ?

Please refer to the Backport tool documentation

slobodanadamovic added a commit to slobodanadamovic/elasticsearch that referenced this pull request Feb 4, 2025
… tests (elastic#121541)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites.
The feature was enabled by default in the elastic#120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves elastic#121536
Resolves elastic#121513
Resolves elastic#121484
Resolves elastic#121478
Resolves elastic#121290
Resolves elastic#121246
Resolves elastic#121242
Resolves elastic#121238
Resolves elastic#121186
Resolves elastic#121131
Resolves elastic#121130
Resolves elastic#121128
Resolves elastic#121014
Resolves elastic#120965
Resolves elastic#120920
Resolves elastic#120890

(cherry picked from commit d1beb01)

# Conflicts:
#	muted-tests.yml
elasticsearchmachine pushed a commit that referenced this pull request Feb 4, 2025
… tests (#121541) (#121663)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites.
The feature was enabled by default in the #120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves #121536
Resolves #121513
Resolves #121484
Resolves #121478
Resolves #121290
Resolves #121246
Resolves #121242
Resolves #121238
Resolves #121186
Resolves #121131
Resolves #121130
Resolves #121128
Resolves #121014
Resolves #120965
Resolves #120920
Resolves #120890

(cherry picked from commit d1beb01)

# Conflicts:
#	muted-tests.yml
elasticsearchmachine pushed a commit that referenced this pull request Feb 4, 2025
… tests (#121541) (#121658)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites.
The feature was enabled by default in the #120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves #121536
Resolves #121513
Resolves #121484
Resolves #121478
Resolves #121290
Resolves #121246
Resolves #121242
Resolves #121238
Resolves #121186
Resolves #121131
Resolves #121130
Resolves #121128
Resolves #121014
Resolves #120965
Resolves #120920
Resolves #120890

(cherry picked from commit d1beb01)

# Conflicts:
#	muted-tests.yml
elasticsearchmachine pushed a commit that referenced this pull request Feb 4, 2025
… tests (#121541) (#121664)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites.
The feature was enabled by default in the #120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests.
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves #121536
Resolves #121513
Resolves #121484
Resolves #121478
Resolves #121290
Resolves #121246
Resolves #121242
Resolves #121238
Resolves #121186
Resolves #121131
Resolves #121130
Resolves #121128
Resolves #121014
Resolves #120965
Resolves #120920
Resolves #120890

(cherry picked from commit d1beb01)

# Conflicts:
#	muted-tests.yml
fzowl pushed a commit to voyage-ai/elasticsearch that referenced this pull request Feb 4, 2025
… tests (elastic#121541)

This PR disables the "queryable built-in roles" feature for the `CoreWithSecurityClientYamlTestSuiteIT` and `DataStreamsClientYamlTestSuiteIT` YAML test suites. 
The feature was enabled by default in the elastic#120323 PR, which asynchronously creates the `.security` index after cluster formation and indexes all built-in roles. The asynchronous creation of the `.security` index introduces non-deterministic behavior in our YAML tests. 
Since these test suites are not intended to verify the queryable built-in roles functionality, having the feature enabled introduced flakiness and unnecessary complexity to handle `.security` in existing tests. These tests would have to exclude the `.security` index in some way (by adjusting permissions or API calls), and in the end cleanup (delete) the `.security` index. This simply adds overhead without much gain. The feature is already test covered by `XPackRestIT` and other integration/REST tests, disabling it here would not compromise test coverage. Instead, it ensures these suites remain deterministic and focused on the behaviors they were designed to verify.

Resolves elastic#121536 
Resolves elastic#121513 
Resolves elastic#121484 
Resolves elastic#121478 
Resolves elastic#121290 
Resolves elastic#121246 
Resolves elastic#121242 
Resolves elastic#121238 
Resolves elastic#121186 
Resolves elastic#121131 
Resolves elastic#121130 
Resolves elastic#121128 
Resolves elastic#121014 
Resolves elastic#120965 
Resolves elastic#120920 
Resolves elastic#120890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Security/Security Security issues without another label Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v8.18.1 v8.19.0 v9.0.0 v9.0.1 v9.1.0

Projects

None yet

3 participants