KAFKA-13793: Add validators for configs that lack validators by RivenSun2 · Pull Request #12010 · apache/kafka

RivenSun2 · 2022-04-07T09:13:26Z

Add validators for configs that lack validators

… cpu and traffic on the broker side increase sharply JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

…sets method JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

2. Optimize the import of package Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

add test Method "testForceMetadataDeleteForPatternSubscriptionDuringRebalance()" Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Author: RivenSun2 riven.sun@zoom.us

� Conflicts: � clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java

RivenSun2 · 2022-04-20T15:26:05Z

Hi @C0urante @divijvaidya
After some thought, we can remove the validator added to the (de)serializer configuration.
Add validation in append(De)serializerToConfig method. It can be compatible with the almost unused cases mentioned above, and can also add verification to the (de)serializer configuration.
I will quickly do it later.
Thanks.

RivenSun2 · 2022-04-21T00:27:39Z

Hi @showuon @C0urante @divijvaidya
Recently resubmitted the changes, if you have time, please help to review it again.
Thanks a lot.

RivenSun2 · 2022-04-22T01:25:09Z

Hi @showuon @guozhangwang
Could you help to review this PR?
Thanks.

C0urante

@RivenSun2 it's fine to modify the CommonClientConfigs class without a KIP.

C0urante · 2022-04-22T01:34:22Z

+        else {
+            if (newConfigs.get(KEY_DESERIALIZER_CLASS_CONFIG) == null)
+                throw new ConfigException(KEY_DESERIALIZER_CLASS_CONFIG + " configuration for KafkaConsumer must be non-null.");
+        }


Suggested change

else {

if (newConfigs.get(KEY_DESERIALIZER_CLASS_CONFIG) == null)

throw new ConfigException(KEY_DESERIALIZER_CLASS_CONFIG + " configuration for KafkaConsumer must be non-null.");

}

else if (newConfigs.get(KEY_DESERIALIZER_CLASS_CONFIG) == null)

throw new ConfigException(KEY_DESERIALIZER_CLASS_CONFIG, null, "must be non-null.");

C0urante · 2022-04-22T01:34:47Z

+        else {
+            if (newConfigs.get(VALUE_DESERIALIZER_CLASS_CONFIG) == null)
+                throw new ConfigException(VALUE_DESERIALIZER_CLASS_CONFIG + " configuration for KafkaConsumer must be non-null.");
+        }


Suggested change

else {

if (newConfigs.get(VALUE_DESERIALIZER_CLASS_CONFIG) == null)

throw new ConfigException(VALUE_DESERIALIZER_CLASS_CONFIG + " configuration for KafkaConsumer must be non-null.");

}

else if (newConfigs.get(VALUE_DESERIALIZER_CLASS_CONFIG) == null)

throw new ConfigException(VALUE_DESERIALIZER_CLASS_CONFIG, null, "must be non-null.");

clolov · 2022-04-22T02:16:15Z

        if (keySerializer != null)
            newConfigs.put(KEY_SERIALIZER_CLASS_CONFIG, keySerializer.getClass());
+        else {
+            if (newConfigs.get(KEY_SERIALIZER_CLASS_CONFIG) == null)


Similar to what Chris Egerton has suggested above these can also be collapsed into else-if

clolov · 2022-04-22T02:20:47Z

        if (valueSerializer != null)
            newConfigs.put(VALUE_SERIALIZER_CLASS_CONFIG, valueSerializer.getClass());
+        else {
+            if (newConfigs.get(VALUE_SERIALIZER_CLASS_CONFIG) == null)


Can you use HashMap#containsKey if checking for the presence of a value or am I misreading something?

Thank you for your suggestion, here we want to check the case where the key exists but the value is null.

Oh, I see. So this check covers the situations a) the key is missing and b) the key is there and the value is null, while what I suggested would only cover a). Is there a possibility that you add a comment saying this above the line?

clolov · 2022-04-22T02:25:11Z


      /************* Authorizer Configuration ***********/
-      .define(AuthorizerClassNameProp, STRING, Defaults.AuthorizerClassName, LOW, AuthorizerClassNameDoc)
+      .define(AuthorizerClassNameProp, STRING, Defaults.AuthorizerClassName, new ConfigDef.NonNullValidator(), LOW, AuthorizerClassNameDoc)


I think Scala allows you to just write ConfigDef.NonNullValidator without the new keyword and the (), but I might be wrong.

