-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Test that built-in roles get synced after rolling upgrade #119841
Test that built-in roles get synced after rolling upgrade #119841
Conversation
Adds a rolling upgrade tests to verify that built-in roles get synced to the `.security` index after rolling upgrade completes.
Pinging @elastic/es-security (Team:Security) |
import static org.hamcrest.Matchers.notNullValue; | ||
import static org.hamcrest.Matchers.nullValue; | ||
|
||
public class QueryableBuiltInRolesUpgradeIT extends AbstractUpgradeTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is only relevant when upgrading from versions before 8.18.0, hence I think it makes no sense to add it to the main branch (9.0.0) as we will only ever update from 8.18.0 to 9.0.0 and both branches support queryable roles feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it might still be worth it to include as a sanity-check that the roles sync does not somehow interfere with the upgrade. It's just a matter of also having the test suite in main
right? Or is it more complex than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing which would be worthy is having es.queryable_built_in_roles_enabled
set to true
for all BWC tests. This is something I plan to change in the near future. But I'm okay to forward-port this PR to the main
in the mean time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- one non-blocking question.
final int numberOfNodes = 3; // defined in build.gradle | ||
waitForNodes(numberOfNodes); | ||
|
||
final Set<TestNodeInfo> nodes = collectNodeInfos(adminClient()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we use clusterHasFeature
instead? Not blocking since the current approach works but clusterHasFeature
seems like a better fit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clusterHasFeature
is not quite fitting my use case right out of the box. I need to know which nodes (based on the version) in the cluster support which features. Knowing that the whole cluster (or just some of the nodes) support the queryable feature, is not very helpful. I need a distinction between old and new nodes and their supported features.
I could make it work similarly to ParameterizedFullClusterRestartTestCase
and keep a copy of old cluster's feature service and then combine decision making with a new feature services (mixed and upgraded), but that also felt too hacky as I simply need to know supported nodes_features
and their versions.
💚 Backport successful
|
…9841) Adds a BWC test to verify that built-in roles get synced to the `.security` index after rolling upgrade completes.
Adds a BWC test to verify that built-in roles get synced to the
.security
index after rolling upgrade completes.