Support reload certs in HTTPS server.#10302
Conversation
|
I don't know how to write unit tests(without 2 real cert), is there any way？ |
|
I think usually we need a manual test. I don't think tls could be easily tested. |
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(make sure to test this locally). 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. |
| httpServer = sb.build(); | ||
| httpServer.start().join(); | ||
| if (config.isEnableTLS()) { | ||
| scheduledExecutorService.schedule(this::updateCert, 10, TimeUnit.SECONDS); |
There was a problem hiding this comment.
| normalInitialize(sb); | ||
| sb.annotatedService() | ||
| .pathPrefix(config.getContextPath()) | ||
| .build(handlers.toArray()); |
There was a problem hiding this 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.
There was a problem hiding this comment.
reconfigure method will creat a new ServerBuild. So I think it is needed.
There was a problem hiding this comment.
sb.buildServerConfig(config()) only set the host and port.
unit test of Armeria can also prove it.
kezhenxu94
left a comment
kezhenxu94
left a comment
There was a problem hiding this comment.
According to line/armeria#4637 this PR can't be merged until they release a new version containing the mentioned Armeria PR.
|
I have not tested it now. I will test it after they |
|
Armeria 1.22 is released you can continue this. https://armeria.dev/release-notes/1.22.0/ |
|
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. |
|
By following #10409, we don't need to support TLS. |
3 participants
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 #.
Update the
CHANGESlog.