[CSS-6503] Add OAuth support for non-charmed external clients

gtato commented 8 months ago

This is the equivalent of the PR opened on the K8S charm, for adding the oauthbearer listener.

In a few words this PR does the following:

adds the oauth interface from hydra
on a relation on that interface, it adds a sasl/oauthbearer listener
it adds a unit test (using both a scram and oauthbearer client) to check if the config is generated correctly
the rest is about reorganizing the protocols and the mechanisms (cause it was a bit messy)
- protocols are: sasl_plain, sasl_ssl and ssl
- mechanisms are: scram-sha-512, oauthbearer, and ssl (this one makes no sense but it is for parity and it matches only with ssl as a protocol)

marcoppenheimer commented 7 months ago

Overall, bar a relatively minor style choice, it looks good! I'll wait for @zmraul to review though, as he was the one that originally did the feature, so will be able to provide a better review.

deusebio commented 4 months ago

@gtato @marcoppenheimer @zmraul

Is this now ready for review? I see there are a number of conflicts. Also, I just want to make sure that this is the final implementation consistent with the spec DA077 before diving in (I see latest commit are preceding the hackathon...was there any code-wise lesson learned there?)

I'm happy to move this forward but I'd like to first understand where we stand

gtato commented 4 months ago

@gtato @marcoppenheimer @zmraul

Is this now ready for review? I see there are a number of conflicts. Also, I just want to make sure that this is the final implementation consistent with the spec DA077 before diving in (I see latest commit are preceding the hackathon...was there any code-wise lesson learned there?)

I'm happy to move this forward but I'd like to first understand where we stand

I will update the PRs on both repos (this and the k8s) shortly.

gtato commented 4 months ago

Thanks @gtato for this! The code looks good! I have mostly nipticks and suggestions, nothing really major, code-wise.

The only major comment would be to have (either or both):

Integration tests

Some documentation on how this can be setup in the form of an how-to guide, either in discourse or as PR description.

I understand 1 can be tricky, and I'm happy to call it nice-to-have. But we should at the very least have some documentation on how this can be setup, such that it will then be on the product team to convert that into an integration tests (hopefully)

I really wanted to add an integration test, but it does require some time (to mock the idp) that I didn't manage to allocate in the previous cycle. In this one it is even harder as the event-bus epics are not even in the roadmap (apart some maintenance).
Makes sense. I think we (@zmraul and I) can make a discourse post with the two ways of setting this: with the oauth integrator and hydra.

deusebio commented 4 months ago

Yes, don't worry. As said, I'm happy to have a ticket for the product team to do this, as long as we have documentation.

deusebio commented 3 months ago

I'm happy with the changes. There are only a couple of points we need to resolve before approval:

Regarding the quote below. I checked the charmed kafka documentation here and there is no post around the integration. I seem to remember that maybe either you or raul had written something. Can you guys make sure that the docs are easily accessible?

Makes sense. I think we (@zmraul and I) can make a discourse post with the two ways of setting this: with the oauth integrator and hydra.

Are the changes from https://github.com/canonical/kafka-operator/pull/197 included here? can we close the other PR as superseded by this one?

Once these are resolved, I'm happy to approve

zmraul commented 3 months ago

* Are the changes from [Oauth #197](https://github.com/canonical/kafka-operator/pull/197) included here? can we close the other PR as superseded by this one?

I believe they are. #197 was made for reference, so I can close it now.

deusebio commented 3 months ago

Great job @gtato !! The code looks very great! Thanks for this very valuable contribution! Once the tests pass, I'm happy to merge this!

deusebio commented 3 months ago

@zmraul I suppose we can close the other PR. would you do that?

deusebio commented 3 months ago

@gtato we have just landed a small PR to avoid the failing tests of sync_docs. Just rebase and I believe all tests should be green! Thanks!

canonical / kafka-operator

[CSS-6503] Add OAuth support for non-charmed external clients #168