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.

Sign up for GitHub

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

Jump to bottom

lua: reset downstream_ssl_connection in StreamInfoWrapper when object is marked dead by Lua GC #14092

Merged
merged 7 commits into from
Nov 23, 2020
Merged

lua: reset downstream_ssl_connection in StreamInfoWrapper when object is marked dead by Lua GC #14092

merged 7 commits into from
Nov 23, 2020

Conversation

MarcinFalkowski
Copy link
Contributor

@MarcinFalkowski MarcinFalkowski commented Nov 19, 2020 •
edited
Loading

Fixes #14091

The problem and fix is similiar to #4312

Risk Level: Low
Testing: regression test, manual testing
Docs Changes: N/A
Release Notes: N/A

@MarcinFalkowski MarcinFalkowski mentioned this pull request Nov 19, 2020
Merged
@MarcinFalkowski
Copy link
Contributor Author

I am not sure, but it looks like CI failed for reasons unrelated to this change (Windows build hanged)

@MarcinFalkowski
Copy link
Contributor Author

MarcinFalkowski commented Nov 19, 2020 •
edited
Loading

@dio just letting you know. Maybe you will be interested in reviewing this.

@MarcinFalkowski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14092 (comment) was created by @MarcinFalkowski.

see: more, trace.

@MarcinFalkowski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14092 (comment) was created by @MarcinFalkowski.

see: more, trace.

@MarcinFalkowski
Merge remote-tracking branch 'origin/master' into lua-downstream-ssl-…
24dc57b 
…connection-segfault-fix

Signed-off-by: Marcin Falkowski <[email protected]>
@dio
Copy link
Member

dio commented Nov 21, 2020

Thank you for this! @MarcinFalkowski could you try to merge main? Hopefully the coverage test failure will be fixed.

@MarcinFalkowski
Merge remote-tracking branch 'origin/master' into lua-downstream-ssl-…
b99f3d5 
…connection-segfault-fix

Signed-off-by: Marcin Falkowski <[email protected]>
dio
dio reviewed Nov 21, 2020
View reviewed changes
Copy link
Member

@dio dio 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. I think you need to merge master. And one super nit and an ask for help.

test/extensions/filters/http/lua/lua_filter_test.cc Outdated

setup(SCRIPT);

auto connection_info = std::make_shared<Ssl::MockConnectionInfo>();
Copy link
Member

Choose a reason for hiding this comment

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

Nit. use const? And since you're here probably the above one too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for a review. I will fix it. Just to be sure - by "above one" you mean the other instance of auto connection_info = std::make_shared<Ssl::MockConnectionInfo>(); in this file?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, you're correct. Thank you so much!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MarcinFalkowski
add const modifier
093ce9f 
Signed-off-by: Marcin Falkowski <[email protected]>
dio
dio approved these changes Nov 21, 2020
View reviewed changes
Copy link
Member

@dio dio left a comment

Choose a reason for hiding this comment

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

Super cool. Thank you for working on this!

@MarcinFalkowski
Copy link
Contributor Author

MarcinFalkowski commented Nov 21, 2020 •
edited
Loading

@dio CI is green. Hope we can merge it now. Thanks for a review

@dio
Copy link
Member

dio commented Nov 23, 2020

@mattklein123 do you want to take another look at this? And should we backport this? Thank you! cc. @cpakulski

