Skip to content

filter: exposed setSni() function to Lua for setting SNI value for upstream connections#17991

Closed
agrawroh wants to merge 1 commit intoenvoyproxy:mainfrom
agrawroh:set_sni_filter
Closed

filter: exposed setSni() function to Lua for setting SNI value for upstream connections#17991
agrawroh wants to merge 1 commit intoenvoyproxy:mainfrom
agrawroh:set_sni_filter

Conversation

@agrawroh
Copy link
Copy Markdown
Member

@agrawroh agrawroh commented Sep 5, 2021 •
edited
Loading

This PR exposes a new method called setSni() in the LUA filter to dynamically set the SNI value from the LUA block. Currently, the auto_sni() only looks at the Host/Authority header to set this value and in our use-case we want to use a different header for setting the SNI while sending requests to the upstream clusters.

Commit Message: exposed a new setSni() function to Lua for setting SNI value for upstream connections.
Additional Description: Added a new function so that SNI value can be set dynamically from the LUA's envoy_on_request() block as auto_sni() only supports setting the Host/Authority header value as SNI currently.
Risk Level: Low
Testing: Unit Tests, Integration Tests
Docs Changes: Added setSni() method description in LUA docs.
Release Notes: Added
Platform Specific Features: N/A

Signed-off-by: Rohit Agrawal rohit.agrawal@databricks.com

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Sep 5, 2021

Hi @agrawroh, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17991 was opened by agrawroh.

see: more, trace.

@agrawroh agrawroh force-pushed the set_sni_filter branch 3 times, most recently from 362b738 to 0763565 Compare September 5, 2021 17:20
@agrawroh agrawroh changed the title filter: exposed setSni() function to Lua for setting SNI value dynamically filter: exposed setSni() function to Lua for setting SNI value for upstream connections Sep 5, 2021
@agrawroh agrawroh force-pushed the set_sni_filter branch 2 times, most recently from ca6fc82 to 19c55bb Compare September 5, 2021 18:41
Shikugawa
Shikugawa reviewed Sep 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Shikugawa Shikugawa left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@agrawroh Thank you for your contribution! I agree with why this feature is needed. But I have one question. Why we should add this support only for lua filter? I think that it will be better to have auto_sni arbitrary header support. WDYT? cc @lizan

test/extensions/filters/http/lua/lua_integration_test.cc Outdated
Copy link
Copy Markdown
Member

@Shikugawa Shikugawa Sep 6, 2021

Choose a reason for hiding this comment

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

Could we add more realistic test? It means that 1. extract header value (it should be hostname for upstream SNI) 2. call setSni

Copy link
Copy Markdown
Member Author

@agrawroh agrawroh Sep 6, 2021

Choose a reason for hiding this comment

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

Sure, let me do that.

@agrawroh
filter: exposed setSni() function to Lua for setting SNI value for up…
ebcbbf8 
…stream connections

Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
@agrawroh
Copy link
Copy Markdown
Member Author

agrawroh commented Sep 6, 2021 •
edited
Loading

@agrawroh Thank you for your contribution! I agree with why this feature is needed. But I have one question. Why we should add this support only for lua filter? I think that it will be better to have auto_sni arbitrary header support. WDYT? cc @lizan

I can also open up another small PR for adding the arbitrary header support to auto_sni.

@Shikugawa
Copy link
Copy Markdown
Member

Shikugawa commented Sep 6, 2021

I can also open up another small PR for adding the arbitrary header support to auto_sni.

Cool. If we have auto_sni arbitrary header support, we no longer need lua specific setSni feature.

@agrawroh
Copy link
Copy Markdown
Member Author

agrawroh commented Sep 6, 2021

I can also open up another small PR for adding the arbitrary header support to auto_sni.

Cool. If we have auto_sni arbitrary header support, we no longer need lua specific setSni feature.

@Shikugawa I opened #17995

@lizan
Copy link
Copy Markdown
Member

lizan commented Sep 14, 2021

With #17995 merged, do we still need this?

@agrawroh
Copy link
Copy Markdown
Member Author

agrawroh commented Sep 15, 2021

With #17995 merged, do we still need this?

No, I'll close it.

@agrawroh agrawroh closed this Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Shikugawa Shikugawa Shikugawa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Shikugawa Shikugawa

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@agrawroh @Shikugawa @lizan