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

Add support for configuring Envoys route idle_timeout #14340

Merged
merged 26 commits into from Nov 29, 2022
Merged

Add support for configuring Envoys route idle_timeout #14340

merged 26 commits into from Nov 29, 2022

Conversation

oulman
Copy link
Contributor

@oulman oulman commented Aug 25, 2022

Description

This PR adds support for configuring the Envoy route level idle_timeout for service-routers and local_app. For our teams scenario, this will let us extend the timeout for long-running gRPC services so that Envoy doesn't terminate the connection prematurely.

For service-routers a IdleTimeout parameter has been added, and for local_app a proxy config local_idle_timeout_ms parameter was created. I followed the RequestTimeout and local_request_timeout_ms definitions closely.

Testing & Reproduction steps

Manual Testing Steps

service-router IdleTimeout

Created a service-router and configured the IdleTimeout to 60s

Kind = "service-router"
Name = "waitserver"
Routes = [
  {
    Destination {
      Service = ""
      RequestTimeout = "60m"
      IdleTimeout = "60s"
    }
  }
]

Dumped the Envoy config and verified that the idle_timeout configuration parameter was set

<snip>
        "name": "waitserver",
        "domains": [
         "*"
        ],
        "routes": [
         {
          "match": {
           "prefix": "/"
          },
          "route": {
           "cluster": "waitserver.default.dc1.internal.ba4be18f-a3c7-7e2f-eff4-a25a8a1fce57.consul",
           "timeout": "3600s",
           "idle_timeout": "60s"
          }
         },
<snip>
local_idle_timeout_ms

Configure local_idle_timeout_ms on a service proxy.config

service {
  name = "waitserver"
  id = "waitserver"
  address = "10.5.0.20"
  port = 9222

  connect {
    sidecar_service {
      port = 10201

      proxy {
        config {
          local_request_timeout_ms = 0
          local_idle_timeout_ms = 0
        }
      }
      check {
        name = "Connect Envoy Sidecar"
        tcp = "10.5.0.20:10201"
        interval ="10s"
      }
    }
  }
}

Dumped the Envoy config and verified that the idle_timeout configuration parameter was set on the public_listener route for local_app

{
     "route_config": {
      "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
      "name": "public_listener",
      "virtual_hosts": [
       {
        "name": "public_listener",
        "domains": [
         "*"
        ],
        "routes": [
         {
          "match": {
           "prefix": "/"
          },
          "route": {
           "cluster": "local_app",
           "timeout": "0s",
           "idle_timeout": "0s"
          }
         }
        ]
       }
      ]
     },

Links

Include any links here that might be helpful for people reviewing your PR (Tickets, GH issues, API docs, external benchmarks, tools docs, etc). If there are none, feel free to delete this section.

Please be mindful not to leak any customer or confidential information. HashiCorp employees may want to use our internal URL shortener to obfuscate links.

PR Checklist

  • updated test coverage
  • external facing docs updated
  • not a security concern

@oulman oulman requested a review from a team as a code owner August 25, 2022 05:11
@github-actions github-actions bot added theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface theme/config Relating to Consul Agent configuration, including reloading theme/envoy/xds Related to Envoy support type/docs Documentation needs to be created/updated/clarified labels Aug 25, 2022
Copy link
Contributor

@boruszak boruszak left a comment

Choose a reason for hiding this comment

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

I reviewed the changelong .txt, service-router.mdx, and envoy.mdx for docs changes.

I have one minor fix, otherwise LGTM. Approving on behalf of consul-docs.

website/content/docs/connect/proxies/envoy.mdx Outdated Show resolved Hide resolved
oulman and others added 2 commits August 25, 2022 17:50
Co-authored-by: Jeff Boruszak <104028618+boruszak@users.noreply.github.com>
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions.

@github-actions github-actions bot added the meta/stale Automatically flagged for inactivity by stalebot label Nov 2, 2022
@oulman
Copy link
Contributor Author

oulman commented Nov 2, 2022

@blake can you remove the stale label so this doesn't get closed? 🙏 Thanks!!

@jkirschner-hashicorp jkirschner-hashicorp added meta/staleproof Exempt from stalebot automation and removed meta/stale Automatically flagged for inactivity by stalebot labels Nov 2, 2022
@jkirschner-hashicorp
Copy link
Contributor

Done. I made this PR staleproof for now!

@oulman
Copy link
Contributor Author

oulman commented Nov 28, 2022

👋 Hi. I wanted to see now that 1.14 is out if this PR could be considered? Thanks!

Copy link
Collaborator

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @oulman. I have a few comments around doc changes and I believe this is missing a test around the local_request_timeout_ms at the xds generation level.

website/content/docs/connect/proxies/envoy.mdx Outdated Show resolved Hide resolved
website/content/docs/connect/proxies/envoy.mdx Outdated Show resolved Hide resolved
@oulman
Copy link
Contributor Author

oulman commented Nov 28, 2022

and I believe this is missing a test around the local_request_timeout_ms at the xds generation level.

Thanks, @dhiaayachi! Is that tests in addition to the changes here?

https://github.com/hashicorp/consul/pull/14340/files#diff-fb5ad005250adf2da36018fec648de9d65d05df9de42eed95310bac480e9c0b4

@dhiaayachi
Copy link
Collaborator

and I believe this is missing a test around the local_request_timeout_ms at the xds generation level.

Thanks, @dhiaayachi! Is that tests in addition to the changes here?

https://github.com/hashicorp/consul/pull/14340/files#diff-fb5ad005250adf2da36018fec648de9d65d05df9de42eed95310bac480e9c0b4

The missing test would be on the rendering side, could be an addition to this test

@oulman
Copy link
Contributor Author

oulman commented Nov 29, 2022

The missing test would be on the rendering side, could be an addition to this test

Ah! Thank you, @dhiaayach!i I have added the local_idle_timeout_ms to that test in this commit.

@david-yu david-yu added the backport/1.14 Changes are backported to 1.14 label Nov 29, 2022
@david-yu
Copy link
Contributor

Hi @dhiaayachi I just checked with @oulman and he has confirmed that he has addressed your comments. Are additional changes needed here?

Copy link
Collaborator

@dhiaayachi dhiaayachi left a comment

Choose a reason for hiding this comment

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

LGTM! it would be good to have @hashicorp/consul-docs review the documentation bits.

Copy link
Contributor

@boruszak boruszak left a comment

Choose a reason for hiding this comment

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

Reviewed 14340.txt, service-router.mdx, and envoy.mdx again at David Yu's request.

Everything LGTM!

@DanStough DanStough merged commit 7e78fb7 into hashicorp:main Nov 29, 2022
DanStough pushed a commit that referenced this pull request Nov 29, 2022
* Add idleTimeout

Co-authored-by: Jeff Boruszak <104028618+boruszak@users.noreply.github.com>
Co-authored-by: Dhia Ayachi <dhia@hashicorp.com>
DanStough pushed a commit that referenced this pull request Nov 29, 2022
* Add idleTimeout

Co-authored-by: James Oulman <oulman@users.noreply.github.com>
Co-authored-by: Jeff Boruszak <104028618+boruszak@users.noreply.github.com>
Co-authored-by: Dhia Ayachi <dhia@hashicorp.com>
@oulman oulman deleted the oulman-add-idle-timeout branch November 30, 2022 19:57
jmurret pushed a commit that referenced this pull request Dec 2, 2022
* Add idleTimeout

Co-authored-by: Jeff Boruszak <104028618+boruszak@users.noreply.github.com>
Co-authored-by: Dhia Ayachi <dhia@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.14 Changes are backported to 1.14 meta/staleproof Exempt from stalebot automation theme/api Relating to the HTTP API interface theme/cli Flags and documentation for the CLI interface theme/config Relating to Consul Agent configuration, including reloading theme/envoy/xds Related to Envoy support type/docs Documentation needs to be created/updated/clarified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants