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

[IMP] runbot: allow custom run path per user #1028

Open
wants to merge 2 commits into
base: 18.0
Choose a base branch
from

Conversation

Williambraecky
Copy link
Contributor

Adds a new setting on users to define a custom path to use when
accessing a running build.
Can be used to directly access a specific page instead of landing on the
home page from a runbot run.

Duplicate model inheritance for res.users
@C3POdoo C3POdoo requested a review from a team January 2, 2025 11:15
@Williambraecky
Copy link
Contributor Author

I had to change the nginx config to allow for the catch-all to keep the path.
Since we were using regex capturing groups in the server_name I had to change all capturing groups to named ones.

I tested this change locally on my nginx proxy:
=== Tested with nginx config:

server {
  set $forward_scheme http;
  set $server         "172.30.100.1";
  set $port           9696;

  listen 80;
listen [::]:80;

listen 443 ssl;
listen [::]:443 ssl;


  server_name ~^72739210\-master(?<db>-[a-z0-9_-]+)?-65851847(?<fingerprint>-[a-z0-9_]+)\.home\.braeckman\.ovh$;

  http2 on;


  # Let's Encrypt SSL
  include conf.d/include/letsencrypt-acme-challenge.conf;
  include conf.d/include/ssl-ciphers.conf;
  ssl_certificate /etc/letsencrypt/live/npm-1/fullchain.pem;
  ssl_certificate_key /etc/letsencrypt/live/npm-1/privkey.pem;


  location ~ /(?<path>.*) {

	  add_header Set-Cookie "tk-72739210=65851847$fingerprint;Domain=runbot230.odoo.com;Max-Age=604800";
    return 307 http://72739210-master$db.runbot230.odoo.com/$path;

  }


  # Custom
  include /data/nginx/custom/server_proxy[.]conf;
}

curl -v https://72739210-master-all-65851847-17f9.home.braeckman.ovh/web/login output:

Same expected output without path

prod -> need to change custom nginx_config

@@ -79,11 +79,11 @@ server {
<t id="server_build_anchor"/>
server {
listen 8080;
server_name ~^<t t-out="re_escape(build.dest)"/>(-[a-z0-9_-]+)?-<t t-out="build._get_run_token()[0]"/>(-[a-z0-9_]+)\.<t t-out="re_escape(build.host)"/>$;
server_name ~^<t t-out="re_escape(build.dest)"/>(?&lt;db&gt;-[a-z0-9_-]+)?-<t t-out="build._get_run_token()[0]"/>(?&lt;fingerprint&gt;-[a-z0-9_]+)\.<t t-out="re_escape(build.host)"/>$;
Copy link
Contributor Author

@Williambraecky Williambraecky Jan 2, 2025

Choose a reason for hiding this comment

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

It's a bit unreadable but &lt;name&gt; is the same as <name>

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we coudn't make it easier to read with something like

Suggested change
server_name ~^<t t-out="re_escape(build.dest)"/>(?&lt;db&gt;-[a-z0-9_-]+)?-<t t-out="build._get_run_token()[0]"/>(?&lt;fingerprint&gt;-[a-z0-9_]+)\.<t t-out="re_escape(build.host)"/>$;
<t t-set="dest" t-value="re_escape(build.dest)"/>
<t t-set="host" t-value="re_escape(build.host)"/>
<t t-set="token" t-value="build._get_run_token()[0] "/>
<t t-set="db" t-value="(?&lt;db&gt;-[a-z0-9_-]+)?"/>
server_name ~^<t t-out="f'{dest}{db}-{token}-[a-z0-9_]+\.{host}"/>$;

(not sure it is better, not tested)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using an f-string didn't work (context couldn't find variables?).
So I stored and output component individually. I think it's slightly more readable.

@Xavier-Do
Copy link
Contributor

Xavier-Do commented Jan 2, 2025

I'm worried that the change to the nginx config may break existing config and custom redirect behaviour, to check in details.

Anyway I need to go more in depth to check if it is needed but in any case, we are trying to avoid config on users:

  • Not all runbot user can have a user
  • critical model

We already have some configuration based on cookies, wouldn't it be better to add that there?

I don't say that this solution is not correct but we may think about that.

@Williambraecky
Copy link
Contributor Author

Williambraecky commented Jan 2, 2025

I'm worried that the change to the nginx config may break existing config and custom redirect behaviour, to check in details.

Anyway I need to go more in depth to check if it is needed but in any case, we are trying to avoid config on users:

  • Not all runbot user can have a user
  • critical model

We already have some configuration based on cookies, wouldn't it be better to add that there?

I don't say that this solution is not correct but we may think about that.

I understand, for the nginx config, our customization on prod will be broken (the cookie will be invalid, but the redirection will work) if the configuration gets generated before we fix the template.

For the user settings, I don't know how practical cookie settings are, if one pays close attention to cookies and clears them often it could cause some confusion as well, what about using res.users.settings ? We can also make a hybrid such that preferences are saved on the user but cookies can also be used to dictate the path to use.

@Xavier-Do
Copy link
Contributor

We can also make a hybrid such that preferences are saved on the user but cookies can also be used to dictate the path to use.

Looks like a good compromise, if the value is not set in the cookie use the user values as a fallback +1. We could do the same thing for "more info".

Using res.user.settings is also a good idea I think, it is the goal of this model IMHO.

@Williambraecky Williambraecky force-pushed the 18.0-user-custom-path-wbr branch 2 times, most recently from 1be3026 to 633c157 Compare January 2, 2025 14:58
@Williambraecky
Copy link
Contributor Author

We can also make a hybrid such that preferences are saved on the user but cookies can also be used to dictate the path to use.

Looks like a good compromise, if the value is not set in the cookie use the user values as a fallback +1. We could do the same thing for "more info".

Using res.user.settings is also a good idea I think, it is the goal of this model IMHO.

Sounds good :)

I didn't do the "more info" stuff now, let's see if we like this system and revisit later if it works out 👍

Adds a new setting on users to define a custom path to use when
accessing a running build.
Can be used to directly access a specific page instead of landing on the
home page from a runbot run.
@Williambraecky Williambraecky force-pushed the 18.0-user-custom-path-wbr branch from 633c157 to 9fc4f2e Compare January 2, 2025 15:12
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.

2 participants