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 80 commits into
base: dev
Choose a base branch
from
Open

[DSIP-76] Support ssl in netty #16673

wants to merge 80 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

davidzollo
davidzollo previously approved these changes Oct 31, 2024
Copy link
Contributor

@davidzollo davidzollo left a comment

Choose a reason for hiding this comment

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

+1
Please add e2e test later

@xdu-chenrj
Copy link
Contributor Author

+1 Please add e2e test later

okay, I will do it

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.

There are still many unaddressed issues in this PR. Such as e2e, K8S, docs, etc...So I don't think we can meet the requirements of merger at present.

@xdu-chenrj xdu-chenrj requested a review from SbloodyS October 31, 2024 13:19
Copy link

sonarcloud bot commented Oct 31, 2024

@@ -200,4 +200,4 @@ jobs:
if [[ ${{ needs.e2e.result }} != 'success' ]]; then
echo "E2E Failed!"
exit -1
fi
fi
Copy link
Member

Choose a reason for hiding this comment

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

Please revert these unrelated changes, this can help the reviewer have a better review.

Comment on lines +14 to +43
To enable SLL authentication, you have two things to do. Firstly, you need to generate `cert.crt` and `private.pem` files.

Step 1: Generate private key (private.pem)

Open the terminal and run the following command to generate a private key:

```bash
openssl genpkey -algorithm RSA -out private.pem -pkeyopt rsa_keygen_bits:2048
```

This command will generate a 2048 bit RSA private key and save it as a private.pem file.

Step 2: Generate Certificate Signing Request (CSR)

Before generating a certificate, you need to generate a Certificate Signing Request (CSR). Run the following command:

```bash
openssl req -new -key private.pem -out request.csr
```

This command will prompt you to enter some information, such as country, state/province, organization name, etc. The information you input will be embedded into the generated certificate.

Step 3: Generate a self signed certificate (cert.crt)

Use CSR to generate self signed certificates. Run the following command:

```bash
openssl x509 -req -days 365 -in request.csr -signkey private.pem -out cert.crt
```

Copy link
Member

Choose a reason for hiding this comment

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

We don't need to address how to generate cert.crt and private.pem.

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.

Why mark these resolved?

@Data
public class NettyRpcConfig {

private 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
private NettySslConfig nettySslConfig;
private NettySslConfig nettySslConfig = new NettySslConfig();

Copy link
Member

Choose a reason for hiding this comment

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

Add default value?

@Data
public class NettySslConfig {

public boolean enabled;
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
public boolean enabled;
public boolean enabled = false;

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
6 participants