Added View API and Rollback API for Versioned Security Configurations #5431

divyanshi-0402 · 2025-06-23T09:16:06Z

Description

Implementing View API and Rollback API for Versioned Security Configurations, build upon the changes from the PR : Versioned Security Configuration Management #5357.

API Design

View Security Configuration Versions

Purpose: View the complete history of security configuration changes, or any specific version of the security configurations.

Endpoint:

To view all the versions : GET /_plugins/_security/api/version
To view a specific version : GET /_plugins/_security/api/version/{versionId}

Access Control: Admin/Security Manager permissions required

Response Format for specific version:

curl -XGET "https://localhost:9200/_plugins/_security/api/version/v2?pretty" -u 'admin:Divvv@admin123' --insecure

{
  "versions": [
    {
      "version_id": "v2",
      "timestamp": "2025-05-23T06:56:20.081933886Z",
      "modified_by": "admin",
      "security_configs": {
        "rolesmapping": {
          "lastUpdated": "2025-05-23T06:56:20.081933886Z",
          "configData": {
             "_meta" : {
              "type" : "rolesmapping",
              "config_version" : 2
            },
            "all_access": {
              "backend_roles": ["admin"],
              "users": [],
              "hosts": [],
              "reserved": false,
              "hidden": false,
              "description": "Maps admin to all_access",
              "and_backend_roles": []
            },
            ...
          }
        },
        "roles": {
          "lastUpdated": "2025-05-23T06:56:20.081933886Z",
          "configData": {
            "_meta" : {
              "type" : "roles",
              "config_version" : 2
            },
            "alerting_full_access": {
              "cluster_permissions": [
                "cluster:admin/opendistro/alerting/*",
                "cluster:admin/opensearch/alerting/*",
                ...
              ],
              "index_permissions": [
                {
                  "index_patterns": ["*"],
                  "allowed_actions": ["indices:admin/mappings/get", ...]
                }
              ],
              "reserved": true,
              "hidden": false,
              "static": false
            },
            ...
          }
        },
        "internalusers": {
          "lastUpdated": "2025-05-23T06:56:20.081933886Z",
          "configData": {
            "_meta" : {
              "type" : "internalusers",
              "config_version" : 2
            },
            "admin": {
              "backend_roles": ["admin"],
              "reserved": true,
              "description": "Demo admin user",
              ...
            },
            "testuser": {
              "backend_roles": [],
              "reserved": false,
              ...
            },
            ...
          }
        },
        ...
      }
    }
  ]
}

Response Format for all versions :

curl -XGET "https://localhost:9200/_plugins/_security/api/version?pretty" -u 'admin:Divvv@admin123' --insecure

{
  "versions": [
    {
      "version_id": "v1",
      "timestamp": "2025-05-22T08:46:11.887620466Z",
      "modified_by": "system",
      "security_configs": {
        "rolesmapping": {
          "lastUpdated": "2025-05-22T08:46:11.887620466Z",
          "configData": {
            "_meta" : {
              "type" : "rolesmapping",
              "config_version" : 2
            },
            "all_access": {
              "backend_roles": ["admin"],
              "users": [],
              "hosts": [],
              "reserved": false,
              "hidden": false,
              "description": "Maps admin to all_access",
              "and_backend_roles": []
            },
            ...
          }
        },
        "internalusers": {
          "lastUpdated": "2025-05-22T08:46:11.887620466Z",
          "configData": {
             "_meta" : {
              "type" : "internalusers",
              "config_version" : 2
            },
            "admin": {
              "backend_roles": ["admin"],
              "reserved": true,
              "hidden": false,
              "description": "Demo admin user",
              ...
            },
            ...
          }
        },
        ...
      }
    },
    {
      "version_id": "v2",
      "timestamp": "2025-05-23T06:56:20.081933886Z",
      "modified_by": "admin",
      "security_configs": {
        "rolesmapping": {
          "lastUpdated": "2025-05-23T06:56:20.081933886Z",
          "configData": {
             "_meta" : {
              "type" : "rolesmapping",
              "config_version" : 2
            },
            "all_access": {
              "backend_roles": ["admin"],
              "users": [],
              "hosts": [],
              "reserved": false,
              "hidden": false,
              "description": "Maps admin to all_access",
              "and_backend_roles": []
            },
            ...
          }
        },
        "internalusers": {
          "lastUpdated": "2025-05-23T06:56:20.081933886Z",
          "configData": {
            "_meta" : {
              "type" : "internalusers",
              "config_version" : 2
            },
            "admin": {
              "backend_roles": ["admin"],
              "reserved": true,
              "hidden": false,
              "description": "Demo admin user",
              ...
            },
            "testuser": {
              "backend_roles": [],
              "reserved": false,
              "hidden": false,
              ...
            },
            ...
          }
        },
        ...
      }
    },
    ...
  ]
}

