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

Docs: Add docs for using external secrets in Kafka Connect #1183

Merged

Conversation

scholzj
Copy link
Member

@scholzj scholzj commented Dec 22, 2018

Type of change

  • Documentation

Description

This PR adds the documentation related to the changes in #1182

Checklist

  • Update documentation

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

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

I added few comments, one more is about using "ConfigMaps" instead of "Config Maps"

@scholzj scholzj changed the base branch from kafka-connect-secrets-and-external-configration to master December 29, 2018 21:55
@scholzj scholzj changed the base branch from master to kafka-connect-secrets-and-external-configration December 29, 2018 22:00
@scholzj scholzj force-pushed the docs-kafka-connect-secrets-and-external-configration branch from ebd3a85 to 6ad45d2 Compare December 29, 2018 22:03
@scholzj scholzj changed the base branch from kafka-connect-secrets-and-external-configration to master December 29, 2018 22:03
@scholzj scholzj force-pushed the docs-kafka-connect-secrets-and-external-configration branch from 6ad45d2 to a3ab9b1 Compare December 29, 2018 22:52
Copy link
Member

@d-laing d-laing left a comment

Choose a reason for hiding this comment

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

Review comments are inline.

[id='assembly-kafka-connect-external-configuration-{context}']

= Using external configuration and secrets

Copy link
Member

Choose a reason for hiding this comment

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

Please reword as follows:

Kafka Connect connectors include built-in configuration using REST. The connector configuration is transferred to Kafka Connect whenever an HTTP request to the connector is made.

Alternatively, the configuration of a Kafka Connect connector can be stored externally as ConfigMaps or Secrets. ConfigMaps and Secrets are standard {ProductPlatformName} resources used for Pod configuration and authentication. This method applies especially to confidential data, such as usernames, passwords, or certificates.

Copy link
Member Author

Choose a reason for hiding this comment

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

@laidan6000 I fixed all the other comments as you suggested. But I'm a bit unsure about this one as I don't think it is correct and makes really sense.

  • The REST and HTTP are the same interface. Technically it should be probably HTTP REST interface
  • The way this works is that the configuration is stored inside Kafka. And the user creates / modifies / deletes the connector and its configuration using the HTTP REST interface.
  • After this PR the connector configuration is not really stored in the config maps or secrets. But the configuration in the HTTP REST commands can reference the values from them and that way to keep them separate and more secure if needed.

I think especially the first paragraph in your suggestion is wrong and confusing. The second could be probably used.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the additional information in the bullet points. In my eagerness to use the verb "store" rather than "externalize", I changed the meaning of the sentence. To be honest, I am struggling to fully understand the approach here. I've edited the first paragraph based on your bullet points -- does it work for you? Please tweak the wording as needed.

==

Kafka Connect connectors are configured using an HTTP REST interface. The connector configuration is stored within Kafka itself and passed to Kafka Connect as part of an HTTP request.

Alternatively, the configuration of a Kafka Connect connector can be externalized as ConfigMaps or Secrets. You can then reference the configuration values in HTTP REST commands (this keeps the configuration separate and more secure, if needed). This method applies especially to confidential data, such as usernames, passwords, or certificates.

ConfigMaps and Secrets are standard {ProductPlatformName} resources used for Pod configuration and authentication.

Copy link
Member Author

Choose a reason for hiding this comment

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

@laidan6000 I did some additional changes to this to make it (hopefully) a bit more clear. Especially to make it more clear that it doesn't store the whole configuration but just some pieces (I think this was probably written incorrectly already by me on the beginning). It would be great if you could have another review. The other comments should be incorporated as you suggested.

Copy link
Member

Choose a reason for hiding this comment

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

It reads very well and makes sense to me. Happy for you to merge the PR.

@d-laing
Copy link
Member

d-laing commented Jan 8, 2019

Added further, minor comments after a second read-through. I've had another try at the assembly introduction -- please tweak the wording if you still think it's confusing.

Copy link
Member

@d-laing d-laing left a comment

Choose a reason for hiding this comment

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

Approved.

@scholzj scholzj merged commit 7c102fa into master Jan 9, 2019
@scholzj scholzj deleted the docs-kafka-connect-secrets-and-external-configration branch January 9, 2019 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants