-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
Note: this targets the |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild 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 |
birdhouse/default.env
Outdated
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
birdhouse/default.env
Outdated
#export __DEFAULT__POSTGRES_PAVICS_USERNAME=postgres-pavics | ||
export __DEFAULT__POSTGRES_PAVICS_PASSWORD=postgres-qwerty | ||
#export __DEFAULT__POSTGRES_MAGPIE_USERNAME=postgres-magpie |
There was a problem hiding this comment.
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.
OK there's a problem with this.... the This is because the current default is
which when run through the delayed evaluation algorithm resolves to To fix this we either need to:
|
How do you all set the Do you have any objections to getting rid of the |
We set explicitly I think the correct fix would be to adjust the delayed eval operation. Something like the following? Not sure if ❯ X=admin
❯ X='{\"${A}\"}'
❯ eval `echo -e $X`
zsh: command not found: "admin"
❯ X='\{\"${A}\"\}'
❯ eval "echo -e $X"
{"admin"} |
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 |
@mishaschwartz 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 This could be used to set the updated value here; birdhouse-deploy/birdhouse/read-configs.include.sh Lines 192 to 194 in 8bf7534
|
Thanks for the clarification. To use your example the issue is still that:
Which is an issue because we want to be able to handle the case where the settings are either:
OR
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. |
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. |
@mishaschwartz Great news! I think this merits a small unittest to avoid regressions. |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild 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 ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1502/NOTEBOOK TEST RESULTS |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild 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 ResultsTests URL : http://daccs-jenkins.crim.ca:80/job/PAVICS-e2e-workflow-tests/job/master/1504/NOTEBOOK TEST RESULTS |
@fmigneault want me to merge this to the |
E2E Test ResultsDACCS-iac Pipeline ResultsBuild 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 |
@mishaschwartz Yes you can merge it in the branch. |
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 forJUPYTERHUB_ADMIN_USERS
properly.Changes
Non-breaking changes
JUPYTERHUB_ADMIN_USERS
to respect other variable settingsBreaking changes
Related Issue / Discussion
Additional Information
Links to other issues or sources.
birdhouse_daccs_configs_branch: master
birdhouse_skip_ci: false