junrao commented on code in PR #20844:
URL: https://github.com/apache/kafka/pull/20844#discussion_r2535892223
##########
connect/runtime/src/main/java/org/apache/kafka/connect/connector/policy/AllowlistConnectorClientConfigOverridePolicy.java:
##########
@@ -40,7 +40,13 @@ public class AllowlistConnectorClientConfigOverridePolicy
extends AbstractConnec
private static final String ALLOWLIST_CONFIG_DOC = "List of client
configurations that can be overridden by " +
"connectors. If empty, connectors can't override any client
configurations.";
private static final ConfigDef CONFIG_DEF = new ConfigDef()
- .define(ALLOWLIST_CONFIG, ConfigDef.Type.LIST,
ALLOWLIST_CONFIG_DEFAULT, ConfigDef.Importance.MEDIUM, ALLOWLIST_CONFIG_DOC);
+ .define(
+ ALLOWLIST_CONFIG,
+ ConfigDef.Type.LIST,
+ ALLOWLIST_CONFIG_DEFAULT,
+ ConfigDef.ValidList.anyNonDuplicateValues(true, false),
+ ConfigDef.Importance.MEDIUM, ALLOWLIST_CONFIG_DOC
Review Comment:
Should we put the two params on separate lines?
##########
docs/upgrade.html:
##########
@@ -137,9 +137,12 @@ <h5><a id="upgrade_420_notable"
href="#upgrade_420_notable">Notable changes in 4
<li>
LIST-type configurations now enforce stricter validation:
<ul>
- <li>Null values are no longer accepted for most LIST-type
configurations, except those that explicitly
- allow a null default value or where a null value has a
well-defined semantic meaning.</li>
- <li>Duplicate entries within the same list are no longer
permitted.</li>
+ <li>Null values are no longer accepted for most LIST-type
configurations. Exceptions apply only to
+ configurations that have null as their default value,
as users cannot explicitly assign null values
+ in configuration files.</li>
+ <li>Most LIST-type configurations no longer accept
duplicate entries, except in cases where duplicates
+ are explicitly supported. For backward compatibility,
if users configure duplicate entries when they
+ are not accepted, duplicate entries will be ignored
and a warning will be logged.</li>
Review Comment:
> Empty lists are no longer allowed, except in configurations where an empty
list has a well-defined
> semantic meaning.
For some configurations, an empty list causes the system to not work. Empty
lists are no longer allowed in those configurations.
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -536,6 +539,14 @@ Object parseValue(ConfigKey key, Object value, boolean
isSet) {
// otherwise assign setting its default value
parsedValue = key.defaultValue;
}
+ if (key.validator instanceof ValidList && parsedValue instanceof List)
{
+ List<?> originalListValue = (List<?>) parsedValue;
+ parsedValue =
originalListValue.stream().distinct().collect(Collectors.toList());
+ if (originalListValue.size() != ((List<?>) parsedValue).size()) {
+ LOGGER.warn("Duplicate configuration \"{}\" values are found.
Duplicates will be removed. The original value " +
Review Comment:
> Duplicate configuration \"{}\" values are found.
Configuration key \"{}\" contains duplicate values.
##########
docs/upgrade.html:
##########
@@ -139,7 +139,9 @@ <h5><a id="upgrade_420_notable"
href="#upgrade_420_notable">Notable changes in 4
<ul>
<li>Null values are no longer accepted for most LIST-type
configurations, except those that explicitly
allow a null default value or where a null value has a
well-defined semantic meaning.</li>
- <li>Duplicate entries within the same list are no longer
permitted.</li>
+ <li>Most LIST-type configurations no longer accept
duplicate entries, except in cases where duplicates
Review Comment:
I feel that "any non-duplicate values, empty list, null" is quite verbose.
"Valid Values" refers to valid values in the list. So, it's probably better to
keep things the old way. If the InList is provided, we list the values in the
InList. Otherwise, just leave "Valid Values" empty.
##########
clients/src/main/java/org/apache/kafka/common/config/internals/BrokerSecurityConfigs.java:
##########
@@ -169,14 +169,14 @@ public class BrokerSecurityConfigs {
.define(SslConfigs.SSL_TRUSTMANAGER_ALGORITHM_CONFIG, STRING,
SslConfigs.DEFAULT_SSL_TRUSTMANAGER_ALGORITHM, MEDIUM,
SslConfigs.SSL_TRUSTMANAGER_ALGORITHM_DOC)
.define(SslConfigs.SSL_ENDPOINT_IDENTIFICATION_ALGORITHM_CONFIG,
STRING, SslConfigs.DEFAULT_SSL_ENDPOINT_IDENTIFICATION_ALGORITHM, LOW,
SslConfigs.SSL_ENDPOINT_IDENTIFICATION_ALGORITHM_DOC)
.define(SslConfigs.SSL_SECURE_RANDOM_IMPLEMENTATION_CONFIG,
STRING, null, LOW, SslConfigs.SSL_SECURE_RANDOM_IMPLEMENTATION_DOC)
- .define(SslConfigs.SSL_CIPHER_SUITES_CONFIG, LIST,
Collections.emptyList(), MEDIUM, SslConfigs.SSL_CIPHER_SUITES_DOC)
+ .define(SslConfigs.SSL_CIPHER_SUITES_CONFIG, LIST, List.of(),
ConfigDef.ValidList.anyNonDuplicateValues(true, false), MEDIUM,
SslConfigs.SSL_CIPHER_SUITES_DOC)
.define(SslConfigs.SSL_ENGINE_FACTORY_CLASS_CONFIG, CLASS, null,
LOW, SslConfigs.SSL_ENGINE_FACTORY_CLASS_DOC)
// Sasl Configuration
.define(BrokerSecurityConfigs.SASL_MECHANISM_INTER_BROKER_PROTOCOL_CONFIG,
STRING, SaslConfigs.DEFAULT_SASL_MECHANISM, MEDIUM,
BrokerSecurityConfigs.SASL_MECHANISM_INTER_BROKER_PROTOCOL_DOC)
- .define(BrokerSecurityConfigs.SASL_ENABLED_MECHANISMS_CONFIG,
LIST, BrokerSecurityConfigs.DEFAULT_SASL_ENABLED_MECHANISMS, MEDIUM,
BrokerSecurityConfigs.SASL_ENABLED_MECHANISMS_DOC)
+ .define(BrokerSecurityConfigs.SASL_ENABLED_MECHANISMS_CONFIG,
LIST, BrokerSecurityConfigs.DEFAULT_SASL_ENABLED_MECHANISMS,
ConfigDef.ValidList.anyNonDuplicateValues(false, false), MEDIUM,
BrokerSecurityConfigs.SASL_ENABLED_MECHANISMS_DOC)
Review Comment:
It seems that an empty list was allowed for this one before. For example, if
the security protocol is not sasl, it's ok to set this one to empty? Ditto for
SASL_KERBEROS_PRINCIPAL_TO_LOCAL_RULES_CONFIG.
##########
clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java:
##########
@@ -536,6 +539,14 @@ Object parseValue(ConfigKey key, Object value, boolean
isSet) {
// otherwise assign setting its default value
parsedValue = key.defaultValue;
}
+ if (key.validator instanceof ValidList && parsedValue instanceof List)
{
+ List<?> originalListValue = (List<?>) parsedValue;
+ parsedValue =
originalListValue.stream().distinct().collect(Collectors.toList());
+ if (originalListValue.size() != ((List<?>) parsedValue).size()) {
Review Comment:
Could we add a test to verify that duplicates are removed when retrieving
the config value?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
