You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/07/09 18:27:52 UTC
[GitHub] [kafka] dielhennr opened a new pull request #11011: KAFKA-13051: Require Principal Serde and add default
dielhennr opened a new pull request #11011:
URL: https://github.com/apache/kafka/pull/11011
Adding a default PrincipalBuilderClassProp and requiring that any custom config setting implements KafkaPrincipalSerde.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667216030
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1971,5 +1974,7 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
s"${KafkaConfig.FailedAuthenticationDelayMsProp}=$failedAuthenticationDelayMs should always be less than" +
s" ${KafkaConfig.ConnectionsMaxIdleMsProp}=$connectionsMaxIdleMs to prevent failed" +
s" authentication responses from timing out")
+
+ require(classOf[KafkaPrincipalSerde].isAssignableFrom(getClass(KafkaConfig.PrincipalBuilderClassProp)), "A principal serde must be defined")
Review comment:
I believe `isAssignableFrom` will throw NPE if the argument is null. Can we add another requirement for a non-null 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667506596
##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/DefaultKafkaPrincipalSerde.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.kafka.common.security.authenticator;
+
+import org.apache.kafka.common.errors.SerializationException;
+import org.apache.kafka.common.message.DefaultPrincipalData;
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.protocol.MessageUtil;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.security.auth.KafkaPrincipalSerde;
+
+import java.nio.ByteBuffer;
+
+public interface DefaultKafkaPrincipalSerde extends KafkaPrincipalSerde {
Review comment:
Is there a reason why we don't add these defaults to KafkaPrincipalSerde?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667164215
##########
File path: docs/upgrade.html
##########
@@ -21,6 +21,7 @@
<h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3.0.0</a></h5>
<ul>
+ <li>A Principal Serde defenition is now required. A default has been set for KafkaPrincipalBuilder which implements KafkaPrincipalSerde</li>
Review comment:
We can probably be more verbose. Maybe something like this:
> Custom principal builder implementations specified through <code>principal.builder.class</code> must now implement the <code>KafkaPrincipalSerde</code> interface to allow for forwarding between brokers. Additionally, the default value for this configuration has been changed to<code>org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder</code>, which was already the implicit behavior when no principal builder was defined. See <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller">KIP-590</a> for more details about the usage of <code>KafkaPrincipalSerde</code>.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r668258168
##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/DefaultKafkaPrincipalSerde.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.kafka.common.security.authenticator;
+
+import org.apache.kafka.common.errors.SerializationException;
+import org.apache.kafka.common.message.DefaultPrincipalData;
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.protocol.MessageUtil;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.security.auth.KafkaPrincipalSerde;
+
+import java.nio.ByteBuffer;
+
+public interface DefaultKafkaPrincipalSerde extends KafkaPrincipalSerde {
Review comment:
Fair enough. @dielhennr Can we move this back to where it was? I guess I don't see a problem letting the test principal builder implementations extend `DefaultKafkaPrincipalBuilder` directly.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667506307
##########
File path: docs/upgrade.html
##########
@@ -21,6 +21,10 @@
<h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3.0.0</a></h5>
<ul>
+ <li>Custom principal builder implementations specified through <code>principal.builder.class</code> must now implement the
+ <code>KafkaPrincipalSerde</code> interface to allow for forwarding between brokers. Additionally, the default value for this configuration has
+ been changed to <code>org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder</code>, which was already the implicit behavior
+ when no principal builder was defined. See <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller">KIP-590</a> for more details about the usage of KafkaPrincipalSerde.</li>
Review comment:
A couple of comments:
1. This should probably not be at the top of the list of changes since it only affects people who have custom principal builders.
2. The change in default doesn't seem worth mentioning since it was the behavior already. Is there any reason people would want to know this?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667164215
##########
File path: docs/upgrade.html
##########
@@ -21,6 +21,7 @@
<h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3.0.0</a></h5>
<ul>
+ <li>A Principal Serde defenition is now required. A default has been set for KafkaPrincipalBuilder which implements KafkaPrincipalSerde</li>
Review comment:
We can probably be more verbose. Maybe something like this:
> Custom principal builder implementations specified through <code>principal.builder.class</code> must now implement the <code>KafkaPrincipalSerde</code> interface to allow for forwarding between brokers. Additionally, the default value for this configuration has been changed to<code>org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder</code>, which was already the implicit behavior when no principal builder was defined. See <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller">KIP-590</a> for more details about the usage of <code>KafkaPrincipalSerde</code>
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r668098050
##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/DefaultKafkaPrincipalSerde.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.kafka.common.security.authenticator;
+
+import org.apache.kafka.common.errors.SerializationException;
+import org.apache.kafka.common.message.DefaultPrincipalData;
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.protocol.MessageUtil;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.security.auth.KafkaPrincipalSerde;
+
+import java.nio.ByteBuffer;
+
+public interface DefaultKafkaPrincipalSerde extends KafkaPrincipalSerde {
Review comment:
This is in a public package, so do we intend people to mix it in themselves? What are the pros/cons of this approach vs having the default implementation in `KafkaPrincipalSerde`?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dielhennr commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
dielhennr commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r668286722
##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/DefaultKafkaPrincipalSerde.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.kafka.common.security.authenticator;
+
+import org.apache.kafka.common.errors.SerializationException;
+import org.apache.kafka.common.message.DefaultPrincipalData;
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.protocol.MessageUtil;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.security.auth.KafkaPrincipalSerde;
+
+import java.nio.ByteBuffer;
+
+public interface DefaultKafkaPrincipalSerde extends KafkaPrincipalSerde {
Review comment:
@hachikuji Done
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #11011:
URL: https://github.com/apache/kafka/pull/11011
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667506307
##########
File path: docs/upgrade.html
##########
@@ -21,6 +21,10 @@
<h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3.0.0</a></h5>
<ul>
+ <li>Custom principal builder implementations specified through <code>principal.builder.class</code> must now implement the
+ <code>KafkaPrincipalSerde</code> interface to allow for forwarding between brokers. Additionally, the default value for this configuration has
+ been changed to <code>org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder</code>, which was already the implicit behavior
+ when no principal builder was defined. See <a href="https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller">KIP-590</a> for more details about the usage of KafkaPrincipalSerde.</li>
Review comment:
A couple of comments:
1. This should probably not be at the top of the list of changes since it only affects people who have custom principal builders.
2. The change in default doesn't seem worth mentioning since it was the behavior already.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667225066
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1975,6 +1975,8 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
s" ${KafkaConfig.ConnectionsMaxIdleMsProp}=$connectionsMaxIdleMs to prevent failed" +
s" authentication responses from timing out")
- require(classOf[KafkaPrincipalSerde].isAssignableFrom(getClass(KafkaConfig.PrincipalBuilderClassProp)), "A principal serde must be defined")
+ require(KafkaConfig.PrincipalBuilderClassProp != null, "principal.builder.class must be non-null")
Review comment:
We need to check `getClass(KafkaConfig.PrincipalBuilderClassProp))`, right? Maybe we can create a `val`.
```scala
val principalBuilderClass = getClass(KafkaConfig.PrincipalBuilderClassProp)
require(principalBuilderClass != null, "principal.builder.class must be defined")
require(classOf[KafkaPrincipalSerde].isAssignableFrom(principalBuilderClass),
"principal.builder.class must implement KafkaPrincipalSerde")
```
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r668258168
##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/DefaultKafkaPrincipalSerde.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.kafka.common.security.authenticator;
+
+import org.apache.kafka.common.errors.SerializationException;
+import org.apache.kafka.common.message.DefaultPrincipalData;
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.protocol.MessageUtil;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.security.auth.KafkaPrincipalSerde;
+
+import java.nio.ByteBuffer;
+
+public interface DefaultKafkaPrincipalSerde extends KafkaPrincipalSerde {
Review comment:
Fair enough. @dielhennr Can we move this back to where it was? I guess I don't see a problem letting the test principal builder implementations extend `KafkaPrincipalBuilder` directly.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667174963
##########
File path: docs/upgrade.html
##########
@@ -21,6 +21,7 @@
<h5><a id="upgrade_300_notable" href="#upgrade_300_notable">Notable changes in 3.0.0</a></h5>
<ul>
+ <li>Custom principal builder implementations specified through principal.builder.class must now implement the KafkaPrincipalSerde interface to allow for forwarding between brokers. Additionally, the default value for this configuration has been changed to org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder, which was already the implicit behavior when no principal builder was defined. See <a href=https://cwiki.apache.org/confluence/display/KAFKA/KIP-590%3A+Redirect+Zookeeper+Mutation+Protocols+to+The+Controller>KIP-590</a> for more details about the usage of KafkaPrincipalSerde.</li>
Review comment:
Can you use `<code>` tags around `principal.builder.class`, `KafkaPrincipalSerde`, and `org.apache.kafka.common.security.authenticator.DefaultKafkaPrincipalBuilder`?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r668056225
##########
File path: clients/src/main/java/org/apache/kafka/common/security/authenticator/DefaultKafkaPrincipalSerde.java
##########
@@ -0,0 +1,51 @@
+/*
+ * 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.kafka.common.security.authenticator;
+
+import org.apache.kafka.common.errors.SerializationException;
+import org.apache.kafka.common.message.DefaultPrincipalData;
+import org.apache.kafka.common.protocol.ByteBufferAccessor;
+import org.apache.kafka.common.protocol.MessageUtil;
+import org.apache.kafka.common.security.auth.KafkaPrincipal;
+import org.apache.kafka.common.security.auth.KafkaPrincipalSerde;
+
+import java.nio.ByteBuffer;
+
+public interface DefaultKafkaPrincipalSerde extends KafkaPrincipalSerde {
Review comment:
This was my doing. I was looking for an easy way to mix in the default implementation since we have a bunch of custom principal builders in the tests.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667216030
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1971,5 +1974,7 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
s"${KafkaConfig.FailedAuthenticationDelayMsProp}=$failedAuthenticationDelayMs should always be less than" +
s" ${KafkaConfig.ConnectionsMaxIdleMsProp}=$connectionsMaxIdleMs to prevent failed" +
s" authentication responses from timing out")
+
+ require(classOf[KafkaPrincipalSerde].isAssignableFrom(getClass(KafkaConfig.PrincipalBuilderClassProp)), "A principal serde must be defined")
Review comment:
I believe `isAssignableFrom` will throw NPE if the argument is null. Can we add another requirement for a non-null value so that we can give the user a nicer message? Also, it is useful if the error messages mention `principal.builder.class` explicitly.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #11011: KAFKA-13051: Require Principal Serde and add default
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #11011:
URL: https://github.com/apache/kafka/pull/11011#discussion_r667216030
##########
File path: core/src/main/scala/kafka/server/KafkaConfig.scala
##########
@@ -1971,5 +1974,7 @@ class KafkaConfig(val props: java.util.Map[_, _], doLog: Boolean, dynamicConfigO
s"${KafkaConfig.FailedAuthenticationDelayMsProp}=$failedAuthenticationDelayMs should always be less than" +
s" ${KafkaConfig.ConnectionsMaxIdleMsProp}=$connectionsMaxIdleMs to prevent failed" +
s" authentication responses from timing out")
+
+ require(classOf[KafkaPrincipalSerde].isAssignableFrom(getClass(KafkaConfig.PrincipalBuilderClassProp)), "A principal serde must be defined")
Review comment:
I believe `isAssignableFrom` will throw NPE if the argument is null. Can we add another requirement for a non-null value so that we can give the user a nicer message?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org