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-76] Support ssl in netty #16673

Open
wants to merge 24 commits into
base: dev
Choose a base branch
from
Open

[DSIP-76] Support ssl in netty #16673

wants to merge 24 commits into from

Conversation

xdu-chenrj
Copy link
Contributor

@xdu-chenrj xdu-chenrj commented Sep 27, 2024

close #16688

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

Need to discuss this first in #16672

@SbloodyS SbloodyS changed the title support other business systems to run in DS [DSIP] Support other business systems to run in DS Sep 27, 2024
@wangxj3
Copy link
Contributor

wangxj3 commented Sep 28, 2024

I think you miss the code of master-server .The task of other system sholud call other system in master-server .

@xdu-chenrj xdu-chenrj reopened this Oct 11, 2024
@github-actions github-actions bot removed the UI ui and front end related label Oct 11, 2024
@wangxj3 wangxj3 changed the title [DSIP] Support other business systems to run in DS [DSIP] Support ssl in netty Oct 12, 2024
@wangxj3 wangxj3 changed the title [DSIP] Support ssl in netty [DSIP-76] Support ssl in netty Oct 12, 2024
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

This requires adding at least one of the following tests to ensure stability.

  • cluster-test
  • api-test
  • e2e-test

@SbloodyS SbloodyS added this to the 3.3.0 milestone Oct 12, 2024
@SbloodyS SbloodyS added the DSIP label Oct 12, 2024
@xdu-chenrj
Copy link
Contributor Author

This requires adding at least one of the following tests to ensure stability.

  • cluster-test
  • api-test
  • e2e-test

I will complete it later

@xdu-chenrj
Copy link
Contributor Author

This requires adding at least one of the following tests to ensure stability.

  • cluster-test
  • api-test
  • e2e-test

Is it adding UT, testing functions in the class?

@SbloodyS
Copy link
Member

SbloodyS commented Oct 12, 2024

Is it adding UT, testing functions in the class?

UT and these tests are different types of tests. You can find the corresponding examples in the following links.

@xdu-chenrj
Copy link
Contributor Author

Is it adding UT, testing functions in the class?

UT and these tests are different types of tests. You can find the corresponding examples in the following links.

Thank you

@github-actions github-actions bot added the CI&CD label Oct 16, 2024
@SbloodyS
Copy link
Member

I found that it has not reached a coverage rate of 60%. How can I increase coverage? Is it possible to increase arbitrarily?

Yes. You should add more UT.

Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

Missing docs, unit-test.

Comment on lines +146 to +147
- name: SslShellTaskE2ETest
class: org.apache.dolphinscheduler.e2e.cases.ssl.SslShellTaskE2ETest
Copy link
Member

Choose a reason for hiding this comment

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

This test case is not running normally, the cluster is not started. You should check it.
https://github.com/apache/dolphinscheduler/actions/runs/11365198466?pr=16673

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case is not running normally, the cluster is not started. You should check it. https://github.com/apache/dolphinscheduler/actions/runs/11365198466?pr=16673

can you tell me where the log of cluster startup exception is? how to determine if a cluster is starting up properly? thank you

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is still unaddressed. Don't resolve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that all the checks have been passed, why hasn't this problem been resolved yet

Copy link
Member

Choose a reason for hiding this comment

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

SslShellTaskE2ETest is not executed at all in e2e test cases.

@SbloodyS SbloodyS added miss:docs missing documents in PR miss:tests missing unit tests in PR labels Oct 17, 2024
@xdu-chenrj
Copy link
Contributor Author

Missing docs, unit-test.

ok

Comment on lines 53 to 58
strategy:
matrix:
case:
- name: SslShellTaskE2ETest
class: org.apache.dolphinscheduler.e2e.cases.ssl.SslShellTaskE2ETest

Copy link
Member

Choose a reason for hiding this comment

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

Why adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

add an entrance for running E2E in consolidation, k8s.yml, where the running image is a standalone one

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is meaningful for the current e2e-k8s tests. Please remove this modification.

.github/workflows/e2e.yml Outdated Show resolved Hide resolved
docs/docs/en/guide/installation/cluster.md Outdated Show resolved Hide resolved
docs/docs/en/guide/installation/cluster.md Outdated Show resolved Hide resolved
docs/docs/en/guide/installation/cluster.md Outdated Show resolved Hide resolved
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.

It's better to add a new class NettyRpcConfig, and NettySslConfig as a field in NettyRpcConfig, we will add more configuration in the latest.

Comment on lines +44 to +45
RegistryConfiguration.class,
NettySslConfig.class})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RegistryConfiguration.class,
NettySslConfig.class})
RegistryConfiguration.class})

Comment on lines 64 to 65
RegistryConfiguration.class,
NettySslConfig.class})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RegistryConfiguration.class,
NettySslConfig.class})
RegistryConfiguration.class})

Comment on lines 99 to 101
@Autowired
NettySslConfig nettySslConfig;

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Autowired
NettySslConfig nettySslConfig;

Comment on lines 59 to 60
RegistryConfiguration.class,
NettySslConfig.class})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RegistryConfiguration.class,
NettySslConfig.class})
RegistryConfiguration.class})

Comment on lines 76 to 77
@Autowired
NettySslConfig nettySslConfig;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Autowired
NettySslConfig nettySslConfig;

Copy link
Member

@Gallardot Gallardot left a comment

Choose a reason for hiding this comment

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

If this DSIP is to be merged, I believe there is more work to be done.

How will certificates be managed? How will certificate expiration be handled? Is there a certificate rotation and renewal mechanism?
If deploy on K8S through Helm charts, how can SSL be enabled? Are there relevant configuration options in the Helm charts? Can E2E-K8S add corresponding test cases for this?

Comment on lines 53 to 58
strategy:
matrix:
case:
- name: SslShellTaskE2ETest
class: org.apache.dolphinscheduler.e2e.cases.ssl.SslShellTaskE2ETest

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is meaningful for the current e2e-k8s tests. Please remove this modification.

Copy link

sonarcloud bot commented Oct 19, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend CI&CD document DSIP e2e e2e test miss:docs missing documents in PR miss:tests missing unit tests in PR test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DSIP-76][RPC] Support ssl at RPC
5 participants