mishaschwartz commented 10 months ago

Overview

We checked that all username variables are respected in the source code. We checked that there are no instances of the default being hardcoded (the assumption being that no one would ever actually change the variable from the default).

The only issue found was that the MAGPIE_ADMIN_USERNAME variable was not used to set the default value for JUPYTERHUB_ADMIN_USERS properly.

Changes

Non-breaking changes

Updates default value for JUPYTERHUB_ADMIN_USERS to respect other variable settings

Breaking changes

Related Issue / Discussion

Resolves #417

Additional Information

Links to other issues or sources.

birdhouse_daccs_configs_branch: master birdhouse_skip_ci: false

mishaschwartz commented 10 months ago

Note: this targets the fix-node-details branch. If that branch is pulled in first, update the target of this PR to master

crim-jenkins-bot commented 10 months ago

E2E Test Results

DACCS-iac Pipeline Results

Build URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2413/
Result : failure

BIRDHOUSE_DEPLOY_BRANCH : security-defaults
DACCS_CONFIGS_BRANCH : master
PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master
PAVICS_SDI_BRANCH : master

DESTROY_INFRA_ON_EXIT : true
PAVICS_HOST : https://host-140-154.rdext.crim.ca

Infrastructure deployment failed. Instance has not been destroyed. @matprov

mishaschwartz commented 10 months ago

OK there's a problem with this.... the JUPYTERHUB_ADMIN_USERS cannot be made a DELAYED_EVAL variable and keep backwards compatibility.

This is because the current default is

JUPYTERHUB_ADMIN_USERS="{'admin'}"

which when run through the delayed evaluation algorithm resolves to {admin} which is not valid python.

To fix this we either need to:

