Always write empty role descriptor fields to index #110424
Conversation
jfreden
added
:Security/Security
|
Pinging @elastic/es-security (Team:Security) |
jfreden
added
test-full-bwc
| true, | ||
| featureService.clusterHasFeature(clusterService.state(), SECURITY_ROLES_METADATA_FLATTENED) | ||
| ); | ||
|
|
There was a problem hiding this comment.
I went back and forth on this but landed on that the cleanest approach is to handle the special case for the security index defaults and metadata_flattened logic here.
Simply using the docCreation is true is not enough since it's also used in the ApiKeyService in a couple of places, to create role descriptors. Since we don't do the same upsert operations on api keys, the default values are not needed and the metadata_falttened field is not needed. This could be fixed by providing a couple of extra parameters to toXContent or a context object of some sort, that would need to be populated by all callers. In the end this turned in to many booleans, instead of actually doing the logic where we have all the context.
The downside with this approach is that the role descriptor logic is now spread out.
| * Make sure all top level fields for a RoleDescriptor have default values to make sure they can be set to empty in an upsert | ||
| * call to the roles API | ||
| */ | ||
| public void testAllTopFieldsHaveEmptyDefaultsForUpsert() throws IOException, IllegalAccessException { |
There was a problem hiding this comment.
The purpose of this test is to catch any new top level role descriptor fields that do not provide defaults since it's easy to forget to add that and there is no guarantee that a regular unit test would catch that.
| {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["read"]}]}, "test3": | ||
| {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["write"]}]}}}"""; | ||
|
|
||
| {"cluster": ["all"],"indices": [{"names": ["*"],"privileges": ["read"]}], "description": "something"}, "test3": |
There was a problem hiding this comment.
Added some more explicit validation that we can reset some top level fields. These things are already implicitly caught by other tests but I think it's nice to do some more validation.
|
Serverless CI Failure is unrelated: |
|
In the description you mention |
|
Yes, there is a check if runAs != null and exclude it if it is |
* Always write empty role descriptor fields to index
Resolves: #110416
A RoleDescriptor's
toXContentfunction doesn't serialize some fields that are not set. This currently works because when we do an update, we will always overwrite the full RoleDescriptor in the index to make sure fields that are not set, will be updated properly.In the bulk role update, we use an upsert, that combined with not knowing what fields are set leads to us not clearing fields that are set to nothing in requests. To fix that we have some alternatives:
If we go with the first alternative, we can't leverage the noop functionality of the upsert anymore and for large volumes of roles being written, we would do a lot of unnecessary IO to write to the Lucene index. Since this is the expected behaviour of the Connectors project, I'm hesitant to going down that path.
This PR explores the second alternative to write empty fields for:
runAsremoteIndicesremoteClusterdescription