-
Notifications
You must be signed in to change notification settings - Fork 2.4k
[Rule-based Autotagging] add security attributes to rules #19232
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.
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
base: main
Are you sure you want to change the base?
Conversation
7d53efb
to
07a5cd4
Compare
❌ Gradle check result for 07a5cd4: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Ruirui Zhang <[email protected]>
07a5cd4
to
eb67fce
Compare
❌ Gradle check result for eb67fce: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
/** | ||
* Key representing the username subfield. | ||
*/ | ||
public static final String USERNAME = "username"; |
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.
Can we make this more generic so that there's no reference to username or role?
I'm not sure how WLM works, but I know that a request will match to the best workload group. How does the scoring work for matching? Can we assign weights to different attributes to help with the matching problem to determine the best group?
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.
I believed we need to reference to username or role because they are auto tagging's attributes. I just made them Attribute instead of a String to make it clearer
@peternied we've been discussion an extensible <-> extending plugin relationship here, especially since you can now define optional relationships with extending -> extensible plugin. In this case, WLM needs attributes from security, but it differs from other plugins because its in the core. There's certainly prevalent usage of common-utils today but we should not introduce a dependency on common-utils in the core repo. (In general, we need to work towards removal of common-utils). IdentityPlugin is one avenue, but that extension point is quite simple and only used for username and has no notion of roles or other attributes. We could certainly entertain expanding on the notion of subject to add roles, but I know there were differing opinions on that and as a result the IdentityPlugin can only be used to get username. In this case, by making WLM extensible it can ask other plugins to define an attribute extractor to feed it back attributes (and weights to break ties) given a REST Request. For security that could be username + roles and it would give higher precedence to direct username matching to workload groups. |
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.
I have the following initial set of comments which might change rest of the PR changes hence not reviewing it further.
...commons/common/src/main/java/org/opensearch/rule/attribute_extractor/AttributeExtractor.java
Outdated
Show resolved
Hide resolved
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/autotagging/Attribute.java
Outdated
Show resolved
Hide resolved
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/autotagging/Attribute.java
Show resolved
Hide resolved
...es/autotagging-commons/common/src/main/java/org/opensearch/rule/autotagging/FeatureType.java
Outdated
Show resolved
Hide resolved
...ing-commons/common/src/main/java/org/opensearch/rule/storage/DefaultAttributeValueStore.java
Outdated
Show resolved
Hide resolved
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/SecurityAttribute.java
Show resolved
Hide resolved
❌ Gradle check result for 756409d: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
756409d
to
209f55d
Compare
❌ Gradle check result for 209f55d: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
209f55d
to
8b097bb
Compare
❌ Gradle check result for 8b097bb: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/autotagging/Attribute.java
Outdated
Show resolved
Hide resolved
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/SecurityAttribute.java
Outdated
Show resolved
Hide resolved
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/SecurityAttribute.java
Outdated
Show resolved
Hide resolved
modules/autotagging-commons/common/src/main/java/org/opensearch/rule/SecurityAttribute.java
Outdated
Show resolved
Hide resolved
8b097bb
to
8f1a17c
Compare
❌ Gradle check result for 8f1a17c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
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.
I had two minor comments, just address them and send it to approval with the maintainers
...es/autotagging-commons/common/src/main/java/org/opensearch/rule/autotagging/FeatureType.java
Outdated
Show resolved
Hide resolved
return Optional.of(possibleMatch.getValue()); | ||
} | ||
} | ||
return trie.keySet() |
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.
I think rather than iterating over all the keys since we are simply interested in finding the valid prefixes of the key then iterating over the prefixes would be much faster. Do look into get related methods in patricia trie to make it more efficient
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.
If we iterate thru prefix then it's be back to the previous implementation
List<String> results = new ArrayList<>();
for (int i = key.length(); i >= 0; i--) {
String prefix = key.substring(0, i);
String value = trie.get(prefix);
if (value != null && !value.isEmpty()) {
results.add(value);
}
}
I looked into the patricia trie implementation, we can't use prefixMap method (it finds all keys in the trie that has the prefix, not the other way around) and don't have the access to the trie root (so we can't just walk the trie from top to bottom)
8f1a17c
to
5762c3e
Compare
❌ Gradle check result for 5762c3e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
5762c3e
to
9ef3651
Compare
❌ Gradle check result for 9ef3651: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
9ef3651
to
4c9ccd7
Compare
4c9ccd7
to
de0e826
Compare
de0e826
to
3d5e169
Compare
❌ Gradle check result for 3d5e169: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
3d5e169
to
0d53b23
Compare
0d53b23
to
422aeab
Compare
Signed-off-by: Ruirui Zhang <[email protected]>
422aeab
to
aad51ba
Compare
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.
This is a large PR. Will be great if we can break this down into smaller more reviewable PRs?
❌ Gradle check result for aad51ba: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Description
This PR enhances rule-based auto-tagging functionality by introducing security-related attributes. Specifically, we aim to extract user information (username and role) from the security context of incoming requests. In many real-world environments, security context is essential for customers with multiple teams, departments, or security boundaries. By enabling the use of security attributes in auto-tagging rules, this project will provide more practical and convenient ways to define the tenants.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
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.