Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

listener: support keepalive configuration #38467

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jronak
Copy link
Contributor

@jronak jronak commented Feb 16, 2025

Commit Message: This change introduces the ability to configure TCP keepalive settings on additional listener addresses. Currently users have to set the corresponding socket options directly and with this change users can enable TCP keepalive for both primary and additional addresses via the listener configuration similar to the cluster configuration.

Additional Description: Configuration example:

static_resources:
  listeners:
  - name: "listener"
    address:
      socket_address:
        address: 127.0.0.1
        port_value: 10000
    # TCP keepalive configuration for the primary address.
    tcp_keepalive:
      keepalive_time: 60000       # time in milliseconds before probing starts
      keepalive_interval: 10000   # interval between probes in milliseconds
      keepalive_probes: 5         # number of abandoned probes before closing connection
    additional_addresses:
      - address:
          socket_address:
            address: 127.0.0.2
            port_value: 10001
        # TCP keepalive configuration for the additional address.
        tcp_keepalive:
          keepalive_time: 60000
          keepalive_interval: 10000
          keepalive_probes: 5

Risk Level: low
Testing: unit and integration
Docs Changes: NA
Release Notes: NA

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #38467 was opened by jronak.

see: more, trace.

@jronak jronak force-pushed the ronakj/listener_keepalive branch from 9fb64d8 to e5edbba Compare February 16, 2025 17:55
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

High-level comment:
prior to additional-addresses, the listener to address 1-1 mapping introduced some address related config knobs in the listener.
I think that in order to ensure that a coherent config design is used, the fields in the listener level will impact all the addresses (both the primary and additional), and the additional address will be able to override the config (just as being done for socket options).
I also think that it would be better to have a common-address-options (similar to UpstreamConnectionOptions) that can be used to optionally override these values.

@adisuissa
Copy link
Contributor

/wait

Signed-off-by: Ronak Jain <[email protected]>
@jronak
Copy link
Contributor Author

jronak commented Feb 21, 2025

Thanks for the comments @adisuissa

prior to additional-addresses, the listener to address 1-1 mapping introduced some address related config knobs in the listener.
I think that in order to ensure that a coherent config design is used, the fields in the listener level will impact all the addresses (both the primary and additional), and the additional address will be able to override the config (just as being done for socket options).

Updated the PR to use apply keepalive configuration across all the additional addresses and ability override/disable keepalive at each additional address level.

I also think that it would be better to have a common-address-options (similar to UpstreamConnectionOptions) that can be used to optionally override these values.

Happy to update the PR to use common address options, am I understanding the definition correctly?

message AddressOptions {
  core.v3.TcpKeepalive tcp_keepalive ;
}

message AdditionalAddress {
  ...
  AddressOptions address_options_override;
}

message Listener {
  ...
  AddressOptions common_address_options;
} 

What about socket_options field?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants