Skip to content
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

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

Closed
wants to merge 3 commits into from

Conversation

@mfussenegger
Copy link
Contributor

commented Sep 1, 2014

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 ?

import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertThat;

public class LogConfiguratorTest {

This comment has been minimized.

Copy link
@spinscale

spinscale Sep 11, 2014

Member

can you extend ElasticsearchTestCase?


@Test
public void testResolveConfigValidFilename() throws Exception {
File tempDir = Files.createTempDir();

This comment has been minimized.

Copy link
@spinscale

spinscale Sep 11, 2014

Member

can you use a TemporaryFolder rule, so the temp files are getting cleaned up, regardless if a test succeeds or fails?

File tempFileYml = File.createTempFile("logging.", ".yml", tempDir);
File tempFileYaml = File.createTempFile("logging.", ".yaml", tempDir);

OutputStreamWriter out = new OutputStreamWriter(new FileOutputStream(tempFileYml), StandardCharsets.UTF_8);

This comment has been minimized.

Copy link
@spinscale

spinscale Sep 11, 2014

Member

could be shorted using com.google.common.io.Files.write

@@ -0,0 +1,85 @@
/*
* Licensed to CRATE Technology GmbH ("Crate") under one or more contributor

This comment has been minimized.

Copy link
@spinscale

spinscale Sep 11, 2014

Member

can we change this to the ES license? :-)

@spinscale

This comment has been minimized.

Copy link
Member

commented Sep 11, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2014

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.

loadConfig(file, settingsBuilder);
break;
}
}

This comment has been minimized.

Copy link
@javanna

javanna Nov 12, 2014

Member

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.

This comment has been minimized.

Copy link
@uboness

uboness Nov 18, 2014

Contributor

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.

This comment has been minimized.

Copy link
@javanna

javanna Nov 18, 2014

Member

@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.

@Test
public void testResolveConfigValidFilename() throws Exception {
File tempFileYml = File.createTempFile("logging.", ".yml", temporaryFolder.getRoot());
File tempFileYaml = File.createTempFile("logging.", ".yaml", temporaryFolder.getRoot());

This comment has been minimized.

Copy link
@javanna

javanna Nov 12, 2014

Member

you can create these files using temporaryFolder.newFile(filename)

This comment has been minimized.

Copy link
@javanna

javanna Nov 18, 2014

Member

Actually, I think we can do this without a TemporaryFolder. We can just write a new file within the globalTempDir() that is already available, no need to create a new folder. Also, this should make it possible to merge this new class with the existing LogginConfigurationTests.

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

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).

@javanna javanna added :Core/Infra/Core and removed review labels Nov 12, 2014
@mfussenegger

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

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

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

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

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

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

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2014

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

This comment has been minimized.

Copy link
Member

commented Nov 12, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2014

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.

@@ -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);

This comment has been minimized.

Copy link
@javanna

javanna Nov 18, 2014

Member

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.

This comment has been minimized.

Copy link
@mfussenegger

mfussenegger Nov 18, 2014

Author Contributor

So you would also disallow logging.YAML ?

This comment has been minimized.

Copy link
@javanna

javanna Nov 18, 2014

Member

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.

This comment has been minimized.

Copy link
@mfussenegger

mfussenegger Nov 18, 2014

Author Contributor

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");

?

This comment has been minimized.

Copy link
@javanna

javanna Nov 18, 2014

Member

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.

public class LogConfiguratorTest extends ElasticsearchTestCase {

@Rule
public TemporaryFolder temporaryFolder = new TemporaryFolder();

This comment has been minimized.

Copy link
@javanna

javanna Nov 18, 2014

Member

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

This comment has been minimized.

Copy link
Member

commented Nov 18, 2014

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.

@mfussenegger

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2014

Added another fixup.

javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 19, 2014
…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 added a commit to javanna/elasticsearch that referenced this pull request Nov 19, 2014
…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 javanna added >bug v1.4.1 and removed >enhancement labels Nov 19, 2014
javanna added a commit to javanna/elasticsearch that referenced this pull request Nov 19, 2014
…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 javanna closed this in 2dcb1f5 Nov 19, 2014
@javanna

This comment has been minimized.

Copy link
Member

commented Nov 19, 2014

Thanks a lot @mfussenegger I just merged this.

@jpountz

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2014

@javanna should this fix go to 1.3.6?

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 21, 2014

@jpountz why not. will backport it shortly.

@javanna

This comment has been minimized.

Copy link
Member

commented Nov 21, 2014

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

@clintongormley 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 clintongormley changed the title Settings: LogConfigurator resolveConfig also reads .rpmnew or .bak files LogConfigurator resolveConfig also reads .rpmnew or .bak files Jun 8, 2015
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.