[GitHub] [pulsar] michaeljmarshall opened a new pull request, #15328: [feature][Function Worker] Support disabling non-TLS service port

[GitBox <gi...@apache.org>]

michaeljmarshall opened a new pull request, #15328:
URL: https://github.com/apache/pulsar/pull/15328

   Master Issue: https://github.com/apache/pulsar/issues/11548
   
   ### Motivation
   
   Some use security sensitive users prefer to disable plaintext ports to ensure that there is no accidentally non-encrypted traffic. We added the ability to disable the non-TLS server on the broker https://github.com/apache/pulsar/pull/11681, and this PR adds the same feature for the function worker.
   
   I have two questions about the implementation:
   
   1. Is it a breaking change to update the worker id logic? It looks like the existing logic has two different methods of construction. The first is modified in this PR and the second is below. They differ only slightly. https://github.com/apache/pulsar/blob/74eae1a729c33c9fead9d54594bdee5fac8ab153/pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java#L1681-L1685
   2. In the `initializeWorkerConfigFromBrokerConfig` method, I am updating the worker config so that it maps the broker's ports to the worker's ports for all values. This seems logical to me since there are so many other mapped ports, and when a user is disabling the plaintext port on the broker, it's highly likely that they'd also want to disable the plaintext port for the broker's function worker.
   
   ### Modifications
   
   * Update log lines to accurately indicate when the Function Worker is starting the HTTP or the HTTPS servers
   * Skip creation of the function worker's HTTP server when the `workerPort` is `null`
   * Update `WorkerConfig#getWorkerId()` so that it uses the TLS port when the plaintext port is null.
   * Update the documentation.
   * Update the broker's `initializeWorkerConfigFromBrokerConfig` method to align with the new changes.
   * Update a test to verify that the non-TLS port service does not get started.
   
   ### Verifying this change
   
   I added a test and verified the associated logs, too.
   
   ### Does this pull request potentially affect one of the following parts:
   
   This change does not affect any defaults.
   
   ### Documentation
   
   - [x] `doc-added`
   This PR includes updates to the docs.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] Technoboy- merged pull request #15328: [feature][Function Worker] Support disabling non-TLS service port

[GitBox <gi...@apache.org>]

Technoboy- merged PR #15328:
URL: https://github.com/apache/pulsar/pull/15328


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [pulsar] michaeljmarshall commented on pull request #15328: [feature][Function Worker] Support disabling non-TLS service port

[GitBox <gi...@apache.org>]

michaeljmarshall commented on PR #15328:
URL: https://github.com/apache/pulsar/pull/15328#issuecomment-1110266002

   Given that this support is already available in the broker, proxy, and WebSocket proxy, I am going to merge this now that tests have passed.


-- 
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: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org