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

Always take the first declared SASL/PLAIN auth type #5990

Closed
wants to merge 3 commits into from
Closed

Always take the first declared SASL/PLAIN auth type #5990

wants to merge 3 commits into from

Conversation

pan3793
Copy link
Member

@pan3793 pan3793 commented Jan 17, 2024

🔍 Description

Issue References 🔗

#5185 changed the type of kyuubi.authentication from Seq(ordered) to Set(unordered), which break the assumption of

Note that: for SASL authentication, KERBEROS and PLAIN auth types are supported
at the same time, and only the first specified PLAIN auth type is valid.

Describe Your Solution 🔧

Restore the type to Seq

Types of changes 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

UT is updated

Checklist 📝

Be nice. Be informative.

@pan3793 pan3793 requested a review from bowenliang123 January 17, 2024 14:00
bowenliang123
bowenliang123 approved these changes Jan 17, 2024
View reviewed changes
Copy link
Contributor

@bowenliang123 bowenliang123 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing it.

@github-actions github-actions bot added the kind:documentation Documentation is a feature! label Jan 17, 2024
@codecov-commenter
Copy link

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (7b38889) 61.12% compared to head (acae25f) 61.10%.

Files Patch % Lines
...e/authentication/KyuubiAuthenticationFactory.scala 62.50% 0 Missing and 3 partials ⚠️
.../apache/kyuubi/server/http/ThriftHttpServlet.scala 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##             master    #5990      +/-   ##
============================================
- Coverage     61.12%   61.10%   -0.03%     
  Complexity       23       23              
============================================
  Files           622      622              
  Lines         37035    37036       +1     
  Branches       5023     5024       +1     
============================================
- Hits          22637    22629       -8     
- Misses        11952    11957       +5     
- Partials       2446     2450       +4

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

@pan3793 pan3793 self-assigned this Jan 18, 2024
@pan3793 pan3793 added this to the v1.8.1 milestone Jan 18, 2024
@pan3793 pan3793 closed this in 3b2e674 Jan 18, 2024
pan3793 added a commit that referenced this pull request Jan 18, 2024
@pan3793
[KYUUBI #5990] Always take the first declared SASL/PLAIN auth type
9f4a81a 
# 🔍 Description
## Issue References 🔗

#5185 changed the type of `kyuubi.authentication` from `Seq`(ordered) to `Set`(unordered), which break the assumption of
```
Note that: for SASL authentication, KERBEROS and PLAIN auth types are supported
at the same time, and only the first specified PLAIN auth type is valid.
```

## Describe Your Solution 🔧

Restore the type to Seq

## Types of changes 🔖

- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

UT is updated

---

# Checklist 📝

- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes #5990 from pan3793/auth-plain.

Closes #5990

acae25f [Cheng Pan] fix doc
cef7dba [Cheng Pan] fix
87b370f [Cheng Pan] Always take the first declared SASL/PLAIN auth type

Authored-by: Cheng Pan <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
(cherry picked from commit 3b2e674)
Signed-off-by: Cheng Pan <[email protected]>
@pan3793
Copy link
Member Author

pan3793 commented Jan 18, 2024

Thanks, merged to master/1.8

zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this pull request Feb 5, 2024
@pan3793 @zhaohehuhu
[KYUUBI apache#5990] Always take the first declared SASL/PLAIN auth type
79172bf 
# 🔍 Description
## Issue References 🔗

apache#5185 changed the type of `kyuubi.authentication` from `Seq`(ordered) to `Set`(unordered), which break the assumption of
```
Note that: for SASL authentication, KERBEROS and PLAIN auth types are supported
at the same time, and only the first specified PLAIN auth type is valid.
```

## Describe Your Solution 🔧

Restore the type to Seq

## Types of changes 🔖

- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

UT is updated

---

# Checklist 📝

- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes apache#5990 from pan3793/auth-plain.

Closes apache#5990

acae25f [Cheng Pan] fix doc
cef7dba [Cheng Pan] fix
87b370f [Cheng Pan] Always take the first declared SASL/PLAIN auth type

Authored-by: Cheng Pan <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
zhaohehuhu pushed a commit to zhaohehuhu/incubator-kyuubi that referenced this pull request Mar 21, 2024
@pan3793 @zhaohehuhu
[KYUUBI apache#5990] Always take the first declared SASL/PLAIN auth type
0d04ba7 
# 🔍 Description
## Issue References 🔗

apache#5185 changed the type of `kyuubi.authentication` from `Seq`(ordered) to `Set`(unordered), which break the assumption of
```
Note that: for SASL authentication, KERBEROS and PLAIN auth types are supported
at the same time, and only the first specified PLAIN auth type is valid.
```

## Describe Your Solution 🔧

Restore the type to Seq

## Types of changes 🔖

- [x] Bugfix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to change)

## Test Plan 🧪

UT is updated

---

# Checklist 📝

- [x] This patch was not authored or co-authored using [Generative Tooling](https://www.apache.org/legal/generative-tooling.html)

**Be nice. Be informative.**

Closes apache#5990 from pan3793/auth-plain.

Closes apache#5990

acae25f [Cheng Pan] fix doc
cef7dba [Cheng Pan] fix
87b370f [Cheng Pan] Always take the first declared SASL/PLAIN auth type

Authored-by: Cheng Pan <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowenliang123 bowenliang123 bowenliang123 approved these changes

Assignees

@pan3793 pan3793

Labels
kind:documentation Documentation is a feature! module:common module:server
Projects
None yet
Milestone
v1.8.1
Development

Successfully merging this pull request may close these issues.
3 participants
@pan3793 @codecov-commenter @bowenliang123