LogConfigurator resolveConfig also reads .rpmnew or .bak files #7457

mfussenegger · 2014-09-01T09:21:45Z

Currently the LogConfigurator will attempt to read any file that starts with "logging." in the env.configFile() path.

A common practise is to suffix files with .bak or in case of package managers they often add a suffix if the file has been modified upstream.

Elasticsearch will read those values anyway and that might lead to very confusing logging behaviour.

I am not sure how to best approach this, either have a whitelist or a backlist of suffixes that are allowed/disallowed ?

spinscale · 2014-09-11T06:57:52Z

src/test/java/org/elasticsearch/common/logging/log4j/LogConfiguratorTest.java

+import static org.hamcrest.core.Is.is;
+import static org.junit.Assert.assertThat;
+
+public class LogConfiguratorTest {


can you extend ElasticsearchTestCase?

spinscale · 2014-09-11T07:02:52Z

This only allows for loading of .yaml files, however LogConfigurator.loadConfig() calls SettingsBuilder.loadFromUrl(), which is also able to load json and properties files. Should we cater for this as well? What do you think?

mfussenegger · 2014-09-11T09:13:18Z

I've added a fixup commit which includes your suggested changes.

Not sure about json, I don't really mind to add support for json files, but I guess then it should also be documented? Actually in the documentation it mentions that just "logging.yml" is loaded so the implementation here doesn't quite match the behaviour, but I didn't want to break backward compatibility.

javanna · 2014-11-12T10:45:52Z

src/main/java/org/elasticsearch/common/logging/log4j/LogConfigurator.java

+                                loadConfig(file, settingsBuilder);
+                                break;
+                            }
+                        }


with the current code a logging.whatever.yaml file would be loaded. I wonder if this is our intention or a side-effect of the current behaviour. Honestly I would be in favour of simplifying this further and even have something like if (file.getFileName().toString().equals("logging.yaml") || file.getFileName().toString().equals("logging.yml") ) unless we want to extend this to json and properties files, which I think would be off-topic in this PR.

we need to support whatever extensions Settings supports and that includes json & java properties formats. I wouldn't worry about restricting it to these three (yml, json, properties) with regards to bwc.

@uboness you replied to and outdated comment, have a look at this other comment to see what we settled on. If I read your comment right, that should be ok.

javanna · 2014-11-12T10:50:46Z

Hi @mfussenegger sorry it took a while to give you some more feedback, thanks a lot for updating your PR. I think it's really close and I would eventually address loading of json or properties files in a different issue. I left two minor comments, if you can address those this is ready to go. Maybe we could also add to the docs here that we load the logging.yaml (not only logging.yml).

mfussenegger · 2014-11-12T11:11:44Z

Hi @javanna thanks for the feedback. I've added another fixup with the changes you mentioned.

javanna · 2014-11-12T11:32:32Z

Hey @mfussenegger I apologize but I just realized that in fact we currently support both json and properties formats while with this PR we would remove support for them. So @spinscale was right, we should expand to logging.properties and logging.json. Also I would update the docs page by briefly adding the supported formats. And if you feel like it, we could add a couple of tests that verify that the json and properties format work (otherwise I will do it afterwards).

clintongormley · 2014-11-12T13:22:00Z

This logic should also be applied to eg config/elasticsearch.yml etc, as we're seeing the same issues there.

javanna · 2014-11-12T15:33:04Z

Going to open a separate issue for other config files, let's focus exclusively on logging on this PR. @mfussenegger after some thinking I find your initial approach better than what I suggested, since we support multiple logging configuration files that get merged. Thus we can leave it at logging.*.suffix where suffix is either yaml, yml, json or properties. This solution would be more flexible and backwards compatible and at the same time fix the original problem. Thanks a lot for your help and sorry for changing direction :)

mfussenegger · 2014-11-12T17:01:14Z

K, I'll look into the json/properties support and update the PR accordingly. But might take a bit until I get back to this.

javanna · 2014-11-12T17:25:43Z

