-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[Entitlements] Add support for IT tests of always allowed actions (take 2) #124429
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
Changes from all commits
3bac7e7
8416d90
2c601c0
ce9ba18
28f3d1e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,9 +11,11 @@ | |
|
|
||
| import org.elasticsearch.client.internal.node.NodeClient; | ||
| import org.elasticsearch.common.Strings; | ||
| import org.elasticsearch.core.CheckedConsumer; | ||
| import org.elasticsearch.core.CheckedRunnable; | ||
| import org.elasticsearch.core.SuppressForbidden; | ||
| import org.elasticsearch.entitlement.runtime.api.NotEntitledException; | ||
| import org.elasticsearch.env.Environment; | ||
| import org.elasticsearch.logging.LogManager; | ||
| import org.elasticsearch.logging.Logger; | ||
| import org.elasticsearch.rest.BaseRestHandler; | ||
|
|
@@ -70,7 +72,7 @@ public class RestEntitlementsCheckAction extends BaseRestHandler { | |
| private static final Logger logger = LogManager.getLogger(RestEntitlementsCheckAction.class); | ||
|
|
||
| record CheckAction( | ||
| CheckedRunnable<Exception> action, | ||
| CheckedConsumer<Environment, Exception> action, | ||
| EntitlementTest.ExpectedAccess expectedAccess, | ||
| Class<? extends Exception> expectedExceptionIfDenied, | ||
| Integer fromJavaVersion | ||
|
|
@@ -79,15 +81,15 @@ record CheckAction( | |
| * These cannot be granted to plugins, so our test plugins cannot test the "allowed" case. | ||
| */ | ||
| static CheckAction deniedToPlugins(CheckedRunnable<Exception> action) { | ||
| return new CheckAction(action, SERVER_ONLY, NotEntitledException.class, null); | ||
| return new CheckAction(env -> action.run(), SERVER_ONLY, NotEntitledException.class, null); | ||
| } | ||
|
|
||
| static CheckAction forPlugins(CheckedRunnable<Exception> action) { | ||
| return new CheckAction(action, PLUGINS, NotEntitledException.class, null); | ||
| return new CheckAction(env -> action.run(), PLUGINS, NotEntitledException.class, null); | ||
| } | ||
|
|
||
| static CheckAction alwaysDenied(CheckedRunnable<Exception> action) { | ||
| return new CheckAction(action, ALWAYS_DENIED, NotEntitledException.class, null); | ||
| return new CheckAction(env -> action.run(), ALWAYS_DENIED, NotEntitledException.class, null); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -135,7 +137,7 @@ static CheckAction alwaysDenied(CheckedRunnable<Exception> action) { | |
| entry( | ||
| "createInetAddressResolverProvider", | ||
| new CheckAction( | ||
| VersionSpecificNetworkChecks::createInetAddressResolverProvider, | ||
| env -> VersionSpecificNetworkChecks.createInetAddressResolverProvider(), | ||
| SERVER_ONLY, | ||
| NotEntitledException.class, | ||
| 18 | ||
|
|
@@ -215,6 +217,12 @@ static CheckAction alwaysDenied(CheckedRunnable<Exception> action) { | |
| .filter(entry -> entry.getValue().fromJavaVersion() == null || Runtime.version().feature() >= entry.getValue().fromJavaVersion()) | ||
| .collect(Collectors.toUnmodifiableMap(Entry::getKey, Entry::getValue)); | ||
|
|
||
| private final Environment environment; | ||
|
|
||
| public RestEntitlementsCheckAction(Environment environment) { | ||
| this.environment = environment; | ||
| } | ||
|
|
||
| @SuppressForbidden(reason = "Need package private methods so we don't have to make them all public") | ||
| private static Method[] getDeclaredMethods(Class<?> clazz) { | ||
| return clazz.getDeclaredMethods(); | ||
|
|
@@ -230,13 +238,10 @@ private static Stream<Entry<String, CheckAction>> getTestEntries(Class<?> action | |
| if (Modifier.isStatic(method.getModifiers()) == false) { | ||
| throw new AssertionError("Entitlement test method [" + method + "] must be static"); | ||
| } | ||
| if (method.getParameterTypes().length != 0) { | ||
| throw new AssertionError("Entitlement test method [" + method + "] must not have parameters"); | ||
| } | ||
|
|
||
| CheckedRunnable<Exception> runnable = () -> { | ||
| final CheckedConsumer<Environment, Exception> call = createConsumerForMethod(method); | ||
| CheckedConsumer<Environment, Exception> runnable = env -> { | ||
| try { | ||
| method.invoke(null); | ||
| call.accept(env); | ||
| } catch (IllegalAccessException e) { | ||
| throw new AssertionError(e); | ||
| } catch (InvocationTargetException e) { | ||
|
|
@@ -258,6 +263,17 @@ private static Stream<Entry<String, CheckAction>> getTestEntries(Class<?> action | |
| return entries.stream(); | ||
| } | ||
|
|
||
| private static CheckedConsumer<Environment, Exception> createConsumerForMethod(Method method) { | ||
| Class<?>[] parameters = method.getParameterTypes(); | ||
| if (parameters.length == 0) { | ||
| return env -> method.invoke(null); | ||
| } | ||
| if (parameters.length == 1 && parameters[0].equals(Environment.class)) { | ||
| return env -> method.invoke(null, env); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (If we ever acquire a second injectable parameter here, I'd probably change this to loop over the parameters, building the argument list based on the parameter types, much like a real DI framework would.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really see the value in forcing the args to be in a certain order. Like, what if I want to inject A and C? Do I need to declare B for no reason just so I have the right args in the right order? Also, why is It will not surprise you that I'd prefer just to have DI. 😂 |
||
| throw new AssertionError("Entitlement test method [" + method + "] must have no parameters or 1 parameter (Environment)"); | ||
| } | ||
|
|
||
| private static void createURLStreamHandlerProvider() { | ||
| var x = new URLStreamHandlerProvider() { | ||
| @Override | ||
|
|
@@ -421,6 +437,14 @@ public static Set<String> getCheckActionsAllowedInPlugins() { | |
| .collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| public static Set<String> getAlwaysAllowedCheckActions() { | ||
| return checkActions.entrySet() | ||
| .stream() | ||
| .filter(kv -> kv.getValue().expectedAccess().equals(ALWAYS_ALLOWED)) | ||
| .map(Entry::getKey) | ||
| .collect(Collectors.toSet()); | ||
| } | ||
|
|
||
| public static Set<String> getDeniableCheckActions() { | ||
| return checkActions.entrySet() | ||
| .stream() | ||
|
|
@@ -455,7 +479,7 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli | |
| logger.info("Calling check action [{}]", actionName); | ||
| RestResponse response; | ||
| try { | ||
| checkAction.action().run(); | ||
| checkAction.action().accept(environment); | ||
| response = new RestResponse(RestStatus.OK, Strings.format("Succesfully executed action [%s]", actionName)); | ||
| } catch (Exception e) { | ||
| var statusCode = checkAction.expectedExceptionIfDenied.isInstance(e) | ||
|
|
@@ -468,5 +492,4 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli | |
| channel.sendResponse(response); | ||
| }; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| package org.elasticsearch.entitlement.qa; | ||
|
|
||
| import com.carrotsearch.randomizedtesting.annotations.Name; | ||
| import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; | ||
|
|
||
| import org.elasticsearch.entitlement.qa.test.RestEntitlementsCheckAction; | ||
| import org.junit.ClassRule; | ||
|
|
||
| public class EntitlementsAlwaysAllowedIT extends AbstractEntitlementsIT { | ||
|
|
||
| @ClassRule | ||
| public static EntitlementsTestRule testRule = new EntitlementsTestRule(true, null); | ||
|
|
||
| public EntitlementsAlwaysAllowedIT(@Name("actionName") String actionName) { | ||
| super(actionName, true); | ||
| } | ||
|
|
||
| @ParametersFactory | ||
| public static Iterable<Object[]> data() { | ||
| return RestEntitlementsCheckAction.getAlwaysAllowedCheckActions().stream().map(action -> new Object[] { action }).toList(); | ||
| } | ||
|
|
||
| @Override | ||
| protected String getTestRestCluster() { | ||
| return testRule.cluster.getHttpAddresses(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the "Elastic License | ||
| * 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side | ||
| * Public License v 1"; you may not use this file except in compliance with, at | ||
| * your election, the "Elastic License 2.0", the "GNU Affero General Public | ||
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| package org.elasticsearch.entitlement.qa; | ||
|
|
||
| import com.carrotsearch.randomizedtesting.annotations.Name; | ||
| import com.carrotsearch.randomizedtesting.annotations.ParametersFactory; | ||
|
|
||
| import org.elasticsearch.entitlement.qa.test.RestEntitlementsCheckAction; | ||
| import org.junit.ClassRule; | ||
|
|
||
| public class EntitlementsAlwaysAllowedNonModularIT extends AbstractEntitlementsIT { | ||
|
|
||
| @ClassRule | ||
| public static EntitlementsTestRule testRule = new EntitlementsTestRule(false, null); | ||
|
|
||
| public EntitlementsAlwaysAllowedNonModularIT(@Name("actionName") String actionName) { | ||
| super(actionName, true); | ||
| } | ||
|
|
||
| @ParametersFactory | ||
| public static Iterable<Object[]> data() { | ||
| return RestEntitlementsCheckAction.getAlwaysAllowedCheckActions().stream().map(action -> new Object[] { action }).toList(); | ||
| } | ||
|
|
||
| @Override | ||
| protected String getTestRestCluster() { | ||
| return testRule.cluster.getHttpAddresses(); | ||
| } | ||
| } |
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 pretty neat. Your own tiny DI framework!