Skip to content

Conversation

@Yao-MR
Copy link

@Yao-MR Yao-MR commented Dec 18, 2025

Why are the changes needed?

Move the most module related with auth to common for further reuse

How was this patch tested?

Test with all the UT

Was this patch authored or co-authored using generative AI tooling?

NO

@Yao-MR Yao-MR changed the title [WIP] Move the Auth module from server to common for reuse Move the Auth module from server to common for reuse Dec 18, 2025
@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (18a5550) to head (974bf93).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
.../service/authentication/AuthenticationFilter.scala 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #7280   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         697     698    +1     
  Lines       43587   43627   +40     
  Branches     5893    5893           
======================================
- Misses      43587   43627   +40     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@turboFei
Copy link
Member

It looks like a break change.

I saw you want to enable the authentication for metrics port.

How about using the reflection to inject the AuthenticationFilter in kyuubi-metrics module?

val holder = new FilterHolder(new AuthenticationFilter(conf))
contextHandler.addFilter(holder, "/v1/*", EnumSet.allOf(classOf[DispatcherType]))

Please add an config item for the new feature and disable it by defaults.

@Yao-MR
Copy link
Author

Yao-MR commented Dec 22, 2025

It looks like a break change.

I saw you want to enable the authentication for metrics port.

How about using the reflection to inject the AuthenticationFilter in kyuubi-metrics module?

val holder = new FilterHolder(new AuthenticationFilter(conf))
contextHandler.addFilter(holder, "/v1/*", EnumSet.allOf(classOf[DispatcherType]))

Please add an config item for the new feature and disable it by defaults.

@turboFei yes, it is a break change , and also we can use reflection to reslove this for reuse,

but there is a problem that all the class releated with authoration is in the server module,
and the reflection will be complexed and not easy to read

what is your idea?

@turboFei
Copy link
Member

@pan3793
how do you think about?

either keeping the package name same or using reflection.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants