[RFC] Retire support for V6 configuration #4493
Description
Preface
Note: I am asking for opinions and comments on the following topic because it influences the work on #3870.
Current situation
Configuration file formats
At the moment, the security plugin supports two slightly different configuration file formats:
In the *.yml files, these can be told apart by the _meta.config_version property. Internally, there these can be told apart by the V6 vs V7 suffix of class names: RoleV6 vs RoleV7. V7 in class names corresponds to config_version = 2.
Example for the differences:
New, V7:
_meta:
type: "roles"
config_version: 2
example_role:
cluster_permissions:
- CLUSTER_COMPOSITE_OPS
index_permissions:
- index_patterns:
- "foo*"
dls: 'query'
allowed_actions:
- CRUD
Old, V6:
example_role:
cluster:
- CLUSTER_COMPOSITE_OPS
indices:
'logstash-*':
'*':
- CRUD
- CREATE_INDEX
'_dls_': 'query'
Actually, the number 6 in the class names still stems from the Elasticsearch version 6. Thus, the roles.yml configuration type allows the specification of document types, even though these are not supported any more in OpenSearch 2. Consequently, the implementation actually ignores this information.
Thus, these configuration files stem from the ODFE times, when it was still available for Elasticsearch 6.
Further changes include some cleanup and restructuring of the configuration file format.
Privilege evaluation code
Even though the central privilege evaluation code is located in the class PrivilegeEvaluator, the actual logic which interprets the the roles is located in the classes ConfigModelV6 and ConfigModelV7:
- https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java
- https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
Depending on the used configuration version, either ConfigModelV6 and ConfigModelV7 is used. Even though both classes should apply the same logic, both contain separate implementations. Both implementations are very complex: ConfigModelV6 has 1316 lines of code and ConfigModelV7 has 1267 lines of code.
Especially for security critical code, it seems to be an anti-pattern to have such fundamental and critical code doubled up. It increases maintenance effort and cognitive load.
An additional drawbacks of the current approach is while the central SecurityDynamicConfiguration includes a generic parameter, it is virtually impossible to use this parameter in a safe way. Most APIs just use SecurityDynamicConfiguration<?> or do unsafe casts.
Current work
In the context of #3870, we are going to replace major parts of the ConfigModel classes by new code. As the goal of the code is performance improvements, its complexity will actually grow a bit compared to the old implementation. See here for the current state:
This implementation presently only supports the RoleV7 classes.
I have doubts whether implementing the same code also for RoleV6 would make sense. Creating also support for RoleV6 would cause significant effort and significant increase of the code complexity. Meanwhile, I would actually have doubts whether anyone is still using this configuration file format on OpenSearch 2.
Proposal: Drop support V6 configuration file formats
Thus, I would propose to drop support for the V6 configuration file formats. I really believe that this is largely just dead code, which just causes maintenance effort.
Alternative proposal: Behind the scenes conversion from V6 to V7.
Alternatively, I would propose to refactor the configuration infrastructure so that the V7 file formats become "first class citizens", while the V6 file formats get "second class citizens", which are never directly processed. The configuration infrastructure code would take care to automatically convert any V6 configuration to V7 configuration. Regular application code would then never need to touch V6 classes.
The V7 classes could then actually drop the V7 suffix to show that these are the main classes.