Skip to content

feat(ssl): added timeout option#5475

Merged
soloio-bulldozer[bot] merged 1 commit intokgateway-dev:masterfrom
gunnar-solo:add-tls-timeout
Oct 19, 2021
Merged

feat(ssl): added timeout option#5475
soloio-bulldozer[bot] merged 1 commit intokgateway-dev:masterfrom
gunnar-solo:add-tls-timeout

Conversation

@gunnar-solo
Copy link
Copy Markdown
Contributor

@gunnar-solo gunnar-solo commented Oct 14, 2021

closes solo-io#5438

Description

Passthrough functionality for "TLS handshake timeout"--added new proto option to ssl.proto: transport_socket_connect_timeout

Context

Envoy has supported this for a bit, and this PR will allow gloo-edge to support the functionality as well.

Checklist:

  • I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
  • If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
  • I followed guidelines laid out in the Gloo Edge contribution guide
  • I opened a draft PR or added the work in progress label if my PR is not ready for review
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves solo-io#5438

@gunnar-solo gunnar-solo force-pushed the add-tls-timeout branch 2 times, most recently from d27dc4e to 7699185 Compare October 15, 2021 13:46
@solo-changelog-bot
Copy link
Copy Markdown

Issues linked to changelog:
solo-io#5438

Copy link
Copy Markdown
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

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

Looks good! Small question and ask

// If present and nonzero, the amount of time to allow incoming connections to complete any
// transport socket negotiations. If this expires before the transport reports connection
// establishment, the connection is summarily closed.
google.protobuf.Duration transport_socket_connect_timeout = 10;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this field is 0, what is the default behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Comment thread projects/gloo/pkg/translator/listener.go
@gunnar-solo gunnar-solo force-pushed the add-tls-timeout branch 2 times, most recently from 618022d to bb1d369 Compare October 18, 2021 12:27
@gunnar-solo gunnar-solo marked this pull request as ready for review October 18, 2021 12:39
@gunnar-solo gunnar-solo requested a review from a team as a code owner October 18, 2021 12:39
Copy link
Copy Markdown
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

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

Small comment about the unit test, but the rest looks good!

Comment thread projects/gloo/pkg/plugins/tcp/plugin_test.go
passthrough functionality for "TLS handshake timeout".  Envoy has supported this [for a bit](envoyproxy/envoy#13610), and this
PR will allow gloo-edge to support the functionality as well.

[Referenced Documentation Here](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/listener/v3/listener_components.proto#config-listener-v3-filterchain)

closes solo-io#5438
Copy link
Copy Markdown
Contributor

@elcasteel elcasteel left a comment

Choose a reason for hiding this comment

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

LGTM!
FYI Github will squash commits before merging so you don't need to rebase and it's a little easier for reviewers to see the history of comments being addressed if you maintain the original commit history.

@soloio-bulldozer soloio-bulldozer Bot merged commit 28712c8 into kgateway-dev:master Oct 19, 2021
@gunnar-solo gunnar-solo deleted the add-tls-timeout branch October 19, 2021 16:48
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.

make TLS handshake timeout configurable

3 participants