This should be fine, I see the same in this file : new RaftConfig.ControllerQuorumVotersValidator()

Okay, sounds sensible.

Can a STRING configuration be null? I wonder if this validator can actually trigger

clolov · 2022-04-22T02:34:40Z

          CommonClientConfigs.SECURITY_PROTOCOL_CONFIG,
          ConfigDef.Type.STRING,
          CommonClientConfigs.DEFAULT_SECURITY_PROTOCOL,
+          in(Utils.enumOptions(classOf[SecurityProtocol]):_*),


This is very interesting. I got thrown off by reading this after seeing in(Utils.enumOptions(SecurityProtocol.class)) because I couldn't understand why do we unpack the arguments in one of the situations, but not in the other. I suspect this is because in the Java files we don't use the Scala in function?

Actually referring to the ConfigDef.ValidString.in(String... validStrings) method.

Ah, I see. The Scala file imports the in method

kafka/core/src/main/scala/kafka/admin/BrokerApiVersionsCommand.scala

Line 32 in d47c759

import org.apache.kafka.common.config.ConfigDef.ValidString._

. Then I am confused - why do we need to unpack the arguments to this method in the Scala files, but not unpack them in the Java ones?

Maybe Scala does not support indefinite parameters, that is String... param

Fascinating, I wasn't aware of this. Okay, this makes sense as well.

RivenSun2 · 2022-04-22T03:49:34Z

Hi @C0urante @clolov
if you have time, please help to review it again.
Thanks a lot.

C0urante

LGTM! Thanks @RivenSun2, feel free to address the nits at your discretion.

I haven't taken a close look at any of the broker or Streams changes, but Connect+clients seem solid.

C0urante · 2022-04-22T12:52:04Z

+        if (securityProtocol == SecurityProtocol.SASL_PLAINTEXT || securityProtocol == SecurityProtocol.SASL_SSL) {
+            if (clientSaslMechanism == null || clientSaslMechanism.trim().isEmpty()) {
+                throw new ConfigException(SaslConfigs.SASL_MECHANISM, null, "When the " + CommonClientConfigs.SECURITY_PROTOCOL_CONFIG +
+                        " configuration enables the SASL mechanism, mechanism must be non-null or non-empty string.");


Nit:

Suggested change

" configuration enables the SASL mechanism, mechanism must be non-null or non-empty string.");

" configuration enables SASL, mechanism must be non-null and non-empty");