Response format if the version doesn't exist :

curl -XGET "https://localhost:9200/_plugins/_security/api/version/v3?pretty" -u 'admin:Divvv@admin123' --insecure
{
  "status" : "NOT_FOUND",
  "message" : "Version v3 not found"
}

Response if try to access with non admin role :

curl -XGET "https://localhost:9200/_plugins/_security/api/version?pretty" -u 'test_user:TestAPI@123' --insecure                       
{
  "status" : "FORBIDDEN",
  "message" : "No permission to access REST API: User test_user with Security roles [read_only_role, own_index] does not have any role privileged for admin access. No client TLS certificate found in request"
}

Rollback to desired Security Configuration Versions

Purpose : Allows rollback to immediate or any desired version of security

Endpoint:

To rollback to immediate previous version : POST /_plugins/_security/api/rollback
To rollback to a specific version : POST /_plugins/_security/api/rollback/version/{versionID}

Access Control: Admin/Security Manager permissions required

Response format to rollback to immediate previous version :

[.opendistro_security_config_versions] index has 8 versions 

curl -XPOST "https://localhost:9200/_plugins/_security/api/rollback" -u 'admin:Divvv@admin123' --insecure           
{
  "status" : "OK",
  "message" : "config rolled back to version v7"
}


 Detected changes in security configuration: [{"op":"add","path":"/internalusers/testuser","value":{"attributes":{},"backend_roles":[],"hash":"$2y$12$3q6lxV8DBxvifcRJ4tMuUeclwyJzawJTzhY.JcKEpL1TdWy.JeUPu","hidden":false,"opendistro_security_roles":[],"reserved":false,"static":false}}]
 Successfully saved version v9 to .opendistro_security_config_versions

Response format to rollback to a specific version :

curl -XPOST "https://localhost:9200/_plugins/_security/api/rollback/version/v3" -u 'admin:Divvv@admin123' --insecure
{
  "status" : "OK",
  "message" : "config rolled back to version v3"
}% 


Detected changes in security configuration: [{"op":"remove","path":"/internalusers/testuser"}]
Successfully saved version v10 to .opendistro_security_config_versions

Response format if the version doesn't exist :

curl -XPOST "https://localhost:9200/_plugins/_security/api/rollback/version/v13" -u 'admin:Divvv@admin123' --insecure
{
  "status" : "NOT_FOUND",
  "message" : "Version v13 not found"
}

Response if try to access with non admin role :

curl -XPOST "https://localhost:9200/_plugins/_security/api/rollback?pretty" -u 'test_user:TestAPI@123' --insecure 
{
  "status" : "FORBIDDEN",
  "message" : "No permission to access REST API: User test_user with Security roles [read_only_role, own_index] does not have any role privileged for admin access. No client TLS certificate found in request"
}

Test details for multi-node testing:

Created two nodes : opensearch-node1 and opensearch-node1

node 1 :

./bin/opensearch -E cluster.name=my-cluster -E node.name=node1 -E path.data=./data1 -E http.port=9200 -E transport.port=9300

[node1] Successfully saved version v1 to .opensearch_security_config_versions

[INFO ][o.o.s.c.ConfigurationRepository] [node1] Node 'node1' initialized

node 2 :

./bin/opensearch -E cluster.name=my-cluster -E node.name=node2 -E path.data=./data2 -E http.port=9201 -E transport.port=9301

[INFO ][o.o.s.c.ConfigurationRepository] [node2] Node 'node2' initialized

Rollback initiated from node-1:

curl -XPOST "https://172.19.119.134:9200/_plugins/_security/api/rollback" -u 'admin:admin' --insecure
{
  "status" : "OK",
  "message" : "config rolled back to version v1"
}%                   

[node1] Detected changes in security configuration: [{"op":"remove","path":"/internalusers/testuser"}]
[node1] Successfully saved version v3 to .opensearch_security_config_versions

Rollback initiated from node-2 :

curl -XPOST "https://172.19.119.134:9201/_plugins/_security/api/rollback/version/v1" -u 'admin:admin' --insecure                                       
{
  "status" : "OK",
  "message" : "config rolled back to version v1"
}

