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

[DSIP] Support ssl at RPC #15802

Closed
wants to merge 22 commits into from
Closed

Conversation

fuchanghai
Copy link
Member

Purpose of the pull request

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

If your pull request contain incompatible change, you should also add it to docs/docs/en/guide/upgrede/incompatible.md

Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

Could you please add doc and ut for this?

Comment on lines 738 to 747


/**
* netty config enable
*/
public static final String NETTY_SSL_ENABLE = "netty.ssl.enable";

public static final String NETTY_SSL_KEY_PATH = "netty.ssl.key.path";

public static final String NETTY_SSL_CERT_PATH = "netty.ssl.cert.path";
Copy link
Member

Choose a reason for hiding this comment

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

Pleas move this kind of config into application.yaml, we shouldn't add new config into common.properties.

rpc:
   ssl:
       enable: true
       cert-file: 
       key-file: 

Copy link
Member Author

Choose a reason for hiding this comment

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

i add config into common.properties,Because if the master service uses SSL authentication, the worker must also be configured。 so i think only config into common better than config both into master and worker

Copy link
Contributor

Choose a reason for hiding this comment

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

Overall LGTM, Even if these configs are in common.properties, the common.properties of the master and worker may have different configs. I think it is necessary to deal with the inconsistency in encryption.

So I agree with @ruanwenjun that we could move these configs to application.yaml

@ruanwenjun ruanwenjun changed the title [improvement] netty ssl [DSIP] Support ssl at RPC Apr 6, 2024
@ruanwenjun ruanwenjun added the feature new feature label Apr 6, 2024
@ruanwenjun
Copy link
Member

Since this will import new feature, please create a DSIP issue, you can follow #14102

@fuchanghai
Copy link
Member Author

Could you please add doc and ut for this?

of course ,I will add it recently

…n/java/org/apache/dolphinscheduler/extract/base/NettyRemotingClient.java

Co-authored-by: Wenjun Ruan <[email protected]>
@SbloodyS SbloodyS added the 3.3.0 label Apr 11, 2024
@SbloodyS SbloodyS added this to the 3.3.0 milestone Apr 11, 2024
@fuchanghai
Copy link
Member Author

I want to wait until my code structure is ok and everyone’s review has passed before I add UT. @ruanwenjun @rickchengx

@Gallardot
Copy link
Member

The documentation on how to use SSL also needs to be updated. At the same time, the Helm charts need to be updated to support this capability. @fuchanghai

@fuchanghai
Copy link
Member Author

The documentation on how to use SSL also needs to be updated. At the same time, the Helm charts need to be updated to support this capability. @fuchanghai

get

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2024

Codecov Report

Attention: Patch coverage is 53.84615% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 39.86%. Comparing base (60b019b) to head (626be70).

❗ Current head 626be70 differs from pull request most recent head 3c11124. Consider uploading reports for the commit 3c11124 to get more accurate results

Files Patch % Lines
...duler/extract/base/client/NettyRemotingClient.java 27.27% 6 Missing and 2 partials ⚠️
...duler/extract/base/server/NettyRemotingServer.java 27.27% 6 Missing and 2 partials ⚠️
...e/dolphinscheduler/server/master/MasterServer.java 0.00% 1 Missing ⚠️
...e/dolphinscheduler/server/worker/WorkerServer.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15802      +/-   ##
============================================
- Coverage     39.93%   39.86%   -0.08%     
+ Complexity     5081     5061      -20     
============================================
  Files          1369     1369              
  Lines         45635    45662      +27     
  Branches       4869     4872       +3     
============================================
- Hits          18224    18201      -23     
- Misses        25513    25558      +45     
- Partials       1898     1903       +5     

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

Copy link

sonarcloud bot commented May 9, 2024

…etty-secruity

# Conflicts:
#	dolphinscheduler-alert/dolphinscheduler-alert-server/src/main/java/org/apache/dolphinscheduler/alert/rpc/AlertRpcServer.java
#	dolphinscheduler-api/src/main/java/org/apache/dolphinscheduler/api/ApiApplicationServer.java
#	dolphinscheduler-api/src/test/java/org/apache/dolphinscheduler/api/service/LoggerServiceTest.java
#	dolphinscheduler-extract/dolphinscheduler-extract-base/src/main/java/org/apache/dolphinscheduler/extract/base/NettyRemotingClient.java
#	dolphinscheduler-extract/dolphinscheduler-extract-base/src/main/java/org/apache/dolphinscheduler/extract/base/server/NettyRemotingServer.java
#	dolphinscheduler-extract/dolphinscheduler-extract-base/src/main/java/org/apache/dolphinscheduler/extract/base/server/NettyRemotingServerFactory.java
#	dolphinscheduler-extract/dolphinscheduler-extract-base/src/test/java/org/apache/dolphinscheduler/extract/base/client/SingletonJdkDynamicRpcClientProxyFactoryTest.java
#	dolphinscheduler-master/src/main/java/org/apache/dolphinscheduler/server/master/rpc/MasterRpcServer.java
#	dolphinscheduler-microbench/src/main/java/org/apache/dolphinscheduler/microbench/rpc/RpcBenchMarkTest.java
#	dolphinscheduler-worker/src/main/java/org/apache/dolphinscheduler/server/worker/rpc/WorkerRpcServer.java
@fuchanghai fuchanghai reopened this Aug 18, 2024
@github-actions github-actions bot added the test label Aug 18, 2024
@SbloodyS SbloodyS removed this from the 3.3.0 milestone Sep 5, 2024
@davidzollo
Copy link
Contributor

Thanks for the contribution from changhai , there are some conflicts, and now @wangxj3 will submit a new PR to solve this issue refer to this PR.

Thanks again

@davidzollo davidzollo closed this Sep 23, 2024
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.

7 participants