Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KafkaProperties.buildProperties() calls validate before kafkaProducerFactory uses customizers that can still change the validated properties. #41126

Closed
geertvb opened this issue Jun 17, 2024 · 6 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@geertvb
Copy link

geertvb commented Jun 17, 2024

The default kafka producer factory bean gets kafka properties by calling KafkaProperties.buildProducerProperties method.

@Bean
@ConditionalOnMissingBean(ProducerFactory.class)
public DefaultKafkaProducerFactory<?, ?> kafkaProducerFactory(KafkaConnectionDetails connectionDetails,
	ObjectProvider<DefaultKafkaProducerFactoryCustomizer> customizers, ObjectProvider<SslBundles> sslBundles) {
  Map<String, Object> properties = this.properties.buildProducerProperties(sslBundles.getIfAvailable());
  ...
}

Behind the scenes this calls Ssl.buildProperties method

public Map<String, Object> buildProperties(SslBundles sslBundles) {
  validate();
  ...

which calls validate() and prematurely validates some things

private void validate() {
  MutuallyExclusiveConfigurationPropertiesException.throwIfMultipleNonNullValuesIn((entries) -> {
    entries.put("spring.kafka.ssl.key-store-key", getKeyStoreKey());
    entries.put("spring.kafka.ssl.key-store-location", getKeyStoreLocation());
  });

but these properties can still be added, modified or deleted by the customizers in the kafkaProducerFactory method

@Bean
@ConditionalOnMissingBean(ProducerFactory.class)
public DefaultKafkaProducerFactory<?, ?> kafkaProducerFactory(KafkaConnectionDetails connectionDetails,
	ObjectProvider<DefaultKafkaProducerFactoryCustomizer> customizers, ObjectProvider<SslBundles> sslBundles) {
  ...
  customizers.orderedStream().forEach((customizer) -> customizer.customize(factory));
  return factory;
}

These validations should be done after the customizers.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 17, 2024
@wilkinsona
Copy link
Member

Thanks for the report but I'm afraid I don't think I understand the problem. How can a DefaultKafkaProducerFactoryCustomizer change the properties that have been bound to KafkaProperties? Are you suggesting that the changes made to the producer's configs by any customizer should be mapped back to KafkaProperties and then the validation should be performed?

@wilkinsona wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label Jun 17, 2024
@geertvb
Copy link
Author

geertvb commented Jun 17, 2024

The properties used by kafka producer factory is a combination of the properties generated by the buildProperties method and some customizations. The final set of properties should be validated. The properties set produced by buildProperties may not be valid but after the customizations are applied it may and vice versa.
I will provide a minimal example if you prefer.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 17, 2024
@wilkinsona
Copy link
Member

The properties set produced by buildProperties may not be valid but after the customizations are applied it may and vice versa.

The intention of the customization is that it allows you to configure things that cannot be configured using the properties. If you're partially configuring something using properties and then completing and correcting it using a customizer, we'd recommend configuring it all using properties or all using a customizer.

I will provide a minimal example if you prefer.

Yes please, it may demonstrate a situation where a split approach is required. If it does, we may want to consider some additional properties to avoid the need.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jun 17, 2024
@geertvb
Copy link
Author

geertvb commented Jun 17, 2024

Regardless of the intention of the customizer, the best moment to validate the state of a constructed bean is when all configuration has been applied. I consider other approaches a workaround and we already have a workaround on our side.
I just wanted to report this conceptual problem.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 17, 2024
@wilkinsona
Copy link
Member

Regardless of the intention of the customizer, the best moment to validate the state of a constructed bean is when all configuration has been applied

That's what we're doing. The validation is being applied to KafkaProperties at a time when all properties have been bound to it.

I just wanted to report this conceptual problem.

Thanks. I don't think such a problem exists as there's some separation between the properties and their validation and the customization of a producer factory so I'll close this one.

@wilkinsona wilkinsona closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2024
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Jun 17, 2024
@geertvb
Copy link
Author

geertvb commented Jun 17, 2024

KafkaProperties is not a (real) bean, just a properties container/helper. Validation should be at the right moment in the kafkaProducerFactory bean its lifecycle (e.g. afterPropertiesSet)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

3 participants