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

wip: socks 5 test #132

Closed
wants to merge 18 commits into from
Closed

Conversation

mario-s
Copy link
Contributor

@mario-s mario-s commented May 2, 2022

Closes #110

  • new test dependency

  • check if environment variables to the Kafka cluster and proxy are set

  • if not test will be skipped

  • values for cluster and proxy are taken from env variables

  • add sample in README

  • I've read the contributing section of the project CONTRIBUTING.md.

  • Signed CLA

@mario-s mario-s changed the title Feature/socks test socks 5 test May 2, 2022
Copy link
Collaborator

@crepererum crepererum left a comment

Choose a reason for hiding this comment

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

For #110 to be closed we need some SOCKS5 test running within CI as well. Could you have a look at the CircleCI configs and see if you find a way to get this to work?

Also it would be nice if we can offer some documentation on how to test this locally. There are two docker compose files (one for Kafka and one for redpanda) and I think that would be a good place to integrate some SOCKS5 proxy running in a container.

README.md Outdated
```console
$ KAFKA_CONNECT=0.0.0.0:9093 SOCKS_PROXY=localhost:8080 cargo test --features full
```
To avoid known issues with the [test library](https://github.com/yanganto/test-with/issues/18), it is recommended to clean up before the first test.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I consider that a pretty major drawback. This is the reason why we use some custom macro for the other environment variables. Would be great if you could use a similar approach.

Copy link
Contributor Author

@mario-s mario-s May 5, 2022

Choose a reason for hiding this comment

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

Sure, I changed that to use a macro. However the drawback of this approach is, that you don't see that the test is skipped.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, the macro is not ideal. My hope is that rust-lang/rfcs#3221 will be accepted and we can get the best of both worlds. But until then the skipping won't be reported properly.

Copy link

@yanganto yanganto May 23, 2022

Choose a reason for hiding this comment

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

Hi,
this is something you may want to use for this scenario.
https://crates.io/crates/test-with

If you are using CI tool for testing, or under development with source code changing, you do not need cargo clean before testing.

Besides, the test report from cargo will change to the following format for a better understanding.

running 2 tests
test tests::test_ignored ... ignored, because variable not found: NOTHING
test tests::test_works ... ok

test result: ok. 1 passed; 0 failed; 1 ignored; 0 measured; 0 filtered out; finished in 0.00s

If there is any issue on the crate, an issue is always welcome. :)

Choose a reason for hiding this comment

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

By the way, there is more than one issue in #[ignore(if)], so it might not be the better solution to solve. It is good to keep ignoring in runtime and ignoring under conditions separately.

The following are related PR and issues in Rust, for you to know more about these if you are interested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not to worried about the CI and the env variable handling there. Ignoring tests and build time instead of runtime is IMHO a messy DX (developer experience). Rskafka is already a somewhat complicated crate -- especially when you consider the integration tests. Sure, not having a proper "ignore" status for skipped tests is bad DX as well, but I consider this the lesser evil.

So let's wait until Rust implements the required feature. I've created #139 to track this on our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You could assign the #139 to me as well, I would be more than happy to integrate the feature, when Rust is ready.

@mario-s mario-s changed the title socks 5 test wip: socks 5 test May 4, 2022
@mario-s
Copy link
Contributor Author

mario-s commented May 9, 2022

I tested it with "Charles" on my local machine, before opening this PR. Unfortunately I was not able to integrate any SOCKS5 Proxy into the docker compose file for the Kafka cluster yet.

@mario-s
Copy link
Contributor Author

mario-s commented May 10, 2022

Update: I found a Rust SOCK5 proxy that works. It can be easily installed via Cargo. But if you like a Docker image, I think it is better to create a new one, since the existing images are rather old or contain no hint about the version.

@crepererum
Copy link
Collaborator

@mario-s what about https://hub.docker.com/r/serjs/go-socks5-proxy/ ? That seems to be a simple proxy packaged as a docker image.

@mario-s
Copy link
Contributor Author

mario-s commented May 11, 2022

@crepererum Thanks for the hint. I already tried the that proxy server in docker. Included in the docker-compose file, as well as a standalone docker container. But a connection to the Kafka cluster could not be established during the test.
I ran the test with this command: TEST_INTEGRATION=1 KAFKA_CONNECT=0.0.0.0:9093 SOCKS_PROXY=localhost:1080 cargo test --features transport-socks5
My next attempt was to pass the name of the container for the KAFKA_CONNECT variable, because I thought the Proxy just doesn't see this IP. But then all other integration test, which use the cluster, did not pass.
Therefore I'm not even sure if there is a way to embed a proxy into the docker-compose.

I also tried out the Kafka-Proxy project, same result, but different cause.

@crepererum
Copy link
Collaborator

@mario-s I see, it's a bit of mess with the compose files because they bind the brokers to localhost. I've played around with this is and came up with a solution in #135: It basically adds new broker identities/hosts that are visible to the proxy. Funnily this is way eaiser for CircleCI because there we have true virtual host names for the brokers that are resolvable by the client. Another trick that I've added is that KAFKA_CONNECT now accepts a list of brokers which also allows us to pass in an invalid broker just to test that everything still works if we're doing that. I cannot add you as a reviewer (IIRC this only works for org-members) but it would appreciate if you could have a look and add your review.

@crepererum
Copy link
Collaborator

#135 is now merged. Thanks again to @mario-s for the work that was also included the merged PR.

@crepererum crepererum closed this May 24, 2022
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.

Enable SOCKS5 integration test
3 participants