Skip to content

Fail fast on invalid entitlement patches #128071

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+16 −11
Open

Fail fast on invalid entitlement patches #128071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9d70624
Fail fast on invalid entitlement patches
prdoyle May 14, 2025
46bafb7
Don't peel off `PolicyParserException`
prdoyle May 14, 2025
e511ccf
Merge branch 'main' into patch-fail-fast
prdoyle May 14, 2025
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
11 changes: 6 additions & 5 deletions libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,10 @@ public static Map<String, Policy> createPluginPolicies(
return pluginPolicies;
}

/**
* @throws PolicyParserException if the supplied policy is formatted incorrectly
* @throws IllegalStateException for any other error parsing the patch, such as nonexistent module names
*/
public static Policy parseEncodedPolicyIfExists(
String encodedPolicy,
String version,
Expand All @@ -106,11 +110,8 @@ public static Policy parseEncodedPolicyIfExists(
version
);
}
} catch (Exception ex) {
logger.warn(
Strings.format("Found a policy patch with invalid content. The patch will not be applied. Layer [%s]", layerName),
ex
);
} catch (IOException | RuntimeException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Is the RuntimeException necessary to catch? Could we just convert IOException to UncheckedIOException and leave the rest to propagate on their own? There is something to be said for wrapping any exception, though (not just these) since it includes the layer name? Does the PolicyParserException include that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems user-friendly to call out, loud and clear, that the patch they're applying is the cause of the problem, and I felt that PolicyParserException was already clear on that point. PolicyParserException may not name the layer, but it does describe the exact problem in detail.

I caught RuntimeException only because the set IOException | RuntimeException covers everything (besides Error) that this method could possibly throw. Happy to tweak it.

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 be very clear and convert the old log message (that happened for all Exception) to a runtime exception? The cause will still be included, so the PolicyParserException would still be visible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I did. The parts of the old log message I omitted are no longer true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying you don't want me to peel off PolicyParseException? Sure, that's an easy change.

throw new IllegalStateException("Unable to parse policy patch for layer [" + layerName + "]", e);
}
}
return null;
Expand Down
16 changes: 10 additions & 6 deletions ...itlement/src/test/java/org/elasticsearch/entitlement/runtime/policy/PolicyUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ public void testNoPatchWithVersionMismatch() {

public void testNoPatchWithValidationError() {

// Nonexistent module names
var policyPatch = """
versions:
- 9.0.0
Expand All @@ -149,13 +150,15 @@ public void testNoPatchWithValidationError() {
StandardCharsets.UTF_8
);

var policy = PolicyUtils.parseEncodedPolicyIfExists(base64EncodedPolicy, "9.0.0", true, "test-plugin", Set.of());

assertThat(policy, nullValue());
assertThrows(
IllegalStateException.class,
() -> PolicyUtils.parseEncodedPolicyIfExists(base64EncodedPolicy, "9.0.0", true, "test-plugin", Set.of())
);
}

public void testNoPatchWithParsingError() {

// no <version> or <policy> field
var policyPatch = """
entitlement-module-name:
- load_native_libraries
Expand All @@ -167,9 +170,10 @@ public void testNoPatchWithParsingError() {
StandardCharsets.UTF_8
);

var policy = PolicyUtils.parseEncodedPolicyIfExists(base64EncodedPolicy, "9.0.0", true, "test-plugin", Set.of());

assertThat(policy, nullValue());
assertThrows(
IllegalStateException.class,
() -> PolicyUtils.parseEncodedPolicyIfExists(base64EncodedPolicy, "9.0.0", true, "test-plugin", Set.of())
);
}

public void testMergeScopes() {
Expand Down