Skip to content

Kafka stabiltiy test#127

Closed
howardjohn wants to merge 2 commits intoistio:masterfrom
howardjohn:kafka
Closed

Kafka stabiltiy test#127
howardjohn wants to merge 2 commits intoistio:masterfrom
howardjohn:kafka

Conversation

@howardjohn
Copy link
Member

No description provided.

@howardjohn howardjohn requested review from costinm and mandarjog May 7, 2019 19:40
@GregHanson
Copy link
Member

@howardjohn will this test eventually change as kafka support is merged into envoy? Several PR's have started going in the past few days, such as envoyproxy/envoy#4950

@howardjohn
Copy link
Member Author

@GregHanson I had no idea about that, thanks. We will need to add support to istio as well, right? Then just change port names to kafka-?

@GregHanson
Copy link
Member

yes, eventually we will have to pull this into istio - kafka is something that comes up pretty frequently with istio in my experience. @cmluciano might be able to provide some more insight into this from an envoy perspective. As it stands now, the codec has been merged into envoy but not the filter?

@GregHanson
Copy link
Member

but basically it sounds like envoy still has some work to do on its end, so the test as it stands now ( with basic ServiceEntry with Resolution type NONE) is definitely a good test to have for the time being

@howardjohn
Copy link
Member Author

The ServiceEntry won't change when we get the envoy filter working though , it is needed because Istio won't create the entries for every instance of the headless service so they cannot talk to each other without it, unless I am doing it wrong

Copy link
Member

@sdake sdake left a comment

Choose a reason for hiding this comment

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

/lgtm

Great work @howardjohn! While Envoy might end up merging Kafka functionality, if it doesn't have it . today, we have a desire to dead chicken test a common set of widely used open source projects with Istio. This meets that bar IMO, and adds value to our overall testing.

If/When Envoy includes native support for handling Kafka, we can either remove this test or rework it. As things stand today, I'd like to see it merged.

I have to acknowledge I don't keep up with Envoy as I am mostly interested in the control plane, so if Envoy experts disagree on timing (as in Envoy will have this functionality in the next 3 weeks prior to 1.2 release), now is the time to say something :).

In a worse case scenario, the work would need a revert if it in some way interfered with Envoy.

@sdake sdake self-assigned this Jun 4, 2019
@howardjohn
Copy link
Member Author

@mandarjog @utka can you give owners approval

Copy link
Member

@GregHanson GregHanson left a comment

Choose a reason for hiding this comment

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

Please add a license comment to the kafka python app file. Other than that looks good. As @sdake said, it's a good base line test until more support is added in Envoy

@GregHanson
Copy link
Member

/approve

@istio-testing
Copy link
Contributor

@howardjohn: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 18, 2019
@howardjohn howardjohn closed this Jul 25, 2019
@hansel801
Copy link

Hi, have you tried Kafka with sidecar + mTLS enabled? Wondering if this works...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase Indicates a PR needs to be rebased before being merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants