Operator/role mapping #89667

grcevski · 2022-08-26T16:17:49Z

This PR adds support for /_security/role_mapping for file based settings. We create similar reserved cluster state information about the role mappings, even though they are not part of the cluster state.

Since the role_mappings are not saved in the cluster state, we needed to create separate reserved state action wrapper for any non-cluster state updates.

Relates to #89183

elasticsearchmachine · 2022-08-26T16:18:13Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2022-08-26T16:18:13Z

Hi @grcevski, I've created a changelog YAML for you.

grcevski · 2022-08-26T16:18:59Z

...src/main/java/org/elasticsearch/action/support/ReservedStateAwareHandledTransportAction.java

+ * An extension of the {@link HandledTransportAction} class, which wraps the doExecute call with a check for clashes
+ * with the reserved cluster state.
+ */
+public abstract class ReservedStateAwareHandledTransportAction<Request extends ActionRequest, Response extends ActionResponse> extends


Since the role mappings don't use TransportMasterNodeAction (which is cluster state specific) we add this small wrapper class to ensure the reserved state information is checked before allowing REST actions to create/update/delete role mappings.

grcevski · 2022-08-26T16:19:44Z

server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java

-                format("Failed to process request [%s] with errors: [%s]", request, String.join(", ", errors))
-            );
-        }
+        ReservedClusterStateService.validateForReservedState(state, handlerName.get(), modifiedKeys(request), request.toString());


Since this check is common now, I moved it to ReservedClusterStateService to be reused.

grcevski · 2022-08-26T16:20:12Z

server/src/main/java/org/elasticsearch/action/support/master/TransportMasterNodeAction.java


    // package private for testing
