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

Magpie: ensure that the MAGPIE_ADMIN_USERNAME variable is respected #418

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

mishaschwartz
Copy link
Collaborator

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

Additional Information

Links to other issues or sources.

birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false

@github-actions github-actions bot added component/jupyterhub Related to JupyterHub as development frontend with notebooks documentation Improvements or additions to documentation labels Jan 16, 2024
@mishaschwartz
Copy link
Collaborator Author

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
Copy link
Collaborator

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

@@ -84,19 +84,14 @@ export SERVER_LICENSE_URL='${__DEFAULT__SERVER_LICENSE_URL}'
# Those will not be set explicitly as defaults to ensure they are overridden explicitly by the instance.
# These values would be detected only if the instance was configured using a copy of 'env.local.example'.
export __DEFAULT__MAGPIE_SECRET=itzaseekrit
#export __DEFAULT__MAGPIE_ADMIN_USERNAME=admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this one remain to warn about it not being modified?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I was debating whether to keep them or remove them....

I guess I was thinking that it's OK if the username stays as "admin" as long as we recommend they change the password.

But in the end, we're just warning the user that we recommend that it should be changed from the default. If they want to keep it, they can.

I guess I can put them back in

Comment on lines 89 to 91
#export __DEFAULT__POSTGRES_PAVICS_USERNAME=postgres-pavics
export __DEFAULT__POSTGRES_PAVICS_PASSWORD=postgres-qwerty
#export __DEFAULT__POSTGRES_MAGPIE_USERNAME=postgres-magpie
Copy link
Collaborator

Choose a reason for hiding this comment

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

These might be relevant to warn as well. They should be used everywhere in the configs.
Since this is for database endpoint connection rather than Magpie user profile, it should not be involved in other scripts than direct interactions between Magpie/Twitcher and Postgres.

@mishaschwartz
Copy link
Collaborator Author

mishaschwartz commented Jan 16, 2024

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

fmigneault commented Jan 17, 2024

@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;

v="`eval "echo \\$${i}"`"
eval 'export ${i}="`eval "echo ${v}"`"'
echo "delayed eval '$(env |grep "${i}=")'"

@mishaschwartz
Copy link
Collaborator Author

@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.

@github-actions github-actions bot added the ci/deployment Related to deployment utilities and scripts label Jan 22, 2024
@mishaschwartz
Copy link
Collaborator Author

@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
Copy link
Collaborator

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

@crim-jenkins-bot
Copy link
Collaborator

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) ==================
    
  

@github-actions github-actions bot added ci/operations Continuous Integration components ci/tests Issues or changes related to tests scripts labels Jan 22, 2024
@crim-jenkins-bot
Copy link
Collaborator

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
Copy link
Collaborator Author

@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
Copy link
Collaborator

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
Copy link
Collaborator

@mishaschwartz Yes you can merge it in the branch.

@mishaschwartz mishaschwartz merged commit 24950ba into fix-node-details Jan 25, 2024
4 of 5 checks passed
@mishaschwartz mishaschwartz deleted the security-defaults branch January 25, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/deployment Related to deployment utilities and scripts ci/operations Continuous Integration components ci/tests Issues or changes related to tests scripts component/jupyterhub Related to JupyterHub as development frontend with notebooks documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants