Deprecate opendistro_security_roles and add opensearch_security_roles #5113
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| * | ||
| * Modifications Copyright OpenSearch Contributors. See | ||
| * GitHub history for details. | ||
| */ | ||
|
|
||
| package org.opensearch.security.api; | ||
|
|
||
| import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.DIRECT_SECURITY_ROLES; | ||
|
|
||
| public class InternalUsersRestApiDirectRolesIntegrationTest extends AbstractInternalUsersRestApiIntegrationTest { | ||
| @Override | ||
| protected String getRoleField() { | ||
| return DIRECT_SECURITY_ROLES; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| * | ||
| * Modifications Copyright OpenSearch Contributors. See | ||
| * GitHub history for details. | ||
| */ | ||
|
|
||
| package org.opensearch.security.api; | ||
|
|
||
| import static org.opensearch.security.dlic.rest.api.InternalUsersApiAction.OPENDISTRO_SECURITY_ROLES; | ||
|
|
||
| public class InternalUsersRestApiOpenDistroRolesIntegrationTest extends AbstractInternalUsersRestApiIntegrationTest { | ||
| @Override | ||
| protected String getRoleField() { | ||
| return OPENDISTRO_SECURITY_ROLES; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,13 +33,17 @@ | |
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnore; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| import org.opensearch.security.securityconf.Hashed; | ||
| import org.opensearch.security.securityconf.Hideable; | ||
| import org.opensearch.security.securityconf.StaticDefinable; | ||
|
|
||
| public class InternalUserV7 implements Hideable, Hashed, StaticDefinable { | ||
|
|
||
| private final Logger log = LogManager.getLogger(InternalUserV7.class); | ||
|
|
||
| private String hash; | ||
| private boolean reserved; | ||
| private boolean hidden; | ||
|
|
@@ -51,6 +55,7 @@ public class InternalUserV7 implements Hideable, Hashed, StaticDefinable { | |
| private Map<String, String> attributes = Collections.emptyMap(); | ||
| private String description; | ||
| private List<String> opendistro_security_roles = Collections.emptyList(); | ||
| private List<String> direct_security_roles = Collections.emptyList(); | ||
|
|
||
| private InternalUserV7(String hash, boolean reserved, boolean hidden, List<String> backend_roles, Map<String, String> attributes) { | ||
| super(); | ||
|
|
@@ -116,7 +121,18 @@ public List<String> getOpendistro_security_roles() { | |
| } | ||
|
|
||
| public void setOpendistro_security_roles(List<String> opendistro_security_roles) { | ||
| log.warn("Deprecated configuration opendistro_security_roles set. Kindly use direct_security_roles instead."); | ||
| this.opendistro_security_roles = opendistro_security_roles; | ||
| this.direct_security_roles = opendistro_security_roles; | ||
| } | ||
|
Comment on lines
123
to
+127
There was a problem hiding this comment. Sorry for the delay, I was a bit busy with other stuff. I have thought about this log message again. This method will be called by Jackson when it de-serializes the internal user JSON. It will be called for each user object. As in the security config index, all user records are stored as one big JSON in a single document, Jackson needs to de-serialize all user objects, even the security plugin needs only one (for example for a REST request). This will have the effect that whenever the user config is being loaded from the index, you'll get this log message repeated for each user stored in that index. On clusters with many users, these will be quite a lot repeated log messages at once. This would happen in these cases:
I think such repeated log messages won't be helpful to the user. I would guess that we need a different solution to notify the user about the deprecated attribute. There was a problem hiding this comment. No worries. Thanks for the detailed explanation. My understanding was that the log lines would be added only during initial read and during updates (limited to a specific user); And that once the configuration is in memory, it won't be fetched from the index unless a flush was called. I will remove the log line to avoid excessive logging.
Do you think documentation should be enough? something like- https://opensearch.org/docs/latest/breaking-changes/#deprecate-non-inclusive-terms There was a problem hiding this comment.
This is actually a question which I cannot answer just on my own. This really depends on the deprecation philosophy/strategy of the whole project. I am not sure if there is actually something like this formally defined. In my own humble opinion, a good solution would be like following (disclaimer: I really do not want to give a direction here, I am just sharing my private opinion): There are two main alternative options:
There was a problem hiding this comment.
nit: This might get problematic with number of clients? For e.g. client A using old attribute v/s client B using the new attribute. I am more aligned with option 1 - i.e. track with a major update if required. Alternatively, we could introduce new APIs to deprecate old functionality. i.e. new APIs return new attribute names, existing ones use old one. (would have been cleaner if this was done when APIs moved to _plugins/_security from _opendistro API paths) There was a problem hiding this comment.
Yes, good point. |
||
|
|
||
| public List<String> getDirect_security_roles() { | ||
| return direct_security_roles; | ||
| } | ||
|
|
||
| public void setDirect_security_roles(List<String> direct_security_roles) { | ||
|
There was a problem hiding this comment. How does the serialized JSON produced from this bean look like? I reckon, it will contain both attributes, even if one is unused, correct? There was a problem hiding this comment. correct, I have not removed/modified the old attribute to keep it backwards compatible. Do you think it will lead to confusion? There was a problem hiding this comment. I am generally wondering how clients consuming that API should handle this. This actually already applies to the security Dashboards plugin, which uses this API. Ideally, an API could just switch the the new attribute and use that. However, in this case, all API clients would also need to have a kind of detection mechanism to see which attribute is the one that is actually in use. This is especially critical for the PATCH API, which such clients cannot use anymore without making a GET in advance to find out what attribute would be in use. Generally, that seems to be not a good design of an API. There was a problem hiding this comment.
Since both the fields are maintained and updated on changes, clients using existing field will not break. As the existing field is being deprecated, guidance to use new field will be documented. As both the fields are available, I don't think a detection mechanism is required. I agree it would have been cleaner to switch to the new attribute, however I am not sure if we can do it without breaking existing clients. See example below- |
||
| this.opendistro_security_roles = direct_security_roles; | ||
| this.direct_security_roles = direct_security_roles; | ||
| } | ||
|
|
||
| public Map<String, String> getAttributes() { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indirectly related: What's the migration path towards having only the new attribute?
This will only log warnings when the REST API is used. No warnings will be logged for usage of the securityadmin tool or just the existing data in the config index.
Will there be an additional change in the future to address existing data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at other migrations/deprecation of terms, I think we can announce deprecation/removal in docs.
e.g. - https://opensearch.org/docs/latest/breaking-changes/#deprecate-non-inclusive-terms
Yes, I was thinking Security plugin should log a warning during initialization if configs have
opendistro_security_rolepresent. will this suffice?