-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Bootstrap entitlements for testing #129268
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
e1166dc
to
d331569
Compare
…source. Using getResource makes this sensitive to unrelated classpath entries, such as the entitlement bridge library, that get prepended to the classpath.
Using the root logger makes this sensitive to unrelated logging, such as from the entitlement library.
…titlements. Taking the actual module name from the class doesn't work in tests, where those classes are loaded from the classpath and so their module info is misleading.
7bcefba
to
ba15751
Compare
...ls-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchTestBasePlugin.java
Outdated
Show resolved
Hide resolved
...ls-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchTestBasePlugin.java
Show resolved
Hide resolved
build-tools/src/main/java/org/elasticsearch/gradle/test/TestBuildInfoPlugin.java
Outdated
Show resolved
Hide resolved
@@ -45,4 +45,16 @@ configure(childProjects.values()) { | |||
*/ | |||
apply plugin: 'elasticsearch.build' | |||
} | |||
|
|||
// This is for any code potentially included in the server at runtime. |
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 just add this to the plugin-scanner project for now. In general we want to move more configuration where it belongs. adding code that might be needed in the future in sometime just adds clutter for now.
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.
It needs to apply to every lib except plugin-scanner.
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.
(Really, plugin-scanner shouldn't be in libs
in the first place.)
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 added some remarks regarding the build changes that I think we need to address.
…eneratorTest. The template generator also strips these, so we need to do so to make this pass on Windows. Note that we use replace("\r", "") where the template generator uses replace("\\r", ""). The latter didn't work for me when I tried it on Windows, for reasons I'm not aware of.
.../framework/src/main/java/org/elasticsearch/entitlement/runtime/policy/TestPolicyManager.java
Outdated
Show resolved
Hide resolved
...ls-internal/src/main/java/org/elasticsearch/gradle/internal/ElasticsearchTestBasePlugin.java
Outdated
Show resolved
Hide resolved
@@ -184,7 +184,7 @@ private ChangelogEntry makeHighlightsEntry(int pr, boolean notable) { | |||
} | |||
|
|||
private String getResource(String name) throws Exception { | |||
return Files.readString(Paths.get(Objects.requireNonNull(this.getClass().getResource(name)).toURI()), StandardCharsets.UTF_8); | |||
return Files.readString(Paths.get(Objects.requireNonNull(this.getClass().getResource(name)).toURI()), StandardCharsets.UTF_8).replace("\r", ""); |
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.
what is that for?
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 test wasn't working on Windows. Tag @brianseeders
@prdoyle I noticed that file entitlement checks likely don't work at all in tests as they are using |
|
||
// Fire up entitlements | ||
try { | ||
TestEntitlementBootstrap.bootstrap(javaTmpDir, maybePath(System.getProperty("tests.config"))); |
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 can only find a single reference of tests.config
where this references elasticsearch.yml, though bootstrap expects a dir.
mvn -Dtests.gce=true -Dtests.config=/path/to/config/file/elasticsearch.yml clean test
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.
Hmm I don't recall why I added that now. 🤔
import java.util.stream.Stream; | ||
|
||
public class TestPathLookup implements PathLookup { | ||
final Map<BaseDir, Collection<Path>> tempDirPaths; |
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.
nit: tempDirPaths
should better be renamed to baseDirPaths
or similar
This test works by altering the logging on the root logger. With entitlements enabled, that will cause additional log statements to appear, which interferes with the test.
Add entitlement enforcement during ordinary unit tests.
This does not yet cover tests that run ES nodes; only ordinary unit tests.
See ES-11597.