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

Add "sasl_protocol: oauthbearer" which does not add default handlers #1093

Open
wants to merge 5 commits into
base: 7.1.x
Choose a base branch
from

Conversation

sverrehu
Copy link

Description

Using the existing sasl_protocol: oauth will add callback handlers for Confluent-provided classes. This PR adds a similar sasl_protocol: oauthbearer that will not automatically set up any callback handlers.

Fixes #1089

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • [x ] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Molecule test, and test in our local cluster.

Test Configuration:

Checklist:

  • [x ] My code follows the style guidelines of this project
  • [ x] I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • [ x] I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • [ x] I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules
  • Any variable changes have been validated to be backwards compatible

erikgb
erikgb previously approved these changes Jun 20, 2022
Copy link
Contributor

@erikgb erikgb left a comment

Choose a reason for hiding this comment

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

LGTM, but we could consider a separate PR to catch up the missing re-gen of md-files.

@utkarsh5474 utkarsh5474 requested a review from a team June 21, 2022 10:01
@sverrehu sverrehu force-pushed the oauthbearer-without-adding-confluent-classes branch 3 times, most recently from 09538ac to d4c4e90 Compare June 27, 2022 05:49
@sverrehu sverrehu force-pushed the oauthbearer-without-adding-confluent-classes branch from d4c4e90 to b9f0f12 Compare June 28, 2022 20:17
'io.confluent.kafka.server.plugins.auth.token.TokenBearerServerLoginCallbackHandler'
final_dict['listener.name.' + listener_name + '.oauthbearer.sasl.jaas.config'] =\
'org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule required publicKeyPath=\"' + oauth_pem_path + '\";'
if listeners_dict[listener].get('sasl_protocol', default_sasl_protocol).upper() == 'OAUTH':
Copy link
Member

Choose a reason for hiding this comment

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

Should we not update this condition to check the property confluent_server_enabled That way probably we don't need to add another mechanism.

Copy link
Contributor

Choose a reason for hiding this comment

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

does there need to be 2 if statements

            if self.normalize_sasl_protocol(listeners_dict[listener].get('sasl_protocol', default_sasl_protocol)) == 'OAUTHBEARER':
                if listeners_dict[listener].get('sasl_protocol', default_sasl_protocol).upper() == 'OAUTH':

Copy link

cla-assistant bot commented Oct 31, 2023

CLA assistant check
All committers have signed the CLA.

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.

4 participants