update all current deployments that use the default
update the delayed evaluation algorithm to handle this edge case
fix this a different way by deprecating the JUPYTERHUB_ADMIN_USERS and either:
1. Creating a different variable in a different format that will be parsed correctly by the delayed evaluation algorithm
2. Not setting the jupyterhub admin users here at all but instead flagging a user as an admin in jupyterhub based on whether they are an admin in Magpie (this requires an update to https://github.com/Ouranosinc/jupyterhub but should be pretty easy to do)

mishaschwartz commented 10 months ago

@fmigneault @tlvu @eyvorchuk

How do you all set the JUPYTERHUB_ADMIN_USERS variable currently? Are you all just using the default or is anyone customizing it in env.local?

Do you have any objections to getting rid of the JUPYTERHUB_ADMIN_USERS variable and making it so that any users who are members of the administrators group in Magpie also get admin permissions in Jupterhub?

fmigneault commented 10 months ago

We set explicitly export JUPYTERHUB_ADMIN_USERS="{'admin'}".

I think the correct fix would be to adjust the delayed eval operation. It looks like it is its echo call that gets rid of the single quotes. It should not assume them to be shell quotes, but "special characters" in this case.

Something like the following? Not sure if -e is portable for the instances that needed sh and backticks.

❯ X=admin
❯ X='{\"${A}\"}'
❯ eval `echo -e $X`
zsh: command not found: "admin"
❯ X='\{\"${A}\"\}'
❯ eval "echo -e $X"
{"admin"}

mishaschwartz commented 10 months ago

@fmigneault

The issue is when this is used:

$ export JUPYTERHUB_ADMIN_USERS="{'admin'}"
$ echo ${JUPYTERHUB_ADMIN_USERS}
{'admin'}                                              # quotes are still there
$ eval "echo ${JUPYTERHUB_ADMIN_USERS}"
{admin}                                                # quotes are gone

Note that this works:

$ eval 'echo ${JUPYTERHUB_ADMIN_USERS}'
{'admin'}

But it breaks for other delayed eval variables:

$ X=123
$ Y='${X}'
$ eval 'echo $Y'
${X}                                             # we expect "123" not "${X}"

So it is not a valid solution for our use-case

fmigneault commented 10 months ago

@mishaschwartz My bad, I over copy-pasted my test samples, which might have caused confusion.

What I meant to display was only:

❯ A=admin
❯ X='\{\"${A}\"\}'
❯ eval "echo -e $X"
{"admin"}

This seems to work just like currently (ie: quotes are not changed from eval "echo ${JUPYTERHUB_ADMIN_USERS}"). The only distinction is adding -e to allow \ support (might even not be necessary depending on shell implementations).

This could be used to set the updated value here; https://github.com/bird-house/birdhouse-deploy/blob/8bf753405e8dd732d69fafdf953e5b56fccb024f/birdhouse/read-configs.include.sh#L192-L194

mishaschwartz commented 10 months ago

@fmigneault

Thanks for the clarification. To use your example the issue is still that:

❯ X="{'admin'}"
❯ eval "echo -e $X"
{admin}

Which is an issue because we want to be able to handle the case where the settings are either:

export JUPYTERHUB_ADMIN_USERS="{'admin'}"

OR

export JUPYTERHUB_ADMIN_USERS='{\"${MAGPIE_ADMIN_USERNAME}\"}'

Your example works well for the second case but doesn't work for the first case.

I'm playing around with it too and I'll see if I can come up with a solution that doesn't require us to ask people to change their env.local file. But we may have to fall back to that if we can't make this backwards compatible with the existing settings.

mishaschwartz commented 10 months ago

@fmigneault

Ok I think I've figured it out. There was a confusion of quoting so I added an extra step that evaluates the value of the delayed eval variable in one step and then exports it in a second step. That allows for some different quoting options and allows it to work with the old settings as well.

See here: https://github.com/bird-house/birdhouse-deploy/pull/418/files#diff-86bb1b7f48133ba418ade714466e5d695b8ee9fb0924e2fc12752680efdb9879R261-R262

fmigneault commented 10 months ago

@mishaschwartz Great news! I think this merits a small unittest to avoid regressions.

crim-jenkins-bot commented 10 months ago

E2E Test Results

DACCS-iac Pipeline Results

Build URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2431/
Result : success

BIRDHOUSE_DEPLOY_BRANCH : security-defaults
DACCS_CONFIGS_BRANCH : master
PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master
PAVICS_SDI_BRANCH : master

DESTROY_INFRA_ON_EXIT : true
PAVICS_HOST : https://host-140-90.rdext.crim.ca

PAVICS-e2e-workflow-tests Pipeline Results

Tests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1502/

NOTEBOOK TEST RESULTS

    
[2024-01-22T18:58:22.217Z] ============================= test session starts ==============================
[2024-01-22T18:58:22.217Z] platform linux -- Python 3.9.16, pytest-7.3.1, pluggy-1.0.0
[2024-01-22T18:58:22.217Z] rootdir: /home/jenkins/agent/workspace/PAVICS-e2e-workflow-tests_master
[2024-01-22T18:58:22.218Z] plugins: anyio-3.6.1, dash-2.10.0, nbval-0.9.6, tornasync-0.6.0.post2, xdist-3.3.1
[2024-01-22T18:58:22.218Z] collected 264 items
[2024-01-22T18:58:22.218Z] 
[2024-01-22T18:58:33.102Z] notebooks-auth/geoserver.ipynb ..................                        [  6%]
[2024-01-22T18:59:07.847Z] notebooks-auth/test_cowbird_jupyter.ipynb ..........                     [ 10%]
[2024-01-22T18:59:14.091Z] notebooks-auth/test_thredds.ipynb ...........                            [ 14%]
[2024-01-22T18:59:24.149Z] pavics-sdi-master/docs/source/notebooks/WCS_example.ipynb .......        [ 17%]
[2024-01-22T18:59:33.882Z] pavics-sdi-master/docs/source/notebooks/WFS_example.ipynb ......         [ 19%]
[2024-01-22T18:59:44.534Z] pavics-sdi-master/docs/source/notebooks/WMS_example.ipynb ........       [ 22%]
[2024-01-22T19:07:18.406Z] pavics-sdi-master/docs/source/notebooks/climex.ipynb ............        [ 27%]
[2024-01-22T19:07:18.982Z] pavics-sdi-master/docs/source/notebooks/eccc-geoapi-climate-stations.ipynb . [ 27%]
[2024-01-22T19:07:27.612Z] ...............                                                          [ 33%]
[2024-01-22T19:07:37.300Z] pavics-sdi-master/docs/source/notebooks/eccc-geoapi-xclim.ipynb .....    [ 35%]
[2024-01-22T19:07:44.599Z] pavics-sdi-master/docs/source/notebooks/esgf-dap.ipynb ......            [ 37%]
[2024-01-22T19:08:01.042Z] pavics-sdi-master/docs/source/notebooks/forecasts.ipynb ......           [ 39%]
[2024-01-22T19:08:06.812Z] pavics-sdi-master/docs/source/notebooks/opendap.ipynb .......            [ 42%]
[2024-01-22T19:08:11.346Z] pavics-sdi-master/docs/source/notebooks/pavics_thredds.ipynb .....       [ 44%]
[2024-01-22T19:13:25.283Z] pavics-sdi-master/docs/source/notebooks/regridding.ipynb ............... [ 50%]
[2024-01-22T19:14:40.722Z] .............                                                            [ 54%]
[2024-01-22T19:14:43.847Z] pavics-sdi-master/docs/source/notebooks/rendering.ipynb ....             [ 56%]
[2024-01-22T19:14:46.434Z] pavics-sdi-master/docs/source/notebooks/subset-user-input.ipynb ........ [ 59%]
[2024-01-22T19:15:03.906Z] .................                                                        [ 65%]
[2024-01-22T19:15:12.222Z] pavics-sdi-master/docs/source/notebooks/subsetting.ipynb ......          [ 68%]
[2024-01-22T19:15:13.609Z] pavics-sdi-master/docs/source/notebook-components/weaver_example.ipynb . [ 68%]
[2024-01-22T19:15:32.232Z] .........                                                                [ 71%]
[2024-01-22T19:15:41.795Z] finch-master/docs/source/notebooks/dap_subset.ipynb ...........          [ 76%]
[2024-01-22T19:15:51.285Z] finch-master/docs/source/notebooks/finch-usage.ipynb ......              [ 78%]
[2024-01-22T19:15:53.197Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-1DataAccess.ipynb . [ 78%]
[2024-01-22T19:15:56.280Z] ......                                                                   [ 81%]
[2024-01-22T19:16:02.882Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-2Subsetting.ipynb . [ 81%]
[2024-01-22T19:16:19.180Z] .............                                                            [ 86%]
[2024-01-22T19:16:29.201Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-3Climate-Indicators.ipynb . [ 86%]
[2024-01-22T19:17:05.767Z] ....s.                                                                   [ 89%]
[2024-01-22T19:17:13.914Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-4Ensembles.ipynb . [ 89%]
[2024-01-22T19:17:29.473Z] ...                                                                      [ 90%]
[2024-01-22T19:17:41.710Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-5Visualization.ipynb . [ 90%]
[2024-01-22T19:18:06.237Z] ......                                                                   [ 93%]
[2024-01-22T19:18:07.620Z] notebooks/hummingbird.ipynb ............                                 [ 97%]
[2024-01-22T19:20:41.802Z] notebooks/stress-tests.ipynb ......                                      [100%]
[2024-01-22T19:20:41.802Z] 
[2024-01-22T19:20:41.802Z] ================= 263 passed, 1 skipped in 1338.67s (0:22:18) ==================

crim-jenkins-bot commented 10 months ago

E2E Test Results

DACCS-iac Pipeline Results

Build URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2432/
Result : failure

BIRDHOUSE_DEPLOY_BRANCH : security-defaults
DACCS_CONFIGS_BRANCH : master
PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master
PAVICS_SDI_BRANCH : master

DESTROY_INFRA_ON_EXIT : true
PAVICS_HOST : https://host-140-90.rdext.crim.ca

PAVICS-e2e-workflow-tests Pipeline Results

Tests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1504/

NOTEBOOK TEST RESULTS

    
[2024-01-22T20:40:58.621Z] ============================= test session starts ==============================
[2024-01-22T20:40:58.621Z] platform linux -- Python 3.9.16, pytest-7.3.1, pluggy-1.0.0
[2024-01-22T20:40:58.621Z] rootdir: /home/jenkins/agent/workspace/PAVICS-e2e-workflow-tests_master@2
[2024-01-22T20:40:58.621Z] plugins: anyio-3.6.1, dash-2.10.0, nbval-0.9.6, tornasync-0.6.0.post2, xdist-3.3.1
[2024-01-22T20:40:58.621Z] collected 264 items
[2024-01-22T20:40:58.621Z] 
[2024-01-22T20:41:09.788Z] notebooks-auth/geoserver.ipynb ..................                        [  6%]
[2024-01-22T20:41:46.362Z] notebooks-auth/test_cowbird_jupyter.ipynb ..........                     [ 10%]
[2024-01-22T20:41:50.352Z] notebooks-auth/test_thredds.ipynb ...........                            [ 14%]
[2024-01-22T20:41:59.271Z] pavics-sdi-master/docs/source/notebooks/WCS_example.ipynb .......        [ 17%]
[2024-01-22T20:42:09.349Z] pavics-sdi-master/docs/source/notebooks/WFS_example.ipynb ......         [ 19%]
[2024-01-22T20:42:12.793Z] pavics-sdi-master/docs/source/notebooks/WMS_example.ipynb .FFFFFFF       [ 22%]
[2024-01-22T20:49:41.192Z] pavics-sdi-master/docs/source/notebooks/climex.ipynb ............        [ 27%]
[2024-01-22T20:49:41.192Z] pavics-sdi-master/docs/source/notebooks/eccc-geoapi-climate-stations.ipynb . [ 27%]
[2024-01-22T20:49:46.941Z] ...............                                                          [ 33%]
[2024-01-22T20:49:57.346Z] pavics-sdi-master/docs/source/notebooks/eccc-geoapi-xclim.ipynb .....    [ 35%]
[2024-01-22T20:50:04.358Z] pavics-sdi-master/docs/source/notebooks/esgf-dap.ipynb ......            [ 37%]
[2024-01-22T20:50:22.395Z] pavics-sdi-master/docs/source/notebooks/forecasts.ipynb ......           [ 39%]
[2024-01-22T20:50:29.225Z] pavics-sdi-master/docs/source/notebooks/opendap.ipynb .......            [ 42%]
[2024-01-22T20:50:34.554Z] pavics-sdi-master/docs/source/notebooks/pavics_thredds.ipynb .....       [ 44%]
[2024-01-22T20:54:22.428Z] pavics-sdi-master/docs/source/notebooks/regridding.ipynb ............... [ 50%]
[2024-01-22T20:55:41.323Z] .............                                                            [ 54%]
[2024-01-22T20:55:45.417Z] pavics-sdi-master/docs/source/notebooks/rendering.ipynb ....             [ 56%]
[2024-01-22T20:55:48.006Z] pavics-sdi-master/docs/source/notebooks/subset-user-input.ipynb ........ [ 59%]
[2024-01-22T20:56:04.898Z] .................                                                        [ 65%]
[2024-01-22T20:56:12.498Z] pavics-sdi-master/docs/source/notebooks/subsetting.ipynb ......          [ 68%]
[2024-01-22T20:56:14.420Z] pavics-sdi-master/docs/source/notebook-components/weaver_example.ipynb . [ 68%]
[2024-01-22T20:56:31.726Z] .........                                                                [ 71%]
[2024-01-22T20:56:41.258Z] finch-master/docs/source/notebooks/dap_subset.ipynb ...........          [ 76%]
[2024-01-22T20:56:50.649Z] finch-master/docs/source/notebooks/finch-usage.ipynb ......              [ 78%]
[2024-01-22T20:56:52.037Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-1DataAccess.ipynb . [ 78%]
[2024-01-22T20:56:54.844Z] ......                                                                   [ 81%]
[2024-01-22T20:57:01.433Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-2Subsetting.ipynb . [ 81%]
[2024-01-22T20:57:17.420Z] .............                                                            [ 86%]
[2024-01-22T20:57:27.426Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-3Climate-Indicators.ipynb . [ 86%]
[2024-01-22T20:58:09.516Z] ....s.                                                                   [ 89%]
[2024-01-22T20:58:19.502Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-4Ensembles.ipynb . [ 89%]
[2024-01-22T20:58:35.047Z] ...                                                                      [ 90%]
[2024-01-22T20:58:49.957Z] PAVICS-landing-master/content/notebooks/climate_indicators/PAVICStutorial_ClimateDataAnalysis-5Visualization.ipynb . [ 90%]
[2024-01-22T20:59:16.228Z] ......                                                                   [ 93%]
[2024-01-22T20:59:18.914Z] notebooks/hummingbird.ipynb ............                                 [ 97%]
[2024-01-22T21:01:53.337Z] notebooks/stress-tests.ipynb ......                                      [100%]
[2024-01-22T21:01:53.338Z] 
[2024-01-22T21:01:53.338Z] =================================== FAILURES ===================================

mishaschwartz commented 10 months ago

@fmigneault want me to merge this to the fix-node-details branch or would you rather I wait until that is pulled in to master?

crim-jenkins-bot commented 10 months ago

E2E Test Results

DACCS-iac Pipeline Results

Build URL : http://daccs-jenkins.crim.ca:80/job/DACCS-iac-birdhouse/2436/
Result : failure

BIRDHOUSE_DEPLOY_BRANCH : security-defaults
DACCS_CONFIGS_BRANCH : master
PAVICS_E2E_WORKFLOW_TESTS_BRANCH : master
PAVICS_SDI_BRANCH : master

DESTROY_INFRA_ON_EXIT : true
PAVICS_HOST : https://

Infrastructure deployment failed. Instance has not been destroyed. @matprov

fmigneault commented 10 months ago

@mishaschwartz Yes you can merge it in the branch.

bird-house / birdhouse-deploy

Magpie: ensure that the `MAGPIE_ADMIN_USERNAME` variable is respected #418

Overview

Changes

Related Issue / Discussion

Additional Information

E2E Test Results

DACCS-iac Pipeline Results

E2E Test Results

DACCS-iac Pipeline Results

PAVICS-e2e-workflow-tests Pipeline Results

E2E Test Results

DACCS-iac Pipeline Results

PAVICS-e2e-workflow-tests Pipeline Results

E2E Test Results

DACCS-iac Pipeline Results