Skip to content

Conversation

@michaeljmarshall
Copy link
Member

Master Issue: #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 #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.
    workerConfig.setWorkerId(
    "c-" + brokerConfig.getClusterName()
    + "-fw-" + hostname
    + "-" + (workerConfig.getTlsEnabled()
    ? workerConfig.getWorkerPortTls() : workerConfig.getWorkerPort()));
  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

  • doc-added
    This PR includes updates to the docs.

@michaeljmarshall michaeljmarshall added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. area/function area/security labels Apr 26, 2022
@michaeljmarshall michaeljmarshall added this to the 2.11.0 milestone Apr 26, 2022
@michaeljmarshall michaeljmarshall self-assigned this Apr 26, 2022
@github-actions github-actions bot added the doc-complete Your PR changes impact docs and the related docs have been already added. label Apr 26, 2022
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michaeljmarshall
Copy link
Member Author

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.

Copy link
Contributor

@nicoloboschi nicoloboschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Technoboy- Technoboy- merged commit 0775bc0 into apache:master May 1, 2022
@michaeljmarshall michaeljmarshall deleted the support-disabling-nontls-service-ports-function branch August 23, 2022 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/function area/security doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. doc-complete Your PR changes impact docs and the related docs have been already added.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants