feat: add opensearch-cedarling plugin - #13921
Merged
Merged
Conversation
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete OpenSearch Cedarling demo: documentation, Gradle build and wrapper, plugin implementation (search extension, response processor, settings/REST handler), test/benchmark suite with resources, and sample config/data for end-to-end demo and performance measurements. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mo-auto
added
area-documentation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 26
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/opensearch-cedarling/build.gradle`:
- Line 15: The build.gradle currently sets an invalid placeholder Maven group
("group = \"RenameGroup\"") which breaks publication coordinates for pluginZip;
replace that placeholder with a proper reverse-DNS Maven group (e.g.,
"com.yourcompany.project" or your organization’s groupId) by updating the group
property in build.gradle so pluginZip and any publications use valid
coordinates.
- Around line 1-8: Move the existing buildscript block that adds
org.opensearch.gradle:build-tools onto the script classpath so it appears before
the import org.opensearch.gradle.test.RestIntegTestTask and before the apply
plugin declarations (opensearch.opensearchplugin, opensearch.yaml-rest-test,
opensearch.pluginzip); this ensures the OpenSearch build-tools classes are on
the classpath during script compilation and prevents ClassNotFoundException for
RestIntegTestTask and the custom plugin ids.
- Line 102: The build uses a non-reproducible nightly dependency implementation
"io.jans:cedarling-java:0.0.0-nightly"; replace that artifact reference with a
pinned released version (for example "io.jans:cedarling-java:2.0.0") or add
dependency locking/constraints to force an approved version. Update the
implementation declaration in build.gradle (the line containing implementation
"io.jans:cedarling-java:0.0.0-nightly") to the chosen released version or
configure Gradle dependency constraints/lockfile to ensure a stable,
reproducible cedarling-java version.
In `@demos/opensearch-cedarling/gradlew.bat`:
- Around line 1-94: The gradlew.bat file uses LF-only endings which breaks
Windows label/goto parsing (see labels like :findJavaFromJavaHome, :execute,
:fail and the use of %~dp0); convert demos/opensearch-cedarling/gradlew.bat to
CRLF line endings and commit the updated file, and add a .gitattributes rule to
enforce CRLF for all batch files (e.g. add an entry for *.bat to force text
eol=crlf) so future commits preserve CRLF.
In `@demos/opensearch-cedarling/query_ext.json`:
- Around line 8-10: Remove the hardcoded JWT found in the JSON "tokens" object
under the "Jans::Userinfo_token" key and replace it with a non-secret
placeholder (e.g., "<REPLACE_WITH_JWT>") or load it at runtime from a secret
source; update any demo/test logic that reads query_ext.json to read the token
from an environment variable or secret manager instead of embedding it (check
code that references "tokens" or "Jans::Userinfo_token" to change lookup), and
ensure any real token used for testing is revoked/rotated and added to CI/dev
secrets rather than committed.
In `@demos/opensearch-cedarling/README.md`:
- Around line 190-192: Remove the user-facing TODO from the README by deleting
the "## TODO:" section that contains "Check linting and javadoc warnings", and
instead create a tracked issue (or add to the PR) to address linting and Javadoc
warnings; update the README.md to remove the unresolved instruction and, if
desired, add a short note linking to the issue number that will track the
remaining work.
- Around line 107-163: The README still links to external repo URLs for
settings.json, records.txt, query.json, pipeline.json and query_ext.json; update
the references in demos/opensearch-cedarling/README.md to point to the local
repo files (use relative paths like ./settings.json, ./records.txt,
./query.json, ./pipeline.json, ./query_ext.json) and ensure those files are
present in the demo directory so links resolve after migrating from
github.com/jgomer2001/pipelines-plugin; look for the sections that currently
reference each external URL (the occurrences around the example curl commands
and the "Here" links) and replace them with the corresponding relative paths.
In `@demos/opensearch-cedarling/settings.gradle`:
- Line 10: The rootProject.name is still set to the template value
"plugin-template"; change the project name to "opensearch-cedarling" by updating
the rootProject.name assignment (symbol: rootProject.name) so build artifacts
and documentation match the actual plugin name
("opensearch-cedarling"/cedarling.zip). Ensure the updated name is the only
change in settings.gradle and verify packaging produces the expected artifact
name.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchExtBuilder.java`:
- Around line 61-68: Replace the unsafe direct cast in
CedarlingSearchResponseProcessor: instead of using
CedarlingSearchExtBuilder.class.cast(exts.get(0)) which assumes the Cedarling
extension is first and can throw ClassCastException, call the helper
CedarlingSearchExtBuilder.fromExtBuilderList(exts) to locate and return the
CedarlingSearchExtBuilder instance (or null) from the exts List; update the
variable assignment to use that method and handle the null case accordingly.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 91-95: The code mutates the ES hit's `_source` by calling
appendExtraAttributes(map, ...) on the map returned from hit.getSourceAsMap();
instead create a new copy (e.g., new HashMap<>(hit.getSourceAsMap())) and pass
that copy to appendExtraAttributes and any policy-evaluation logic so
authorization metadata is not written back into the original hit or able to
overwrite real document fields. Update both call sites of appendExtraAttributes
in CedarlingSearchResponseProcessor (the local variable `map` usage after
hit.getSourceAsMap() and the later call around the 145-155 region) to operate on
a copied Map for evaluation and keep the original hit source unchanged when
constructing the response.
- Around line 93-105: The catch block for cedarlingService.authorize currently
adds the hit to authorized on exception (in CedarlingSearchResponseProcessor
around appendExtraAttributes / cedarlingService.authorize), which is unsafe;
change the error handling to fail-closed by removing the authorized.add(hit)
from the catch, log the error with context (use logger.error with a descriptive
message and exception), and optionally record a metric or counter for
authorization failures (e.g., increment a failure metric or note in
decisionsTook) so the hit is omitted when authorize throws instead of being
included.
- Around line 108-124: The code uses searchHits.getTotalHits() when constructing
the new SearchHits, leaking the original (pre-filter) total; replace that with a
TotalHits representing the filtered count by creating a new
org.apache.lucene.search.TotalHits using authorized.size() and
TotalHits.Relation.EQUAL_TO and pass that into the SearchHits constructor
(update the import to org.apache.lucene.search.TotalHits); keep the rest of the
SearchHits construction and subsequent SearchResponseSections (sections -> new
SearchResponseSections(...)) unchanged so pagination and displayed totals
reflect authorized.size() instead of searchHits.getTotalHits().
- Around line 62-74: request.source() can be null and casting exts.get(0)
directly is unsafe; replace the current logic that reads request.source().ext()
and casts exts.get(0) with a null check for request.source() and use the helper
CedarlingSearchExtBuilder.fromExtBuilderList(exts) to defensively locate the
Cedarling extension (handle the empty/absent result and return the original
response), and remove the direct CedarlingSearchExtBuilder.class.cast(...) usage
so unrelated ext lists won't cause ClassCastException.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingService.java`:
- Around line 14-17: The init() method currently flips the shared state unsafely
causing races; make CedarlingAdapter and started visible and updatable
atomically by declaring the adapter field volatile (or using
AtomicReference<CedarlingAdapter>) and avoid setting started=false before
reload; instead construct and validate a new CedarlingAdapter instance locally,
and only after successful validation atomically replace the published adapter
and set started=true. Also ensure authorize() and
CedarlingSearchResponseProcessor use the volatile adapter (or
AtomicReference.get()) and add a null/started check to reject requests when no
valid adapter is published (throw an appropriate exception or return an error),
so failed init does not leave the service returning unfiltered hits.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.java`:
- Around line 27-45: PluginSettings.from() currently treats missing string
fields incorrectly (optString() returns "" not null) and returns null for
missing bootstrapProperties which leads to NPEs downstream in
SettingsService.reloadPluginSettings(); update PluginSettings.from() to validate
required fields explicitly: check ps.bootstrapProperties != null, and use
job.optString("searchActionName").isEmpty() and
job.optString("schemaPrefix").isEmpty() to detect missing strings, and throw a
descriptive IllegalArgumentException (or a custom ValidationException) listing
which required field is missing instead of returning null; alternatively change
the factory signature to return Optional<PluginSettings> and make
SettingsService.reloadPluginSettings() handle the empty Optional with a clear
error message rather than allowing an NPE when dereferencing the result.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.java`:
- Around line 54-99: handlePut currently persists the raw request body into
CedarlingPlugin.SETTINGS_KEY without validation or size checks; update handlePut
to (1) enforce a reasonable max payload size (e.g., reject > N bytes with
BytesRestResponse(RestStatus.PAYLOAD_TOO_LARGE)) before parsing, (2) parse the
payload into a JSONObject and validate required fields bootstrapProperties,
searchActionName, and schemaPrefix exist and have expected types, returning a
4xx (BAD_REQUEST) with a clear message if validation fails, and only then create
the ClusterUpdateSettingsRequest (cusr) and call
CedarlingPlugin.getClusterAdminClient().updateSettings(...); this prevents
storing malformed/oversized payloads that later fail in
SettingsService.reloadPluginSettings and gives immediate client feedback.
- Around line 34-36: The routes() method in SettingsRestHandler currently
returns a raw List; change its signature to return a parameterized List to be
type-safe (e.g., change public List routes() to public List<RestHandler.Route>
routes()) and keep the body constructing List.of(new RestHandler.Route(GET,
PATH), new RestHandler.Route(PUT, PATH)) unchanged so callers and the compiler
see the proper generic type for RestHandler.Route.
- Around line 122-125: In SettingsRestHandler remove the commented-out
alternative error response inside the catch(Exception e) block—delete the line
"//channel.sendResponse(new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR,
"Error processing audit request: " + e.getMessage()));" so only the active error
handling remains (the channel.sendResponse(new BytesRestResponse(channel, e))
call) and no dead/commented code is left in that catch block.
- Around line 59-65: The Content-Type check in SettingsRestHandler uses
request.getAllHeaderValues("Content-Type") and checks
List.contains("application/json"), which fails for values like
"application/json; charset=utf-8"; change the hasJsonHeader logic to iterate the
header values and normalize each value (trim, toLowerCase) then split on ';' or
use startsWith("application/json") on the normalized string to detect the base
media type; update the Optional.ofNullable(...).map(...) lambda that computes
hasJsonHeader to return true when any header value matches the normalized base
media type so requests with parameters (e.g., charset) are accepted.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/SettingsService.java`:
- Around line 23-40: The pluginSettings field is accessed concurrently in
getSettings() without safe publication; mark the pluginSettings field as
volatile and prevent concurrent reloads by guarding the
reloadPluginSettings(AbstractScopedSettings) call with a lock (e.g., a private
final ReentrantLock or synchronized block) so readers either see a
fully-initialized PluginSettings or wait while reload completes; update
getSettings(), reloadPluginSettings(...) and any writers that assign
pluginSettings to use this lock (and ensure CedarlingService.started/useLogging
mutations are similarly synchronized if they share concurrency concerns).
- Around line 66-75: PluginSettings.from(job, lastUpdated) can return null and
is immediately dereferenced; change the code to first assign the result to a
temporary variable, check for null, and if null log an error and return (or skip
reloading) instead of assigning to the field, otherwise assign to the
pluginSettings field and call
CedarlingService.getInstance().init(pluginSettings.getBootstrapProperties(),
pluginSettings.isLogCedarlingLogs()); ensure getSettings() logic isn't left with
a null pluginSettings reference.
In
`@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/AlterSuiteListener.java`:
- Around line 20-41: The Properties reader created by
Files.newBufferedReader(propertiesFilePath, UTF_8) is never closed; wrap the
reader passed to prop.load(...) in a try-with-resources to ensure it is closed
(e.g., open a Reader in a try(...) block before calling prop.load), keep the
rest of the logic that populates parameters and reads the queryFile the same,
and change the catch block to log the thrown Throwable (pass the exception
object to logger.error) so the stack trace is preserved; update references
around XmlSuite suite, propertiesFilePath, prop.load, and
Files.newBufferedReader accordingly.
In
`@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java`:
- Around line 167-170: getADecimal's implementation uses ranma.nextInt(max - min
+ 1) + min which yields [min, max] while its comment says [min, max) and
benchmark expects 2024..2027; change the implementation to ranma.nextInt(max -
min) + min to produce a uniformly distributed value in [min, max) (or
alternatively update the comment and benchmark to include the inclusive max),
and ensure any call sites (e.g., the call using (2024, 2028)) are adjusted to
match the chosen convention so doc, tests, and code agree.
- Around line 36-37: The Authorization header is using URL-safe Base64
(Base64.getUrlEncoder()) which is incorrect for HTTP Basic auth; replace
Base64.getUrlEncoder() with Base64.getEncoder() when encoding (the bytes
variable creation) so the "Basic " + new String(bytes, UTF_8) produces standard
Base64 per RFC 7617; locate this in BenchmarkTest.java around the bytes
assignment/NetworkUtil construction and update the encoder call accordingly.
- Around line 64-71: The payload is built with repeated String concatenation in
BenchmarkTest (variable payload inside the loop using
bulkEntryTemplate/getAString/getADecimal/ranma/MAX_GPA), which is O(n²) and
causes excessive allocation; replace it with a StringBuilder (append each
formatted entry) and after the loop call toString() to get the final payload,
and compute byte length using an explicit charset (e.g., StandardCharsets.UTF_8)
instead of platform default when calling getBytes for the logger.info call.
In `@demos/opensearch-cedarling/src/test/resources/testng.properties`:
- Around line 4-6: Remove the hardcoded credentials in testng.properties (the
"user" and "password" entries) and replace them with placeholders that instruct
tests to read from environment variables or CI secret store (e.g.,
user=${TEST_USER} and password=${TEST_PASSWORD}). Update the test harness/setup
to obtain credentials from process environment or system properties (e.g.,
System.getenv("TEST_USER") / System.getenv("TEST_PASSWORD") or corresponding
test framework config) and fail fast with a clear error if those vars are
missing; ensure CI pipeline injects the secrets into TEST_USER and TEST_PASSWORD
instead of committing real credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05cfffc1-8244-425d-9e6a-540b7722dcab
📒 Files selected for processing (27)
demos/opensearch-cedarling/README.mddemos/opensearch-cedarling/benchmark.mddemos/opensearch-cedarling/build.gradledemos/opensearch-cedarling/gradle.propertiesdemos/opensearch-cedarling/gradlewdemos/opensearch-cedarling/gradlew.batdemos/opensearch-cedarling/pipeline.jsondemos/opensearch-cedarling/query.jsondemos/opensearch-cedarling/query_ext.jsondemos/opensearch-cedarling/records.txtdemos/opensearch-cedarling/settings.gradledemos/opensearch-cedarling/settings.jsondemos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingPlugin.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchExtBuilder.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponse.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingService.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/SettingsService.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.javademos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/AlterSuiteListener.javademos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.javademos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/NetworkUtil.javademos/opensearch-cedarling/src/test/resources/log4j2-test.xmldemos/opensearch-cedarling/src/test/resources/query.jsondemos/opensearch-cedarling/src/test/resources/suite.xmldemos/opensearch-cedarling/src/test/resources/testng.properties
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.java (1)
59-60:⚠️ Potential issue | 🟡 MinorParse the
Content-Typemedia type instead of exact-list matching.
List.contains("application/json")rejects valid requests such asContent-Type: application/json; charset=utf-8. Normalize each header value and compare only the base media type.Proposed fix
boolean hasJsonHeader = Optional.ofNullable(request.getAllHeaderValues("Content-Type")) - .map(l -> l.contains("application/json")).orElse(false); + .orElse(Collections.emptyList()).stream() + .filter(Objects::nonNull) + .map(v -> v.split(";", 2)[0].trim().toLowerCase(Locale.ROOT)) + .anyMatch("application/json"::equals);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.java` around lines 59 - 60, The current hasJsonHeader check uses list.contains("application/json") which fails for media types with parameters; update the logic in SettingsRestHandler where hasJsonHeader is computed (using request.getAllHeaderValues("Content-Type")) to iterate each header value, split on ';' or parse the media type, trim and lowercase the base type, and compare it to "application/json" (case-insensitive) so values like "application/json; charset=utf-8" are accepted.demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.java (1)
27-43:⚠️ Potential issue | 🟠 MajorFail fast instead of returning
nullfor invalid settings.
from()still returnsnullfor missing required fields, so callers must defensively handle a nullable factory result or risk NPEs during reload/processing. Prefer throwing a descriptiveIllegalArgumentExceptionfor each missing field, or change the API to returnOptional<PluginSettings>.Proposed fix
ps.bootstrapProperties = job.optJSONObject("bootstrapProperties"); if (ps.bootstrapProperties == null) { - logger.warn("Undefined 'bootstrapProperties'"); - return null; + throw new IllegalArgumentException("Undefined 'bootstrapProperties'"); } @@ ps.searchActionName = job.optString("searchActionName", null); if (ps.searchActionName == null) { - logger.warn("Undefined 'searchActionName'"); - return null; + throw new IllegalArgumentException("Undefined 'searchActionName'"); } @@ ps.schemaPrefix = job.optString("schemaPrefix", null); if (ps.schemaPrefix == null) { - logger.warn("Undefined 'schemaPrefix'"); - return null; + throw new IllegalArgumentException("Undefined 'schemaPrefix'"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.java` around lines 27 - 43, The factory method (PluginSettings.from) currently returns null when required keys are missing (ps.bootstrapProperties, ps.searchActionName, ps.schemaPrefix); update it to fail fast by throwing IllegalArgumentException with a clear message for each missing field instead of returning null so callers don't get nullable results—e.g., replace the logger.warn + return null branches for ps.bootstrapProperties, ps.searchActionName and ps.schemaPrefix with throws that include the missing field name and context (e.g., "Missing required plugin setting 'schemaPrefix' in PluginSettings.from").demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java (1)
80-86:⚠️ Potential issue | 🟠 MajorHandle a missing Cedarling
tbacextension after lookup.
fromExtBuilderList(exts)can returnnullwhen the request has other search extensions but not the Cedarling one;cseb.getParams()then throws and fails an otherwise valid search. Return the original response when the Cedarling extension is absent.Proposed fix
CedarlingSearchExtBuilder cseb = CedarlingSearchExtBuilder.fromExtBuilderList(exts); + if (cseb == null) { + logger.debug("No Cedarling 'tbac' ext in request"); + return response; + } + SearchHits searchHits = response.getHits();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java` around lines 80 - 86, CedarlingSearchExtBuilder.fromExtBuilderList(exts) can return null causing cseb.getParams() to NPE; update CedarlingSearchResponseProcessor so right after CedarlingSearchExtBuilder cseb = CedarlingSearchExtBuilder.fromExtBuilderList(exts); you check if (cseb == null) and immediately return the original response (leave it unmodified) instead of proceeding to call cseb.getParams(); this prevents a NullPointerException when the Cedarling extension is absent.demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java (1)
67-72:⚠️ Potential issue | 🟡 MinorMake bulk JSON generation locale- and charset-stable.
String.format(...)uses the JVM default locale, so float formatting can emit commas in some locales and produce invalid JSON. Also keep the byte count deterministic withUTF_8.Proposed fix
- sb.append(String.format(bulkEntryTemplate, getAString(), + sb.append(String.format(Locale.ROOT, bulkEntryTemplate, getAString(), getADecimal(2024, 2028), ranma.nextFloat() * MAX_GPA)); @@ - logger.info("Payload of {} {} documents generated ({} bytes)", entries, indexName, payload.getBytes().length); + logger.info("Payload of {} {} documents generated ({} bytes)", entries, indexName, payload.getBytes(UTF_8).length);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java` around lines 67 - 72, The bulk JSON generation uses String.format(...) which is locale-sensitive and payload.getBytes() which uses the platform default charset; change the String.format call that builds each bulk entry to String.format(Locale.ROOT, bulkEntryTemplate, getAString(), getADecimal(2024, 2028), ranma.nextFloat() * MAX_GPA) (import java.util.Locale) to force invariant formatting and then compute the byte count with payload.getBytes(StandardCharsets.UTF_8) (import java.nio.charset.StandardCharsets) when logging so both number formatting and byte length are deterministic across locales and platforms; update references in the loop that use bulkEntryTemplate, getAString(), getADecimal(), ranma, MAX_GPA and the logger.info call accordingly.demos/opensearch-cedarling/README.md (1)
107-107:⚠️ Potential issue | 🟡 MinorPoint
settings.jsonat the migrated local file.The README still sends users to the external demo repo for
settings.json, even though this PR migrates the demo into this repository. Use the local relative path so the docs remain self-contained.Proposed fix
-Check the file [settings.json](https://raw.githubusercontent.com/jgomer2001/pipelines-plugin/refs/heads/main/settings.json) and fill the value corresponding to the policy store URI. +Check the file [settings.json](settings.json) and fill the value corresponding to the policy store URI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/README.md` at line 107, The README currently links to an external settings.json URL; update the README.md instruction to point to the migrated local settings.json file (use the appropriate relative path to the local settings.json in this repo instead of the raw.githubusercontent.com URL) so docs are self-contained and users edit the in-repo policy store URI; modify the line referencing settings.json in the README to reference the local relative path (e.g., ./settings.json or the correct repo-relative path) and ensure the wording still instructs the user to fill the policy store URI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/opensearch-cedarling/settings.gradle`:
- Around line 1-8: The settings.gradle header references an outdated Gradle doc
URL/version
("https://docs.gradle.org/6.5.1/userguide/multi_project_builds.html"); update
that comment to point to the current Gradle documentation (e.g. use
"https://docs.gradle.org/current/userguide/multi_project_builds.html" or mention
Gradle 8.x) so the settings.gradle file's comment accurately links to up-to-date
multi-project build docs.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 96-100: The code constructs a new HashMap from
hit.getSourceAsMap() outside the try-catch which can throw a
NullPointerException when _source is disabled; move the call to
hit.getSourceAsMap() and the new HashMap<>(...) inside the try block in
CedarlingSearchResponseProcessor (where SearchHit hit is processed), check if
the returned source map is null and if so skip this hit (do not call
appendExtraAttributes), otherwise proceed to call appendExtraAttributes(map,
pluginSettings.getSchemaPrefix(), hit.getIndex(), hit.getId()); ensure any
exceptions remain caught by the existing try-catch.
In
`@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java`:
- Line 102: The benchmark's search requests in BenchmarkTest (calls to
nu.sendPost with indexName + "/_search?size=" + entries) can exceed OpenSearch's
index.max_result_window; update the test setup to set the index setting before
issuing searches (e.g., call the index settings API to set
index.max_result_window to the desired value such as 100000 for the index
referenced by indexName), or alternatively enforce a safe cap on entries before
constructing the query or implement pagination (search_after/scroll) when
entries > max_result_window; locate the setup/fixture code in BenchmarkTest and
add the index settings update (or the cap/pagination logic) to ensure queries do
not return HTTP 413.
---
Duplicate comments:
In `@demos/opensearch-cedarling/README.md`:
- Line 107: The README currently links to an external settings.json URL; update
the README.md instruction to point to the migrated local settings.json file (use
the appropriate relative path to the local settings.json in this repo instead of
the raw.githubusercontent.com URL) so docs are self-contained and users edit the
in-repo policy store URI; modify the line referencing settings.json in the
README to reference the local relative path (e.g., ./settings.json or the
correct repo-relative path) and ensure the wording still instructs the user to
fill the policy store URI.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 80-86: CedarlingSearchExtBuilder.fromExtBuilderList(exts) can
return null causing cseb.getParams() to NPE; update
CedarlingSearchResponseProcessor so right after CedarlingSearchExtBuilder cseb =
CedarlingSearchExtBuilder.fromExtBuilderList(exts); you check if (cseb == null)
and immediately return the original response (leave it unmodified) instead of
proceeding to call cseb.getParams(); this prevents a NullPointerException when
the Cedarling extension is absent.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.java`:
- Around line 27-43: The factory method (PluginSettings.from) currently returns
null when required keys are missing (ps.bootstrapProperties,
ps.searchActionName, ps.schemaPrefix); update it to fail fast by throwing
IllegalArgumentException with a clear message for each missing field instead of
returning null so callers don't get nullable results—e.g., replace the
logger.warn + return null branches for ps.bootstrapProperties,
ps.searchActionName and ps.schemaPrefix with throws that include the missing
field name and context (e.g., "Missing required plugin setting 'schemaPrefix' in
PluginSettings.from").
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.java`:
- Around line 59-60: The current hasJsonHeader check uses
list.contains("application/json") which fails for media types with parameters;
update the logic in SettingsRestHandler where hasJsonHeader is computed (using
request.getAllHeaderValues("Content-Type")) to iterate each header value, split
on ';' or parse the media type, trim and lowercase the base type, and compare it
to "application/json" (case-insensitive) so values like "application/json;
charset=utf-8" are accepted.
In
`@demos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java`:
- Around line 67-72: The bulk JSON generation uses String.format(...) which is
locale-sensitive and payload.getBytes() which uses the platform default charset;
change the String.format call that builds each bulk entry to
String.format(Locale.ROOT, bulkEntryTemplate, getAString(), getADecimal(2024,
2028), ranma.nextFloat() * MAX_GPA) (import java.util.Locale) to force invariant
formatting and then compute the byte count with
payload.getBytes(StandardCharsets.UTF_8) (import
java.nio.charset.StandardCharsets) when logging so both number formatting and
byte length are deterministic across locales and platforms; update references in
the loop that use bulkEntryTemplate, getAString(), getADecimal(), ranma, MAX_GPA
and the logger.info call accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8bac8ea0-1c47-43e5-8248-87bd403229d0
📒 Files selected for processing (7)
demos/opensearch-cedarling/README.mddemos/opensearch-cedarling/query_ext.jsondemos/opensearch-cedarling/settings.gradledemos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/PluginSettings.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.javademos/opensearch-cedarling/src/test/java/io/jans/cedarling/opensearch/BenchmarkTest.java
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java (1)
80-86:⚠️ Potential issue | 🟠 MajorGuard the missing Cedarling extension before dereferencing
cseb.
fromExtBuilderList(exts)can returnnullwhen the request hasextentries but no Cedarlingtbacentry. Line 86 then fails the whole search with an NPE instead of returning the original response.Proposed fix
CedarlingSearchExtBuilder cseb = CedarlingSearchExtBuilder.fromExtBuilderList(exts); + if (cseb == null) { + logger.debug("No Cedarling 'tbac' ext in request"); + return response; + } + SearchHits searchHits = response.getHits(); Iterator<SearchHit> it = searchHits.iterator();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java` around lines 80 - 86, CedarlingSearchExtBuilder.fromExtBuilderList(exts) can return null so guard the result before dereferencing: in CedarlingSearchResponseProcessor where you assign cseb from fromExtBuilderList(exts), check if cseb == null and if so skip the Cedarling-specific processing (do not call cseb.getParams() or use cseb) and return or pass through the original SearchResponse (or the unmodified SearchHits) instead; update the code path that builds the authorized list to only run when cseb is non-null to avoid the NPE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/opensearch-cedarling/benchmark.md`:
- Line 8: Update the phrase "server side processing" in the sentence "The
average query response time is computed. This does not include network latency -
only server side processing" to use the hyphenated compound adjective
"server-side processing" so it reads "...only server-side processing".
- Line 16: Replace the unstable slug link "https://slugs.do-api.dev/" in the
sentence mentioning Digital Ocean with a stable, canonical DigitalOcean product
page URL (e.g., https://www.digitalocean.com/products/droplets) so the sentence
reads using a durable reference to DigitalOcean's Droplets product instead of
the slugs.do-api.dev host.
- Around line 39-41: The doc instructs users to edit
`src/test/resources/testng.properties` from the repo root but that path is wrong
for this demo; update the benchmark.md instructions so they either: 1) tell the
user to cd into the demo directory (mentioning the demo by name) before editing
`testng.properties`, or 2) show the correct relative path to the
`testng.properties` file as used by this demo; also ensure the note about
enabling `useCedarling` and editing `query.json` references the correct
`query.json` location and the `useCedarling` property name so users are not
misled.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 94-105: The code currently accumulates decisionsTook only when
authorize succeeds and divides average_decision_time by the original hit count,
skewing latency; modify the loop around cedarlingService.authorize(...) so you
measure elapsed time in a finally block and increment a new evaluatedDecisions
counter only when an authorization call was actually attempted (i.e., for
non-null source maps) — update any final average_decision_time calculation to
divide by evaluatedDecisions instead of the original hit count; refer to the
variables decisionsTook, evaluatedDecisions (new),
cedarlingService.authorize(...), and the loop handling SearchHit to implement
this change (also replicate the same fix around lines 132-133).
- Around line 126-129: The response currently replaces only SearchHits but
reuses sections.aggregations() and sections.suggest(), exposing metrics from
unauthorized docs; in CedarlingSearchResponseProcessor where sections is rebuilt
into new SearchResponseSections(...) using mySearchHits and shardResults, detect
if the original sections contain aggregations or suggestions (e.g.,
sections.aggregations(), sections.suggest()) and either reject the request
(throw an appropriate exception) or strip those sections by passing null/empty
placeholders into the new SearchResponseSections call instead of the original
objects; ensure the decision is applied consistently wherever
SearchResponseSections is reconstructed so post-filtering cannot leak
aggregation/suggestion data.
---
Duplicate comments:
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 80-86: CedarlingSearchExtBuilder.fromExtBuilderList(exts) can
return null so guard the result before dereferencing: in
CedarlingSearchResponseProcessor where you assign cseb from
fromExtBuilderList(exts), check if cseb == null and if so skip the
Cedarling-specific processing (do not call cseb.getParams() or use cseb) and
return or pass through the original SearchResponse (or the unmodified
SearchHits) instead; update the code path that builds the authorized list to
only run when cseb is non-null to avoid the NPE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcebbed2-2313-48d5-81e2-26513e70230c
📒 Files selected for processing (3)
demos/opensearch-cedarling/benchmark.mddemos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.javademos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.java
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
demos/opensearch-cedarling/README.md (1)
107-107:⚠️ Potential issue | 🟡 MinorUse the local migrated
settings.json.This README still sends users to the old external demo repo for
settings.json; use the local file so the migrated demo remains self-contained.Suggested doc fix
-Check the file [settings.json](https://raw.githubusercontent.com/jgomer2001/pipelines-plugin/refs/heads/main/settings.json) and fill the value corresponding to the policy store URI. +Check the file [settings.json](settings.json) and fill the value corresponding to the policy store URI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/README.md` at line 107, Update the README.md instruction that links to the external settings.json to point to the local migrated settings.json in this repo (replace the external raw URL with a relative path like ./settings.json or docs/settings.json as appropriate); edit the sentence in demos/opensearch-cedarling/README.md that currently says "Check the file https://raw.githubusercontent.com/..." so it references the local settings.json file and instructs the user to fill the policy store URI there.demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java (2)
80-86:⚠️ Potential issue | 🟠 MajorHandle missing Cedarling
tbacextension before dereferencing.
fromExtBuilderList(exts)can returnnull; Line 86 then throws when the request has extensions but not the Cedarling extension. Return the original response whentbacis absent.Proposed fix
CedarlingSearchExtBuilder cseb = CedarlingSearchExtBuilder.fromExtBuilderList(exts); + if (cseb == null) { + logger.debug("No Cedarling 'tbac' ext in request"); + return response; + } SearchHits searchHits = response.getHits(); Iterator<SearchHit> it = searchHits.iterator();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java` around lines 80 - 86, CedarlingSearchExtBuilder.fromExtBuilderList(exts) can return null and cseb.getParams() (tbac) may be absent; before dereferencing cseb or tbac in processResponse (where CedarlingSearchExtBuilder cseb = CedarlingSearchExtBuilder.fromExtBuilderList(exts)), add a null-check: if cseb is null or cseb.getParams() (tbac) is null/empty, simply return the original response unchanged instead of continuing to build the authorized list; ensure you reference the existing variables/methods CedarlingSearchExtBuilder.fromExtBuilderList, cseb, and cseb.getParams() when adding the guard.
102-104:⚠️ Potential issue | 🟡 MinorCompute decision latency from actual authorization attempts.
decisionsTookis only updated whenauthorize(...)returns successfully, but the average is divided by all hits. This skewsaverage_decision_time; count attempted authorizations and record elapsed time infinally.Proposed fix
long decisionsTook = 0; + int evaluatedDecisions = 0; do { SearchHit hit = it.next(); Map<String, Object> map = Optional.ofNullable(hit.getSourceAsMap()) .map(HashMap::new).orElse(new HashMap<>()); try { appendExtraAttributes(map, pluginSettings.getSchemaPrefix(), hit.getIndex(), hit.getId()); long temp = System.nanoTime(); - boolean allowed = cedarlingService.authorize(tokens, action, map, context); - decisionsTook += (System.nanoTime() - temp); + boolean allowed; + try { + allowed = cedarlingService.authorize(tokens, action, map, context); + } finally { + decisionsTook += (System.nanoTime() - temp); + evaluatedDecisions++; + } if (allowed) { authorized.add(hit); } @@ authorizedHitsCount = authorized.size(); //compute average decision time per document in micro seconds - avgDecisionTime = Math.round(decisionsTook / (1000.0d * searchHits.getHits().length)); + avgDecisionTime = evaluatedDecisions == 0 + ? -1 + : Math.round(decisionsTook / (1000.0d * evaluatedDecisions));Also applies to: 130-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java` around lines 102 - 104, Wrap the cedarlingService.authorize(tokens, action, map, context) call in a try/finally: record start = System.nanoTime() immediately before calling authorize, then in the finally block always add (System.nanoTime() - start) to decisionsTook and increment the counter used to compute average_decision_time (the attempted-authorization count variable), so elapsed time is recorded even when authorize throws or returns unsuccessfully; apply the same try/finally change to the other authorize invocation around lines 130-131 as well.demos/opensearch-cedarling/benchmark.md (1)
40-41:⚠️ Potential issue | 🟡 MinorPoint benchmark users to the configured query template.
Line 41 still says to edit
query.json, butBenchmarkTestreads the template from thequeryFileparameter intestng.properties. Refer to that configured path so users do not edit the wrong file.Suggested doc fix
- Edit the file `src/test/resources/testng.properties` accordingly. For `entries`, a value like `10000` (ten thousand documents) is OK. Using a higher value may require tweaking OpenSearch `index.max_result_window` property, see [index settings](https://docs.opensearch.org/latest/install-and-configure/configuring-opensearch/index-settings/) -- If the plugin will be in use for this particular test, set property `useCedarling` to `true`, and edit the accompanying file `query.json` supplying the tokens - these can be obtained via Tarp as mentioned in the README file +- If the plugin will be in use for this particular test, set property `useCedarling` to `true`, and edit the query template configured by the `queryFile` property supplying the tokens - these can be obtained via Tarp as mentioned in the README file🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/opensearch-cedarling/benchmark.md` around lines 40 - 41, The docs currently tell users to edit query.json, but BenchmarkTest reads the query template from the queryFile property in testng.properties; update the guidance to point users to the configured path (the queryFile value) instead of always editing query.json, and mention that the template used by BenchmarkTest is determined by the queryFile parameter in testng.properties so they should edit that file or change queryFile to the desired template location when using useCedarling=true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@demos/opensearch-cedarling/benchmark.md`:
- Around line 40-41: The docs currently tell users to edit query.json, but
BenchmarkTest reads the query template from the queryFile property in
testng.properties; update the guidance to point users to the configured path
(the queryFile value) instead of always editing query.json, and mention that the
template used by BenchmarkTest is determined by the queryFile parameter in
testng.properties so they should edit that file or change queryFile to the
desired template location when using useCedarling=true.
In `@demos/opensearch-cedarling/README.md`:
- Line 107: Update the README.md instruction that links to the external
settings.json to point to the local migrated settings.json in this repo (replace
the external raw URL with a relative path like ./settings.json or
docs/settings.json as appropriate); edit the sentence in
demos/opensearch-cedarling/README.md that currently says "Check the file
https://raw.githubusercontent.com/..." so it references the local settings.json
file and instructs the user to fill the policy store URI there.
In
`@demos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java`:
- Around line 80-86: CedarlingSearchExtBuilder.fromExtBuilderList(exts) can
return null and cseb.getParams() (tbac) may be absent; before dereferencing cseb
or tbac in processResponse (where CedarlingSearchExtBuilder cseb =
CedarlingSearchExtBuilder.fromExtBuilderList(exts)), add a null-check: if cseb
is null or cseb.getParams() (tbac) is null/empty, simply return the original
response unchanged instead of continuing to build the authorized list; ensure
you reference the existing variables/methods
CedarlingSearchExtBuilder.fromExtBuilderList, cseb, and cseb.getParams() when
adding the guard.
- Around line 102-104: Wrap the cedarlingService.authorize(tokens, action, map,
context) call in a try/finally: record start = System.nanoTime() immediately
before calling authorize, then in the finally block always add
(System.nanoTime() - start) to decisionsTook and increment the counter used to
compute average_decision_time (the attempted-authorization count variable), so
elapsed time is recorded even when authorize throws or returns unsuccessfully;
apply the same try/finally change to the other authorize invocation around lines
130-131 as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aeec0111-2add-4fe4-bf04-d9780fa69fbc
📒 Files selected for processing (4)
demos/README.mddemos/opensearch-cedarling/README.mddemos/opensearch-cedarling/benchmark.mddemos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/CedarlingSearchResponseProcessor.java
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/opensearch-cedarling/build.gradle`:
- Around line 76-77: The plugin metadata paths are incorrect: update the
licenseFile and noticeFile settings in the build script so packaging validation
succeeds; change licenseFile rootProject.file('LICENSE.txt') to point to the
actual root LICENSE (e.g., rootProject.file('LICENSE')), and for noticeFile
either remove the noticeFile configuration or add a valid file at
demos/opensearch-cedarling/NOTICE.txt and point noticeFile to that path; adjust
the licenseFile and noticeFile entries referenced in the build.gradle so
bundlePlugin packaging can find the metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d9f95a6-7671-4f61-bbdf-b8f2f6d66901
📒 Files selected for processing (3)
demos/opensearch-cedarling/build.gradledemos/opensearch-cedarling/src/main/java/io/jans/cedarling/opensearch/rest/SettingsRestHandler.javademos/opensearch-cedarling/src/test/resources/query.json
Signed-off-by: jgomer2001 <bonustrack310@gmail.com>
moabu
approved these changes
Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Prepare
Description
Target issue
closes #13493
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
New Features
Documentation
Demos & Tests
Chores