Project

General

Profile

Actions

Support #20949

closed

Playground sends user to workbench 2 by default

Added by Peter Amstutz 8 months ago. Updated 9 days ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Due date:
Story points:
-
Release relationship:
Auto

Subtasks 2 (0 open2 closed)

Task #20963: Review 20949-playground-wb2-by-defaultResolvedLucas Di Pentima09/29/2023Actions
Task #21047: Review 20949-wb-redirects-improvementsResolvedPeter Amstutz10/06/2023Actions

Related issues

Related to Arvados - Feature #20732: Playground welcome bannerResolvedSarah Zaranek09/26/2023Actions
Actions #1

Updated by Peter Amstutz 8 months ago

  • Assigned To set to Lucas Di Pentima
Actions #2

Updated by Peter Amstutz 8 months ago

Actions #3

Updated by Lucas Di Pentima 8 months ago

  • Status changed from New to In Progress
Actions #4

Updated by Peter Amstutz 8 months ago

  • Target version changed from Development 2023-09-27 sprint to Development 2023-10-11 sprint
Actions #5

Updated by Lucas Di Pentima 8 months ago

Updates at commit 88e1c3e - branch 20949-playground-wb2-by-default (saltstack repo, not applied yet)

  • Cleans up unused account.
  • Applies Peter's changes from #20688 on pirca's workbench pillars.

The dry-run output for this is:

lucas@salt-1:~$ salt 'workbench*pirca*' state.highstate test=true pillar='{"env":"lucas"}'
workbench.pirca.playg:
----------
          ID: nginx_install-/var/www/.passenger
    Function: file.directory
        Name: /var/www/.passenger
      Result: None
     Comment: The following files will be changed:
              /var/www/.passenger: user - root
     Started: 18:47:37.558496
    Duration: 1.6 ms
     Changes:
              ----------
              /var/www/.passenger:
                  ----------
                  user:
                      root
----------
          ID: deb backports
    Function: pkgrepo.managed
        Name: deb  http://deb.debian.org/debian/ buster-backports main
      Result: None
     Comment: Package repo 'deb  http://deb.debian.org/debian/ buster-backports main' would be configured. This may cause pkg states to behave differently than stated if this action is repeated without test=True, due to the differences in the configured repositories.
     Started: 18:47:40.806728
    Duration: 94.294 ms
     Changes:
              ----------
              repo:
                  deb  http://deb.debian.org/debian/ buster-backports main
----------
          ID: apt.refresh_db
    Function: module.run
        Name: pkg.refresh_db
      Result: None
     Comment: Module function pkg.refresh_db is set to execute
     Started: 18:47:41.374116
    Duration: 1.299 ms
     Changes:
----------
          ID: users_absent_user_nico
    Function: user.absent
        Name: nico
      Result: None
     Comment: User nico set for removal
     Started: 18:47:47.273341
    Duration: 0.916 ms
     Changes:
