Skip to content

Added example with oauth/oidc example with CCloud logicalCluster and identityPoolId #1593

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sarwarbhuiyan
Copy link
Contributor

The following example shows how to configure the producer with OAUTH. For Confluent Cloud where we need to pass extensions for "logicalCluster" and "identityPoolId" we need to pass some additional paramters.

@sarwarbhuiyan sarwarbhuiyan requested a review from a team as a code owner June 24, 2023 01:17
@pranavrth
Copy link
Member

This example is more specific to Confluent Cloud. Can we move it to a new file?

@pranavrth
Copy link
Member

Just make sure that this example is tested.

@@ -131,5 +124,8 @@ def main(args):
help="Token URL.")
parser.add_argument('--scopes', dest="scopes", required=True, nargs='+',
help="Scopes requested from OAuth server.")

parser.add_argument('--logical-cluster', dest="logical_cluster", required=False, help="Logical Cluster.")
Copy link
Member

Choose a reason for hiding this comment

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

If we move this to CC specific example, does it make sense to mark these are required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original example is using an outdated callback based method so this example applies there too. Also, since the only difference between CC and AK/CP is this one param, it doesn't make sense to split it into another file. There's a wider task of updating all the examples to be using the recommended approach.

The purpose of examples is to educate.

Copy link
Member

Choose a reason for hiding this comment

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

We can improve old example as well and create a new example to cater mainly CC usecase. The idea is to make it easy for non CC user to use SASL as well. With everything specific to CC, user might get confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not just specific to CC though. If you don't include the logicalClusterId and identityPoolId in the cli params, it's just just works against any AK or CP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just don't want code that'll be copy pasted for a 2 line difference. The cli works with both CP and CC.

@sarwarbhuiyan
Copy link
Contributor Author

the example was tested against confluent cloud

@homestealth
Copy link

@pranavrth there seems to be two issues here:

  1. the original client example for oauth seems to be using the incorrect callbackhandler. it should be using the librd one - can we fix this asap?
  2. ccloud example is missing (for sasl extensions) - this is ok in a separate file, but we should create it

@cla-assistant
Copy link

cla-assistant bot commented Aug 15, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ pranavrth
❌ sarwarbhuiyan
You have signed the CLA already but the status is still pending? Let us recheck it.

@sarwarbhuiyan
Copy link
Contributor Author

@pranavrth there seems to be two issues here:

  1. the original client example for oauth seems to be using the incorrect callbackhandler. it should be using the librd one - can we fix this asap?
  2. ccloud example is missing (for sasl extensions) - this is ok in a separate file, but we should create it

@homestealth the ccloud example is here in my PR with two additional parameters that does the trick and since this is a cli example I've added optional parameters to the cli which make it work. It seems overkill to have a separate file for the exact same code to be copied.

@pranavrth
Copy link
Member

I think it is fine to keep only one file but we should add a comment stating the two new configs are for CC case only.

Please also sign the CLA.

@pranavrth
Copy link
Member

@emasab - Any comment from your side before we can merge this?

@dylanmei
Copy link

This is a valuable example, since it is config-based and removes the callback. How can we close this out?

@jriester
Copy link
Member

Hi everyone, any plans to have this uploaded in some capacity? Ran into this issue today and this example would have been very helpful.

@pranavrth
Copy link
Member

@sarwarbhuiyan - Can you please address my last comment to add some documentation? Please sign the CLA as well. We should merge this PR.

@jriester
Copy link
Member

jriester commented May 6, 2024

Hi team, two more customers have reported issues with our current example. GTS has been finding this pull and giving the customer the example here, any way we can move this to live?

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.

5 participants