You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/11/17 16:06:56 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #1786: 1463 remove beanutils
dlmarion opened a new pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-730495743
@dlmarion This change seems to have caused a regression. VolumeIT is failing, seemingly due to something related to SetGoalState. I am not sure exactly what is broken, but it could be the use of `FileReader` for arguments passed in as URLs. Some test files may be stored in jars, and my suspicion is that you cannot do `url.toFile()` for these. I'm also seeing errors reading Metrics configuration files, likely due to the same reason, because the config properties are probably contained in a jar, and we need to have a Reader that uses the URL directly, rather than call `toFile()`. This is only a theory at this point.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-729091982
Made the changes you requested. I'm not sure what spam you were getting previously, but I did the following and did not get any results:
```
mvn clean verify -Psunny
find */target/surefire-reports -name *.txt -exec grep -H beanutils {} \;
find */target/failsafe-reports -name *.txt -exec grep -H beanutils {} \;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#discussion_r525525497
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
##########
@@ -140,8 +140,8 @@ public Buildable withOverrides(Map<String,String> overrides) {
return this;
}
- @SuppressFBWarnings(value = "URLCONNECTION_SSRF_FD",
- justification = "location of props is specified by an admin")
+ @SuppressFBWarnings(value = {"URLCONNECTION_SSRF_FD", "PATH_TRAVERSAL_IN"},
+ justification = "location of props is specified by an admin, path provided by test")
Review comment:
Is this suppression necessary on this method? This method doesn't look like it changed. It might be covered by the suppression elsewhere.
##########
File path: start/src/main/java/org/apache/accumulo/start/classloader/AccumuloClassLoader.java
##########
@@ -86,10 +86,10 @@ public static String getAccumuloProperty(String propertyName, String defaultValu
return defaultValue;
}
try {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setURL(accumuloConfigUrl));
- PropertiesConfiguration config = propsBuilder.getConfiguration();
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(accumuloConfigUrl.getFile())) {
+ config.getLayout().load(config, reader);
Review comment:
I wasn't aware of the `read()` method before. Apparently, we can just call `config.read(reader)` here. I also wasn't aware of `FileHandler` before, which I see is being used when saving files.
I think we should try to be consistent. We should either use:
```java
var config = PropertiesConfiguration();
try (var reader = new FileReader()) {
config.read(reader);
}
// and
try (var writer = new FileWriter()) {
config.write(writer);
}
```
OR
```java
var config = new PropertiesConfiguration();
var fileHandler = new FileHandler(config);
fileHandler.load(file);
// and
fileHandler.save(file);
```
Ultimately, they will both do the same thing, but I'm not sure which one is cleaner, since we still need to handle exceptions. Either way, but we should consistently use one pattern or the other.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-729190890
I ran the commands you gave me on the main branch (after removing the lines in the log4j properties file) and I saw the log statements you were talking about. Then I switched over to this branch and ran the same commands, nothing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-729093118
Just realized I looked at the wrong files. Ran the following and didn't get any matches either:
```
find */target/surefire-reports -name *-output.txt -exec grep -H beanutils {} \;
find */target/failsafe-reports -name *-output.txt -exec grep -H beanutils {} \;
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-730553970
Looks good. Thanks for the assist.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#discussion_r525298718
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/SiteConfiguration.java
##########
@@ -203,15 +204,18 @@ private SiteConfiguration(Map<String,String> config) {
}
// load properties from config file
+ @SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "path provided by test")
private static AbstractConfiguration getPropsFileConfig(URL accumuloPropsLocation) {
if (accumuloPropsLocation != null) {
- var propsBuilder = new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setURL(accumuloPropsLocation));
- try {
- return propsBuilder.getConfiguration();
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(accumuloPropsLocation.getFile())) {
+ config.getLayout().load(config, reader);
} catch (ConfigurationException e) {
throw new IllegalArgumentException(e);
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration", e1);
Review comment:
This can be combined with the existing `ConfigurationException`
##########
File path: core/src/test/java/org/apache/accumulo/core/conf/SiteConfigurationTest.java
##########
@@ -77,6 +78,8 @@ public void testFile() {
assertEquals("256M", conf.get(Property.TSERV_WALOG_MAX_SIZE));
assertEquals("org.apache.accumulo.core.cryptoImpl.AESCryptoService",
conf.get(Property.INSTANCE_CRYPTO_SERVICE));
+ assertEquals(System.getenv("USER"), conf.get("general.test.user.name"));
+ assertEquals("/tmp/test/dir", conf.get("general.test.user.dir"));
Review comment:
Nice checks to test interpolation!
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##########
@@ -194,14 +196,17 @@ public static ClientConfiguration create() {
* the path to the configuration file
* @since 1.9.0
*/
- public static ClientConfiguration fromFile(File file) {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setFile(file));
- try {
- return new ClientConfiguration(Collections.singletonList(propsBuilder.getConfiguration()));
+ public static ClientConfiguration fromFile(File file) throws FileNotFoundException {
Review comment:
This changes exceptions thrown on a public API method.
##########
File path: assemble/pom.xml
##########
@@ -106,11 +106,6 @@
<artifactId>jaxb-core</artifactId>
<optional>true</optional>
</dependency>
- <dependency>
- <groupId>commons-beanutils</groupId>
- <artifactId>commons-beanutils</artifactId>
- <optional>true</optional>
- </dependency>
Review comment:
Can probably also remove the entry from `assemble/src/main/assemblies/component.xml`
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##########
@@ -194,14 +196,17 @@ public static ClientConfiguration create() {
* the path to the configuration file
* @since 1.9.0
*/
- public static ClientConfiguration fromFile(File file) {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setFile(file));
- try {
- return new ClientConfiguration(Collections.singletonList(propsBuilder.getConfiguration()));
+ public static ClientConfiguration fromFile(File file) throws FileNotFoundException {
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(file)) {
+ config.getLayout().load(config, reader);
+ return new ClientConfiguration(Collections.singletonList(config));
} catch (ConfigurationException e) {
throw new IllegalArgumentException("Bad configuration file: " + file, e);
+ } catch (FileNotFoundException fnfe) {
+ throw fnfe;
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration", e1);
Review comment:
It'd be better to add these exceptions to the existing `ConfigurationException` using multi-catch. That way, we don't introduce new exceptions in the public API method.
##########
File path: test/src/main/java/org/apache/accumulo/test/metrics/MetricsFileTailer.java
##########
@@ -153,11 +147,12 @@ private Configuration loadMetricsConfig() {
}
return sub;
-
- } catch (ConfigurationException ex) {
+ } catch (ConfigurationException e) {
throw new IllegalStateException(
String.format("Could not find configuration file \'%s\' on classpath",
MetricsTestSinkProperties.METRICS_PROP_FILENAME));
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration", e1);
Review comment:
This can be combined with the existing `ConfigurationException`
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
##########
@@ -225,14 +230,15 @@ private static ClientConfiguration loadFromSearchPath(List<String> paths) {
for (String path : paths) {
File conf = new File(path);
if (conf.isFile() && conf.canRead()) {
- FileBasedConfigurationBuilder<PropertiesConfiguration> propsBuilder =
- new FileBasedConfigurationBuilder<>(PropertiesConfiguration.class)
- .configure(new Parameters().properties().setFile(conf));
- try {
- configs.add(propsBuilder.getConfiguration());
+ var config = new PropertiesConfiguration();
+ try (var reader = new FileReader(conf)) {
+ config.getLayout().load(config, reader);
+ configs.add(config);
log.info("Loaded client configuration file {}", conf);
} catch (ConfigurationException e) {
throw new IllegalStateException("Error loading client configuration file " + conf, e);
+ } catch (IOException e1) {
+ throw new UncheckedIOException("IOExcetion creating configuration", e1);
Review comment:
This can be combined with the existing `ConfigurationException`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-729151190
> To be clear, you are saying that the spam is showing up in the master or tserver logs when starting Accumulo, and the entries in ` test/src/main/resources/log4j2-test.properties` suppress that? There are no logger settings in the `conf` directory for beanutils.
If you do this on the `main` branch:
```diff
--- a/test/src/main/resources/log4j2-test.properties
+++ b/test/src/main/resources/log4j2-test.properties
@@ -121,3 +120,0 @@ logger.29.level = info
-logger.30.name = org.apache.commons.beanutils.converters
-logger.30.level = info
-
```
Then, do `mvn clean verify -Dtest=blah -Psunny -Dspotbugs.skip -Dcheckstyle.skip`
Then, do `grep converters test/target/mini-tests/org.apache.accumulo.test.functional.ReadWriteIT_interleaved/logs/*`
You will see them. You probably aren't seeing it because you're looking for `beanutil`, but our log4j configuration is set to only show the last two elements of the logger name, so the part with `beanutil` gets truncated.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion merged pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-729113404
> Just realized I looked at the wrong files. Ran the following and didn't get any matches either:
>
> ```
> find */target/surefire-reports -name *-output.txt -exec grep -H beanutils {} \;
> find */target/failsafe-reports -name *-output.txt -exec grep -H beanutils {} \;
> ```
The spam was in the log4j logs when starting up. They may show up in Mini logs, or maybe via Uno. You can see what I mean by removing the logging config changes without your other changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] dlmarion commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
dlmarion commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-729120125
To be clear, you are saying that the spam is showing up in the master or tserver logs when starting Accumulo, and the entries in ` test/src/main/resources/log4j2-test.properties` suppress that? There are no logger settings in the `conf` directory for beanutils.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #1786: 1463 remove beanutils
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1786:
URL: https://github.com/apache/accumulo/pull/1786#issuecomment-730547399
> @dlmarion This change seems to have caused a regression. VolumeIT is failing, seemingly due to something related to SetGoalState. I am not sure exactly what is broken, but it could be the use of `FileReader` for arguments passed in as URLs. Some test files may be stored in jars, and my suspicion is that you cannot do `url.toFile()` for these. I'm also seeing errors reading Metrics configuration files, likely due to the same reason, because the config properties are probably contained in a jar, and we need to have a Reader that uses the URL directly, rather than call `toFile()`. This is only a theory at this point.
I believe I have fixed it in #1795
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org