[node1] Detected changes in security configuration: [{"op":"remove","path":"/internalusers/testuser"}]
[node1] Successfully saved version v5 to .opensearch_security_config_versions

Issues Resolved

Related to #5093

Testing

Unit Tests and Integration Tests included

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Divyanshi Chouksey <divvv@amazon.com>

nagarajg17 · 2025-06-23T10:49:51Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+    private ValidationResult<SecurityConfiguration> viewAllVersions() throws IOException {
+        SecurityConfigVersionDocument doc = versionsLoader.loadFullDocument();
+
+        return ValidationResult.error(OK, (builder, params) -> {


Why error? Should it be ValidationResult.success?

+1. Should be ValidationResult.success here.

ValidationResult.success only takes a SecurityConfiguration as content object, so in order to build a custom JSON I had to use this.

.error() is not the correct usage. You can simply create response class that extends ToXContentObject and then return an instance of that object here.
You can refer this file: https://github.com/opensearch-project/security/pull/5389/files#diff-157c9f69cf6c49e618db3748a7803470432b527ce78fe5251228d093daa5483e

We cannot use ValidationResult.error, it is very misleading and wrong pattern

nagarajg17 · 2025-06-23T11:09:06Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+            builder.field("security_configs");
+            Map<String, Object> plainConfigs = new LinkedHashMap<>();
+            for (Map.Entry<String, SecurityConfigVersionDocument.HistoricSecurityConfig<?>> entry : ver.getSecurity_configs().entrySet()) {
+                Map<String, Object> scMap = new LinkedHashMap<>();


nit: Update variable name

nagarajg17 · 2025-06-23T11:32:24Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+        var maybeVer = doc.getVersions().stream().filter(v -> versionId.equals(v.getVersion_id())).findFirst();
+
+        if (maybeVer.isEmpty()) {
+            return ValidationResult.error(NOT_FOUND, payload(NOT_FOUND, "Version " + versionId + " not found"));
+        }


Do we need this here? We seem to be doing in rollbackCommon() as well

Ack, doing it once.

This might make more sense to keep in rollbackCommon() since you would need to check this anyway for any caller of this function

nagarajg17 · 2025-06-23T12:26:12Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+                    throw new IOException("Rollback aborted: Could not fetch current config for cType '" + cTypeName + "'");
+                }
+
+                SecurityDynamicConfiguration<?> sdc = SecurityDynamicConfiguration.empty(cType);


nit: rename or expand it

since everything is in context of security you can name the variables as updateConfig or rollBackConfig since this indicates a new entry to be created from existing entry.

nagarajg17 · 2025-06-23T12:31:34Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+                CType<?> cType = CType.fromString(cTypeName);
+                if (cType == null) {
+                    throw new IOException("Rollback aborted: Unknown config type '" + cTypeName + "' found in version");
+                }


nit: This check needs to be done before checking data for that cType?

Also exception type IOException doesn't seem appropriate. IllegalStateException?
Check in other places as well

this should never be null since we are responsible for storing the information. A Ctype will be unknown only when security plugin moves away from current CTypes. Also this should throw NPE since we use cTypes as keys. Another option would be NoSuchFieldException.

nagarajg17 · 2025-06-23T12:41:35Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+                sdc.setPrimaryTerm(currentConfig.getPrimaryTerm());
+
+                for (Map.Entry<String, ?> configEntry : sc.getConfigData().entrySet()) {
+                    if ("_meta".equals(configEntry.getKey())) {


Why do we skip _meta? Good to add comments for these kinds for readers of code to understand

+1.

What is the expected behavior when config version changes? i.e. roles change from v7 to v8.

Including _meta creates a duplicate entry for the field.

Currently we have rollback functionality for same versions of config types.

Including _meta creates a duplicate entry for the field

Why? which other code is adding _meta field?

nagarajg17 · 2025-06-23T12:46:17Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+            for (Map.Entry<CType<?>, SecurityDynamicConfiguration<?>> entry : configsToApply.entrySet()) {
+                AbstractApiAction.saveAndUpdateConfigsAsync(
+                    securityApiDependencies,
+                    client,
+                    entry.getKey(),
+                    entry.getValue(),
+                    new ActionListener<IndexResponse>() {
+                        @Override
+                        public void onResponse(IndexResponse r) {
+                            latch.countDown();
+                        }
+
+                        @Override
+                        public void onFailure(Exception e) {
+                            log.error("Rollback: failed to write config for cType={} : {}", entry.getKey().toLCString(), e.getMessage(), e);
+                            latch.countDown();
+                        }
+                    }
+                );
+                log.info("Rollback: wrote config data for cType={}", entry.getKey().toLCString());
+            }
+
+            if (!latch.await(20, java.util.concurrent.TimeUnit.SECONDS)) {
+                throw new IOException("Timeout while writing rolled-back configs");
+            }


Can we move this to another function?

Timeout value can be constant

Magic variables should be declared as class level constants.

nagarajg17 · 2025-06-23T12:47:29Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+
+        for (Map.Entry<CType<?>, SecurityDynamicConfiguration<?>> entry : backups.entrySet()) {
+            try {
+                AbstractApiAction.saveAndUpdateConfigs(securityApiDependencies, client, entry.getKey(), entry.getValue()).actionGet();


High: This won't update cache

Can we test for this as well

Have we tested the sync vs async behavior here?

Yes, async updates cache and triggers update for version index as well, sync doesn't.

nagarajg17 · 2025-06-23T12:48:54Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+        return builder;
+    }
+
+    private boolean isConfigEqual(SecurityDynamicConfiguration<?> currentConfig, SecurityDynamicConfiguration<?> targetConfig) {


nit: This can be util function I think. Its reusable by any other code

nagarajg17 · 2025-06-23T12:50:48Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+        }
+    }
+
+    private void rollbackConfigsToSecurityIndex(SecurityConfigVersionDocument.Version<?> versionData) throws IOException {


Variable names are not clear. Reading is bit tough here. Need to go up and down to know what variable is.
Rename such that names give meaning of that it is intended for. For ex: currentConfig used here is a good naming, easy to understand

nagarajg17 · 2025-06-23T12:55:01Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+            return ValidationResult.error(NOT_FOUND, payload(NOT_FOUND, "Version " + versionId + " not found"));
+        }
+
+        SecurityConfigVersionsLoader.sortVersionsById(doc.getVersions());


Do we need sorting here?

Not required, removed.

cwperks · 2025-06-23T17:18:31Z

curl -XPOST "https://172.19.119.134:9200/_plugins/_security/api/rollback" -u 'admin:admin' --insecure
{
  "status" : "OK",
  "message" : "config rolled back to version v1"
}%                   

[node1] Detected changes in security configuration: [{"op":"remove","path":"/internalusers/testuser"}]
[node1] Successfully saved version v3 to .opensearch_security_config_versions

In the context of the changes I understand why this operation will create a v3, but I do think its a bit confusing that a rollback creates a new "version".

cwperks · 2025-06-23T17:20:42Z

Is @lastUpdated specific to the individual config type or does it apply to the entire version?

cwperks · 2025-06-23T17:23:19Z

src/integrationTest/java/org/opensearch/security/api/RollbackVersionApiIntegrationTest.java

+        return PLUGINS_PREFIX + "/api";
+    }
+
+    private String RollbackBase() {


If these are all constants can we declare them as constant fields at the top of this file?

cwperks · 2025-06-23T17:34:21Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+    private static final Logger log = LogManager.getLogger(RollbackVersionApiAction.class);
+
+    private static final List<Route> routes = addRoutesPrefix(
+        ImmutableList.of(new Route(POST, "/rollback"), new Route(POST, "/rollback/version/{versionID}"))


By passing versionID in does this effectively support moving forward as well?

Does it make sense to introduce a dryRun url param that would return the patch applied as its output without actually applying the patch?

It's essentially rollback, providing versionID allows users to rollback to any particular version.
Yes, we are separately working on a dryRun API for previewing the patch applied and it's impact before actually applying it.

DarshitChanpura

Thank you for adding these APIs. I have added few comments from my initial review.

One outstanding question I have is, do admin's have an option to choose a particular config to rollback when rolling back to previous version? I'm not exactly aware of a scenario, but hypothetically an admin may have removed a user and also updated a role simultaneously, this would bring us to the scenario I mentioned.

DarshitChanpura · 2025-06-23T17:23:16Z

src/main/java/org/opensearch/security/dlic/rest/api/Endpoint.java

-    SSL;
+    SSL,
+    VIEW_VERSION,
+    ROLLBACK_VERSION;


These two should be grouped as version? A singular permission that allows rest-admin privileges to the view and rollback. Similar to other endpoints already present here.

View API is just GET request, while rollback API is a POST request, hence kept them separate. But will look into this.

Just version may not sound similar to for ex: say roles because GET /role/{}, PUT /role/{} is intuitive to understand but GET /version/{}, PUT /version/{} is not that intuitive. PUT/POST /version/{} is not updating its entity, here it is doing rolling back which is not intuitive to understand

DarshitChanpura · 2025-06-23T17:23:40Z

src/main/java/org/opensearch/security/dlic/rest/api/RestApiAdminPrivilegesEvaluator.java

@@ -71,6 +71,8 @@ default String build() {
        .put(Endpoint.ROLESMAPPING, action -> buildEndpointPermission(Endpoint.ROLESMAPPING))
        .put(Endpoint.TENANTS, action -> buildEndpointPermission(Endpoint.TENANTS))
        .put(Endpoint.SSL, action -> buildEndpointActionPermission(Endpoint.SSL, action))
+        .put(Endpoint.VIEW_VERSION, action -> buildEndpointPermission(Endpoint.VIEW_VERSION))
+        .put(Endpoint.ROLLBACK_VERSION, action -> buildEndpointPermission(Endpoint.ROLLBACK_VERSION))


similar to the comment above, this should be clubbed as one permission.

DarshitChanpura · 2025-06-23T17:30:03Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+    private static final Logger LOGGER = LogManager.getLogger(ViewVersionApiAction.class);
+
+    private static final List<Route> routes = addRoutesPrefix(
+        ImmutableList.of(new Route(Method.GET, "/version"), new Route(Method.GET, "/version/{versionID}"))


get alll versions endpoint should be changed to /versions

DarshitChanpura · 2025-06-23T17:30:47Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+    private ValidationResult<SecurityConfiguration> viewAllVersions() throws IOException {
+        SecurityConfigVersionDocument doc = versionsLoader.loadFullDocument();
+
+        return ValidationResult.error(OK, (builder, params) -> {


+1. Should be ValidationResult.success here.

DarshitChanpura · 2025-06-23T17:34:20Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+    private static final Logger log = LogManager.getLogger(RollbackVersionApiAction.class);
+
+    private static final List<Route> routes = addRoutesPrefix(
+        ImmutableList.of(new Route(POST, "/rollback"), new Route(POST, "/rollback/version/{versionID}"))


should be renamed to /version/rollback and /version/rollback/{versionId}

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

DarshitChanpura · 2025-06-23T18:14:41Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+
+        if (maybeVer.isEmpty()) {
+            return ValidationResult.error(NOT_FOUND, payload(NOT_FOUND, "Version " + versionId + " not found"));
+        }


this method is pretty much identical with the one above, see if you can club it together to re-use code.

DarshitChanpura · 2025-06-23T18:20:16Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+
+        return ValidationResult.error(OK, (builder, params) -> {
+            XContentBuilder inner = buildVersionsJsonBuilder(List.of(maybeVer.get()));
+            builder.copyCurrentStructure(JsonXContent.jsonXContent.createParser(null, null, BytesReference.bytes(inner).streamInput()));


is see this pattern being repeated, 2 things:

Can you use fromXContent and toXContent instead of buildVersionsJsonBuilder and copyCurrentStructure.

See if you can over-load implementation of payload method in Responses.java that accepts JsonContent.

Ack will check this out.

src/test/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiActionValidationTest.java

src/test/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiTest.java

Signed-off-by: Divyanshi Chouksey <divvv@amazon.com>

divyanshi-0402 · 2025-06-25T08:26:31Z

curl -XPOST "https://172.19.119.134:9200/_plugins/_security/api/rollback" -u 'admin:admin' --insecure
{
  "status" : "OK",
  "message" : "config rolled back to version v1"
}%                   

[node1] Detected changes in security configuration: [{"op":"remove","path":"/internalusers/testuser"}]
[node1] Successfully saved version v3 to .opensearch_security_config_versions

In the context of the changes I understand why this operation will create a v3, but I do think its a bit confusing that a rollback creates a new "version".

We are tracking all the changes in the version index, the rollback triggers an update to the security configurations hence creating a new version to track it.

divyanshi-0402 · 2025-06-25T08:27:32Z

Is @lastUpdated specific to the individual config type or does it apply to the entire version?

It's specific to individual config type

divyanshi-0402 · 2025-06-25T08:30:16Z

Thank you for adding these APIs. I have added few comments from my initial review.

One outstanding question I have is, do admin's have an option to choose a particular config to rollback when rolling back to previous version? I'm not exactly aware of a scenario, but hypothetically an admin may have removed a user and also updated a role simultaneously, this would bring us to the scenario I mentioned.

Thanks for the review!
Right now users can just rollback to a provided version for security configuration. So as of now we don't have option to choose a particular config.

Signed-off-by: Divyanshi Chouksey <77962346+divyanshi-0402@users.noreply.github.com>

nagarajg17 · 2025-06-25T18:40:04Z

src/main/java/org/opensearch/security/dlic/rest/api/Endpoint.java

-    SSL;
+    SSL,
+    VIEW_VERSION,
+    ROLLBACK_VERSION;


Just version may not sound similar to for ex: say roles because GET /role/{}, PUT /role/{} is intuitive to understand but GET /version/{}, PUT /version/{} is not that intuitive. PUT/POST /version/{} is not updating its entity, here it is doing rolling back which is not intuitive to understand

nagarajg17 · 2025-06-25T18:42:32Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+    private ValidationResult<SecurityConfiguration> viewAllVersions() throws IOException {
+        SecurityConfigVersionDocument doc = versionsLoader.loadFullDocument();
+
+        return ValidationResult.error(OK, (builder, params) -> {


We cannot use ValidationResult.error, it is very misleading and wrong pattern

nagarajg17 · 2025-06-25T18:44:04Z

src/main/java/org/opensearch/security/dlic/rest/api/ViewVersionApiAction.java

+    private ValidationResult<SecurityConfiguration> viewSpecificVersion(String versionId) throws IOException {
+        SecurityConfigVersionDocument doc = versionsLoader.loadFullDocument();
+
+        var maybeVer = doc.getVersions().stream().filter(v -> versionId.equals(v.getVersion_id())).findFirst();


nit: what does maybeVer mean?

nagarajg17 · 2025-06-25T18:48:45Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+        var maybeVer = doc.getVersions().stream().filter(v -> versionId.equals(v.getVersion_id())).findFirst();
+
+        if (maybeVer.isEmpty()) {
+            return ValidationResult.error(NOT_FOUND, payload(NOT_FOUND, "Version " + versionId + " not found"));
+        }


This might make more sense to keep in rollbackCommon() since you would need to check this anyway for any caller of this function

nagarajg17 · 2025-06-25T18:58:24Z

src/main/java/org/opensearch/security/dlic/rest/api/RollbackVersionApiAction.java

+                sdc.setPrimaryTerm(currentConfig.getPrimaryTerm());
+
+                for (Map.Entry<String, ?> configEntry : sc.getConfigData().entrySet()) {
+                    if ("_meta".equals(configEntry.getKey())) {


Including _meta creates a duplicate entry for the field

Why? which other code is adding _meta field?

Added View API and Rollback API for Security Config Versions

8fa6094

Signed-off-by: Divyanshi Chouksey <divvv@amazon.com>

divyanshi-0402 requested review from cwperks, DarshitChanpura, derek-ho, nibix, peternied, RyanL1997, reta, shikharj05 and willyborankin as code owners June 23, 2025 09:16

Merge branch 'main' into restAPIs

7b3c979

nagarajg17 reviewed Jun 23, 2025

View reviewed changes

cwperks reviewed Jun 23, 2025

View reviewed changes

DarshitChanpura reviewed Jun 23, 2025

View reviewed changes

divyanshi-0402 and others added 2 commits June 24, 2025 11:42

Merge branch 'main' into restAPIs

6fac88a

Addresses PR comments

a802c33

Signed-off-by: Divyanshi Chouksey <divvv@amazon.com>

Merge branch 'main' into restAPIs

9a3dc90

Signed-off-by: Divyanshi Chouksey <77962346+divyanshi-0402@users.noreply.github.com>

divyanshi-0402 mentioned this pull request Jun 25, 2025

Added documentation for Experimental Security Configuration Versioning Management and APIs opensearch-project/documentation-website#10143

Open

1 task

nagarajg17 reviewed Jun 25, 2025

View reviewed changes

nagarajg17 mentioned this pull request Jul 7, 2025

Simulation API pranathz/security#2

Open

5 tasks

Added View API and Rollback API for Versioned Security Configurations #5431

Are you sure you want to change the base?

Added View API and Rollback API for Versioned Security Configurations #5431

Uh oh!

Conversation

divyanshi-0402 commented Jun 23, 2025

Description

API Design

View Security Configuration Versions

Rollback to desired Security Configuration Versions

Test details for multi-node testing:

Issues Resolved

Testing

Check List

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!

DarshitChanpura Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

Choose a reason for hiding this comment

DarshitChanpura Jun 25, 2025 •

edited

Loading