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

http: T5762: api: make API socket backend communication the one and only default #2508

Merged
merged 1 commit into from
Nov 20, 2023
Merged

http: T5762: api: make API socket backend communication the one and only default #2508

merged 1 commit into from
Nov 20, 2023

Conversation

c-po
Copy link
Member

@c-po c-po commented Nov 20, 2023

Change Summary

Why: Smoketests fail as they can not establish IPv6 connection to uvicorn backend server.

#2481 added a bunch of new smoketests.

While debugging those failing, it was uncovered, that uvicorn only listens on IPv4 connections

vyos@vyos# netstat -tulnp | grep 8080
(Not all processes could be identified, non-owned process info
 will not be shown, you would have to be root to see it all.)
tcp        0      0 127.0.0.1:8080          0.0.0.0:*               LISTEN      -

As the CLI already has an option to move the API communication from an IP to a UNIX domain socket, the best idea is to make this the default way of communication, as we never directly talk to the API server but rather use the NGINX reverse proxy.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes)
  • Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • Other (please describe):

Related Task(s)

Related PR(s)

Component(s) name

https api

Proposed changes

How to test

Smoketest result

vyos@vyos:~$ /usr/libexec/vyos/tests/smoke/cli/test_service_https.py
test_api_auth (__main__.TestHTTPSService.test_api_auth) ... ok
test_api_config_file (__main__.TestHTTPSService.test_api_config_file) ... ok
test_api_configure (__main__.TestHTTPSService.test_api_configure) ... ok
test_api_generate (__main__.TestHTTPSService.test_api_generate) ... ok
test_api_reset (__main__.TestHTTPSService.test_api_reset) ... ok
test_api_show (__main__.TestHTTPSService.test_api_show) ... ok
test_certificate (__main__.TestHTTPSService.test_certificate) ... ok
test_server_block (__main__.TestHTTPSService.test_server_block) ... ok

----------------------------------------------------------------------
Ran 8 tests in 76.139s

OK

Checklist:

  • I have read the CONTRIBUTING document
  • I have linked this PR to one or more Phabricator Task(s)
  • I have run the components SMOKETESTS if applicable
  • My commit headlines contain a valid Task id
  • My change requires a change to the documentation
  • I have updated the documentation accordingly

@vyosbot vyosbot requested a review from a team November 20, 2023 09:17
@vyosbot vyosbot requested review from dmbaturin and removed request for a team November 20, 2023 09:17
@c-po c-po marked this pull request as draft November 20, 2023 09:18
@c-po c-po marked this pull request as ready for review November 20, 2023 14:17
@vyosbot vyosbot requested a review from a team November 20, 2023 14:17
@c-po c-po force-pushed the t5762-https-api-socket branch 2 times, most recently from 4f511b6 to 4416e86 Compare November 20, 2023 16:11
@c-po
http: T5762: api: make API socket backend communication the one and o…
f5e43b1 
…nly default

Why: Smoketests fail as they can not establish IPv6 connection to uvicorn
backend server.

vyos#2481 added a bunch of new smoketests.

While debugging those failing, it was uncovered, that uvicorn only listens on
IPv4 connections

vyos@vyos# netstat -tulnp | grep 8080
(Not all processes could be identified, non-owned process info
 will not be shown, you would have to be root to see it all.)
tcp        0      0 127.0.0.1:8080          0.0.0.0:*               LISTEN      -

As the CLI already has an option to move the API communication from an IP to a
UNIX domain socket, the best idea is to make this the default way of
communication, as we never directly talk to the API server but rather use the
NGINX reverse proxy.
@c-po c-po force-pushed the t5762-https-api-socket branch from 4416e86 to f5e43b1 Compare November 20, 2023 16:17
@dmbaturin dmbaturin merged commit 3ab206b into vyos:current Nov 20, 2023
7 checks passed
@c-po
Copy link
Member Author

c-po commented Nov 20, 2023

@Mergifyio backport sagitta

@mergify Mergify
Copy link
Contributor

mergify bot commented Nov 20, 2023 •
edited
Loading

backport sagitta

✅ Backports have been created

@c-po c-po deleted the t5762-https-api-socket branch November 20, 2023 18:21
@mergify mergify bot mentioned this pull request Nov 20, 2023
Merged
c-po added a commit that referenced this pull request Nov 20, 2023
@c-po
Merge pull request #2515 from vyos/mergify/bp/sagitta/pr-2508
03740f1 
http: T5762: api: make API socket backend communication the one and only default (backport #2508)
@c-po c-po mentioned this pull request Nov 21, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sever-sever sever-sever sever-sever approved these changes

@jestabro jestabro jestabro approved these changes

@dmbaturin dmbaturin dmbaturin approved these changes

@sarthurdev sarthurdev Awaiting requested review from sarthurdev sarthurdev was automatically assigned from vyos/reviewers

@zdc zdc Awaiting requested review from zdc zdc was automatically assigned from vyos/reviewers

Assignees

@c-po c-po

Labels
current
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@c-po @dmbaturin @jestabro @sever-sever