C0urante · 2022-04-22T12:53:27Z


    @Override
    protected Map<String, Object> postProcessParsedConfig(final Map<String, Object> parsedValues) {
+        CommonClientConfigs.postValidateSaslMechanismConfig(this);


This is pretty smooth--you've only added one line to an existing method in this class, and we now get intelligent, conditional validation of the SASL mechanism config. Nice job!

C0urante · 2022-04-22T12:57:22Z

+        configs.put(SaslConfigs.SASL_MECHANISM, " ");
+        ce = assertThrows(ConfigException.class, () -> new AdminClientConfig(configs));
+        assertTrue(ce.getMessage().contains(SaslConfigs.SASL_MECHANISM));


Nit: I don't think it's worth adding this test case. At this point we're really verifying the behavior of the AbstractConfig and ConfigDef classes and how they automatically trim strings, not any special logic in the AdminClient class itself.

Yes, agree with you, this configuration validation is not the behavior of AdminClient. But I hope to follow ProducerConfigTest or ConsumerConfigTest and hand over this configuration verification to AdminClientConfigTest

I agree with @C0urante, we don't need to have that test case in all these classes.

Can we also remove it from ConsumerConfig and ProducerConfig?

C0urante · 2022-04-22T12:59:55Z

+        SecurityProtocol securityProtocol = SecurityProtocol.forName(config.getString(CommonClientConfigs.SECURITY_PROTOCOL_CONFIG));
+        String clientSaslMechanism = config.getString(SaslConfigs.SASL_MECHANISM);
+        if (securityProtocol == SecurityProtocol.SASL_PLAINTEXT || securityProtocol == SecurityProtocol.SASL_SSL) {
+            if (clientSaslMechanism == null || clientSaslMechanism.trim().isEmpty()) {


Nit: We don't need to trim here, do we?

Suggested change

if (clientSaslMechanism == null || clientSaslMechanism.trim().isEmpty()) {

if (clientSaslMechanism == null || clientSaslMechanism.isEmpty()) {

Strings containing only spaces are also considered illegal values, as indicated in the corresponding testCase.

configs.put(SaslConfigs.SASL_MECHANISM, " "); ce = assertThrows(ConfigException.class, () -> new ConsumerConfig(configs));

Isn't the string automatically trimmed here by the AbstractConfig?

Oh, you are right. The ConfigDef.parseType method already does this logic.
Thanks.

RivenSun2 · 2022-04-23T14:17:14Z

Hi @showuon @guozhangwang
could you help to review this PR?
Thanks.

RivenSun2 · 2022-04-25T01:03:26Z

Hi @showuon @guozhangwang
could you help to review this PR?
Thanks.

RivenSun2 · 2022-04-28T04:01:36Z

Hi @guozhangwang @mimaison
could you help to review this PR?
Thanks.

mimaison

Thanks for the PR, it's a nice cleanup. I left a few comments.

mimaison · 2022-05-03T15:03:07Z

    protected static Map<String, Object> appendDeserializerToConfig(Map<String, Object> configs,
                                                                    Deserializer<?> keyDeserializer,
                                                                    Deserializer<?> valueDeserializer) {
+        // validate deserializer configuration, if the passed deserializer instance is null, the user must explicitly set a valid deserializer configuration value


Do we need these extra checks?
If I don't set serdes with trunk, I get an error:

Exception in thread "main" org.apache.kafka.common.config.ConfigException: Missing required configuration "key.deserializer" which has no default value. at org.apache.kafka.common.config.ConfigDef.parseValue(ConfigDef.java:493) at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:483) at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:113) at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:133) at org.apache.kafka.clients.consumer.ConsumerConfig.<init>(ConsumerConfig.java:641) at org.apache.kafka.clients.consumer.KafkaConsumer.<init>(KafkaConsumer.java:666)

AbstractConfig should enforce each field has a valid value. We need to perform extra validation when we need multiple fields to have compatible values.

The situation you describe should be that the key.deserializer parameter is not set.

The changes here are to deal with two situations:

As described in JIRA, when the user explicitly sets null, expect a ConfigException instead of a NullPointerException.
eg: props.put("key.serializer", null);

In order to be compatible with the usage scenarios of some users, as @C0urante mentioned, the user will only pass in the serializer instance when building the KafkaProducer:

I'm wondering if people might be constructing ProducerConfig and ConsumerConfig instances in order to do some kind of validation before storing a configuration somewhere, that's then used later to create a producer/consumer. If so, they might be omitting the (de)serializer properties (or rather, just using null as a placeholder) if the (de)serializer isn't known at that time and they plan on using the constructor that accepts already-instantiated Serializer/Deserializer instances.

mimaison · 2022-05-03T18:23:48Z

+        configs.put(SaslConfigs.SASL_MECHANISM, " ");
+        ce = assertThrows(ConfigException.class, () -> new AdminClientConfig(configs));
+        assertTrue(ce.getMessage().contains(SaslConfigs.SASL_MECHANISM));


I agree with @C0urante, we don't need to have that test case in all these classes.

mimaison · 2022-05-03T18:40:32Z

            "a.bootstrap.servers", "servers-one",
            "b.bootstrap.servers", "servers-two",
-            "security.protocol", "SASL",
+            "security.protocol", "SSL",


We currently don't have checks in any of the *Config classes (this is what this PR is addressing). But obviously when creating an actual client, the value of security.protocol is used and only at that point you get an exception with a bad value is set. So MirrorMaker can't run with security.protocol=SASL.

For MirrorMaker, you get:

[2022-05-03 20:32:41,008] ERROR Stopping due to error (org.apache.kafka.connect.mirror.MirrorMaker:313) org.apache.kafka.common.KafkaException: Failed to create new KafkaAdminClient at org.apache.kafka.clients.admin.KafkaAdminClient.createInternal(KafkaAdminClient.java:545) at org.apache.kafka.clients.admin.Admin.create(Admin.java:143) at org.apache.kafka.connect.util.ConnectUtils.lookupKafkaClusterId(ConnectUtils.java:52) at org.apache.kafka.connect.mirror.MirrorMaker.addHerder(MirrorMaker.java:236) at java.base/java.lang.Iterable.forEach(Iterable.java:75) at org.apache.kafka.connect.mirror.MirrorMaker.<init>(MirrorMaker.java:137) at org.apache.kafka.connect.mirror.MirrorMaker.<init>(MirrorMaker.java:149) at org.apache.kafka.connect.mirror.MirrorMaker.main(MirrorMaker.java:300) Caused by: java.lang.IllegalArgumentException: No enum constant org.apache.kafka.common.security.auth.SecurityProtocol.SASL at java.base/java.lang.Enum.valueOf(Enum.java:240) at org.apache.kafka.common.security.auth.SecurityProtocol.valueOf(SecurityProtocol.java:26) at org.apache.kafka.common.security.auth.SecurityProtocol.forName(SecurityProtocol.java:72) at org.apache.kafka.clients.ClientUtils.createChannelBuilder(ClientUtils.java:103) at org.apache.kafka.clients.admin.KafkaAdminClient.createInternal(KafkaAdminClient.java:516)

With this PR, you would get:

[2022-05-03 20:38:13,763] ERROR Stopping due to error (org.apache.kafka.connect.mirror.MirrorMaker:313) org.apache.kafka.common.config.ConfigException: Invalid value SASL for configuration security.protocol: String must be one of: PLAINTEXT, SSL, SASL_PLAINTEXT, SASL_SSL at org.apache.kafka.common.config.ConfigDef$ValidString.ensureValid(ConfigDef.java:961) at org.apache.kafka.common.config.ConfigDef.parseValue(ConfigDef.java:499) at org.apache.kafka.common.config.ConfigDef.parse(ConfigDef.java:483) at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:113) at org.apache.kafka.common.config.AbstractConfig.<init>(AbstractConfig.java:146) at org.apache.kafka.clients.admin.AdminClientConfig.<init>(AdminClientConfig.java:234) at org.apache.kafka.clients.admin.Admin.create(Admin.java:143) at org.apache.kafka.connect.util.ConnectUtils.lookupKafkaClusterId(ConnectUtils.java:52) at org.apache.kafka.connect.mirror.MirrorMaker.addHerder(MirrorMaker.java:236) at java.base/java.lang.Iterable.forEach(Iterable.java:75) at org.apache.kafka.connect.mirror.MirrorMaker.<init>(MirrorMaker.java:137) at org.apache.kafka.connect.mirror.MirrorMaker.<init>(MirrorMaker.java:149) at org.apache.kafka.connect.mirror.MirrorMaker.main(MirrorMaker.java:300)

mimaison · 2022-05-03T19:13:38Z


      /************* Authorizer Configuration ***********/
-      .define(AuthorizerClassNameProp, STRING, Defaults.AuthorizerClassName, LOW, AuthorizerClassNameDoc)
+      .define(AuthorizerClassNameProp, STRING, Defaults.AuthorizerClassName, new ConfigDef.NonNullValidator(), LOW, AuthorizerClassNameDoc)


Can a STRING configuration be null? I wonder if this validator can actually trigger

RivenSun2 · 2022-05-03T23:40:23Z

Can a STRING configuration be null? I wonder if this validator can actually trigger

Adding NonNullValidator here may only constrain code writing. For example, when we construct KafkaConfig with code, as in testCase: testInvalidAuthorizerClassName, this parameter cannot be explicitly set to null.

� Conflicts: � core/src/main/scala/kafka/server/KafkaConfig.scala

RivenSun2 · 2022-05-04T00:14:41Z

Hi @mimaison Thank you for your review.
I just resubmitted the changes, please help to check again.
Thanks.

RivenSun2 · 2022-05-06T10:36:10Z

Hi @mimaison Thank you for your review.
I just add common testCase, please help to check again.
Thanks.

mimaison · 2022-05-06T16:46:15Z

    }
+
+    @Test
+    public void testInvalidSaslMechanism() {


This is testing SaslConfigs.SASL_MECHANISM, so would it be better to move this test in SaslConfigsTest

If we move this testCase to SaslConfigsTest, the checkstyle of SaslConfigsTest will fail.
Disallowed import - org.apache.kafka.clients.CommonClientConfigs.[ImportControl]

You can modify this file to prevent those errors.

Actually since this is testing logic that is in CommonClientConfigs, maybe it's better to keep this here. Otherwise we'd move postValidateSaslMechanismConfig() in SaslConfigs

Thanks @C0urante and @mimaison .
I also think it's better to keep this here.

mimaison · 2022-05-06T16:47:20Z

+        ce = assertThrows(ConfigException.class, () -> new TestConfig(configs));
+        assertTrue(ce.getMessage().contains(SaslConfigs.SASL_MECHANISM));
+
+        configs.put(SaslConfigs.SASL_MECHANISM, " ");


Let's also drop this last case. This is testing AbstractConfig and it seems unrelated to the rest of this PR.

mimaison

Thanks for the PR, LGTM

RivenSun2 and others added 30 commits September 19, 2021 17:59

fix issue : KafkaConsumer cannot jump out of the poll method, and the…

4169106

… cpu and traffic on the broker side increase sharply JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

fix this issue for AUTO_PATTERN and AUTO_TOPICS in cleanUpConsumedOff…

cd84dbc

…sets method JIRA link : https://issues.apache.org/jira/browse/KAFKA-13310 Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

1. fix NPE issue

39ee67a

2. Optimize the import of package Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit changeCodes by showuon review

9aa7bd7

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

fix package auto-import issue by IDEA Editor

5e09d03

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit reviewed codeChange

8b1382a

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit reviewed codeChange

7346e88

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

commit test code

0a77af1

add test Method "testForceMetadataDeleteForPatternSubscriptionDuringRebalance()" Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

fix merge conflicts

9e544b2

Author: RivenSun2 <riven.sun@zoom.us> Reviewers: Luke Chen <showuon@gmail.com>

Merge branch 'apache:trunk' into trunk

3334bc5

commit code changes for review

a5f6f81

Author: RivenSun2 riven.sun@zoom.us

commit code changes for showuon's review

12359c2

commit code changes for showuon's review

359af7d

commit code changes for guozhangwang's review

82b3585

fix issue for ConsumerBounceTest testClose case failed

a98bd87

fix issue for KafkaConsumerTest testCase failed

efef844

fix issue for testCloseManualAssignment case failed

12fb32c

LeaveGroupResponse should be prepared conditionally

996b8d9

fix issue for testInvalidGroupMetadata case

de1580c

Merge branch 'apache:trunk' into trunk

587a2bd

Merge branch 'trunk' of https://github.com/apache/kafka into trunk

c5e7595

� Conflicts: � clients/src/test/java/org/apache/kafka/clients/consumer/KafkaConsumerTest.java

branch conflicts resolved

f78969c

in order to retrigger jenkins tests

e0ab2e5

Merge branch 'apache:trunk' into trunk

8b8c78f

Merge branch 'apache:trunk' into trunk

282692f

Merge branch 'apache:trunk' into trunk

e7c66e0

Merge branch 'apache:trunk' into trunk

77718c0

Merge branch 'apache:trunk' into trunk

d7b986e

Merge branch 'apache:trunk' into trunk

3006625

Merge branch 'apache:trunk' into trunk

7788665

RivenSun2 added 2 commits April 21, 2022 00:01

commit code change

a052478

commit code change

d47c759

C0urante reviewed Apr 22, 2022

View reviewed changes

clolov reviewed Apr 22, 2022

View reviewed changes

commit code change for review

7b1d25c

C0urante approved these changes Apr 22, 2022

View reviewed changes

commit code change for C0urante's review

e215c8f

RivenSun2 requested a review from showuon April 25, 2022 01:02

mimaison reviewed May 3, 2022

View reviewed changes

RivenSun2 added 2 commits May 4, 2022 07:48

Merge branch 'trunk' of https://github.com/apache/kafka into KAFKA-13793

081cea5

� Conflicts: � core/src/main/scala/kafka/server/KafkaConfig.scala

commit code changes for mimaison's review

a9bba88

add common testCase testInvalidSaslMechanism

c582e0e

mimaison reviewed May 6, 2022

View reviewed changes

remove unrelated case

9eb0669

mimaison approved these changes May 9, 2022

View reviewed changes

mimaison merged commit df507e5 into apache:trunk May 9, 2022

	" configuration enables the SASL mechanism, mechanism must be non-null or non-empty string.");
	" configuration enables SASL, mechanism must be non-null and non-empty");

	if (clientSaslMechanism == null \|\| clientSaslMechanism.trim().isEmpty()) {
	if (clientSaslMechanism == null \|\| clientSaslMechanism.isEmpty()) {

Conversation

RivenSun2 commented Apr 7, 2022

Uh oh!

RivenSun2 commented Apr 20, 2022

Uh oh!

RivenSun2 commented Apr 21, 2022

Uh oh!

RivenSun2 commented Apr 22, 2022

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mimaison May 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RivenSun2 commented Apr 22, 2022

Uh oh!

C0urante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mimaison May 3, 2022 •

edited

Loading

RivenSun2 May 3, 2022 •

edited

Loading

mimaison May 3, 2022 •

edited

Loading