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

internal/envoy/v3/bootstrap: Make configurable about keepalive settings. #3235

Closed

Conversation

binoue
Copy link

@binoue binoue commented Dec 31, 2020

This PR is an Implementation for allowing users to tune TCP keep alive settings.
I think this feature is already argued in #2652 and would help for #3214.

Fixes #2652
Fixes #3214

Signed-off-by: binoue banji-inoue@cybozu.co.jp

This change allows users to tune envoy's keepalive settings.

Fixes projectcontour#2652
Fixes projectcontour#3214

Signed-off-by: binoue <banji-inoue@cybozu.co.jp>
@codecov
Copy link

codecov bot commented Dec 31, 2020

Codecov Report

Merging #3235 (c6c378e) into main (eedc2d0) will decrease coverage by 0.06%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3235      +/-   ##
==========================================
- Coverage   75.43%   75.37%   -0.07%     
==========================================
  Files          97       97              
  Lines        6025     6034       +9     
==========================================
+ Hits         4545     4548       +3     
- Misses       1371     1376       +5     
- Partials      109      110       +1     
Impacted Files Coverage Δ
cmd/contour/bootstrap.go 0.00% <0.00%> (ø)
internal/envoy/bootstrap.go 55.17% <83.33%> (+7.34%) ⬆️
internal/envoy/v3/bootstrap.go 91.97% <100.00%> (ø)
internal/dag/cache.go 95.25% <0.00%> (-0.80%) ⬇️

@binoue
Copy link
Author

binoue commented Jan 4, 2021

@youngnick

Hi! As #2652 is not progressing, I tried to make TCP keep-alive settings of Envoy configurable.
Would you look at this PR because I am not sure this implementation is acceptable for Contour?

@sunjayBhatia
Copy link
Member

This change is currently only changing the keepalive settings between Envoy and Contour (the change to UpstreamConnectionOptions here is only for the static contour xDS cluster) so I don't think it solves the linked issues.

@binoue
Copy link
Author

binoue commented Jan 6, 2021

@sunjayBhatia

This change is currently only changing the keepalive settings between Envoy and Contour (the change to UpstreamConnectionOptions here is only for the static contour xDS cluster) so I don't think it solves the linked issues.

Thank you for your comment!
As I was misunderstanding, I will fix this PR

@stevesloka
Copy link
Member

Hey @binoue just wanted to touch base again on this PR, were looks like there were a couple more things to clean up first. Let us know when you're ready for another review! =)

@github-actions
Copy link

Marking this PR stale since there has been no activity for 14 days. It will be closed if there is no activity for another 90 days.

@github-actions github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 28, 2021
@binoue
Copy link
Author

binoue commented Feb 24, 2021

@stevesloka

Thank you for your comment!
However, I could not take enough time to implement this feature for a while, so let me close this PR

@binoue binoue closed this Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
3 participants