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

Implement basic testing for the kafka output module #10834

Merged
merged 3 commits into from
Feb 28, 2019
Merged

Implement basic testing for the kafka output module #10834

merged 3 commits into from
Feb 28, 2019

Conversation

Popsiclestick
Copy link
Contributor

This PR is for #10652

I've implemented the same TCP connection test found in the elasticsearch output test. This is a basic test.

Example output below

:; go build && ./filebeat test output -c kafka-filebeat.yml
TLS... WARN Kafka output doesn't support TLS testing
Kafka: localhost:9092...
  parse host... OK
  dns lookup... OK
  addresses: ::1, 127.0.0.1
  dial up... OK
Kafka: 127.0.0.1:9092...
  parse host... OK
  dns lookup... OK
  addresses: 127.0.0.1
  dial up... OK
Kafka: 0.0.0.0:9092...
  parse host... OK
  dns lookup... OK
  addresses: 0.0.0.0
  dial up... OK

Note:
The test will function whether or not the kafka server requires TLS. I did look to implement the transport.TestTLSDialer as well. But the TLS config objects which are passed don't look to be standardized between outputs. Kafka morphs the original object to integrate with sarama. The kafka client only has access to sarama.Config which doesn't contain the original object to perform tlscommon.LoadTLSConfig. Though I am likely missing something.

I am happy to implement any recommendations.

@Popsiclestick Popsiclestick requested a review from a team as a code owner February 19, 2019 22:17
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@jsoriano
Copy link
Member

@Popsiclestick this is looking good, thanks! Before merging this change you will have to sign the CLA.

To obtain the TLS configuration you may try to do the reverse operation of tlscommon.BuildModuleConfig, with the TLS config stored in the sarama config object. But I am not sure if at this point all the information is available.

@jsoriano jsoriano self-requested a review February 20, 2019 14:15
@jsoriano
Copy link
Member

jenkins, test this

@Popsiclestick
Copy link
Contributor Author

I signed it. Strange though, I had already signed the agreement last week. Not sure, but hopefully it works now.

@jsoriano
Copy link
Member

@Popsiclestick sorry, it seems that there was some kind of problem with CLA checker, could you try to sign it again?

@Popsiclestick
Copy link
Contributor Author

Signed again :)

@jsoriano
Copy link
Member

@Popsiclestick right, it seems to be working now 🎉

Do you want to give another try to TLS config? If not I think this is ready to go, and TLS can be added later.

@Popsiclestick
Copy link
Contributor Author

Wooh! @jsoriano

I think we should merge this in for now. If I get some more time in the future, I can look at trying to get TLS added later.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Thanks!

@jsoriano
Copy link
Member

@Popsiclestick oh sorry, I almost forgot it! I think this change should have an entry in CHANGELOG.next.asciidoc, could you add it?

@Popsiclestick
Copy link
Contributor Author

@jsoriano Everything look good now? 👍

@jsoriano
Copy link
Member

jenkins, test this

@jsoriano
Copy link
Member

@Popsiclestick looks good, thanks! I'll merge it when everything is green 🙂

@jsoriano jsoriano merged commit 2011b10 into elastic:master Feb 28, 2019
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.

3 participants