Feature/auth and tls for brokers #179

magraef · 2024-04-03T19:30:15Z

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

magraef · 2024-04-03T19:38:16Z

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 · 2024-04-03T22:23:42Z

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 · 2024-04-03T23:31:45Z

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 · 2024-04-04T15:48:24Z

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 · 2024-04-04T22:45:11Z

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 · 2024-04-07T15:24:22Z

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 · 2024-04-08T22:13:59Z

@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 · 2024-04-09T11:55:39Z

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 · 2024-04-10T14:49:56Z

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 · 2024-04-10T17:19:36Z

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.

…afka.Reader and kafka.Dialer

* adjust natsjetstream.go consumer and streams must be configured *after* successfully creating the client

…sful kafka connection on creation like nats controller

…d auth

lerenn · 2024-04-12T12:40:37Z

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 !

lerenn · 2024-04-12T14:18:27Z

pkg/extensions/brokers/natsjetstream/natsjetstream.go

+}
+
+// registerStream creates or updates a stream based on the given configuration at the broker.
+func (c *Controller) registerStream() error {


There is a problem here, examples are broken because it should be possible to use Streams without creating them. But this change made it mandatory, which will break some usages.

We should put back the stream registration as an option, i.e. put it back to WithStreamConfig :)

Ah yes, sorry. I had to move it from the WithStreamConfig and WithConsumerConfig function, because register a stream or a consumer requires a fully configured connection to execute. If the broker is secured with TLS and or Auth, we would necessarily require WithConnectionOpts to be executed beforehand. I wanted to avoid that.

No problem ☺️

lerenn · 2024-04-12T14:20:01Z

pkg/extensions/brokers/natsjetstream/natsjetstream.go

-	js, err := jetstream.New(nc)
+// registerConsumer set consumer configuration for creates or updates a consumer based on the given configuration
+// at the broker.
+func (c *Controller) registerConsumer() error {


Same here, this should be relocated in the WithConsumerConfig

…rresponding config was set via ControllerOption

magraef · 2024-04-12T16:11:52Z

@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 · 2024-04-12T16:28:10Z

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 · 2024-04-12T16:32:06Z

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

lerenn marked this pull request as draft April 7, 2024 15:25

magraef force-pushed the feature/auth-and-tls-for-brokers branch from 7ae5632 to 2e1e552 Compare April 8, 2024 22:03

magraef added 12 commits April 11, 2024 01:12

* add WithTLS and WithSasl to kafka.go and use it for kafka.Writer, k…

d8aa908

…afka.Reader and kafka.Dialer

* add WithConnectionOpts to nats.go and natsjetstream.go

cde9e82

* add some documentation to README.md

4b61b03

* add some documentation to README.md **2

ef5b719

* add nats and jetstream dagger container with TLS and basic auth

cec1631

* add test for connection to core nats with TLS and basic auth

3c815fe

* add test for connection to nats jetstream with TLS and basic auth

a7d0de4

* adjust natsjetstream.go consumer and streams must be configured *after* successfully creating the client

* add connection test on creating kafka controller to validate succes…

2c40afb

…sful kafka connection on creation like nats controller

* add kafka brokers with tls and tls and basic auth

788e852

* add tests for testing connection to kafka with tls and tls with auth

d50e06b

* add test for issue169 use app and user with all brokers with tls an…

6dcd806

…d auth

* clean code the self-signed certificate stuff

c9335cc

magraef force-pushed the feature/auth-and-tls-for-brokers branch from a82db4e to c9335cc Compare April 10, 2024 23:29

magraef added 3 commits April 11, 2024 02:09

fix linter errors

63d70a8

fix check generated lerenn#2

4a224b4

update codegen

1869a4e

magraef marked this pull request as ready for review April 11, 2024 01:10

fix linter stuff

e12161a

lerenn reviewed Apr 12, 2024

View reviewed changes

registerStream and registerConsumer should only be executed if the co…

ef49f0d

…rresponding config was set via ControllerOption

lerenn merged commit def7b53 into lerenn:main Apr 12, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/auth and tls for brokers #179

Feature/auth and tls for brokers #179

magraef commented Apr 3, 2024 •

edited

Loading

magraef commented Apr 3, 2024

lerenn commented Apr 3, 2024

magraef commented Apr 3, 2024

lerenn commented Apr 4, 2024

magraef commented Apr 4, 2024 •

edited

Loading

lerenn commented Apr 7, 2024

magraef commented Apr 8, 2024 •

edited

Loading

lerenn commented Apr 9, 2024

lerenn commented Apr 10, 2024

magraef commented Apr 10, 2024 •

edited

Loading

lerenn commented Apr 12, 2024 •

edited

Loading

lerenn Apr 12, 2024 •

edited

Loading

magraef Apr 12, 2024

lerenn Apr 12, 2024

lerenn Apr 12, 2024

magraef commented Apr 12, 2024

lerenn commented Apr 12, 2024

lerenn commented Apr 12, 2024

Feature/auth and tls for brokers #179

Feature/auth and tls for brokers #179

Conversation

magraef commented Apr 3, 2024 • edited Loading

magraef commented Apr 3, 2024

lerenn commented Apr 3, 2024

magraef commented Apr 3, 2024

lerenn commented Apr 4, 2024

magraef commented Apr 4, 2024 • edited Loading

lerenn commented Apr 7, 2024

magraef commented Apr 8, 2024 • edited Loading

lerenn commented Apr 9, 2024

lerenn commented Apr 10, 2024

magraef commented Apr 10, 2024 • edited Loading

lerenn commented Apr 12, 2024 • edited Loading

lerenn Apr 12, 2024 • edited Loading

Choose a reason for hiding this comment

magraef Apr 12, 2024

Choose a reason for hiding this comment

lerenn Apr 12, 2024

Choose a reason for hiding this comment

lerenn Apr 12, 2024

Choose a reason for hiding this comment

magraef commented Apr 12, 2024

lerenn commented Apr 12, 2024

lerenn commented Apr 12, 2024

magraef commented Apr 3, 2024 •

edited

Loading

magraef commented Apr 4, 2024 •

edited

Loading

magraef commented Apr 8, 2024 •

edited

Loading

magraef commented Apr 10, 2024 •

edited

Loading

lerenn commented Apr 12, 2024 •

edited

Loading

lerenn Apr 12, 2024 •

edited

Loading