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.

Sign up for GitHub

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

Jump to bottom

[improve][fn] Support OAuth2 in Go instance #22323

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

[improve][fn] Support OAuth2 in Go instance #22323

wants to merge 4 commits into from

Conversation

jiangpengcheng
Copy link
Contributor

Fixes #22322

Main Issue: #xyz

PIP: #xyz

Motivation

The Go instance doesn't support OAuth2 authentication yet, it's better to support it

Modifications

Support the OAuth2 in Go instance

Verifying this change

  • Make sure that the change passes the CI checks.

  • This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: jiangpengcheng#30

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 22, 2024
@lhotari
Copy link
Member

lhotari commented Mar 22, 2024

How do we test this?

@jiangpengcheng
Copy link
Contributor Author

How do we test this?

Seems no, there are no OAuth2 tests for python and java instance too, and there is even no AuthenticationProviderOAuth2, so it's hard to setup a cluster with "OAuth2" provider for testing

@jiangpengcheng jiangpengcheng force-pushed the support_oauth2_for_go_function branch 3 times, most recently from 978e376 to 19d9ab0 Compare March 28, 2024 01:39
@mattisonchao
Copy link
Member

mattisonchao commented Apr 6, 2024 •
edited

Hi, IMO

The pulsar-client-go testing should ensure the oauth2 is working or not. Pulsar-function should ensure we applied the correct configuration for it.

:)

So, would you mind adding the configuration applied testing?

@lhotari
Copy link
Member

lhotari commented Apr 6, 2024

Hi, IMO

The pulsar-client-go testing should ensure the oauth2 is working or not. Pulsar-function should ensure we applied the correct configuration for it.

:)

So, would you mind adding the configuration applied testing?

An integration test would also be possible, but that does require effort and knowledge of how Pulsar system level integration tests are developed.
Using WireMock for mocking the OAuth2 end points is one way to get it working without a full blown IDP like Keycloak.

@jiangpengcheng
Copy link
Contributor Author

Hi, IMO

The pulsar-client-go testing should ensure the oauth2 is working or not. Pulsar-function should ensure we applied the correct configuration for it.

:)

So, would you mind adding the configuration applied testing?

Added tests for checking the arguments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RobertIndie RobertIndie RobertIndie left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support OAuth2 authentication in Go instance of pulsar functions
4 participants
@jiangpengcheng @lhotari @mattisonchao @RobertIndie