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

Bump up kafkaAvroSerde to support SSL for Schema Registry #1898

Merged
merged 6 commits into from
Sep 29, 2020

Conversation

themightylaz
Copy link
Contributor

fix: Support Datahub to connect to Kafka Schema Registry using SSL
Related to: #1861

Test result:

  1. Change kafkaAvroSerde to version io.confluent:kafka-streams-avro-serde:5.5.1
  2. Rebuilt app
  3. Ran COMPOSE_DOCKER_CLI_BUILD=1 DOCKER_BUILDKIT=1 docker-compose -p datahub build
  4. Pushed to dockerhub
  5. Deployed to our AWS EKS cluster using Helm
  6. Verify that Datahub can connect to Confluent Schema Registry using SSL
  7. Opened Datahub frontend and confirmed that data was loaded

You should also configure Schema Registry to use security (contrib/kubernetes/datahub/values.yaml):

  • kafkastore.security.protocol=SSL
  • kafkastore.ssl.truststore.location=/var/ssl/private/kafka.server.truststore.jks
  • kafkastore.ssl.truststore.password=test1234

Note! The logs might say something similar to 'kafkastore.ssl.truststore.password was supplied but isn't a known config', but this is wrong and can be ignored.

Added note to docs/FAQ

docs/faq.md Outdated
@@ -116,3 +116,6 @@ You can call the [rest.li](https://github.com/linkedin/rest.li) API to ingest me
## Does Kafka support SSL? If so, how?

Yes. We are using the Spring Boot framework to start our apps, including setting up Kafka. You can [use environment variables to set system properties](https://docs.spring.io/spring-boot/docs/current/reference/html/spring-boot-features.html#boot-features-external-config-relaxed-binding-from-environment-variables), including [Kafka properties](https://docs.spring.io/spring-boot/docs/current/reference/html/appendix-application-properties.html#integration-properties). From there you can set your SSL configuration for Kafka.

If Schema Registry is configured to use security (SSL), then you also need to set the following config: https://docs.confluent.io/current/kafka/encryption.html#encryption-ssl-schema-registry.
Note! The logs might say something similar to 'kafkastore.ssl.truststore.password was supplied but isn't a known config', but this is wrong and can be ignored.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a link to a ticket or Slack thread for this known issue?

Copy link
Contributor Author

@themightylaz themightylaz Sep 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened Request #41858 for Confluent asking this, no answer yet.... and yes, needs a clarification before it can be said to be a bug.

Copy link
Contributor Author

@themightylaz themightylaz Sep 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, not wrong log actually. This log
kindred-datahub-datahub-gms-79b6587bb-t62vj datahub-gms 08:49:56.302 [main] WARN o.a.k.c.producer.ProducerConfig - The configuration 'kafkastore.ssl.truststore.password' was supplied but isn't a known config.
means the configuration kafkastore.ssl.truststore.password' is not a configuration required for the producer. These WARN message can be safely ignored. Each of Datahub services are passed a full set of configuration but may not require all the configurations that are passed to them. These warn messages indicate that the service was passed a configuration that is not relevant to it and can be safely ignored.

Btw, did the build pass? Was thinking to update that comment according to explanation above...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I didn't wait for the CI to pass. Will test and see if our newly added notification (#1873) works :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mars-lan, yes, I saw something failed and wanted to double-check but then saw again that it was merged....
Now I am unsure of the status...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. Testing the notification and fixing the build at the same time :)

Copy link
Contributor

@mars-lan mars-lan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping the docs current.

@mars-lan mars-lan merged commit b26d6fe into datahub-project:master Sep 29, 2020
@themightylaz
Copy link
Contributor Author

closing #1861

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants