Skip to content

fixed plugins unknown setting and disabled_rest_categories#6021

Merged
cwperks merged 3 commits intoopensearch-project:mainfrom
ThyTran1402:fix/audit_log_setting
Mar 23, 2026
Merged

fixed plugins unknown setting and disabled_rest_categories#6021
cwperks merged 3 commits intoopensearch-project:mainfrom
ThyTran1402:fix/audit_log_setting

Conversation

@ThyTran1402
Copy link
Contributor

@ThyTran1402 ThyTran1402 commented Mar 18, 2026

Description

  • Fixed: Audit log NONE sentinel value not respected in dynamic configuration, and misleading "unknown setting" error for ignore_users
  • New behavior:
    AuditCategory.parse() now treats a single "NONE" value (case-insensitive) as an empty set, consistent with the existing behavior in the static opensearch.yml path. This fixes disabled_rest_categories and disabled_transport_categories for both config paths.
    Filter.from(Map) (the dynamic/REST path) now applies the same "NONE" → empty set conversion for ignore_users that fromSettingStringSet() already applied for the static path.
    The correct key for opensearch.yml is plugins.security.audit.config.ignore_users (note the .config. segment); using the wrong key plugins.security.audit.ignore_users is a documentation/usage issue — no code change required

Issues Resolved

Testing

  • Added testNoneViaMap() unit test to AuditConfigFilterTest covering the previously untested Filter.from(Map) code path, verifying that "NONE", "None", and "none" all correctly clear disabled categories and ignored users when set via the REST API.
  • Existing testNone() test continues to cover the static opensearch.yml path.

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: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
@cwperks
Copy link
Member

cwperks commented Mar 23, 2026

TY for this PR @ThyTran1402 can you please fix the CHANGELOG conflict error?

cwperks
cwperks previously approved these changes Mar 23, 2026
@cwperks
Copy link
Member

cwperks commented Mar 23, 2026

Otherwise this PR LGTM! 🚢

Signed-off-by: Thy Tran <58045538+ThyTran1402@users.noreply.github.com>
@ThyTran1402
Copy link
Contributor Author

TY for this PR @ThyTran1402 can you please fix the CHANGELOG conflict error?

Yup. I already fixed the changelog merge conflict 😃. Thank you for reviewing the PR.

@codecov
Copy link

codecov bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.93%. Comparing base (2b86187) to head (7f1048c).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6021      +/-   ##
==========================================
+ Coverage   73.88%   73.93%   +0.04%     
==========================================
  Files         440      440              
  Lines       27229    27265      +36     
  Branches     4044     4055      +11     
==========================================
+ Hits        20119    20159      +40     
+ Misses       5199     5193       -6     
- Partials     1911     1913       +2     
Files with missing lines Coverage Δ
...ensearch/security/auditlog/config/AuditConfig.java 97.16% <100.00%> (+0.04%) ⬆️
...ensearch/security/auditlog/impl/AuditCategory.java 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cwperks cwperks merged commit def20e4 into opensearch-project:main Mar 23, 2026
66 checks passed
Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

thank you for this contribution @ThyTran1402

getOrDefault(properties, FilterEntries.IGNORE_USERS.getKey(), DEFAULT_IGNORED_USERS)
);
final List<String> rawIgnoredUsers = getOrDefault(properties, FilterEntries.IGNORE_USERS.getKey(), DEFAULT_IGNORED_USERS);
final Set<String> ignoredAuditUsers = rawIgnoredUsers.size() == 1 && "NONE".equalsIgnoreCase(rawIgnoredUsers.get(0))
Copy link
Member

Choose a reason for hiding this comment

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

why not filter out NONE from rawIngoredUsers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm I think NONE is the sentinel config value, not actual username. It means don't ignore any users. So, rawIngoredUsers holds exactly what's in the config, keeping the input separate from the output logic. If we filtered it, we could lose the information that the user explicitly configured rather than just providing an empty list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, NONE is like special handling not regular filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants