search

lerenn / asyncapi-codegen

An AsyncAPI Golang Code generator that generates all Go code from the broker to the application/user. Just plug your application to your favorite message broker!
Apache License 2.0
78 stars 21 forks source link

Feature/auth and tls for brokers #179

Closed magraef closed 2 months ago

magraef commented 3 months ago

Add support of configuration of broker authentication and TLS. Should close #169

magraef commented 3 months ago

I have made a few local tests with the configs, for integration tests I lack some knowledge of the setup. We needed two docker containers like in the normal test cases where the brokers are configured with auth and or TLS. That doesn't seem to be that easy to me. Maybe you @lerenn have an idea?

lerenn commented 3 months ago

Thanks @magraef for your great work !

If that's okay for you, I'll add the brokers with auth/TLS to brokers on your PR. I'll just change the existing containers in test as if it works with the auth/TLS, then it'll work without them.

Or I can add a dedicated test to test TLS with each broker by adding a authenticated/TLS version of each broker.

I would go on the first one, but I would be interested on what you think about that. What do you think ? :)

magraef commented 3 months ago

I think the "cleanest" option would be to set up a test suite specifically for this use case that runs a docker container for nats and one for kafka and then runs a simple test case with writing a message and reading a message against a broker that is secured with TLS and authentication. I don't think we should make the regular tests more complicated than necessary.

I had started to build a test case and dont use the asyncapi_test.BrokerControllers(t) in the TestSuite and instead create new ones manually. But i dont know where the docker containers are created and builded. I also always change the host to localhost in asyncapi_test.BrokerControllers(t) so that I can get the test to run at all :P

But I know that you are already working on the contributer documentation, so I think that will be clearer.

lerenn commented 3 months ago

Yes, sorry about that !

I'm using Dagger (a tool made by the guys behind Docker) for CI and tests so it is a bit different as a classic docker-compose. I was a bit occupied with some minor bugs, but I will definitely do this Contributors documentation :D

I think you're right, the specific test is better. If you want to take a look there is the CI script in build/ci and pkg/ci but it's not really the best of codes and there is one of my PRs waiting for completion to improve that, so I'm okay to do the testing and push it to your branch if that's good for you.

If you prefer to do it, I won't complain obviously, but you've already done so much that seems normal to help on that :)

magraef commented 3 months ago

I haven't used Dagger yet, but I'll take a look at build/ci and pkg/ci an prepare something. No worries at all. I will be glad to handle that and add it to the PR. Learning new things is always a plus :)

lerenn commented 3 months ago

I'll put it in draft until then, but do not hesitate to ask me if there is anything unclear about how I have done the Dagger part :)

It maybe not the most clear/efficient way, and that's why I'm trying to change it in another MR.

magraef commented 3 months ago

@lerenn Sorry about the force push; I messed up the rebase a bit.

I have making progress working with dagger, and i like it. I had to make some changes and found a few things that are not optimal in my opinion, but I will add those details to the open dagger issue once Im finished with this one.

Let me know if you're okay with that approach. If so, I can do the same changes for Kafka. Unfortunately, setting up TLS for Kafka isn't as straightforward as it is for Nats because Kafka prefers .jks files and I want to avoid using system commands to set up TLS cleanly. There should be support for PEM format, but its not that good documented. I will see if I can find a a good solution. Looking forward to your feedback.

lerenn commented 3 months ago

No problem @magraef for the force push ! That's your fork, so IMHO you do what you prefer 😉

I'll take a look as soon as possible! Every improvement is good to have. Thanks for being so determined in this 💪

lerenn commented 3 months ago

Hello @magraef ! Sorry, for the delay. I reviewed the change on the CI and I'm all good for this ! This is neat and clean. Good for me to continue if you want :)

magraef commented 3 months ago

Hey @lerenn ,

thanks for your feedback. Here are then the corresponding changes I made for Kafka.

Additionally I have added a test to check if Kafka is working properly in the kafka.NewController function. If it is not reachable or responsive, kafka.NewController will return an error like the one for the NATS controllers. Default is enabled and you can disable this with WithConnectionTest(false) but I believe it's useful to know if there's a problem right away on startup.

You might have noticed that not all tests are in the test folder. I suggest we customize the dagger test so that all tests in the repository are executed with the command: go test -v ./.... This way, every contributor can easily check if all tests are running.

Im considering simplifying local development. Currently, to run and debug tests locally, I always need to modify the hosts in the tests and start the necessary services locally. I have created a docker-compose file with the brokers, TLS and authentication. I'm not sure if there's a better approach with Dagger or if you've already found a better way.

lerenn commented 2 months ago

Thanks for the improvements ! 💪

I do agree for the check on Kafka, and if there is any trouble, worst case is we switch the default behavior.

For the local development, normally you just have to launch the test with dagger and in the Makefile, you can specify which test to run. However, it's a bit too much for now to pop a docker container for each test, so we should simplify it I think. We can discuss this further on another PR/issue if you want (I'm not against adding a docker-compose, if it's easier to develop with) :)

I'll merge this one !

magraef commented 2 months ago

@lerenn Great that there are the examples and that you have noticed that. But I think it would be better if we either write tests for the ConfigOpts or add a step in the ci to run the examples automatically.

lerenn commented 2 months ago

Yup, the nice person that proposed to add dagger to the Github CI/CD pipeline didn't add the examples and I missed it when I reviewed it. It should be there also, I'll add it to the test related issue :)

lerenn commented 2 months ago

It's merged ! Thanks again for your amazing work ☺️