Skip to content

Remove explicit refresh_interval setting for Security system indices#97815

Merged
elasticsearchmachine merged 6 commits intoelastic:mainfrom
albertzaharovits:remove-refresh-setting-for-security-system-indices
Jul 20, 2023
Merged

Remove explicit refresh_interval setting for Security system indices#97815
elasticsearchmachine merged 6 commits intoelastic:mainfrom
albertzaharovits:remove-refresh-setting-for-security-system-indices

Conversation

@albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Jul 19, 2023 •
edited
Loading

Security system indices should rely on the implicit index.refresh_interval setting,
which is tuned by "usecase", rather than have it hard-coded by the Security plugin.
@albertzaharovits albertzaharovits self-assigned this Jul 19, 2023
@albertzaharovits
Copy link
Contributor Author

albertzaharovits commented Jul 19, 2023

@elasticsearchmachine run elasticsearch-ci/bwc
@albertzaharovits albertzaharovits added :Security/Security Security issues without another label >non-issue labels Jul 19, 2023
@albertzaharovits albertzaharovits marked this pull request as ready for review July 19, 2023 17:11
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Jul 19, 2023
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Jul 19, 2023

Pinging @elastic/es-security (Team:Security)
ywangd
ywangd approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM
...lusterTest/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerIntegTests.java Outdated
}
}

public void testTemplateSettingsDontAffectSecurityIndex() throws Exception {
Copy link
Member

@ywangd ywangd Jul 20, 2023

Choose a reason for hiding this comment

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

I like this test. Can we also assert that explicit updating the settings is rejected as well? I am aware this is blocked in general for system indices. But the code is away from here. There also have been bugs in how it handles settings that are not explicitly configured. So I think an explicit assertion here helps make it clear.
Copy link
Contributor Author

@albertzaharovits albertzaharovits Jul 20, 2023

Choose a reason for hiding this comment

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

I tacked on an explicit settings update (that fails) in the same test, after the index had been created: 6fe4de1, in order to minimize the overhead of a new test, and changed the test method name ecb6b77 to better reflect the scope of the test.
@albertzaharovits albertzaharovits added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 20, 2023
@elasticsearchmachine elasticsearchmachine merged commit b4fd3c8 into elastic:main Jul 20, 2023
@albertzaharovits albertzaharovits deleted the remove-refresh-setting-for-security-system-indices branch July 20, 2023 08:33
@albertzaharovits albertzaharovits mentioned this pull request Jul 20, 2023
Merged
albertzaharovits added a commit that referenced this pull request Jul 20, 2023
@albertzaharovits
Remove explicit refresh_interval from identity-provider internal index (
6b1ff15 
#97835)

The preference is now to rely on the implicit refresh_interval value
when creating internal indices.

Related #97815
@jfreden jfreden mentioned this pull request Oct 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.10.0

3 participants

@albertzaharovits @elasticsearchmachine @ywangd