@mfussenegger thanks again!
@clint regarding your comment on config/elasticsearch.yml etc. I double checked and it seems like only logging.yml suffers from this problem, elasticsearch.yml gets loaded by explicit name, so I am not wrong there's no need to create another issue ;)

mfussenegger · 2014-11-18T13:07:27Z

I've updated the PR so that .json and .properties are also allowed suffixes.

Also rebased it with current master and already squashed the first few commits.

javanna · 2014-11-18T13:52:21Z

src/main/java/org/elasticsearch/common/logging/log4j/LogConfigurator.java

@@ -118,8 +119,14 @@ public static void resolveConfig(Environment env, final ImmutableSettings.Builde
            Files.walkFileTree(env.configFile().toPath(), EnumSet.of(FileVisitOption.FOLLOW_LINKS), Integer.MAX_VALUE, new SimpleFileVisitor<Path>() {
                @Override
                public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException {
-                    if (file.getFileName().toString().startsWith("logging.")) {
-                        loadConfig(file, settingsBuilder);
+                    String fileName = file.getFileName().toString().toLowerCase(Locale.ENGLISH);


I wonder about the added toLowerCase...I understand why you did it, but I'm not sure we should change this here. This way we would not only load less files (as we skip non allowed suffixes), but also more files at the same time (think of potentially any LOGGING* or Logging* file that was ignored up until now). It's a minor thing but I would open another issue for discussion on making the filename case-insensitive and discuss it separately.

So you would also disallow logging.YAML ?

I think we already do since we don't lowercase the suffix. That's fine with me. I think it's consistent with what we usually do with conf files. Let's try and add some docs for this to make it clear.

Without the changes in this PR it would match as anything starting with logging. matches.
So maybe the allowed suffixes should be

(".YML", ".YAML", ".JSON", ".PROPERTIES", ".yml", ".yaml", ".json", ".properties");

?

I see what you mean, that we somehow break bw comp but I would leave only the lowercase variants to make this consistent with other places where we load files e.g. we would never load elasticsearch.YML . I think this change is acceptable and in the scope of the PR since we are in fact limiting the files that are getting loaded as logging configuration.

javanna · 2014-11-18T13:56:08Z

src/test/java/org/elasticsearch/common/logging/log4j/LogConfiguratorTest.java

+public class LogConfiguratorTest extends ElasticsearchTestCase {
+
+    @Rule
+    public TemporaryFolder temporaryFolder = new TemporaryFolder();


Not sure you got my previous comment on this, it might have gotten lost with the rebase. I think we can avoid using the TemporaryFolder rule but just rely on the facilities provided by the randomized testing library. We can just create a new directory within the globalTempDir() that is already available (via the newTempDir() static method), and then create the file within this temp dir that gets cleaned after each test by default. Also, this should make it possible to merge this new class with the existing LogginConfigurationTests. Makes sense?

javanna · 2014-11-18T13:57:47Z

Thanks @mfussenegger it's very close, I left one minor comment on making the filename case-insensitive and one more around testing. Let me know if you have any question.

…yaml

mfussenegger · 2014-11-18T19:34:25Z

Added another fixup.

…r suffix Make sure that files such as logging.yml.rpmnew or logging.yml.bak are not loaded as logging configuration. Only files that start with the "logging." prefix and end with ".yaml", ".yml", ".json" and ".properties" suffix get loaded. Closes elastic#7457

javanna · 2014-11-19T11:22:08Z

Thanks a lot @mfussenegger I just merged this.

jpountz · 2014-11-21T09:56:38Z

@javanna should this fix go to 1.3.6?

javanna · 2014-11-21T10:15:49Z

@jpountz why not. will backport it shortly.

javanna · 2014-11-21T10:57:58Z

hey @jpountz I looked into backporting but that would require other changes that have not been backported: 59307408 .

…r suffix Make sure that files such as logging.yml.rpmnew or logging.yml.bak are not loaded as logging configuration. Only files that start with the "logging." prefix and end with ".yaml", ".yml", ".json" and ".properties" suffix get loaded. Closes elastic#7457

clintongormley added the adoptme label Aug 26, 2014

bloonix mentioned this pull request Sep 10, 2014

Loading wrong configuration files #7674

Closed

spinscale reviewed Sep 11, 2014
View reviewed changes

clintongormley added the review label Oct 18, 2014

clintongormley added :Core/Infra/Settings Settings infrastructure and APIs >enhancement labels Nov 11, 2014

clintongormley assigned javanna Nov 11, 2014

javanna removed the help wanted adoptme label Nov 12, 2014

javanna reviewed Nov 12, 2014
View reviewed changes

javanna added :Core/Infra/Core Core issues without another label and removed review labels Nov 12, 2014

javanna mentioned this pull request Nov 12, 2014

logging config in a svn working copy fails to load with elasticsearch 1.3 #8040

Closed

javanna mentioned this pull request Nov 12, 2014

Custom logger.yml location #7395

Closed

mfussenegger added 2 commits November 18, 2014 14:04

only read log configuration files ending with .yml or .yaml

e0c8ad6

fixup! only read log configuration files ending with .yml or .yaml

91a1f48

javanna reviewed Nov 18, 2014
View reviewed changes

fixup! fixup! only read log configuration files ending with .yml or .…

5cce501

…yaml

javanna added v2.0.0-beta1 v1.5.0 labels Nov 19, 2014

javanna added >bug v1.4.1 and removed >enhancement labels Nov 19, 2014

javanna closed this in 2dcb1f5 Nov 19, 2014

clintongormley changed the title ~~LogConfigurator resolveConfig also reads .rpmnew or .bak files~~ Settings: LogConfigurator resolveConfig also reads .rpmnew or .bak files Nov 25, 2014

clintongormley removed the :Core/Infra/Core Core issues without another label label Nov 25, 2014

clintongormley changed the title ~~Settings: LogConfigurator resolveConfig also reads .rpmnew or .bak files~~ LogConfigurator resolveConfig also reads .rpmnew or .bak files Jun 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LogConfigurator resolveConfig also reads .rpmnew or .bak files #7457

LogConfigurator resolveConfig also reads .rpmnew or .bak files #7457

mfussenegger commented Sep 1, 2014

spinscale Sep 11, 2014

spinscale commented Sep 11, 2014

mfussenegger commented Sep 11, 2014

javanna Nov 12, 2014

uboness Nov 18, 2014

javanna Nov 18, 2014

javanna commented Nov 12, 2014

mfussenegger commented Nov 12, 2014

javanna commented Nov 12, 2014

clintongormley commented Nov 12, 2014

javanna commented Nov 12, 2014

mfussenegger commented Nov 12, 2014

javanna commented Nov 12, 2014

mfussenegger commented Nov 18, 2014

javanna Nov 18, 2014

mfussenegger Nov 18, 2014

javanna Nov 18, 2014

mfussenegger Nov 18, 2014

javanna Nov 18, 2014

javanna Nov 18, 2014

javanna commented Nov 18, 2014

mfussenegger commented Nov 18, 2014

javanna commented Nov 19, 2014

jpountz commented Nov 21, 2014

javanna commented Nov 21, 2014

javanna commented Nov 21, 2014

LogConfigurator resolveConfig also reads .rpmnew or .bak files #7457

LogConfigurator resolveConfig also reads .rpmnew or .bak files #7457

Conversation

mfussenegger commented Sep 1, 2014

Choose a reason for hiding this comment

spinscale commented Sep 11, 2014

mfussenegger commented Sep 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Nov 12, 2014

mfussenegger commented Nov 12, 2014

javanna commented Nov 12, 2014

clintongormley commented Nov 12, 2014

javanna commented Nov 12, 2014

mfussenegger commented Nov 12, 2014

javanna commented Nov 12, 2014

mfussenegger commented Nov 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented Nov 18, 2014

mfussenegger commented Nov 18, 2014

javanna commented Nov 19, 2014

jpountz commented Nov 21, 2014

javanna commented Nov 21, 2014

javanna commented Nov 21, 2014