Skip to content

[Improvement] Use Set collection for order-insensitive configs #5185

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
Closed
wants to merge 1 commit into from
Closed

[Improvement] Use Set collection for order-insensitive configs #5185

wants to merge 1 commit into from

Conversation

@bowenliang123
Copy link
Contributor

@bowenliang123 bowenliang123 commented Aug 21, 2023 •
edited
Loading

Why are the changes needed?

  • Use Set collection for order-insensitive configs instead of Seq
    • kyuubi.frontend.thrift.binary.ssl.disallowed.protocols
    • kyuubi.authentication
    • kyuubi.authentication.ldap.groupFilter
    • kyuubi.authentication.ldap.userFilter
    • kyuubi.kubernetes.context.allow.list
    • kyuubi.kubernetes.namespace.allow.list
    • kyuubi.session.conf.ignore.list
    • kyuubi.session.conf.restrict.list
    • kyuubi.session.local.dir.allow.list
    • kyuubi.batch.conf.ignore.list
    • kyuubi.engine.deregister.exception.classes
    • kyuubi.engine.deregister.exception.messages
    • kyuubi.operation.plan.only.excludes
    • kyuubi.server.limit.connections.user.unlimited.list
    • kyuubi.server.administrators
    • kyuubi.metrics.reporters
  • Support skipping blank elements

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before make a pull request

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

No.

@bowenliang123 bowenliang123 changed the title Use set collection for order-insensitive configs Use Set collection for order-insensitive configs Aug 21, 2023
@bowenliang123 bowenliang123 marked this pull request as draft August 21, 2023 13:40
@bowenliang123 bowenliang123 changed the title Use Set collection for order-insensitive configs [Improvement] Use Set collection for order-insensitive configs Aug 22, 2023
@github-actions github-actions bot added the kind:documentation Documentation is a feature! label Aug 22, 2023
@bowenliang123 bowenliang123 marked this pull request as ready for review August 22, 2023 12:15
@codecov-commenter
Copy link

codecov-commenter commented Aug 24, 2023 •
edited
Loading

Codecov Report

Merging #5185 (4b74cd2) into master (f42a7d6) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 4b74cd2 differs from pull request most recent head c656af7. Consider uploading reports for the commit c656af7 to get more accurate results

@@          Coverage Diff           @@
##           master   #5185   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         565     565           
  Lines       31645   31646    +1     
  Branches     4132    4134    +2     
======================================
- Misses      31645   31646    +1
Files Changed Coverage Δ
...ubi/engine/spark/operation/PlanOnlyStatement.scala 0.00% <0.00%> (ø)
...g/apache/spark/kyuubi/SparkSQLEngineListener.scala 0.00% <0.00%> (ø)
...scala/org/apache/kyuubi/config/ConfigBuilder.scala 0.00% <0.00%> (ø)
...scala/org/apache/kyuubi/config/ConfigHelpers.scala 0.00% <0.00%> (ø)
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 0.00% <0.00%> (ø)
...apache/kyuubi/service/TBinaryFrontendService.scala 0.00% <ø> (ø)
...e/authentication/KyuubiAuthenticationFactory.scala 0.00% <0.00%> (ø)
...rvice/authentication/ldap/GroupFilterFactory.scala 0.00% <0.00%> (ø)
...ervice/authentication/ldap/UserFilterFactory.scala 0.00% <0.00%> (ø)
...ala/org/apache/kyuubi/session/SessionManager.scala 0.00% <0.00%> (ø)
... and 4 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bowenliang123 bowenliang123 self-assigned this Aug 24, 2023
@bowenliang123 bowenliang123 added this to the v1.8.0 milestone Aug 24, 2023
@bowenliang123
Copy link
Contributor Author

bowenliang123 commented Aug 25, 2023

Thanks, merged to master(1.8.0).

@bowenliang123 bowenliang123 deleted the conf-toset branch August 25, 2023 02:28
@bowenliang123 bowenliang123 mentioned this pull request Aug 25, 2023
Closed
3 tasks
@pan3793 pan3793 mentioned this pull request Jan 17, 2024
Closed
4 tasks
pan3793 added a commit that referenced this pull request Jan 18, 2024
@pan3793
[KYUUBI #5990] Always take the first declared SASL/PLAIN auth type
3b2e674 
# 🔍 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 <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
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 <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
(cherry picked from commit 3b2e674)
Signed-off-by: Cheng Pan <chengpan@apache.org>
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 <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
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 <chengpan@apache.org>
Signed-off-by: Cheng Pan <chengpan@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pan3793 pan3793 pan3793 approved these changes

Assignees

@bowenliang123 bowenliang123

Labels

kind:documentation Documentation is a feature! module:common module:metrics module:server module:spark

Projects

None yet

Milestone

v1.8.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bowenliang123 @codecov-commenter @pan3793