Add Role Mapping Cleanup Security Migration #114830
Conversation
| @@ -117,6 +117,7 @@ private static IndexVersion def(int id, Version luceneVersion) { | |||
| public static final IndexVersion ENABLE_IGNORE_MALFORMED_LOGSDB = def(8_514_00_0, Version.LUCENE_9_11_1); | |||
| public static final IndexVersion MERGE_ON_RECOVERY_VERSION = def(8_515_00_0, Version.LUCENE_9_11_1); | |||
| public static final IndexVersion UPGRADE_TO_LUCENE_9_12 = def(8_516_00_0, Version.LUCENE_9_12_0); | |||
| public static final IndexVersion ADD_ROLE_MAPPING_MIGRATION = def(8_517_00_0, Version.LUCENE_9_12_0); | |||
There was a problem hiding this comment.
This is needed to allow us to check if an index was created on the same version as the migration. This means that the index was created with all the settings/mappings/docs needed, so we don't need to run the migration (it can be short-circuit and skipped since there is nothing to migrate).
...main/java/org/elasticsearch/xpack/security/action/rolemapping/ReservedRoleMappingAction.java
Outdated
Show resolved
Hide resolved
...curity/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrationExecutor.java
Outdated
Show resolved
Hide resolved
jfreden
force-pushed
the
add_eck_migration_8_16
branch
from
84c38fc
to
d36b152
Compare
| @@ -267,6 +272,11 @@ private static boolean isCreatedOnLatestVersion(IndexMetadata indexMetadata) { | |||
| return indexVersionCreated != null && indexVersionCreated.onOrAfter(IndexVersion.current()); | |||
| } | |||
|
|
|||
| private static Set<String> getFileSettingsMetadataHandlerRoleMappingKeys(ClusterState clusterState) { | |||
There was a problem hiding this comment.
This isn't exactly part of the security index, so this doesn't belong here, but I added it here anyway for simplicity. Ideally the security migration state should be tracked in a separate cluster state handler. I think that's an improvement/refactor that should be done in a separate PR if we keep adding migrations on state outside the security index.
The new state handler would still need to handle the main security index manager state, since we rely heavily on state from it in the framework (is the index available for example).
jfreden
force-pushed
the
add_eck_migration_8_16
branch
from
ddfe166
to
c23b783
Compare
| createNativeRoleMapping("no_name_conflict", Map.of("meta", "test")); | ||
| assertAllRoleMappings(client(), "no_name_conflict", "operator_role_mapping_1"); | ||
| } else if (CLUSTER_TYPE == ClusterType.UPGRADED) { | ||
| waitForSecurityMigrationCompletion(adminClient(), 2); |
There was a problem hiding this comment.
These tests will fail here until the file service change is merged, since the preconditions for the migration are not met.
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @jfreden, I've created a changelog YAML for you. |
| @@ -189,7 +189,7 @@ public List<TemplateRoleName> getRoleTemplates() { | |||
| * This is not used within the mapping process, and does not affect whether the expression matches, nor which roles are assigned. | |||
| */ | |||
| public Map<String, Object> getMetadata() { | |||
| return Collections.unmodifiableMap(metadata); | |||
| return metadata != null ? Collections.unmodifiableMap(metadata) : Collections.emptyMap(); | |||
There was a problem hiding this comment.
These are not set to null anywhere in production code, but could be in the future (since constructor is public) and can in tests (as I found out).
| ) | ||
| ); | ||
|
|
||
| public static class RoleMetadataFlattenedMigration implements SecurityMigration { |
There was a problem hiding this comment.
No changes here, just moved this to a named class since I wanted CleanupRoleMappingDuplicatesMigration to be a named class for testing purposes and it felt weird to have one anonymous migration and one that is not.
| } | ||
| } | ||
|
|
||
| public static class CleanupRoleMappingDuplicatesMigration implements SecurityMigration { |
This PR adds a cleanup security migration for role mapping duplicates that exist both in cluster state and the
.securityindex. The cleanup task is implemented as a security migration.The task compares the names of the role mappings in the operator-defined
settings.jsonand the names of the role mappings in the.securityindex and removes the ones in the.securityindex.Issue
This cleanup is done because there was a bug (fixed here) that caused ECK customers to have the same role mappings with the same names present both in cluster state and the
.securityindex. The effective role mapping is the combination of the.securityindex mapping and the cluster state one. After the bug fix, if the mapping in cluster state is modified (throughsetting.json), the.securityindex will still contain the old mapping and this could lead to unintended behaviour.Dependencies