Simplify setup by removing path rewrite #4653

baierjan · 2022-05-13T15:51:33Z

Make websockets server understand the /api/v1/ws route which is
currently rewritten by a reverse-proxy to /ws. This should remove the
need for having a reverse-proxy as all paths are now handled as are.

There is no need to change anything in the setup as the other route
(i.e. /ws) is also handled in the same way.

Prerequisites:

Add route for WebSockets to allow communication without a proxy #4660

codecov · 2022-05-13T16:08:39Z

Codecov Report

Merging #4653 (dba08b6) into master (7b715e3) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head dba08b6 differs from pull request most recent head 6b1b73a. Consider uploading reports for the commit 6b1b73a to get more accurate results

@@            Coverage Diff             @@
##           master    #4653      +/-   ##
==========================================
- Coverage   98.05%   98.05%   -0.01%     
==========================================
  Files         374      374              
  Lines       34545    34514      -31     
==========================================
- Hits        33874    33843      -31     
  Misses        671      671

Impacted Files	Coverage Δ
t/28-logging.t	`100.00% <0.00%> (ø)`
lib/OpenQA/Log.pm	`100.00% <0.00%> (ø)`
t/ui/23-audit-log.t	`100.00% <0.00%> (ø)`
t/24-worker-engine.t	`100.00% <0.00%> (ø)`
lib/OpenQA/Schema/Result/Comments.pm	`100.00% <0.00%> (ø)`
lib/OpenQA/Schema/ResultSet/Comments.pm	`100.00% <0.00%> (ø)`
lib/OpenQA/WebAPI/Controller/API/V1/Comment.pm	`100.00% <0.00%> (ø)`
lib/OpenQA/Worker/Engines/isotovideo.pm	`97.17% <0.00%> (+0.01%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b715e3...6b1b73a. Read the comment docs.

okurz · 2022-05-14T09:03:17Z

container/webui/nginx.conf

@@ -30,7 +30,7 @@ http {
            proxy_set_header Connection $connection_upgrade;
            proxy_set_header Host $host;

-            rewrite /api/v1/ws/(.*) /ws/$1 break;
+            rewrite /api/v1/ws/(.*) /api/v1/ws/$1 break;


I understand your reasoning in the commit message but to me this looks like /api/v1/ws/<foo> is "rewritten" to /api/v1/ws/<foo> meaning not rewritten at all. What am I overlooking?

Yes, isn't the idea to be able to drop the rewrite completely?

Not rewritten at all, that is the intended behavior (or to put it differently, the rewrite is no longer needed). I could probably delete the rewrite completely, but I am not sure if it has another side-effect, as the same style of rewrite can be found in the liveviewhandler section: rewrite /liveviewhandler/(.*) /liveviewhandler/$1 break;.

If you are sure, it has no other functionality (like disabling other possible rewrites due to the break) I would delete both of them to keep it consistent.

I suppose both rewrites can be deleted then.

okurz

good. And now to testing. Feel free to test on o3+osd as well after trying it out locally including the liveview

baierjan · 2022-05-16T08:45:25Z

good. And now to testing. Feel free to test on o3+osd as well after trying it out locally including the liveview

Hm, the only real code change is adding a route, that one was already tested during development. The liveview was only changed in the template for nginx, which is as far as I know not used on o3 nor osd. So there is not much added value here. I can probably use the setup from docker compose, where the nginx is actually used (in the load-balancer container which is not build by us).

okurz · 2022-05-16T08:47:52Z

good. And now to testing. Feel free to test on o3+osd as well after trying it out locally including the liveview

Hm, the only real code change is adding a route, that one was already tested during development. The liveview was only changed in the template for nginx, which is as far as I know not used on o3 nor osd. So there is not much added value here.

Well, I mean for example just apply the apache changes on o3 or osd and see that everything still works as expected.

baierjan · 2022-05-16T11:59:29Z

good. And now to testing. Feel free to test on o3+osd as well after trying it out locally including the liveview

Hm, the only real code change is adding a route, that one was already tested during development. The liveview was only changed in the template for nginx, which is as far as I know not used on o3 nor osd. So there is not much added value here.

Well, I mean for example just apply the apache changes on o3 or osd and see that everything still works as expected.

I can't apply the apache changes before the code change (and the code change does not require anything else, the new route will just not be used in the current setup). Or do you mean I should apply both manually (because I do not see any added value here)?

Martchus · 2022-05-16T13:42:54Z

The best approach is likely to install the packages generated by the OBS checks locally or on a staging instance and see whether it still works after restarting apache2 and the websocket server. (/etc/apache2/vhosts.d/openqa-common.inc is contained by the openQA package so simply installing the package from the OBS checks will replace both, the apache config and the websockt server code.)

okurz · 2022-05-16T17:23:39Z

Or do you mean I should apply both manually (because I do not see any added value here)?

Yes, sorry, then apply both. Or, create a separate pull request with just the code change first, have it merged, then we apply config changes afterwards

baierjan · 2022-05-19T08:30:35Z

Or do you mean I should apply both manually (because I do not see any added value here)?

Yes, sorry, then apply both. Or, create a separate pull request with just the code change first, have it merged, then we apply config changes afterwards

Right, I did not realize that the apache/nginx templates are actually not templates but can be used directly in some deployments.

I created #4660 with only the first commit changes (adding the new route), this will follow.

okurz · 2022-05-19T09:18:16Z

#4660 merged

So, will you test it out on o3/osd or any other instance?

okurz · 2022-05-19T13:44:33Z

@Mergifyio rebase

There is no need to change anything in the setup as the other route (i.e. `/ws`) is also handled in the same way. On the other hand, the rewrite is unnecessary.

mergify · 2022-05-19T13:45:12Z

rebase

✅ Branch has been successfully rebased

baierjan · 2022-05-20T09:04:26Z

#4660 merged

So, will you test it out on o3/osd or any other instance?

Currently, this is set on osd. So far, no problems detected.

okurz

Ok, counts as successful test for me then :)

baierjan mentioned this pull request May 13, 2022

Add Helm chart for openQA #4650

Merged

okurz reviewed May 14, 2022

View reviewed changes

baierjan force-pushed the jbaier-fix/simplify-setup branch from f1a836f to e13658e Compare May 15, 2022 08:21

okurz reviewed May 16, 2022

View reviewed changes

baierjan force-pushed the jbaier-fix/simplify-setup branch from e13658e to e6fdc7b Compare May 19, 2022 08:26

Simplify setup by removing path rewrite

6b1b73a

There is no need to change anything in the setup as the other route (i.e. `/ws`) is also handled in the same way. On the other hand, the rewrite is unnecessary.

kalikiana force-pushed the jbaier-fix/simplify-setup branch from e6fdc7b to 6b1b73a Compare May 19, 2022 13:45

okurz added the acceptance-tests-needed Needed for code that is required to be tested on a production-like environment label May 19, 2022

okurz removed the acceptance-tests-needed Needed for code that is required to be tested on a production-like environment label May 20, 2022

okurz approved these changes May 20, 2022

View reviewed changes

Martchus approved these changes May 20, 2022

View reviewed changes

mergify bot merged commit ae749dc into master May 20, 2022

okurz deleted the jbaier-fix/simplify-setup branch May 20, 2022 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify setup by removing path rewrite #4653

Simplify setup by removing path rewrite #4653

baierjan commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

okurz May 14, 2022

Martchus May 14, 2022

baierjan May 14, 2022

Martchus May 14, 2022

okurz left a comment

baierjan commented May 16, 2022

okurz commented May 16, 2022

baierjan commented May 16, 2022

Martchus commented May 16, 2022

okurz commented May 16, 2022

baierjan commented May 19, 2022

okurz commented May 19, 2022 •

edited

Loading

okurz commented May 19, 2022

mergify bot commented May 19, 2022

baierjan commented May 20, 2022

okurz left a comment

Simplify setup by removing path rewrite #4653

Simplify setup by removing path rewrite #4653

Conversation

baierjan commented May 13, 2022 • edited Loading

codecov bot commented May 13, 2022 • edited Loading

Codecov Report

okurz May 14, 2022

Choose a reason for hiding this comment

Martchus May 14, 2022

Choose a reason for hiding this comment

baierjan May 14, 2022

Choose a reason for hiding this comment

Martchus May 14, 2022

Choose a reason for hiding this comment

okurz left a comment

Choose a reason for hiding this comment

baierjan commented May 16, 2022

okurz commented May 16, 2022

baierjan commented May 16, 2022

Martchus commented May 16, 2022

okurz commented May 16, 2022

baierjan commented May 19, 2022

okurz commented May 19, 2022 • edited Loading

okurz commented May 19, 2022

mergify bot commented May 19, 2022

✅ Branch has been successfully rebased

baierjan commented May 20, 2022

okurz left a comment

Choose a reason for hiding this comment

baierjan commented May 13, 2022 •

edited

Loading

codecov bot commented May 13, 2022 •

edited

Loading

okurz commented May 19, 2022 •

edited

Loading