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

lua: add connection():ssl() API #3761

Merged
merged 10 commits into from
Jul 6, 2018
Merged

lua: add connection():ssl() API #3761

merged 10 commits into from
Jul 6, 2018

Conversation

dio
Copy link
Member

@dio dio commented Jun 29, 2018

Description:
This patch adds connection():ssl() to StreamHandleWrapper. This
API returns the indication if the underlying connection using SSL or not.

Risk Level: Low

Testing:
Unit and integration tests

Docs Changes:

  • Add connection():ssl() API.

Release Notes:

  • lua: added connection() wrapper and ssl() API.

Related PR: #3760 (merged)
Fixes #3704

Signed-off-by: Dhi Aurrahman dio@rockybars.com

dio added 3 commits June 29, 2018 12:15
This patch adds connection():secure() API to check whether the current
connection is using SSL or not.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice. Small question.

Connection object API
---------------------

secure()
Copy link
Member

Choose a reason for hiding this comment

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

Thing for the future where I'm guessing people are going to ask for SSL/TLS connection properties in Lua, should we instead return an SSL/TLS object? This object could have no methods right now but the presence of the object or nil could indicate secure? Just thinking for the future. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems like a good idea. Will update it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

dio added 5 commits July 4, 2018 06:36
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
If not nil then it is secure and vice versa.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio dio changed the title lua: add connection():secure() API lua: add connection():ssl() API Jul 4, 2018
@ramaraochavali
Copy link
Contributor

@dio Thank you so much for adding this and the protocol API, useful stuff.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice, small doc question.


Returns the current request's underlying :repo:`connection <include/envoy/network/connection.h>`.

Returns a :ref:`metadata object <config_http_filters_lua_connection_wrapper>`.
Copy link
Member

Choose a reason for hiding this comment

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

"metadata object" ? Is this a copy/paste error?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mattklein123 oops. Sorry about that. Fixed in 22680ac.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@mattklein123
Copy link
Member

@dio looks like coverage is below the limit. You might need to look at adding some: https://s3.amazonaws.com/lyft-envoy/coverage/report-master/coverage.html. Unfortunately looks like we have slipped again.

@dio
Copy link
Member Author

dio commented Jul 5, 2018

OK, will take a look.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio
Copy link
Member Author

dio commented Jul 6, 2018

@mattklein123 I guess it looks OK now.

@mattklein123 mattklein123 merged commit fe94528 into envoyproxy:master Jul 6, 2018
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.

3 participants