----------
          ID: server_conf_2
    Function: file.managed
        Name: /etc/nginx/sites-available/arvados_workbench
      Result: None
     Comment: The file /etc/nginx/sites-available/arvados_workbench is set to be changed
              Note: No changes made, actual changes may
              be different due to other states.
     Started: 18:47:51.531261
    Duration: 158.829 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -8,30 +8,58 @@
                       server_name workbench.pirca.arvadosapi.com;
                       listen 443 http2 ssl;
                       index index.html;
                  +    rewrite ^/work_units/(.*) /processes/$1 redirect;
                  +    rewrite ^/container_requests/(.*) /processes/$1 redirect;
                  +    rewrite ^/users/(.*) /user/$1 redirect;
                  +    rewrite ^/groups/(.*) /group/$1 redirect;

                  -    location /composer {
                  -        root /var/www/arvados-composer;
                  +    if ($arg_disposition = attachment) {
                  +        rewrite ^/collections/([^/]*)/(.*) /?redirectToDownload=/c=$1/$2? redirect;
                       }

                  -    location /composer/composer.yml {
                  +    if ($arg_disposition = inline) {
                  +        rewrite ^/collections/([^/]*)/(.*) /?redirectToPreview=/c=$1/$2? redirect;
                  +    }
                  +    rewrite ^/virtual_machines.* /virtual-machines-admin redirect;
                  +    rewrite ^/users/.*/virtual_machines /virtual-machines-user redirect;
                  +    rewrite ^/authorized_keys.* /ssh-keys-admin redirect;
                  +    rewrite ^/users/.*/ssh_keys /ssh-keys-user redirect;
                  +    rewrite ^/containers.* /all_processes redirect;
                  +    rewrite ^/container_requests /all_processes redirect;
                  +    rewrite ^/job.* /all_processes redirect;
                  +    rewrite ^/users/link_account /link_account redirect;
                  +    rewrite ^/search.* /search-results redirect;
                  +    rewrite ^/keep_services.* /keep-services redirect;
                  +    rewrite ^/trash_items.* /trash redirect;
                  +    rewrite ^/themes.* / redirect;
                  +    rewrite ^/keep_disks.* / redirect;
                  +    rewrite ^/user_agreements.* / redirect;
                  +    rewrite ^/nodes.* / redirect;
                  +    rewrite ^/humans.* / redirect;
                  +    rewrite ^/traits.* / redirect;
                  +    rewrite ^/sessions.* / redirect;
                  +    rewrite ^/logout.* / redirect;
                  +    rewrite ^/logged_out.* / redirect;
                  +    rewrite ^/current_token / redirect;
                  +    rewrite ^/logs.* / redirect;
                  +    rewrite ^/factory_jobs.* / redirect;
                  +    rewrite ^/uploaded_datasets.* / redirect;
                  +    rewrite ^/specimens.* / redirect;
                  +    rewrite ^/pipeline_templates.* / redirect;
                  +    rewrite ^/pipeline_instances.* / redirect;
                  +
                  +    location / {
                  +        root /var/www/arvados-workbench2/workbench2;
                  +        try_files $uri $uri/ /index.html;
                  +    }
                  +
                  +    location /config.json {
                           return 200 '{"API_HOST":"pirca.arvadosapi.com"}';
                       }
                  -
                  -    location / {
                  -        proxy_pass http://workbench_upstream;
                  -        proxy_read_timeout 300;
                  -        proxy_connect_timeout 90;
                  -        proxy_redirect off;
                  -        proxy_set_header X-Forwarded-Proto https;
                  -        proxy_set_header Host $http_host;
                  -        proxy_set_header X-Real-IP $remote_addr;
                  -        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
                  -    }
                  -    client_max_body_size 128m;
                       ssl_certificate /etc/letsencrypt/live/workbench.pirca.arvadosapi.com/fullchain.pem;
                       ssl_certificate_key /etc/letsencrypt/live/workbench.pirca.arvadosapi.com/privkey.pem;
                       include snippets/ssl_hardening_default.conf;
                  -    access_log /var/log/nginx/workbench.pirca.arvadosapi.com.access.log combined;
                  -    error_log /var/log/nginx/workbench.pirca.arvadosapi.com.error.log;
                  +    access_log /var/log/nginx/workbench2.pirca.arvadosapi.com.access.log combined;
                  +    error_log /var/log/nginx/workbench2.pirca.arvadosapi.com.error.log;
                   }

----------
          ID: server_state_3
    Function: file.absent
        Name: /etc/nginx/sites-enabled/arvados_workbench_upstream
      Result: None
     Comment: File /etc/nginx/sites-enabled/arvados_workbench_upstream is set for removal
     Started: 18:47:51.693588
    Duration: 0.678 ms
     Changes:
              ----------
              removed:
                  /etc/nginx/sites-enabled/arvados_workbench_upstream
----------
          ID: server_conf_5
    Function: file.managed
        Name: /etc/nginx/sites-available/arvados_workbench2
      Result: None
     Comment: The file /etc/nginx/sites-available/arvados_workbench2 is set to be changed
              Note: No changes made, actual changes may
              be different due to other states.
     Started: 18:47:51.845793
    Duration: 155.596 ms
     Changes:
              ----------
              diff:
                  ---
                  +++
                  @@ -7,11 +7,9 @@
                   server {
                       server_name workbench2.pirca.arvadosapi.com;
                       listen 443 http2 ssl;
                  -    index index.html;

                       location / {
                  -        root /var/www/arvados-workbench2/workbench2;
                  -        try_files $uri $uri/ /index.html;
                  +        return 301 https://workbench.pirca.arvadosapi.com$request_uri;
                       }

                       location /config.json {
----------
          ID: listener_nginx_service
    Function: service.mod_watch
        Name: nginx
      Result: None
     Comment: Service is set to be reloaded
     Started: 18:47:57.819863
    Duration: 14.742 ms
     Changes:

Summary for workbench.pirca.playg
--------------
Succeeded: 221 (unchanged=8, changed=5)
Failed:      0
--------------
Total states run:     221
Total run time:    19.962 s
Actions #6

Updated by Tom Clegg 8 months ago

The only thing I can think to add is that this seems like a good time to update pirca's cluster config as well:

            Workbench1:
              ExternalURL: "https://workbench.{{ stack['domain'] }}" 
            Workbench2:
              ExternalURL: "https://workbench2.{{ stack['domain'] }}" 
Actions #7

Updated by Lucas Di Pentima 8 months ago

Something like this?

diff --git a/pillarstack/arvados_playground/pirca/role/arvados_host.yaml b/pillarstack/arvados_playground/pirca/role/arvados_host.yaml
index 09628b9..4eb4698 100644
--- a/pillarstack/arvados_playground/pirca/role/arvados_host.yaml
+++ b/pillarstack/arvados_playground/pirca/role/arvados_host.yaml
@@ -72,10 +72,8 @@ custom_files:
             RailsAPI:
               InternalURLs:
                 "http://localhost:8004": {}
-            Workbench1:
-              ExternalURL: "https://workbench.{{ stack['domain'] }}" 
             Workbench2:
-              ExternalURL: "https://workbench2.{{ stack['domain'] }}" 
+              ExternalURL: "https://workbench.{{ stack['domain'] }}" 
             WebShell:
               ExternalURL: "https://webshell.{{ stack['domain'] }}" 
             # keep-web
Actions #8

Updated by Tom Clegg 7 months ago

Yes, that looks great. (Sorry for the delay)

Actions #9

Updated by Peter Amstutz 7 months ago

Ah, no that's a bad idea, some thing refer to the Workbench1 key and some things refer to the Workbench2 key, the correct thing to do is just set them both to the same value.

Actions #11

Updated by Peter Amstutz 7 months ago

Tom Clegg wrote in #note-10:

Hm, I also just tried clicking an auto-linked UUID in redmine, and https://arvadosapi.com/pirca-4zz18-y025kja0fuvpvpq redirected to https://workbench.pirca.arvadosapi.com/actions?uuid=pirca-4zz18-y025kja0fuvpvpq which renders an error page. Is our plan to handle this using wb2 code, or nginx config?

It should be handled by nginx config, but it isn't. I didn't include redirects for "actions" because I thought they were only used internally by Workbench 1's own forms and redirects and never visible to the user, but clearly that's not the case.

Actions #12

Updated by Lucas Di Pentima 7 months ago

I'll add those action URL redirectors

Actions #13

Updated by Lucas Di Pentima 7 months ago

Added the following configuration to support query params based redirects (already deployed):

map $request_uri $actions_redirect {
    ~^/actions\?uuid=(.*-4zz18-.*) /collections/$1;
    ~^/actions\?uuid=(.*-j7d0g-.*) /projects/$1;
    ~^/actions\?uuid=(.*-tpzed-.*) /projects/$1;
    ~^/actions\?uuid=(.*-7fd4e-.*) /workflows/$1;
    ~^/actions\?uuid=(.*-xvhdp-.*) /processes/$1;
    ~^/actions\?uuid=(.*) /;
    default 0;
}

server {
    server_name workbench.pirca.arvadosapi.com;
...
    if ($actions_redirect) {
        return 301 $actions_redirect;
    }
...
}
...
Actions #14

Updated by Peter Amstutz 7 months ago

Lucas Di Pentima wrote in #note-13:

Added the following configuration to support query params based redirects (already deployed):

[...]

1. any particular reason to use the "map" feature here instead of just adding to the existing list of rewrites?
2. this should probably be added to the installer?

Actions #15

Updated by Lucas Di Pentima 7 months ago

Peter Amstutz wrote in #note-14:

Lucas Di Pentima wrote in #note-13:

Added the following configuration to support query params based redirects (already deployed):

[...]

1. any particular reason to use the "map" feature here instead of just adding to the existing list of rewrites?
2. this should probably be added to the installer?

The usual rewrite directives are not useful to match query string parameters. Do you see any downside on using map? I haven't read any from the docs.
Yes, I'll add them to the installer.

Actions #16

Updated by Peter Amstutz 7 months ago

Lucas Di Pentima wrote in #note-15:

Peter Amstutz wrote in #note-14:

Lucas Di Pentima wrote in #note-13:

Added the following configuration to support query params based redirects (already deployed):

[...]

1. any particular reason to use the "map" feature here instead of just adding to the existing list of rewrites?
2. this should probably be added to the installer?

The usual rewrite directives are not useful to match query string parameters. Do you see any downside on using map? I haven't read any from the docs.
Yes, I'll add them to the installer.

I don't see any downsides. In fact I wasn't familiar with the map construct, but it looks like it might be cleaner than what I ended up doing with individual rewrites. I was thinking of the rewrites I made that were conditional on query parameters, but that's different from what you're doing here.

Actions #17

Updated by Lucas Di Pentima 7 months ago

Updates at 3757793 - branch 20949-wb-redirects-improvements

  • Adds WB1's action controller URL redirections to installer pillars.
Actions #18

Updated by Lucas Di Pentima 7 months ago

Double-login requirement reported from #21052:

Steps to reproduce:

Be logged into a Google account.
Go to workbench.pirca.arvadosapi.com. You get the "Welcome to Arvados Playground" screen.
Press the Log In button.
On the Google auth page, select the account to log in with.
Expected results: You're logged in and viewing your home project in Workbench.

Actual results: You end up back at the "Welcome to Arvados Playground" page. If you repeat steps 3 and 4, then you end up on Workbench.

I don't believe this happened before the "workbench" hostname became Workbench 2. I have seen it across two different Google accounts. It doesn't seem to happen after you've gone through the dance once. But if we lose sign-ups who end up back at the log in screen and think Playground isn't working, that's a problem.

Actions #19

Updated by Lucas Di Pentima 7 months ago

Applied the following change to nginx so that https://playground.arvados.org doesn't redirect to https://workbench.pirca.arvadosapi.com// (note the double slash at the end). I'm guessing Workbench1 used to fix that, but Workbench2 logs out the user automatically when this happens.

diff --git a/pillarstack/arvados_playground/pirca/node/workbench.pirca.playg.yaml b/pillarstack/arvados_playground/pirca/node/workbench.pirca.playg.yaml
index 744d04c..26404ab 100644
--- a/pillarstack/arvados_playground/pirca/node/workbench.pirca.playg.yaml
+++ b/pillarstack/arvados_playground/pirca/node/workbench.pirca.playg.yaml
@@ -248,7 +248,7 @@ nginx:
             - server_name: playground.arvados.org
             - listen:
               - 443 http2 ssl
-              - return: '301 https://workbench.{{ stack['domain'] }}/$request_uri'
+              - return: '301 https://workbench.{{ stack['domain'] }}$request_uri'
             - include: snippets/ssl_hardening_default.conf
             - ssl_certificate: /etc/letsencrypt/live/playground.arvados.org/fullchain.pem
             - ssl_certificate_key: /etc/letsencrypt/live/playground.arvados.org/privkey.pem
Actions #20

Updated by Peter Amstutz 7 months ago

Lucas Di Pentima wrote in #note-17:

Updates at 3757793 - branch 20949-wb-redirects-improvements

  • Adds WB1's action controller URL redirections to installer pillars.

LGTM

Actions #21

Updated by Lucas Di Pentima 7 months ago

  • Status changed from In Progress to Resolved
Actions #22

Updated by Peter Amstutz 9 days ago

  • Release set to 67
Actions

Also available in: Atom PDF