You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "yswdqz (via GitHub)" <gi...@apache.org> on 2023/01/23 06:11:00 UTC
[GitHub] [skywalking] yswdqz opened a new pull request, #10302: Support reload certs in HTTPS server.
yswdqz opened a new pull request, #10302:
URL: https://github.com/apache/skywalking/pull/10302
- [ ] If this is non-trivial feature, paste the links/URLs to the design doc.
- [ ] Update the documentation to include this new feature.
- [ ] Tests(including UT, IT, E2E) are added to verify the new feature.
- [ ] If it's UI related, attach the screenshots below.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [x] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] yswdqz commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1400552787
I have not tested it now. I will test it after they `armeria` release a new version.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1399937122
I think usually we need a manual test. I don't think tls could be easily tested.
Wait for @kezhenxu94
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng closed pull request #10302: Support reload certs in HTTPS server.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng closed pull request #10302: Support reload certs in HTTPS server.
URL: https://github.com/apache/skywalking/pull/10302
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1400150113
> I think usually we need a manual test. I don't think tls could be easily tested.
> Wait for @kezhenxu94
To test the reloading mechanism, we can recreate new cert pairs with scripts and update them in test and verify the API still works before and after the update, but I'm totally ok if you don't want to test this case. But we need to add a basic case that verify the https works, the same situation exists in our gRPC agent protocol server in this repo. You can take that as an example.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] yswdqz commented on a diff in pull request #10302: Support reload certs in HTTPS server.
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on code in PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#discussion_r1084161217
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -107,15 +149,51 @@ public void addHandler(Object handler, List<HttpMethod> httpMethods) {
"Bind handler {} into http server {}:{}",
handler.getClass().getSimpleName(), config.getHost(), config.getPort()
);
-
+ if (config.isEnableTLS()) {
+ handlers.add(handler);
+ }
sb.annotatedService()
.pathPrefix(config.getContextPath())
.build(handler);
this.allowedMethods.addAll(httpMethods);
}
+ public void updateCert() {
+ FileTime lastModifiedTimeCertNow;
+ FileTime lastModifiedTimeKeyNow;
+ try {
+ lastModifiedTimeCertNow = Files.getLastModifiedTime(tlsCertChainPath);
+ lastModifiedTimeKeyNow = Files.getLastModifiedTime(tlsKeyPath);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ if (lastModifiedTimeCertNow.equals(lastModifiedTimeCert)
+ && lastModifiedTimeKeyNow.equals(lastModifiedTimeKey)) {
+ return;
+ }
+ log.info("TLS cert chain file or TLS key file has been updated, reloading...");
+ httpServer.reconfigure(sb -> {
+ try (InputStream cert = new FileInputStream(tlsCertChainPath.toFile());
+ InputStream key = PrivateKeyUtil.loadDecryptionKey(tlsKeyPath.toString())) {
+ sb.tls(cert, key);
+ } catch (IOException e) {
+ throw new IllegalArgumentException(e);
+ }
+ normalInitialize(sb);
+ sb.annotatedService()
+ .pathPrefix(config.getContextPath())
+ .build(handlers.toArray());
Review Comment:
`reconfigure` method will creat a new ServerBuild. So I think it is needed.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1423925533
Armeria 1.22 is released you can continue this. https://armeria.dev/release-notes/1.22.0/
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] yswdqz commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1399853979
I forget add a unit test, I will do it later.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] kezhenxu94 commented on a diff in pull request #10302: Support reload certs in HTTPS server.
Posted by "kezhenxu94 (via GitHub)" <gi...@apache.org>.
kezhenxu94 commented on code in PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#discussion_r1083912039
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -107,15 +149,51 @@ public void addHandler(Object handler, List<HttpMethod> httpMethods) {
"Bind handler {} into http server {}:{}",
handler.getClass().getSimpleName(), config.getHost(), config.getPort()
);
-
+ if (config.isEnableTLS()) {
+ handlers.add(handler);
+ }
sb.annotatedService()
.pathPrefix(config.getContextPath())
.build(handler);
this.allowedMethods.addAll(httpMethods);
}
+ public void updateCert() {
+ FileTime lastModifiedTimeCertNow;
+ FileTime lastModifiedTimeKeyNow;
+ try {
+ lastModifiedTimeCertNow = Files.getLastModifiedTime(tlsCertChainPath);
+ lastModifiedTimeKeyNow = Files.getLastModifiedTime(tlsKeyPath);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ if (lastModifiedTimeCertNow.equals(lastModifiedTimeCert)
+ && lastModifiedTimeKeyNow.equals(lastModifiedTimeKey)) {
+ return;
+ }
+ log.info("TLS cert chain file or TLS key file has been updated, reloading...");
+ httpServer.reconfigure(sb -> {
+ try (InputStream cert = new FileInputStream(tlsCertChainPath.toFile());
+ InputStream key = PrivateKeyUtil.loadDecryptionKey(tlsKeyPath.toString())) {
+ sb.tls(cert, key);
+ } catch (IOException e) {
+ throw new IllegalArgumentException(e);
+ }
+ normalInitialize(sb);
+ sb.annotatedService()
+ .pathPrefix(config.getContextPath())
+ .build(handlers.toArray());
+ });
+ lastModifiedTimeCert = lastModifiedTimeCertNow;
+ lastModifiedTimeKey = lastModifiedTimeKeyNow;
+ }
+
@Override
public void start() {
- sb.build().start().join();
+ httpServer = sb.build();
+ httpServer.start().join();
+ if (config.isEnableTLS()) {
+ scheduledExecutorService.schedule(this::updateCert, 10, TimeUnit.SECONDS);
Review Comment:
Reuse the existing classes please https://github.com/apache/skywalking/blob/master/oap-server/server-library/library-util/src/main/java/org/apache/skywalking/oap/server/library/util/MultipleFilesChangeMonitor.java
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -107,15 +149,51 @@ public void addHandler(Object handler, List<HttpMethod> httpMethods) {
"Bind handler {} into http server {}:{}",
handler.getClass().getSimpleName(), config.getHost(), config.getPort()
);
-
+ if (config.isEnableTLS()) {
+ handlers.add(handler);
+ }
sb.annotatedService()
.pathPrefix(config.getContextPath())
.build(handler);
this.allowedMethods.addAll(httpMethods);
}
+ public void updateCert() {
+ FileTime lastModifiedTimeCertNow;
+ FileTime lastModifiedTimeKeyNow;
+ try {
+ lastModifiedTimeCertNow = Files.getLastModifiedTime(tlsCertChainPath);
+ lastModifiedTimeKeyNow = Files.getLastModifiedTime(tlsKeyPath);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ if (lastModifiedTimeCertNow.equals(lastModifiedTimeCert)
+ && lastModifiedTimeKeyNow.equals(lastModifiedTimeKey)) {
+ return;
+ }
+ log.info("TLS cert chain file or TLS key file has been updated, reloading...");
+ httpServer.reconfigure(sb -> {
+ try (InputStream cert = new FileInputStream(tlsCertChainPath.toFile());
+ InputStream key = PrivateKeyUtil.loadDecryptionKey(tlsKeyPath.toString())) {
+ sb.tls(cert, key);
+ } catch (IOException e) {
+ throw new IllegalArgumentException(e);
+ }
+ normalInitialize(sb);
+ sb.annotatedService()
+ .pathPrefix(config.getContextPath())
+ .build(handlers.toArray());
Review Comment:
Can you confirm this is needed when we only want to reconfigure the tls cert? I think Armeria will maintain the existing service endpoint if we didn't add new or remove existing service.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] wu-sheng commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1436003425
By following https://github.com/apache/skywalking/pull/10409, we don't need to support TLS.
Close this too.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] yswdqz commented on pull request #10302: Support reload certs in HTTPS server.
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#issuecomment-1424142361
Got it , But when I was writing tests for dynamodb, I found that after loading the certificate on skywalking, aws would not successfully send data to the endpoint, while when loading the certificate on nginx and routing it to skywalking, aws could successfully send data to the endpoint. So the existing https server may have some bugs, I will test and finish this PR after the PR about dynamodb is finished
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [skywalking] yswdqz commented on a diff in pull request #10302: Support reload certs in HTTPS server.
Posted by "yswdqz (via GitHub)" <gi...@apache.org>.
yswdqz commented on code in PR #10302:
URL: https://github.com/apache/skywalking/pull/10302#discussion_r1084185582
##########
oap-server/server-library/library-server/src/main/java/org/apache/skywalking/oap/server/library/server/http/HTTPServer.java:
##########
@@ -107,15 +149,51 @@ public void addHandler(Object handler, List<HttpMethod> httpMethods) {
"Bind handler {} into http server {}:{}",
handler.getClass().getSimpleName(), config.getHost(), config.getPort()
);
-
+ if (config.isEnableTLS()) {
+ handlers.add(handler);
+ }
sb.annotatedService()
.pathPrefix(config.getContextPath())
.build(handler);
this.allowedMethods.addAll(httpMethods);
}
+ public void updateCert() {
+ FileTime lastModifiedTimeCertNow;
+ FileTime lastModifiedTimeKeyNow;
+ try {
+ lastModifiedTimeCertNow = Files.getLastModifiedTime(tlsCertChainPath);
+ lastModifiedTimeKeyNow = Files.getLastModifiedTime(tlsKeyPath);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ if (lastModifiedTimeCertNow.equals(lastModifiedTimeCert)
+ && lastModifiedTimeKeyNow.equals(lastModifiedTimeKey)) {
+ return;
+ }
+ log.info("TLS cert chain file or TLS key file has been updated, reloading...");
+ httpServer.reconfigure(sb -> {
+ try (InputStream cert = new FileInputStream(tlsCertChainPath.toFile());
+ InputStream key = PrivateKeyUtil.loadDecryptionKey(tlsKeyPath.toString())) {
+ sb.tls(cert, key);
+ } catch (IOException e) {
+ throw new IllegalArgumentException(e);
+ }
+ normalInitialize(sb);
+ sb.annotatedService()
+ .pathPrefix(config.getContextPath())
+ .build(handlers.toArray());
Review Comment:
`sb.buildServerConfig(config())` only set the host and port.
[unit test of Armeria](https://github.com/line/armeria/blob/871d87297e4d051241589cb1ae95641cbc83f880/core/src/test/java/com/linecorp/armeria/client/ReconfigurableServerTest.java#L120) can also prove 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.
To unsubscribe, e-mail: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org