You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/01/27 22:42:57 UTC
[GitHub] [logging-log4j2] carterkozak opened a new pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
carterkozak opened a new pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732
Previously lookups between configuration properties weren't evaluated
until they were used in the configuration, which resulted in
behavior more prohibitive for RoutingAppenders than anticipated.
This attempts to provide a safer, more compatible lookup strategy
with consistent behavior between users.
The risk is that changes in edge-case behavior impact the entire
configuraiton surface, not only the more obscure runtime consumers.
For example, a property defined thusly (note the escaped lookup)
`<property name="foo">Date is $${date:YYYY-MM-dd}</property>`
When used would result in `Date is ${date:YYYY-MM-dd}`
rather than `Date is 2022-01-27`, becuase initially when
the configuration is parsed, lookups aren't evaluated on properties,
which previously stripped one level of escapement.
The new behavior is easier to understand, but may result in some
friction for consumers.
**TODO(ckozak): needs additional test coverage, but I'd like to share the concept first**
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#discussion_r798723570
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/PropertiesLookup.java
##########
@@ -29,29 +32,28 @@
*/
public final class PropertiesLookup implements StrLookup {
- /**
- * Configuration from which to read properties.
- */
- private final Map<String, String> properties;
+ /** Logger context properties. */
+ private final Map<String, String> contextProperties;
- /**
- * Constructs a new instance for the given map.
- *
- * @param properties map these.
- */
- public PropertiesLookup(final Map<String, String> properties) {
- this.properties = properties == null
+ /** Configuration properties. */
+ private final Map<String, ConfigurationPropertyResult> configurationProperties;
+
+ public PropertiesLookup(final Property[] configProperties, final Map<String, String> contextProperties) {
+ this.contextProperties = contextProperties == null
? Collections.emptyMap()
- : properties;
+ : contextProperties;
+ this.configurationProperties = configProperties == null
+ ? Collections.emptyMap()
+ : createConfigurationPropertyMap(configProperties);
}
/**
- * Gets the property map.
+ * Constructs a new instance for the given map.
*
- * @return the property map.
+ * @param properties map these.
*/
- public Map<String, String> getProperties() {
Review comment:
@garydgregory This removes the properties getter as it's not clear how the internal state would be reflected by such a getter. In general I prefer to avoid exposing internal state so that it remains an implementation detail, not API, but I don't want to break any functionality that you rely upon.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak merged pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak merged pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#discussion_r794553848
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/LookupResult.java
##########
@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache license, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the license for the specific language governing permissions and
+ * limitations under the license.
+ */
+
+package org.apache.logging.log4j.core.lookup;
+
+public interface LookupResult {
+
+ /** Value of the lookup result. Never null. */
+ String value();
+
+ /**
+ * True if the result should be re-evaluated for other lookups.
+ * This is used by {@link PropertiesLookup} to allow properties to be evaluated against other properties,
+ * because the configuration properties are completely trusted and designed with lookups in mind. It is
+ * unsafe to return true in most cases because it may allow unintended lookups to evaluate other lookups.
+ */
+ default boolean allowRecursiveLookups() {
+ return false;
+ }
Review comment:
I don't think the name of this method is obvious enough. Perhaps `allowLookupEvaluationInValue`?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#discussion_r801118459
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/config/PropertiesPlugin.java
##########
@@ -45,15 +47,62 @@ private PropertiesPlugin() {
@PluginFactory
public static StrLookup configureSubstitutor(@PluginElement("Properties") final Property[] properties,
@PluginConfiguration final Configuration config) {
- if (properties == null) {
- return new Interpolator(config.getProperties());
+ // For backwards compatibility, we unescape all escaped lookups when properties are parsed.
+ // This matches previous behavior for escaped components which were meant to be executed later on.
+ Property[] escapedProperties = new Property[properties == null ? 0 : properties.length];
Review comment:
```suggestion
Property[] unescapedProperties = new Property[properties == null ? 0 : properties.length];
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on a change in pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#discussion_r794552367
##########
File path: log4j-core/src/main/java/org/apache/logging/log4j/core/config/PropertiesPlugin.java
##########
@@ -45,15 +46,14 @@ private PropertiesPlugin() {
@PluginFactory
public static StrLookup configureSubstitutor(@PluginElement("Properties") final Property[] properties,
@PluginConfiguration final Configuration config) {
- if (properties == null) {
- return new Interpolator(config.getProperties());
- }
+ final Property[] props = properties == null ? Property.EMPTY_ARRAY : properties;
final Map<String, String> map = new HashMap<>(config.getProperties());
Review comment:
TODO + test: the configuration context-properties (`config.getProperties()`) are distinct from the configuration properties (`Property[] properties`), and should not allow recursive evaluation.
The PropertiesLookup should take both maps, and return values which don't support recursive evaluation for config context props.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] garydgregory commented on pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
garydgregory commented on pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#issuecomment-1024309838
Maybe this is the wrong place to ask but it feels to me we are going about this backward or I am missing some information. I would find it helpful to work from the documentation down to the code instead of the other way around. IOW, let's define first what it is the feature set is: What we can configure where and why (we don't want infinite recursion), what is the behavior, then implementation can follow. For example, where do we allow recursive lookups? When do they happen? How deep? And so on. I guess what I am looking for is some written agreement on what it is we want to provide to users before we release 2.17.2 and create more behavior differences and confusion since the behavior has already changed enough in 2.17.1 to break existing user configurations. With 2.17.2 (or 2.18.0), we are introducing a 3rd behavior set, so let's try and be done with it ;-)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#issuecomment-1029409723
I’m having a bit of an internal struggle with this — in its current state, it changes the behavior of `$${ctx:foo}` used in properties because today we build properties using substitution, such that the escapement is dropped when the property is built: `${ctx:foo}`, then when the property is referenced, recursive evaluation will then evaluate `${ctx:foo}` (assuming recursive evaluation is allowed)
My change modifies the behavior so that substitution is allowed through properties recursively, but no other lookups, so when the Property is initially constructed, it retains the `$${ctx:foo}` value, and when it’s referenced, the value is expanded to `${ctx:foo}`.
I am convinced the new behavior is better, and easier to reason about. However, changes impacting config are bad. That said, this fixes an issue in which lookup-shaped data in sources used by the configuration are unexpectedly evaluated (e.g. `env:FOO` where FOO includes something like `${java:version}`)
We can play some dirty tricks to minimize impact, however that sort of thing will more than likely come back to haunt us. I can do a `value.replace("$${", "${")` on the raw value to simulate the old behavior, such that old configs continue to work, however there are edge cases in which literal `$${` won’t work as one might expect.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#issuecomment-1024737180
I have updated the description with several more words :-)
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#issuecomment-1024613948
This experiment came out of a discussion with Ralph, and deserves more context in this PR. This is still marked as a draft, and needs a better description of intent and potential impact.
> I would find it helpful to work from the documentation down to the code instead of the other way around
This is tricky. The documentation describes but a sliver of the full breadth of functionality, much of the existing functionality I think is defacto api because it's already used, regardless of whether it's known or understood by any of us.
For what it's worth, this does introduce a behavior change, and that worries me. I think the result of that change puts us in a much better place, but a design along these lines is safer in terms of potential impact to existing configurations: #722
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4j2] carterkozak commented on pull request #732: LOG4J2-3350: Lookup recursion is not allowed, except through config properties
Posted by GitBox <gi...@apache.org>.
carterkozak commented on pull request #732:
URL: https://github.com/apache/logging-log4j2/pull/732#issuecomment-1030409561
I've added a bit of code to unescape escaped lookups, which will allow existing properties to work as expected. Rather than `value.replace("$${", "${")`, I'm using a `StrSubstitutor` with a lookup that always returns `null` and doesn't understand defaults, this way original values are retained, but escaped components are unescaped so that there's no regression from 2.17.0 and earlier.
We had discussed the idea of making such a feature opt-in, however I feel pretty strongly that it should be on-by-default, or shouldn't exist at all. Reason being: In cases where it's needed, it would require a similar level of effort to unescape lookups in logging configurations as to turn on the feature.
At this point, the question is:
Are more concerned with easy upgrades that don't break functionality, or providing the cleanest, most straightforward design at the cost of requiring some configuration changes?
What do you think?
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org