You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by GitBox <gi...@apache.org> on 2020/03/09 09:14:51 UTC
[GitHub] [skywalking] hanahmily opened a new pull request #4470: Enable OAP
gRPC SSL transportation
hanahmily opened a new pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470
Porting to OpenSSL to enable SSL transportation. The server private
key is in the format of PCKS#8, certificates is x509 though.
Signed-off-by: Gao Hongtao <ha...@gmail.com>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io edited a comment on issue #4470: Enable OAP
gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596432355
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=h1) Report
> Merging [#4470](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/d370b303ebedbe7a1b937185c34e86d5a762ba27&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4470 +/- ##
=======================================
Coverage 25.50% 25.50%
=======================================
Files 1244 1244
Lines 28919 28919
Branches 3958 3958
=======================================
Hits 7375 7375
Misses 20865 20865
Partials 679 679
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=footer). Last update [d370b30...d370b30](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392052837
##########
File path: oap-server/server-receiver-plugin/skywalking-sharing-server-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/sharing/server/SharingServerModuleProvider.java
##########
@@ -80,6 +81,15 @@ public void prepare() {
if (config.getGRPCPort() != 0) {
grpcServer = new GRPCServer(Strings.isBlank(config.getGRPCHost()) ? "0.0.0.0" : config.getGRPCHost(), config
Review comment:
I think you miss the deletion of this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng merged pull request #4470: Enable OAP gRPC
SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng merged pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on issue #4470: Enable OAP gRPC
SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-598556447
@wu-sheng e2e test case is added and passed. now I think we can merge it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392178836
##########
File path: test/e2e/e2e-ssl/src/docker/certs/server-key.pem
##########
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
Review comment:
My point is, this is for test. Could we set it forever? Or let's say 100 years?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392578237
##########
File path: test/e2e/e2e-ssl/src/docker/certs/server-key.pem
##########
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
Review comment:
Now we get the 100 years/ century certificates.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392053104
##########
File path: test/e2e/e2e-ssl/e2e-ssl.iml
##########
@@ -0,0 +1,120 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
I think this should not be uploaded? Why git ignore is not working for this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4470: Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r389581247
##########
File path: docs/en/setup/backend/grpc-ssl.md
##########
@@ -0,0 +1,33 @@
+#Support gRPC SSL transportation for OAP server
Review comment:
without space, the header style is not correctly rendered
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4470: Enable OAP gRPC SSL
transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596492802
@hanahmily This is the agent side TLS doc for you, https://github.com/apache/skywalking/blob/master/docs/en/setup/service-agent/java-agent/TLS.md
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392053297
##########
File path: test/e2e/e2e-ssl/src/docker/certs/server-key.pem
##########
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
Review comment:
Are these cert files alway valid? No expired time?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4470: Enable OAP gRPC SSL
transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596588437
SSL for outside only could active when sharing server activated. Am I right?
I don't think we should make the internal SSL CA as same as OAP internal. WDYT?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io edited a comment on issue #4470: Enable OAP
gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596432355
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=h1) Report
> Merging [#4470](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/d5efc97c803d3ea249a157751d6aa7af8b0fcc9b?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `18.91%`.
[](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4470 +/- ##
==========================================
+ Coverage 25.49% 25.49% +<.01%
==========================================
Files 1244 1244
Lines 28895 28919 +24
Branches 3953 3958 +5
==========================================
+ Hits 7367 7374 +7
- Misses 20849 20865 +16
- Partials 679 680 +1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/skywalking/oap/server/core/CoreModuleConfig.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvQ29yZU1vZHVsZUNvbmZpZy5qYXZh) | `0% <ø> (ø)` | :arrow_up: |
| [...ing/oap/server/library/server/grpc/GRPCServer.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItbGlicmFyeS9saWJyYXJ5LXNlcnZlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2xpYnJhcnkvc2VydmVyL2dycGMvR1JQQ1NlcnZlci5qYXZh) | `0% <0%> (ø)` | :arrow_up: |
| [...ing/oap/server/library/client/grpc/GRPCClient.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItbGlicmFyeS9saWJyYXJ5LWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2xpYnJhcnkvY2xpZW50L2dycGMvR1JQQ0NsaWVudC5qYXZh) | `0% <0%> (ø)` | :arrow_up: |
| [...skywalking/oap/server/core/CoreModuleProvider.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvQ29yZU1vZHVsZVByb3ZpZGVyLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...er/sharing/server/SharingServerModuleProvider.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItcmVjZWl2ZXItcGx1Z2luL3NreXdhbGtpbmctc2hhcmluZy1zZXJ2ZXItcGx1Z2luL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9za3l3YWxraW5nL29hcC9zZXJ2ZXIvcmVjZWl2ZXIvc2hhcmluZy9zZXJ2ZXIvU2hhcmluZ1NlcnZlck1vZHVsZVByb3ZpZGVyLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...ap/server/core/remote/client/GRPCRemoteClient.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcmVtb3RlL2NsaWVudC9HUlBDUmVtb3RlQ2xpZW50LmphdmE=) | `71.08% <100%> (+0.35%)` | :arrow_up: |
| [...server/core/remote/client/RemoteClientManager.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvcmVtb3RlL2NsaWVudC9SZW1vdGVDbGllbnRNYW5hZ2VyLmphdmE=) | `80.85% <25%> (-5.52%)` | :arrow_down: |
| [...ache/skywalking/apm/agent/core/jvm/JVMService.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvanZtL0pWTVNlcnZpY2UuamF2YQ==) | `55% <0%> (-1.67%)` | :arrow_down: |
| [...walking/oap/server/core/analysis/Downsampling.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvYW5hbHlzaXMvRG93bnNhbXBsaW5nLmphdmE=) | `100% <0%> (+100%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=footer). Last update [d5efc97...43c9049](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392162664
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/remote/client/RemoteClientManager.java
##########
@@ -55,21 +59,29 @@
private static final Logger logger = LoggerFactory.getLogger(RemoteClientManager.class);
private final ModuleDefineHolder moduleDefineHolder;
+ private final boolean grpcSslEnabled;
+ private final String grpcSslTrustCAPath;
private ClusterNodesQuery clusterNodesQuery;
private volatile List<RemoteClient> usingClients;
private GaugeMetrics gauge;
private int remoteTimeout;
/**
* Initial the manager for all remote communication clients.
- *
- * @param moduleDefineHolder for looking up other modules
+ * @param moduleDefineHolder for looking up other modules
* @param remoteTimeout for cluster internal communication, in second unit.
+ * @param grpcSslEnabled true: enable SSL between clusters, false: plain gRPC between the cluster.
+ * @param grpcSslTrustCAPath trust certificate authorities file path.
*/
- public RemoteClientManager(ModuleDefineHolder moduleDefineHolder, int remoteTimeout) {
+ public RemoteClientManager(ModuleDefineHolder moduleDefineHolder,
Review comment:
done. keep all parameter list the same way.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392052234
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/remote/client/RemoteClientManager.java
##########
@@ -55,21 +59,29 @@
private static final Logger logger = LoggerFactory.getLogger(RemoteClientManager.class);
private final ModuleDefineHolder moduleDefineHolder;
+ private final boolean grpcSslEnabled;
+ private final String grpcSslTrustCAPath;
private ClusterNodesQuery clusterNodesQuery;
private volatile List<RemoteClient> usingClients;
private GaugeMetrics gauge;
private int remoteTimeout;
/**
* Initial the manager for all remote communication clients.
- *
- * @param moduleDefineHolder for looking up other modules
+ * @param moduleDefineHolder for looking up other modules
* @param remoteTimeout for cluster internal communication, in second unit.
+ * @param grpcSslEnabled true: enable SSL between clusters, false: plain gRPC between the cluster.
+ * @param grpcSslTrustCAPath trust certificate authorities file path.
*/
- public RemoteClientManager(ModuleDefineHolder moduleDefineHolder, int remoteTimeout) {
+ public RemoteClientManager(ModuleDefineHolder moduleDefineHolder,
Review comment:
Could you make the core style consistently. `GRPCRemoteClient` client use two constructors for ssl ON/OFF, here using a boolean and empty/null string.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r389776661
##########
File path: docs/en/setup/backend/grpc-ssl.md
##########
@@ -0,0 +1,32 @@
+# Support gRPC SSL transportation for OAP server
+
+For OAP communication we are currently using gRPC, a multi-platform RPC framework that uses protocol buffers for
+message serialization. The nice part about gRPC is that it promotes the use of SSL/TLS to authenticate and encrypt
+exchanges. Now OAP supports to enable SSL transportation for gRPC receivers.
+
+You can follow below steps to enable this feature
+
+## Creating SSL/TLS Certificates
+
+It seems like step one is to generate certificates and key files for encrypting communication. I thought this would be
+fairly straightforward using `openssl` from the command line, However, it may be simpler to use
+[certstrap](https://github.com/square/certstrap), a simple certificate manager written in Go by the folks at Square.
+The app avoids dealing with `openssl`, but has a very simple workflow: create a certificate authority, sign certificates
+with it.
+
+After signing the certificates of OAP server, we should convert private key to a PKCS8 format before placing it into the host.
+
+```
+$ openssl pkcs8 -topk8 -inform PEM -outform PEM -nocrypt -in server.key -out server-key.pem
+```
+
+## Config OAP server
+
+You can enable gRPC SSL by add following lines to `application.yml/core/default`.
+```json
+gRPCSslEnabled: true
+gRPCSslKeyPath: /path/to/server-key.pem
+gRPCSslCertChainPath: /path/to/server.crt
+```
+
+If you port to java agent, refer to [TLS.md](../service-agent/java-agent/TLS.md) to config java agent to enable TLS.
Review comment:
This linked file should be updated too, to keep consistently
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392162764
##########
File path: oap-server/server-receiver-plugin/skywalking-sharing-server-plugin/src/main/java/org/apache/skywalking/oap/server/receiver/sharing/server/SharingServerModuleProvider.java
##########
@@ -80,6 +81,15 @@ public void prepare() {
if (config.getGRPCPort() != 0) {
grpcServer = new GRPCServer(Strings.isBlank(config.getGRPCHost()) ? "0.0.0.0" : config.getGRPCHost(), config
Review comment:
removed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4470: Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r389579586
##########
File path: docs/en/setup/backend/grpc-ssl.md
##########
@@ -0,0 +1,33 @@
+#Support gRPC SSL transportation for OAP server
+
+For OAP communication we are currently using gRPC, a multi-platform RPC framework that uses protocol buffers for
+message serialization. The nice part about gRPC is that it promotes the use of SSL/TLS to authenticate and encrypt
+exchanges. Now OAP support to enable SSL transportation for gRPC receivers.
Review comment:
```suggestion
exchanges. Now OAP supports to enable SSL transportation for gRPC receivers.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4470: Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r389579300
##########
File path: docs/en/setup/backend/grpc-ssl.md
##########
@@ -0,0 +1,33 @@
+#Support gRPC SSL transportation for OAP server
Review comment:
```suggestion
# Support gRPC SSL transportation for OAP server
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4470: Enable OAP gRPC SSL
transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596452724
Please add the ssl into auth test case, single group, e2e
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392052385
##########
File path: oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/remote/client/GRPCRemoteClient.java
##########
@@ -99,7 +110,7 @@ GRPCClient getClient() {
if (Objects.isNull(client)) {
synchronized (GRPCRemoteClient.class) {
if (Objects.isNull(client)) {
- this.client = new GRPCClient(address.getHost(), address.getPort());
+ this.client = new GRPCClient(address.getHost(), address.getPort(), sslContext);
Review comment:
A similar code style inconsistent here, using `sslContext==null` represents no SSL.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] wu-sheng commented on issue #4470: Enable OAP gRPC SSL
transportation
Posted by GitBox <gi...@apache.org>.
wu-sheng commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-598681488
You need to fix CI
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392163280
##########
File path: test/e2e/e2e-ssl/e2e-ssl.iml
##########
@@ -0,0 +1,120 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
deleted it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on issue #4470: Enable OAP gRPC
SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596585337
> 1. Only `ReceiverGRPCHandlerRegister` should support only server-side receiver. You could read the codes, only it has the `#addFilter` for `AuthenticationInterceptor`.
Don't get your point.`ReceiverGRPCHandlerRegister` intends to add `interceptor` or `service` to `NettyServerBuilder` by invoking its `addService` method. But SSL should invoke `NettyServerBuilder#sslContext`. I don't find any link between them. Do I miss something? feel free to correct me.
> 2. If we talk about the core module, you have to implement the client side TLS inside `GRPCRemoteClient`, in oder to make sure it matches the core configuration. And this TLS should be used for internal only.
make sense. I should enable client ssl at the same time. There's also a scenario for `sharing-server`, if we want to enable external SSL and disable internal SSL, client ssl needs to read config from `sharing-server` instead of `core`, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on issue #4470: Enable OAP gRPC
SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596611396
> SSL for outside only could active when sharing server activated. Am I right?
right
> I don't think we should make the internal SSL CA as same as OAP internal. WDYT?
No. SSL becomes more widely accepted. We should support it both internally and externally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io edited a comment on issue #4470: Enable OAP
gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596432355
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=h1) Report
> Merging [#4470](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/4dad85283585acbe9a5eb51056791917d099c307&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4470 +/- ##
=======================================
Coverage 25.54% 25.54%
=======================================
Files 1244 1244
Lines 28869 28869
Branches 3949 3949
=======================================
Hits 7374 7374
Misses 20817 20817
Partials 678 678
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=footer). Last update [4dad852...4dad852](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] kezhenxu94 commented on a change in pull request
#4470: Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
kezhenxu94 commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r389581247
##########
File path: docs/en/setup/backend/grpc-ssl.md
##########
@@ -0,0 +1,33 @@
+#Support gRPC SSL transportation for OAP server
Review comment:
without a space, the header style is not correctly rendered
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] hanahmily commented on a change in pull request #4470:
Enable OAP gRPC SSL transportation
Posted by GitBox <gi...@apache.org>.
hanahmily commented on a change in pull request #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#discussion_r392163501
##########
File path: test/e2e/e2e-ssl/src/docker/certs/server-key.pem
##########
@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
Review comment:
2 years. Is it enough?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [skywalking] codecov-io commented on issue #4470: Enable OAP gRPC
SSL transportation
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #4470: Enable OAP gRPC SSL transportation
URL: https://github.com/apache/skywalking/pull/4470#issuecomment-596432355
# [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=h1) Report
> Merging [#4470](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=desc) into [master](https://codecov.io/gh/apache/skywalking/commit/b059f7f9357e0c6eafad12008c348aeb6fef1864?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `0%`.
[](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #4470 +/- ##
==========================================
- Coverage 24.95% 24.94% -0.01%
==========================================
Files 1231 1231
Lines 28496 28499 +3
Branches 3905 3906 +1
==========================================
- Hits 7111 7110 -1
- Misses 20726 20729 +3
- Partials 659 660 +1
```
| [Impacted Files](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/skywalking/oap/server/core/CoreModuleConfig.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvQ29yZU1vZHVsZUNvbmZpZy5qYXZh) | `0% <ø> (ø)` | :arrow_up: |
| [...ing/oap/server/library/server/grpc/GRPCServer.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItbGlicmFyeS9saWJyYXJ5LXNlcnZlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2xpYnJhcnkvc2VydmVyL2dycGMvR1JQQ1NlcnZlci5qYXZh) | `0% <0%> (ø)` | :arrow_up: |
| [...skywalking/oap/server/core/CoreModuleProvider.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-b2FwLXNlcnZlci9zZXJ2ZXItY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2t5d2Fsa2luZy9vYXAvc2VydmVyL2NvcmUvQ29yZU1vZHVsZVByb3ZpZGVyLmphdmE=) | `0% <0%> (ø)` | :arrow_up: |
| [...ache/skywalking/apm/agent/core/jvm/JVMService.java](https://codecov.io/gh/apache/skywalking/pull/4470/diff?src=pr&el=tree#diff-YXBtLXNuaWZmZXIvYXBtLWFnZW50LWNvcmUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NreXdhbGtpbmcvYXBtL2FnZW50L2NvcmUvanZtL0pWTVNlcnZpY2UuamF2YQ==) | `55% <0%> (-1.67%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=footer). Last update [b059f7f...52764bf](https://codecov.io/gh/apache/skywalking/pull/4470?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services