Move Security to use auto-managed system indices #67114

pugnascotia · 2021-01-06T16:53:07Z

Part of #61656.

Change the Security plugin so that its system indices are managed automatically by the system indices infrastructure.

elasticmachine · 2021-01-06T16:53:11Z

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

…-security

jaymode

Nice work! I left a comment about an edge case that I think needs addressed

jaymode · 2021-01-13T19:14:22Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

@@ -350,53 +308,6 @@ public void prepareIndexIfNeededThenExecute(final Consumer<Exception> consumer,
            } else if (indexState.indexExists() && indexState.isIndexUpToDate == false) {
                throw new IllegalStateException("Index [" + indexState.concreteIndexName + "] is not on the current version."
                        + "Security features relying on the index will not be available until the upgrade API is run on the index");
-            } else if (indexState.indexExists() == false) {


I think we still need to handle the case where the master node doesn't know about the security system index, so for BWC we should maintain this code somehow.

Interesting point.
I think we presently don't handle the cases of index auto-creation and mapping updates too well.
if .security does not yet exist, rolling update is in progress, and an API that creates the index for the first time hits a new node, the index will be created, but the other old nodes will subsequently complain that the index is not up to date (because the format number is greater) or that the mapping is not up to date (because the mapping version is greater).

I think this might be a common issue, I vaguely remember something similar for async search. Would it be possible to defer the system index upgrade (of the mapping and of the settings/metadata) until the rolling upgrade is complete, and let the security business logic deal with possibly storing entities in the old format, or if that's not possible, return a response failure informing that the requested API (with its parameters) is not available in a mixed cluster (similar to how no APIs are available until the cluster state has been recovered)?

I've reinstated the code for creating the security index, while changing the code so that it relies on the auto-create logic. I've also re-added code for the mappings not being up-to-date, where the runnable is ignore and the exception consumer is called. Is that what you had in mind?

I've also changed a number of locations to check for a different in minimum and maximum ES version across the cluster nodes. If there is a different, the auto-create logic will refuse to run, and the SystemIndexManager will not try to update mappings.

I've reinstated the code for creating the security index

I'm not sure this is need, the auto-create should handle it (now that auto-create has a predictable behaviour in a mixed cluster).

I've also re-added code for the mappings not being up-to-date, where the runnable is ignore and the exception consumer is called. Is that what you had in mind?

This fixes the problem, yes.

I've also changed a number of locations to check for a different in minimum and maximum ES version across the cluster nodes.

This is very cool. I like it that there is a clear, testable behavior in a mixed cluster scenario.

Hmm, if the create call is kept around, the rename from prepareIndexIfNeededThenExecute to checkIndexStateThenExecute is less fortunate 👿

albertzaharovits · 2021-01-14T20:12:51Z

@pugnascotia I'm also going to take a look at this tomorrow. I hope that's alright 🙂

pugnascotia · 2021-01-15T14:11:39Z

@albertzaharovits I would be very happy if you could take a look 🙏

albertzaharovits · 2021-01-18T20:52:53Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

@@ -350,53 +308,6 @@ public void prepareIndexIfNeededThenExecute(final Consumer<Exception> consumer,
            } else if (indexState.indexExists() && indexState.isIndexUpToDate == false) {


The changes around here are neat given that this is a refactoring! 👍
But I think we should let the runnable through if the index does not exist, and only then.
If the index exists, and it has an old mapping version we should hold off the runnable, because it can race with the service that updates the mapping. Related side note: can the mapping update service maybe hook into the "auto put mapping action" in a similar fashion that the system index creation hooks into the "index auto create action"?

I changed TransportPutMappingAction to enforce that any attempt to change mappings on a system index must supply the same mappings as the system index descriptor contains. I haven't updated TransportAutoPutMappingAction because I don't understand when that action is used or why.

Not to dwell too much on it, but IMO I think it is worth investing in adapting the auto mapping update action. It is added recently, so I don't expect there to be too obscure behaviors encoded. The reason I mention it is that moving the mapping update from a ClusterStateListener to another (from SecurityIndexManager to SystemIndexManager) is not a great step forward.

Just a suggestion, I haven't investigated it carefully, and it's outside security's concern.

albertzaharovits

I've left two comments about the core of it.
I think we have to think through mapping and metadata upgrades in a mixed cluster scenario, and that index requests don't race with the mapping updates.

Despite that, I think this is looking very promising, we've been eager for a long time to get rid of the update logic in the SecurityIndexManager.

…-security

albertzaharovits

LGTM though the important changes are outside Security Area's purview.
Thank you Rory!

albertzaharovits · 2021-01-21T14:36:10Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+
+    private XContentBuilder getIndexMappings() {
+        try {
+            final XContentBuilder builder = jsonBuilder();


Personally I would prefer the mapping as a resource file, I wonder what's the reason for this change.

I was following the example of other plugins that auto-created their indices. It does also make it harder to change the mappings, versus opening up a jar file and editing the json.

…-security

jaymode · 2021-01-27T21:35:56Z

server/src/main/java/org/elasticsearch/action/admin/indices/create/AutoCreateAction.java

-                        CreateIndexClusterStateUpdateRequest updateRequest = descriptor != null && descriptor.isAutomaticallyManaged()
+                        final boolean isSystemIndex = descriptor != null && descriptor.isAutomaticallyManaged();
+
+                        if (isSystemIndex && state.nodes().getMaxNodeVersion().after(state.nodes().getMinNodeVersion())) {


This concerns me if we cannot auto create a system index in a mixed version cluster even if the descriptor itself does not differ between versions. I think we should allow for creation still. I wonder if we should consider an approach to pull the most up to date version of the descriptor and apply those when creating the index; however there may be cases where that would fail if a feature was used that couldn't be validated on the master. In those cases I would consider falling back to the master version and allowing the master to update the mappings?

I don't think we can pull the most up-to-date version of the descriptor into the master, at least not without building infrastructure to make it pull-able.

How about: in the auto-create case, we always use the master's descriptor. If we release some new feature that needs to create a system index and relies on particular features being present, perhaps it can explicitly create the index using the right settings, either waiting until the cluster is in the right state, or retrying until the creation is successful. Admittedly this is just shifting the problem, but the general system index infra can't know at the moment whether it's OK to attempt creation in a mixed cluster.

I suppose we could extend the descriptor to apply a version range? e.g.

SystemIndexDescriptor.builder().setMinimimVersion(Version.V_8_0_0)

Then the auto-create, explicit create, mappings and settings methods can check the minimum descriptor version against state.nodes().getMinNodeVersion(). Code that relied on new features would still need to concern themselves with whether it was possible yet to create their system index. If that case becomes common/annoying enough, we can build more support infra.

I think I still want the SystemIndexManager to hold off upgrading mappings in a mixed cluster - partly in case of a failed upgrade, and partly to avoid disagreements around mappings versions. What do you think?

the general system index infra can't know at the moment whether it's OK to attempt creation in a mixed cluster

I think this is the crux, and we have to delegate this responsibility to the caller (eg Security(IndexManager)). My suggestion to fail the creation in the mixed cluster scenario was the simplest, in a sense, because the caller doesn't have to guard the system document index call with anything. But I haven't really considered the practical aspect that much more upgrades are done than changes to the system index's mapping. Moreover, most mapping changes are focused on a small portion of the total mapping. Preventing the creation of the system index in all these cases is wasteful, as the creation with the old mapping would be OK.

So, if the decision is to not reject system index creation (or mapping updates) in a mixed cluster scenario (whatever the strategy ultimately is), when indexing a document relying on new mappings (eg a new type of API key) we have to look at the cluster state, and probably reject the indexing (there could be other options, depending on the particular case). I think that's OK; I hoped we could avoid it, but not creating any index in the other cases, is probably not worth it.

...src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java

Allow system indices to be created or auto-created so long as their minimum node version requirement is met.

jaymode · 2021-01-28T19:29:28Z

...in/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

+
+                // `TransportCreateIndexAction` will automatically apply the right mappings, settings and aliases, so none
+                // of that needs to be specified here.
+                CreateIndexRequest request = new CreateIndexRequest(indexState.concreteIndexName).waitForActiveShards(ActiveShardCount.ALL);


Do we need to validate the mappings are good to go in cases where this node is not the master and there are mixed versions in the cluster?

…-security

jaymode

LGTM

Backport of elastic#67114. Part of elastic#61656. Change the Security plugin so that its system indices are managed automatically by the system indices infrastructure. Also add an `origin` field to `CreateIndexRequest` and `UpdateSettingsRequest`.

Re-apply changes from 0c9b9c1, which migrates the `.tasks` system index to be managed automatically by the system indices infrastructure. Changes went into #67114 that, I hope, will avoid the problems we saw before in the BWC tests in CI.

Re-apply changes from 0c9b9c1, which migrates the `.tasks` system index to be managed automatically by the system indices infrastructure. Changes went into elastic#67114 that, I hope, will avoid the problems we saw before in the BWC tests in CI.

Backport of #67114. Part of #61656. Change the Security plugin so that its system indices are managed automatically by the system indices infrastructure. Also add an `origin` field to `CreateIndexRequest` and `UpdateSettingsRequest`.

While backporting elastic#67114 via elastic#68375, I realised that there are existing upgrade scenarios that expect the `SecurityIndexManager` to update index mappings, so in the backport PR, this capability was reinstated. This commit does the same in `master`.

Re-apply changes from 0c9b9c1, which migrates the `.tasks` system index to be managed automatically by the system indices infrastructure. Changes went into #67114 that, I hope, will avoid the problems we saw before in the BWC tests in CI.

While backporting #67114 via #68375, I realised that there are existing upgrade scenarios that expect the `SecurityIndexManager` to update index mappings, so in the backport PR, this capability was reinstated. This commit does the same in `master`.

Move security to use auto-managed system indices

cae51b4

pugnascotia added >refactoring :Security/Security Security issues without another label v8.0.0 v7.12.0 labels Jan 6, 2021

pugnascotia requested a review from jaymode January 6, 2021 16:53

elasticmachine added the Team:Security Meta label for security team label Jan 6, 2021

pugnascotia added 4 commits January 6, 2021 20:48

Fix compilation error

27930e6

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

cfbbbff

…-security

Remove unused method

a3839ab

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

e6a4173

…-security

jaymode requested changes Jan 13, 2021

View reviewed changes

albertzaharovits self-requested a review January 14, 2021 20:10

albertzaharovits reviewed Jan 18, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

d36c527

…-security

pugnascotia requested review from jaymode and albertzaharovits January 19, 2021 14:01

Address review feedback

f570477

albertzaharovits approved these changes Jan 21, 2021

View reviewed changes

albertzaharovits reviewed Jan 21, 2021

View reviewed changes

pugnascotia added 3 commits January 25, 2021 14:57

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

bfc7064

…-security

Address review feedback

0a0a3c4

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

85c71ce

…-security

jaymode reviewed Jan 27, 2021

View reviewed changes

droberts195 reviewed Jan 28, 2021

View reviewed changes

...src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java Outdated Show resolved Hide resolved

...src/main/java/org/elasticsearch/action/admin/indices/settings/put/UpdateSettingsRequest.java Outdated Show resolved Hide resolved

Add minimumNodeVersion to system index descriptors

f9abca6

Allow system indices to be created or auto-created so long as their minimum node version requirement is met.

droberts195 mentioned this pull request Jan 28, 2021

[ML] Automatic management for ML system indices #68044

Merged

pugnascotia added 2 commits January 28, 2021 15:07

Checkstyle

2706be4

Supply Locale to String.format()

d54d060

jaymode reviewed Jan 28, 2021

View reviewed changes

pugnascotia added 2 commits February 1, 2021 11:03

Change SecurityIndexManager to contain a system index descriptor

8d30460

Merge remote-tracking branch 'upstream/master' into 61656-auto-create…

f531258

…-security

jaymode approved these changes Feb 1, 2021

View reviewed changes

pugnascotia merged commit 6ffddf8 into elastic:master Feb 2, 2021

pugnascotia deleted the 61656-auto-create-security branch February 2, 2021 13:43

This was referenced Feb 2, 2021

Move Security to use auto-managed system indices #68375

Merged

Migrate .tasks to be managed automatically #67351

Merged

pugnascotia mentioned this pull request Feb 8, 2021

Migrate .tasks to be managed automatically #68667

Merged

pugnascotia mentioned this pull request Feb 9, 2021

Allow SecurityIndexManager to update index mappings #68729

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move Security to use auto-managed system indices #67114

Move Security to use auto-managed system indices #67114

pugnascotia commented Jan 6, 2021

elasticmachine commented Jan 6, 2021

jaymode left a comment

jaymode Jan 13, 2021

albertzaharovits Jan 18, 2021

pugnascotia Jan 19, 2021

albertzaharovits Jan 21, 2021

albertzaharovits Jan 21, 2021

albertzaharovits commented Jan 14, 2021

pugnascotia commented Jan 15, 2021

albertzaharovits Jan 18, 2021

pugnascotia Jan 19, 2021

albertzaharovits Jan 21, 2021 •

edited

Loading

albertzaharovits left a comment

albertzaharovits left a comment

albertzaharovits Jan 21, 2021

pugnascotia Jan 27, 2021

jaymode Jan 27, 2021

pugnascotia Jan 28, 2021

albertzaharovits Jan 28, 2021

jaymode Jan 28, 2021

jaymode left a comment

		@@ -350,53 +308,6 @@ public void prepareIndexIfNeededThenExecute(final Consumer<Exception> consumer,
		} else if (indexState.indexExists() && indexState.isIndexUpToDate == false) {

Move Security to use auto-managed system indices #67114

Move Security to use auto-managed system indices #67114

Conversation

pugnascotia commented Jan 6, 2021

elasticmachine commented Jan 6, 2021

jaymode 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

albertzaharovits commented Jan 14, 2021

pugnascotia commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

albertzaharovits 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

jaymode left a comment

Choose a reason for hiding this comment

albertzaharovits Jan 21, 2021 •

edited

Loading