-    boolean supportsImmutableState() {
+    boolean supportsReservedState() {


These are simple renames, which were somehow missed before. We did rename from immutable to reserved.

grcevski · 2022-08-26T16:20:45Z

server/src/main/java/org/elasticsearch/reservedstate/service/ReservedClusterStateService.java

+     * @param modified the set of modified keys by the related request
+     * @param request a string representation of the request for error reporting purposes
+     */
+    public static void validateForReservedState(ClusterState state, String handlerName, Set<String> modified, String request) {


Moved here from TransportMasterNodeAction for reuse.

grcevski · 2022-08-26T16:21:29Z

...ain/java/org/elasticsearch/xpack/core/security/action/rolemapping/PutRoleMappingRequest.java

        return new ExpressionRoleMapping(name, rules, roles, roleTemplates, metadata, enabled);
    }
+
+    public static PutRoleMappingRequest fromMapping(ExpressionRoleMapping mapping) {


Helper method used by the reserved handler and test to create the request from the parsed mappings.

grcevski · 2022-08-26T16:22:45Z

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java

+    }
+
+    @SuppressWarnings("unchecked")
+    public Collection<PutRoleMappingRequest> prepare(Object input) {


We convert from expression role mappings to a request to satisfy the native store API contract.

grcevski · 2022-08-26T16:23:39Z

...rc/test/java/org/elasticsearch/xpack/security/LocalReservedSecurityStateHandlerProvider.java

+/**
+ * Mock Security Provider implementation for the {@link ReservedClusterStateHandlerProvider} service interface
+ */
+public class LocalReservedSecurityStateHandlerProvider implements ReservedClusterStateHandlerProvider {


Mock version of the SPI handler provider so I could test properly with integration tests.

…arch into operator/role_mapping

grcevski · 2022-08-26T22:10:38Z

@elasticsearchmachine run elasticsearch-ci/part-1

ywangd

This is looking great. I don't think I'll find anything major. There are places that can use future refactoring. But I think the changes are plenty already for this PR. So we can wait.

Since there are many changes, I just need some more time to double check and read through the tests. But I'd like to make it clear that I am progressing on it by providing some minor optional feedback.

I also have a question: Will there only be a single file-state-update task to run at a time, i.e. it is not possible that two threads observed file changes and start parallel updating?

ywangd · 2022-09-15T05:22:24Z

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java

+        return NAME;
+    }
+
+    public Collection<PutRoleMappingRequest> prepare(List<ExpressionRoleMapping> roleMappings) {


Can this method be private?

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java

ywangd · 2022-09-15T05:26:24Z

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java

+        return new TransformState(prevState.state(), prevState.keys(), ((l) -> nonStateTransform(requests, prevState, l)));
+    }
+
+    private Void nonStateTransform(


Nit: Is there a reason to use Void instead of void? I think using void is simpler because there is no need to return null in the code?

100% I don't remember now why I made it Void, must've been an iteration I had when the consumer was a function.

ywangd · 2022-09-15T08:57:28Z

I think we could use a few more tests. For example, making sure TransportPutRoleMappingAction actually respects the reserved keys. Or maybe this can be tested with ReservedStateAwareHandledTransportAction? Or maybe it is already tested somewhere else since it is extraction refactoring? It's not entirely clear to me.

grcevski · 2022-09-15T13:52:13Z

I also have a question: Will there only be a single file-state-update task to run at a time, i.e. it is not possible that two threads observed file changes and start parallel updating?

File based settings doesn't allow more than one update to run at the same time. So while no synchronization exists in the reserved state service, the file based settings service uses a latch to wait on until all async actions inside the reserved state service to finish, before it checks for new file updates. While an update is going on on certain file contents, the JDK watcher service can queue new file update changes, which will get processed one by one after the previous in-flight update finishes. We have a test for some of this logic in FileSettingsServiceTest.testStopWorksInMiddleOfProcessing and testStopWorksIfProcessingDidntReturnYet.

Co-authored-by: Yang Wang <[email protected]>

grcevski · 2022-09-15T14:01:37Z

I think we could use a few more tests. For example, making sure TransportPutRoleMappingAction actually respects the reserved keys. Or maybe this can be tested with ReservedStateAwareHandledTransportAction? Or maybe it is already tested somewhere else since it is extraction refactoring? It's not entirely clear to me.

Some of this is tested in RoleMappingFileSettingsIT.assertRoleMappingsSaveOK. At the end the method checks that a REST call cannot update any of the reserved role mappings, but this is a good call-out, I'll add explicit tests for validation of the keys in TransportPutRoleMappingAction, similarly as I've done for TransportPutLifecycleActionTests.

I also have tests that perform the validation on TransportMasterNodeAction (TransportMasterNodeActionTests.testRejectImmutableConflictClusterStateUpdate), but not on the ReservedStateAwareHandledTransportAction which I introduced in the refactor. I'll add tests here too, thanks for bringing it up.

…arch into operator/role_mapping

ywangd

LGTM. Thanks for the iterations. I appreciate your patience and dedication to get this across the line. I learnt a lot in the process.

It would be nice if we could have tests for following two scenarios:

Role mappings pass validation but fail to write to index. A easy way to achieve this is to close the security index.
Role mappings succeed, but final cluster state update fails. I am not sure how easy or hard to force cluster state failure at that point.

Happy for them to be follow-ups.

Also my previous experience with Java's WatchService is that it does not work if the file system is NFS. Maybe this is no longer the case? Or maybe this feature does not need work with NFS? Anyhow, even it is an issue, it is not part of this PR. I am bringing it up just in case.

...est/java/org/elasticsearch/action/support/ReservedStateAwareHandledTransportActionTests.java

ywangd · 2022-09-17T12:58:56Z

...ty/src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java

+                          "roles": [ "kibana_user" ],
+                          "rules": { "field": { "username": "*" } },
+                          "metadata": {
+                             "uuid" : "b9a59ba9-6b92-4be2-bb8d-02bb270cb3a7"


Can we add a reserved metadata key/value here, e.g. "_foo": "something"?

...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java

ywangd · 2022-09-17T13:42:11Z

...ty/src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java

+            writeJSONFile(internalCluster().getMasterName(), emptyJSON);
+            boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
+            assertTrue(awaitSuccessful);


Once the empty JSON file is processed, do the role mappings get deleted? If so, I think we should assert the role mappings are now gone and nothing is reserved.

Co-authored-by: Yang Wang <[email protected]>

…arch into operator/role_mapping

grcevski · 2022-09-19T19:38:08Z

It would be nice if we could have tests for following two scenarios:

Role mappings pass validation but fail to write to index. A easy way to achieve this is to close the security index.

Role mappings succeed, but final cluster state update fails. I am not sure how easy or hard to force cluster state failure at that point.

Good idea, I'm going to make a test for 1. I think I can test 2. with some mocked methods, but I guess in that case we are simply testing the flaw in our atomicity across the two stores. Perhaps we can add a test here if we ever fix this situation in the future.

Also my previous experience with Java's WatchService is that it does not work if the file system is NFS. Maybe this is no longer the case? Or maybe this feature does not need work with NFS? Anyhow, even it is an issue, it is not part of this PR. I am bringing it up just in case.

Thanks for this info, I hadn't tested with an NFS file system. I think it's not part of the requirements, but we should definitely document is as a limitation. I'll follow-up with an investigation here.

grcevski · 2022-09-20T17:51:26Z

Thanks for all the help Yang!

Nikola Grcevski added 4 commits August 25, 2022 14:55

Fix FileSettingsService hang on error update

cd4e5ee

Initial code for reserved role mappings

4723726

Handle security disabled.

2f79c83

Tests and conflict check for role mappings

3cfaa82

grcevski added >enhancement :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team v8.5.0 labels Aug 26, 2022

elasticsearchmachine removed the Team:Security Meta label for security team label Aug 26, 2022

Update docs/changelog/89667.yaml

e7736b1

grcevski commented Aug 26, 2022

View reviewed changes

grcevski mentioned this pull request Aug 26, 2022

File Based Settings for Elasticsearch #89183

Closed

10 tasks

Nikola Grcevski added 4 commits August 26, 2022 16:40

Add hashcode/equals for deduplication

e67636d

Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…

39803d4

…arch into operator/role_mapping

Attempt to fix test

aa38aa8

Merge branch 'operator/fix_error_save' into operator/role_mapping

3ecd25b

Nikola Grcevski added 3 commits August 29, 2022 10:28

Merge branch 'main' into operator/role_mapping

ebecdeb

Refactor for more code reuse

6fcf396

Merge branch 'main' into operator/role_mapping

524572b

ywangd self-requested a review August 29, 2022 22:54

Bring back the dedup in test handler provider

83cbdd4

ywangd reviewed Sep 15, 2022

View reviewed changes

Apply nit suggestion

b8d52a6

Co-authored-by: Yang Wang <[email protected]>

Nikola Grcevski added 5 commits September 15, 2022 10:02

Change Void to void.

48094e2

Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…

df7f76c

…arch into operator/role_mapping

Change prepare to private

3889f03

Add reserved transport action tests.

c99d0c3

Add more tests for validation

04046d5

ywangd approved these changes Sep 17, 2022

View reviewed changes

Nikola Grcevski and others added 6 commits September 19, 2022 14:53

Merge branch 'main' into operator/role_mapping

9f8b4bc

Apply suggestion on waiting for future

caeb188

Co-authored-by: Yang Wang <[email protected]>

Use addSuppressed for exception collection

1f2ce32

Co-authored-by: Yang Wang <[email protected]>

Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…

af2a181

…arch into operator/role_mapping

Apply code review suggestions.

f2afc9f

Fix compile error.

fb906d2

Nikola Grcevski added 8 commits September 19, 2022 16:39

Add test with closed security

796e61d

Fix test to handle empty keys

1964bf7

Fix missing throw.

d2af580

Extra assert on null setting.

d41cd20

Merge branch 'main' into operator/role_mapping

e988bde

Fix compile error from merge

5058f74

Fix version bug and test bug.

8ac3287

Spotless

cea0668

grcevski merged commit 2b3dbde into elastic:main Sep 20, 2022

grcevski deleted the operator/role_mapping branch September 20, 2022 14:34

Operator/role mapping #89667

Operator/role mapping #89667

Uh oh!

Conversation

grcevski commented Aug 26, 2022

Uh oh!

elasticsearchmachine commented Aug 26, 2022

Uh oh!

elasticsearchmachine commented Aug 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Aug 26, 2022

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented Sep 15, 2022

Uh oh!

grcevski commented Sep 15, 2022

Uh oh!

grcevski commented Sep 15, 2022

Uh oh!

ywangd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grcevski commented Sep 19, 2022

Uh oh!

grcevski commented Sep 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants