Skip to content

Remove unused bwc variable from ApiKeyService #81964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 10, 2022
Merged

Remove unused bwc variable from ApiKeyService #81964

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd4e1d2
Remove unused bwc variable from ApiKeyService
ywangd Dec 20, 2021
d3e93f3
Merge remote-tracking branch 'origin/master' into apikeyservice-remov…
ywangd Dec 23, 2021
a1cf665
revert format change since it is not a net gain
ywangd Dec 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 36 additions & 9 deletions x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/Subject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,15 +176,42 @@ private Map<String, Object> getRoleDescriptorMap(String key) {
// This following fixed role descriptor is for fleet-server BWC on and before 7.14.
// It is fixed and must NOT be updated when the fleet-server service account updates.
// Package private for testing
static final BytesArray FLEET_SERVER_ROLE_DESCRIPTOR_BYTES_V_7_14 = new BytesArray(
"{\"elastic/fleet-server\":{\"cluster\":[\"monitor\",\"manage_own_api_key\"],"
+ "\"indices\":[{\"names\":[\"logs-*\",\"metrics-*\",\"traces-*\",\"synthetics-*\","
+ "\".logs-endpoint.diagnostic.collection-*\"],"
+ "\"privileges\":[\"write\",\"create_index\",\"auto_configure\"],\"allow_restricted_indices\":false},"
+ "{\"names\":[\".fleet-*\"],\"privileges\":[\"read\",\"write\",\"monitor\",\"create_index\",\"auto_configure\"],"
+ "\"allow_restricted_indices\":false}],\"applications\":[],\"run_as\":[],\"metadata\":{},"
+ "\"transient_metadata\":{\"enabled\":true}}}"
);
static final BytesArray FLEET_SERVER_ROLE_DESCRIPTOR_BYTES_V_7_14 = new BytesArray("""
{
"elastic/fleet-server": {
"cluster": [ "monitor", "manage_own_api_key" ],
"indices": [
{
"names": [
"logs-*",
"metrics-*",
"traces-*",
"synthetics-*",
".logs-endpoint.diagnostic.collection-*"
],
"privileges": [ "write", "create_index", "auto_configure" ],
"allow_restricted_indices": false
},
{
"names": [ ".fleet-*" ],
"privileges": [
"read",
"write",
"monitor",
"create_index",
"auto_configure"
],
"allow_restricted_indices": false
}
],
"applications": [],
"run_as": [],
"metadata": {},
"transient_metadata": {
"enabled": true
}
}
}""");
Copy link
Contributor

@tvernum tvernum Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want this formatting?
We push those bytes around the cluster and store them in ThreadContext, what's the impact of a 25% increase in the size of the string?

Copy link
Member Author

@ywangd ywangd Dec 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I just thought it looks nicer in this format and didn't think of other implications. We don't store it in threadContext or sent it across nodes. But we do parse RoleDescriptor from it and more importantly computing message digest for it. So I think it's better not to have the format.

One alternative to reverting the change is to add a call of .replaceAll("[\\n ]", "") at the end of the text block. So it can leverage the better looking format without paying most of the runtime cost. This approach is not very future proof. But since the value is meant to be immutable for this variable. It is not really a concern.

I am happy to just simply revert given the main purpose of this PR is to getting rid of unused variable. What do you think?

Copy link
Member Author

@ywangd ywangd Dec 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to revert the format change.


private BytesReference getLimitedByRoleDescriptorsBytes() {
final BytesReference bytesReference = (BytesReference) metadata.get(API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY);
Expand Down
39 changes: 0 additions & 39 deletions ...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,45 +200,6 @@ public class ApiKeyService {
Property.NodeScope
);

// This following fixed role descriptor is for fleet-server BWC on and before 7.14.
// It is fixed and must NOT be updated when the fleet-server service account updates.
private static final BytesArray FLEET_SERVER_ROLE_DESCRIPTOR_BYTES_V_7_14 = new BytesArray("""
{
"elastic/fleet-server": {
"cluster": [ "monitor", "manage_own_api_key" ],
"indices": [
{
"names": [
"logs-*",
"metrics-*",
"traces-*",
"synthetics-*",
".logs-endpoint.diagnostic.collection-*"
],
"privileges": [ "write", "create_index", "auto_configure" ],
"allow_restricted_indices": false
},
{
"names": [ ".fleet-*" ],
"privileges": [
"read",
"write",
"monitor",
"create_index",
"auto_configure"
],
"allow_restricted_indices": false
}
],
"applications": [],
"run_as": [],
"metadata": {},
"transient_metadata": {
"enabled": true
}
}
}""");

private final Clock clock;
private final Client client;
private final SecurityIndexManager securityIndex;
Expand Down