@lizan lizan added the backport/review Request to backport to stable releases label Nov 23, 2020
@lizan lizan merged commit 59e2968 into envoyproxy:master Nov 23, 2020
andreyprezotto pushed a commit to andreyprezotto/envoy that referenced this pull request Nov 24, 2020
@MarcinFalkowski @andreyprezotto
lua: reset downstream_ssl_connection in StreamInfoWrapper when object…
ce172b7 
… is marked dead by Lua GC (envoyproxy#14092)

Fixes envoyproxy#14091

The problem and fix is similiar to envoyproxy#4312

Risk Level: Low
Testing: regression test, manual testing
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Marcin Falkowski <[email protected]>
qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020
@MarcinFalkowski @qqustc
lua: reset downstream_ssl_connection in StreamInfoWrapper when object…
f46876c 
… is marked dead by Lua GC (envoyproxy#14092)

Fixes envoyproxy#14091

The problem and fix is similiar to envoyproxy#4312

Risk Level: Low
Testing: regression test, manual testing
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Marcin Falkowski <[email protected]>
Signed-off-by: Qin Qin <[email protected]>
@slonka slonka mentioned this pull request Dec 6, 2020
Closed
@cpakulski cpakulski added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Dec 16, 2020
cpakulski pushed a commit to cpakulski/envoy that referenced this pull request Dec 16, 2020
@MarcinFalkowski @cpakulski
lua: reset downstream_ssl_connection in StreamInfoWrapper when object…
fff6c70 
… is marked dead by Lua GC (envoyproxy#14092)

Fixes envoyproxy#14091

The problem and fix is similiar to envoyproxy#4312

Risk Level: Low
Testing: regression test, manual testing
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Marcin Falkowski <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>
dio pushed a commit that referenced this pull request Dec 22, 2020
@cpakulski @MarcinFalkowski
lua: reset downstream_ssl_connection in StreamInfoWrapper when object…
db0ae3d 
… is marked dead by Lua GC (#14092) (#14449)

Co-authored-by: Marcin Falkowski <[email protected]>
@cpakulski
Copy link
Contributor

Backports to releases 1.13-1.15 are not required because SSL connection is not exposed through LUA in those releases.
Backport completed. Removing "backport/approved" label.

@cpakulski cpakulski removed the backport/approved Approved backports to stable releases label Jan 5, 2021
istio-testing pushed a commit to istio/envoy that referenced this pull request Feb 5, 2021
@tbarrella @Shikugawa
@cpakulski @MarcinFalkowski
Merge envoy/release/v1.16 into release-1.8 (#294)
5b0c5f7 
* backport to 1.16: http: fixing a bug with IPv6 hosts (envoyproxy#14238)

Fixing a bug where HTTP parser offsets for IPv6 hosts did not include [] and Envoy assumed it did.
This results in mis-parsing addresses for IPv6 CONNECT requests and IPv6 hosts in fully URLs over HTTP/1.1

Risk Level: low
Testing: new unit, integration tests
Docs Changes: n/a
Release Notes: inline
Signed-off-by: Shikugawa <[email protected]>
Co-authored-by: alyssawilk <[email protected]>

* backport to 1.16: vrp: allow supervisord to open its log file (envoyproxy#14066) (envoyproxy#14279)

Commit Message: Allow supervisord to open its log file
Additional Description:
Change the default location of the log file and give supervisord
permissions to write to it.

Risk Level: low
Testing: built image locally
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Alex Konradi <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>

* Closing release 1.16.2. (envoyproxy#14308)

Signed-off-by: Christoph Pakulski <[email protected]>

* Kick-off rel 1.16.3. (envoyproxy#14321)

Signed-off-by: Christoph Pakulski <[email protected]>

* lua: reset downstream_ssl_connection in StreamInfoWrapper when object is marked dead by Lua GC (envoyproxy#14092) (envoyproxy#14449)

Co-authored-by: Marcin Falkowski <[email protected]>

* backport to 1.16: tls: fix detection of the upstream connection close event. (envoyproxy#13858) (envoyproxy#14452)

Fixes envoyproxy#13856.

This change also contains the following backports:
- build: Fix some unused variable warnings (envoyproxy#13987)
- test: Check in all TLS test certs (envoyproxy#13702)

Signed-off-by: Piotr Sikora <[email protected]>
Signed-off-by: Christoph Pakulski <[email protected]>

* backport to 1.16: aggregate cluster: fix TLS init issue (envoyproxy#14456)

Additional Description: Based on envoyproxy#14388
Risk Level: Low
Testing: Build and run the repro from envoyproxy#14119 without crashing, `bazel test test/extensions/clusters/aggregate:cluster_test`
Docs Changes: N/A
Release Notes:
envoyproxy#14119

Signed-off-by: Taylor Barrella <[email protected]>

Co-authored-by: Rei Shimizu <[email protected]>
Co-authored-by: Christoph Pakulski <[email protected]>
Co-authored-by: Marcin Falkowski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lizan lizan lizan approved these changes

@dio dio dio approved these changes

Assignees

@dio dio

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Lua filter causes a crash when the script uses streamInfo():downstreamSslConnection()
4 participants
@MarcinFalkowski @dio @cpakulski @lizan