Skip to content

Conversation

@BewareMyPower
Copy link
Contributor

@BewareMyPower BewareMyPower commented Oct 24, 2021

Motivation

The tlsEnabled config was marked as deprecated a long time ago. See #2831 for broker's config and #2988 for functions worker's config. This config is confused and meaningless. Because even if tlsEnabled is true, the client or admin still cannot connect to the TLS advertised address (like pulsar+ssl://localhost:6651) or TLS web service URL (like https://localhost:8081) if the service is not running.

Currently there're two cases that check tlsEnabled.

  1. When a namespace is deleted by admin while tlsEnabled is false, even if isRequestHttps is true, which means the admin sent the original request to https://<host>:<port>, the redirect URL will be the plaintext URL (http://<host>:<port>). However, in this case broker might not have a HTTP service on plaintext web service URL, so redirecting to http://<host>:<port> is illegal.
  2. If tlsEnabled is false in functions worker's config, WorkerConfig#getTlsEnabled will always return false even if the workerPort is configured while workerPortTls is configured.

Modifications

  • Remove the tlsEnabled check in WorkerConfig#getTlsEnabled.
  • Add a isWebServiceTlsEnabled method in ServiceConfiguration, which returns whether there's a TLS web service endpoint. Then replace all isTlsEnabled() calls with isWebServiceTlsEnabled().
  • In PulsarService#isBrokerClientTlsEnabled and WebSocketService#createClientInstance, check isBrokerClientTlsEnabled() for whether to connect a TLS service endpoint.
  • Remove tlsEnabled from ServiceConfiguration and WorkerConfig, as well as the references.

This change could break two existing use cases.

  1. tlsEnabled is true while no TLS web service is running. In this case, forcing client or admin to connect a nonexistent service .
  2. tlsEnabled is false while both TLS and plaintext web service are running. In this case, running a TLS web service is meaningless. I suspect whether there's someone tried to do this. It's more like a mistake.

For built-in admin or client, we have the brokerClientTlsEnabled config to determine whether to connect TLS service for built-in PulsarAdmin and PulsarClient.

Documentation

Add the doc-required label because we need to remove this deprecated config from website page like https://pulsar.apache.org/docs/en/reference-configuration/.

@BewareMyPower BewareMyPower added area/function area/broker doc-required Your PR changes impact docs and you will update later. labels Oct 24, 2021
@BewareMyPower BewareMyPower added this to the 2.10.0 milestone Oct 24, 2021
@BewareMyPower BewareMyPower self-assigned this Oct 24, 2021
@codelipenghui
Copy link
Contributor

@BewareMyPower It would be better to have a discussion in the dev email list.

@BewareMyPower
Copy link
Contributor Author

@codelipenghui Okay, I'll do that.

@BewareMyPower BewareMyPower force-pushed the bewaremypower/remove-tls-enabled branch from 133f614 to 9b45d78 Compare October 25, 2021 03:20
@BewareMyPower
Copy link
Contributor Author

@BewareMyPower
Copy link
Contributor Author

It looks like there's something wrong with Pulsar Functions tests. I'll revert the changes for WorkerConfig first.

@BewareMyPower BewareMyPower marked this pull request as draft October 25, 2021 13:41
@BewareMyPower
Copy link
Contributor Author

It looks like there're still some test failures about Pulsar Functions with TLS, I'll investigate soon.

Error:  Failures: 
Error:    PulsarFunctionTlsTest.setup:154 » NoSuchElement No value present
Error:    PulsarFunctionTlsTest.shutdown:173 NullPointer

@BewareMyPower
Copy link
Contributor Author

These test failed because after reverting changes for WorkerConfig#tlsEnabled, if WorkerConfig#tlsEnabled is false, getTlsEnabled always returns false, then the WorkServer will fail to start. But before it, the integration tests all failed.

It looks like these tests failed when the pulsar-admin tried to create a function. For example

Error:  testAutoSchemaFunction(org.apache.pulsar.tests.integration.functions.go.PulsarFunctionsGoProcessTest)  Time elapsed: 4.294 s  <<< FAILURE!
org.apache.pulsar.tests.integration.docker.ContainerExecException: sh -c /pulsar/bin/pulsar-admin functions create --tenant public --namespace default --name test-autoschema-fn-szzahfzx --className org.apache.pulsar.functions.api.examples.AutoSchemaFunction --inputs test-autoschema-input-sizdmntd --output test-autoshcema-output-ocqocjrz --jar /pulsar/examples/java-test-functions.jar --ram 134217728 failed on 2661fdee46cf315424ae1260c9ec60a6bc5d0346f85f7efa26aa37427a00d0a7 with error code 1
	at org.apache.pulsar.tests.integration.utils.DockerUtils$2.onComplete(DockerUtils.java:284)
	at org.testcontainers.shaded.com.github.dockerjava.core.exec.AbstrAsyncDockerCmdExec$1.onComplete(AbstrAsyncDockerCmdExec.java:51)
	at org.testcontainers.shaded.com.github.dockerjava.core.DefaultInvocationBuilder.lambda$executeAndStream$1(DefaultInvocationBuilder.java:276)
	at java.base/java.lang.Thread.run(Thread.java:829)

@BewareMyPower
Copy link
Contributor Author

I also found some other error logs.

stdout: 2021-10-25T04:01:49,599+0000 [AsyncHttpClient-7-1] WARN  org.apache.pulsar.client.admin.internal.BaseResource - [http://localhost:8080/admin/v3/functions/public/default/test-autoschema-fn-hhfwdrtl] Failed to perform http delete request: javax.ws.rs.InternalServerErrorException: HTTP 500 Internal Server Error

stderr: java.util.concurrent.CompletionException: java.net.ConnectException: https://pulsar-functions-worker-process-asphf-1:6751

Reason: HTTP 500 Internal Server Error
stdout: 
stderr: org.apache.pulsar.client.admin.PulsarAdminException: java.util.concurrent.CompletionException: org.apache.pulsar.client.admin.internal.http.AsyncHttpConnector$RetryException: Could not complete the operation. Number of retries has been exhausted. Failed reason: https://pulsar-functions-worker-process-asphf-1:6751

Reason: HTTP 500 Internal Server Error

Since it's not an important bug or feature, I won't take much time to find the reason. So close this PR at this moment.

@BewareMyPower BewareMyPower removed this from the 2.10.0 milestone Oct 27, 2021
@BewareMyPower BewareMyPower removed area/function area/broker doc-required Your PR changes impact docs and you will update later. labels Oct 27, 2021
@BewareMyPower BewareMyPower deleted the bewaremypower/remove-tls-enabled branch December 14, 2021 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants