Open
Conversation
By default, the bind formula will configure the named process to write all queries into a query.log file which potentially is outside the normal log-rotated dirs, thus filling up the disk. This is rather unexpected on high traffic DNS servers. Disable by default, can be explicity reenabled by setting enable_logging to True. Rework jinja logic to make enable_logging and use_extensive_logging mutually exclusive rather than having them "stacked". It makes no sense to have the fine-grained use_extensive_logging configuration depend on the coarse-grained enable_logging toggle. I am actually tempted to rename enable_logging to enable_query_log which is a much clearer description of the functionality. Comments? Somewhat related, log_dir is /var/log/something for every OS except Red Hat where it is defined as /var/named/data... Any reason not to fix that inconsistency other than the use of the chrooted functionality on Red Hat?
Member
|
I like the approach you took, it LGTM 👍
Perhaps this can be done in another PR |
logging_config was only defined for Debian and xBSD resulting in rendering errors on other OS families. This commit fixes saltstack-formulas#115
Add a deprecation message if enable_logging is used instead of enable_query_log. Abort if both enable_logging/enable_query_log is enabled with use_extensive_logging as these are mutually exclusive now. Fixup tests to _not_ look for the querylog line.
Author
|
@javierbertoli Did that. As there's no pillar.set (contrary to grains.set) I did a bit of a workaround. Have a look, should be passing checks now as well. |
Author
|
@javierbertoli ping? |
| its('content') { should match /^zone\ "100\.51\.198\.in-addr\.arpa"\ {\n\ \ type\ master;\n\ \ file\ "#{zones_directory}\/100\.51\.198\.in-addr\.arpa";\n\ \ \n\ \ notify\ no;\n\};/ } | ||
| # Match logging | ||
| its('content') { should match /^logging\ \{\n\ \ channel\ "querylog"\ {\n\ \ \ \ file\ "#{log_directory}\/query\.log";\n\ \ \ \ print-time\ yes;\n\ \ \};\n\ \ category\ queries\ \{\ querylog;\ \};\n\};/ } | ||
| its('content') { should_not match /^logging\ \{\n\ \ channel\ "querylog"\ {\n\ \ \ \ file\ "#{log_directory}\/query\.log";\n\ \ \ \ print-time\ yes;\n\ \ \};\n\ \ category\ queries\ \{\ querylog;\ \};\n\};/ } |
Member
There was a problem hiding this comment.
As you're moving the logging config to another file, can you either add some tests to make sure the logging_config file/s are created properly, or open an issue so we don't forget to add tests for this? Perhaps I can find some time to add them
|
Hi there! Any news? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
By default, the bind formula will configure the named process to
write all queries into a query.log file which potentially is outside
the normal log-rotated dirs, thus filling up the disk.
This is rather unexpected on high traffic DNS servers.
Disable by default, can be explicity reenabled by setting enable_logging
to True.
Rework jinja logic to make enable_logging and use_extensive_logging mutually
exclusive rather than having them "stacked". It makes no sense to have the
fine-grained use_extensive_logging configuration depend on the coarse-grained
enable_logging toggle.
I am actually tempted to rename enable_logging to enable_query_log which is a
much clearer description of the functionality. Comments?
Somewhat related, log_dir is /var/log/something for every OS except Red Hat
where it is defined as /var/named/data... Any reason not to fix that
inconsistency other than the use of the chrooted functionality on Red Hat?