-
Notifications
You must be signed in to change notification settings - Fork 1k
RANGER-5266: [Generated by Gemini AI]: (security-admin) REST endpoints to reload log configuration / set log level while service is running #620
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: master
Are you sure you want to change the base?
Conversation
…to dynamically reload log config to avoid service restart
… to avoid compile time dependencies of specific logging frameworks
/** | ||
* REST API for logging-related operations. | ||
*/ | ||
@Path("loggers") |
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.
Does this endpoint need authentication and should be allowed only for ranger admin user?
https://github.com/apache/ranger/blob/master/security-admin/src/main/resources/conf.dist/security-applicationContext.xml
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.
Pull Request Overview
This PR adds a new REST endpoint /service/loggers/reload
to dynamically reload logging configuration without requiring a service restart. This is particularly useful for changing log levels to DEBUG for troubleshooting purposes.
- Implements a new REST endpoint that can reload both Log4j2 and Logback configurations
- Uses reflection to detect and work with different SLF4J logging implementations
- Provides runtime log configuration reloading capability for Apache Ranger admin service
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
LogLevelREST.java | New REST controller providing the /loggers/reload endpoint |
RangerLogLevelService.java | Service class that handles the actual log configuration reloading using reflection |
|
||
/** | ||
* An endpoint to dynamically reload the logging configuration from the | ||
* log4j2 properties file on the classpath. |
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.
The comment mentions 'log4j2 properties file' but the implementation supports both Log4j2 and Logback configurations. The comment should be updated to reflect that it reloads logging configuration from the appropriate configuration file (logback.xml or log4j2 configuration).
* log4j2 properties file on the classpath. | |
* appropriate configuration file (e.g., logback.xml or a Log4j2 configuration file) | |
* on the classpath. |
Copilot uses AI. Check for mistakes.
|
||
// Create a JoranConfigurator instance | ||
Class<?> joranConfiguratorClass = Class.forName("ch.qos.logback.classic.joran.JoranConfigurator"); | ||
Object configurator = joranConfiguratorClass.newInstance(); |
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.
The newInstance() method is deprecated since Java 9. Consider using joranConfiguratorClass.getDeclaredConstructor().newInstance() instead for better compatibility with newer Java versions.
Copilot uses AI. Check for mistakes.
java.net.URL configUrl = this.getClass().getClassLoader().getResource("logback.xml"); | ||
if (configUrl == null) { | ||
configUrl = this.getClass().getClassLoader().getResource("logback-test.xml"); |
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.
The hardcoded configuration file paths 'logback.xml' and 'logback-test.xml' should be extracted as constants to improve maintainability and make them configurable if needed.
java.net.URL configUrl = this.getClass().getClassLoader().getResource("logback.xml"); | |
if (configUrl == null) { | |
configUrl = this.getClass().getClassLoader().getResource("logback-test.xml"); | |
java.net.URL configUrl = this.getClass().getClassLoader().getResource(LOGBACK_XML); | |
if (configUrl == null) { | |
configUrl = this.getClass().getClassLoader().getResource(LOGBACK_TEST_XML); |
Copilot uses AI. Check for mistakes.
if (loggerFactoryClassName.startsWith("org.apache.logging.slf4j")) { | ||
reloadLog4j2Configuration(); | ||
} else if (loggerFactoryClassName.startsWith("ch.qos.logback.classic")) { |
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.
The hardcoded SLF4J binding class name prefixes should be extracted as constants to improve maintainability and reduce the risk of typos.
if (loggerFactoryClassName.startsWith("org.apache.logging.slf4j")) { | |
reloadLog4j2Configuration(); | |
} else if (loggerFactoryClassName.startsWith("ch.qos.logback.classic")) { | |
if (loggerFactoryClassName.startsWith(SLF4J_LOG4J2_PREFIX)) { | |
reloadLog4j2Configuration(); | |
} else if (loggerFactoryClassName.startsWith(SLF4J_LOGBACK_PREFIX)) { |
Copilot uses AI. Check for mistakes.
resetMethod.invoke(context); | ||
|
||
// Find the configuration file URL (e.g., logback.xml) | ||
java.net.URL configUrl = this.getClass().getClassLoader().getResource("logback.xml"); |
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.
Consider validating the configuration file path and ensuring it points to an expected location to prevent potential security issues from loading arbitrary configuration files.
Copilot uses AI. Check for mistakes.
…y support logback as logging mechanism for this endpoint
…for a package, client libs for testing log level endpoints
What changes were proposed in this pull request?
New REST endpoint to reload logging configuration to avoid restart of service while changing logging level to DEBUG.
Any new logging config as per requirement can be added to logback.xml which is be loaded when REST endpoint is hit and won't require service restart. Alternatively logLevel can be set for a specific package/class via the REST endpoint directly
How was this patch tested?
Manual testing in the docker setup.
curl -u admin:rangerR0cks! -X POST http://localhost:6080/service/loggers/reload
Run the curl command
curl -X POST \ -H "Content-Type: application/json" \ -u "admin:rangerR0cks\!" \ -d '{"loggerName": "org.apache.ranger", "logLevel": "DEBUG"}' \ "http://localhost:6080/service/loggers/set-level"
Log level for logger 'org.apache.ranger' has been set to 'DEBUG'
Python client testing script
Attaching screenshot
