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

feat(*) wasm_socket unix domain socket support #374

Closed
wants to merge 1 commit into from

Conversation

casimiro
Copy link
Contributor

@casimiro casimiro commented Aug 4, 2023

  • Update wasm_socket port parsing
  • Add test cases (sanity, missing socket file, missing :authority)
  • Update tests relying TEST_NGINX_SERVER_PORT2 to use unix domain socket instead.
  • If possible, drop TEST_NGINX_SERVER_PORT2

Updating tests that were using TEST_NGINX_SERVER_PORT2 ended up being a substantial amount of work on it's own, deserving it's own PR or PRs.

@casimiro casimiro force-pushed the feat/unix-domain-socket branch 2 times, most recently from 76092d4 to 7311cae Compare August 4, 2023 12:18
@coveralls
Copy link

coveralls commented Aug 4, 2023

Pull Request Test Coverage Report for Build 5835182693

  • 15 of 16 (93.75%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.02%) to 91.034%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/common/ngx_wasm_socket_tcp.c 15 16 93.75%
Files with Coverage Reduction New Missed Lines %
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c 1 91.81%
src/common/ngx_wasm_socket_tcp.c 3 86.71%
Totals Coverage Status
Change from base Build 5828356494: 0.02%
Covered Lines: 7544
Relevant Lines: 8287

💛 - Coveralls

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Overall I am weary that our host parsing logic is not integrated within ngx_wasm_sock, which is what I had in mind for this feature... I know the dispatch caller already does some parsing ahead of delegating to ngx_wasm_sock, but moving more parsing logic to ngx_wasm_sock (or validating logic) is also something I would have liked to be explored.

src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c Outdated Show resolved Hide resolved
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c Outdated Show resolved Hide resolved
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c Outdated Show resolved Hide resolved
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c Outdated Show resolved Hide resolved
src/http/proxy_wasm/ngx_http_proxy_wasm_dispatch.c Outdated Show resolved Hide resolved
@casimiro casimiro added the pr/work-in-progress PR: Work in progress label Aug 4, 2023
@casimiro
Copy link
Contributor Author

casimiro commented Aug 4, 2023

Thank you for the detailed review @thibaultcha! I'll change the code to address all comments.

@casimiro casimiro force-pushed the feat/unix-domain-socket branch 5 times, most recently from a098efb to 6df54f6 Compare August 10, 2023 14:24
@casimiro casimiro added pr/work-in-progress PR: Work in progress and removed pr/work-in-progress PR: Work in progress labels Aug 11, 2023
@casimiro casimiro removed the pr/work-in-progress PR: Work in progress label Aug 11, 2023
@thibaultcha thibaultcha added the pr/merge-in-progress PR: Merge in progress (do not push) label Aug 12, 2023
@thibaultcha thibaultcha deleted the feat/unix-domain-socket branch August 17, 2023 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/merge-in-progress PR: Merge in progress (do not push)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants