KAFKA-13328, KAFKA-13329 (1): Add preflight validations for key, value, and header converter classes #14304

C0urante · 2023-08-28T20:26:33Z

Depends on #14303

Adds preflight validation checks for key, value, and header converter classes specified in connector configurations. For each converter type, these checks verify that:

The class has a public, no-args constructor
The class can be instantiated without an exception being thrown
The class is not abstract
- In this case, a list of possible child classes is provided in a user-friendly error message
The class implements the expected interface

There are two components to the linked Jira tickets: this validation (which is fairly straightforward), and more sophisticated logic that leverages ConfigDef objects provided by these plugin classes. For ease of review, I'm planning on filing two PRs for these tickets, but instead of grouping by ticket (which would include all key/value converter changes in one ticket and all header converter changes in another), I think it might be easier to group by the kind of validation we're performing. If that's acceptable, the next PR will add ConfigDef-based validation for key, value, and header converters.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

C0urante · 2023-08-29T19:54:26Z

clients/src/main/java/org/apache/kafka/common/utils/Utils.java

+     * it would be the caller's responsibility to ensure that {@code task} was non-null before attempting to
+     * use a method reference from it.
+     */
+    public static void maybeCloseQuietly(Object maybeCloseable, String name) {


This is overkill for a single use (which is the case in this PR), but a second use case is added for it in #14309.

I found more use-cases for this today, so 👍 on adding this helper.

yashmayya

Thanks Chris, this is another really nice improvement! I just had a few comments and questions.

clients/src/main/java/org/apache/kafka/common/utils/Utils.java

connect/runtime/src/main/java/org/apache/kafka/connect/util/ConcreteSubClassValidator.java

connect/runtime/src/main/java/org/apache/kafka/connect/runtime/ConnectorConfig.java

connect/runtime/src/main/java/org/apache/kafka/connect/util/InstantiableClassValidator.java

gharris1727

We have all of the checks already, why didn't we add the validators! 🙃

Thanks for this, Chris.

gharris1727 · 2023-08-31T23:50:10Z

clients/src/main/java/org/apache/kafka/common/utils/Utils.java

+            String message = Utils.isBlank(childClassNames) ?
+                    name + " is abstract and cannot be created." :
+                    name + " is abstract and cannot be created. Did you mean " + childClassNames + "?";
+            throw new ConfigException(name, cls.getName(), message);


The first argument to this exception should be the config name, not the type name like it's used in the exception message and in the call-sites.

🤦 Thanks, good catch.

In pursuit of simplicity I've taken a stab at using the single-arg ConfigDef constructor, and using a one-size-fits-all error message that just says "This class" instead of, e.g., "Transform". Since all current uses for this code path involve (directly or indirectly) hitting a ConfigDef.Validator as part of config validation, I think the user-facing impact should only go as far as the small wording change. Let me know if you think it's better to preserve existing behavior, though.

I like this simplification, and it makes sense to generalize the error message as part of promoting this to the Utils class. parseForValidate will still attribute the error to the correct key/value.

gharris1727 · 2023-08-31T23:54:55Z

clients/src/main/java/org/apache/kafka/common/utils/Utils.java

+            name = "Class";
+        if (Modifier.isAbstract(cls.getModifiers())) {
+            String childClassNames = Stream.of(cls.getClasses())
+                    .filter(cls::isAssignableFrom)


If AbstractClass.Key implemented the base interface, but didn't extend AbstractClass, this wouldn't show up in this listing. If this was baseClass::isAssignableFrom, it would find classes like that.

I don't know how often people would create Key and Value inner classes without subclassing the outer class, since that seems to be the reason people use the inner classes in the first place. But if you change this to take the baseClass and check baseClass.isAssignableFrom(cls) like both of the call-sites, you could change this filter statement too. I don't think this is a big deal, so feel free to leave this implementation as-is.

I took a stab at something that would accept a base class and relax the filtering logic here accordingly, but it didn't quite feel right since we don't do verification that cls extends baseClass. I guess we could add that check as well and have something like Utils::ensureConcreteSubclass--how does that sound? I'm on the fence about it potentially doing too much for a utility method, but it does provide user-facing advantages.

I guess we could add that check as well and have something like Utils::ensureConcreteSubclass--how does that sound?

Yeah I imagine a ensureConcreteSubclass(Class<?> cls, Class<?> baseClass) that:

asserts that cls is concrete and baseClass.isAssignableFrom(cls) is true

if not valid, suggests child classes that are concrete andbaseClass::isAssignableFrom is true.

I don't think that it would compose well with the current ensureConcrete method though, so it would probably replace what is there already.

I'm on the fence about it potentially doing too much for a utility method, but it does provide user-facing advantages.

I thought that about the child class suggestion logic, but relented for the same reason. It significantly complicates this method which could otherwise be a single if condition, but that complication could help someone quickly resolve an obvious typo.

I don't think that asserting baseClass.isAssignableFrom(cls) is too much for this utility method, because we should always have a baseClass in mind when defining a CLASS configuration. Users of the AbstractConfig::getConfiguredInstance methods already need to provide a baseClass for subclass checking and type safety at runtime, this utility method would help users to remember the same check at validation time.

And worst-case, someone can just specify Object as the baseClass, and effectively disable the baseClass filtering.

connect/runtime/src/main/java/org/apache/kafka/connect/util/InstantiableClassValidator.java

…verter classes

…classes

Co-authored-by: Yash Mayya <yash.mayya@gmail.com>

… common logic into Utils::ensureConcrete

…CloseQuietly

…ableClassValidator::ensureValid

C0urante · 2023-10-31T18:54:41Z

@gharris1727 sorry for the delay, know it's been a minute! I've implemented your suggestions with the latest commit. This should be ready for another round whenever you have time.

gharris1727

LGTM!

C0urante · 2023-12-08T20:00:10Z

Thanks Greg!

…e, and header converter classes (apache#14304) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Greg Harris <greg.harris@aiven.io>

…e, and header converter classes (apache#14304) (#403) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Greg Harris <greg.harris@aiven.io> Co-authored-by: Chris Egerton <chrise@aiven.io>

…e, and header converter classes (apache#14304) Reviewers: Yash Mayya <yash.mayya@gmail.com>, Greg Harris <greg.harris@aiven.io>

C0urante added the connect label Aug 28, 2023

C0urante mentioned this pull request Aug 29, 2023

KAFKA-13327: Gracefully report connector validation errors instead of returning 500 responses #14303

Merged

3 tasks

C0urante force-pushed the kafka-13328-13329-class-validators branch 2 times, most recently from 9f9e2a6 to 89deb99 Compare August 29, 2023 19:46

C0urante mentioned this pull request Aug 29, 2023

KAFKA-13328, KAFKA-13329 (2): Add custom preflight validation support for connector header, key, and value converters #14309

Open

3 tasks

C0urante commented Aug 29, 2023

View reviewed changes

C0urante mentioned this pull request Aug 29, 2023

KAFKA-13327, KAFKA-13328, KAFKA-13329: Clean up preflight connector validation #11369

Closed

3 tasks

yashmayya reviewed Aug 30, 2023

View reviewed changes

gharris1727 reviewed Sep 1, 2023

View reviewed changes

C0urante and others added 8 commits October 31, 2023 12:42

KAFKA-13329: Add preflight validation for connector key and value con…

840f2c9

…verter classes

KAFKA-13328: Add preflight validation for connector header converter …

ac9caf8

…classes

Add utility to close Object instances that may be AutoCloseable

dba0649

Update clients/src/main/java/org/apache/kafka/common/utils/Utils.java

abdc024

Co-authored-by: Yash Mayya <yash.mayya@gmail.com>

Add newline to end of file, fix Javadoc code snippet example, extract…

1d10efa

… common logic into Utils::ensureConcrete

Remove warning about method references from Javadocs for Utils::maybe…

735d35f

…CloseQuietly

Simplify Utils::ensureConcrete, refine exception handling in Instanti…

a554695

…ableClassValidator::ensureValid

Fix failing unit tests

8dfeca5

C0urante force-pushed the kafka-13328-13329-class-validators branch from 95a0ee8 to 8dfeca5 Compare October 31, 2023 16:43

Change Utils::ensureConcrete to Utils::ensureConcreteSubclass

6cf1e13

gharris1727 approved these changes Dec 7, 2023

View reviewed changes

C0urante merged commit dc857fb into apache:trunk Dec 8, 2023
1 check failed

C0urante deleted the kafka-13328-13329-class-validators branch December 8, 2023 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-13328, KAFKA-13329 (1): Add preflight validations for key, value, and header converter classes #14304

KAFKA-13328, KAFKA-13329 (1): Add preflight validations for key, value, and header converter classes #14304

C0urante commented Aug 28, 2023 •

edited

C0urante Aug 29, 2023

gharris1727 Sep 5, 2023

yashmayya left a comment

gharris1727 left a comment

gharris1727 Aug 31, 2023

C0urante Sep 5, 2023

gharris1727 Sep 5, 2023

gharris1727 Aug 31, 2023

C0urante Sep 5, 2023

gharris1727 Sep 5, 2023

C0urante commented Oct 31, 2023

gharris1727 left a comment

C0urante commented Dec 8, 2023

KAFKA-13328, KAFKA-13329 (1): Add preflight validations for key, value, and header converter classes #14304

KAFKA-13328, KAFKA-13329 (1): Add preflight validations for key, value, and header converter classes #14304

Conversation

C0urante commented Aug 28, 2023 • edited

Committer Checklist (excluded from commit message)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yashmayya left a comment

Choose a reason for hiding this comment

gharris1727 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

C0urante commented Oct 31, 2023

gharris1727 left a comment

Choose a reason for hiding this comment

C0urante commented Dec 8, 2023

C0urante commented Aug 28, 2023 •

edited