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

helm: add optional notifications #309

Conversation

diegodelemos
Copy link
Member

@diegodelemos diegodelemos force-pushed the 308-cronjob-email-system-status-report branch 4 times, most recently from bcf7364 to 1b0ec5e Compare May 1, 2020 14:12
@diegodelemos diegodelemos marked this pull request as ready for review May 1, 2020 14:24
@diegodelemos diegodelemos force-pushed the 308-cronjob-email-system-status-report branch from 1b0ec5e to 1e30c34 Compare May 1, 2020 14:58
helm/reana/README.md Outdated Show resolved Hide resolved
helm/reana/README.md Outdated Show resolved Hide resolved
@diegodelemos diegodelemos force-pushed the 308-cronjob-email-system-status-report branch 2 times, most recently from dd662f2 to 544e61f Compare May 19, 2020 11:06
helm/reana/README.md Outdated Show resolved Hide resolved
Copy link
Member

@mvidalgarcia mvidalgarcia left a comment

Choose a reason for hiding this comment

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

After applying the changes in my comments it works well with MailDev.
Just one observation, the pods remain in Completed status, would it possible to clean them up?

helm/reana/templates/reana-server.yaml Show resolved Hide resolved
- name: REANA_ADMIN_ACCESS_TOKEN
valueFrom:
secretKeyRef:
name: {{ include "reana.prefix" . }}-admin-access-token
Copy link
Member

@mvidalgarcia mvidalgarcia May 20, 2020

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that it should be manually created, there is no way I can see we can create the secret with the correct value from the beginning, so this will have to go into docs or the message displayed just after installing REANA. The interaction could look like:

  1. User installs REANA
  2. Creates admin user and/or retrieves the admin token
  3. Then creates secret manually

Or in commands:

$ helm install reana reanahub/reana --wait
NAME: reana
LAST DEPLOYED: Wed Mar 18 10:27:06 2020
NAMESPACE: default
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:

Create your admin user with:
> kubectl exec -ti reana-server-xxxx-yyy -- flask reana-admin users-create-default user@my.org
<reana-admin-access-token>
> read -s REANA_ADMIN_ACCESS_TOKEN
> kubectl create secret generic <prefix>-admin-access-token --from-literal=ADMIN_ACCESS_TOKEN='$REANA_ADMIN_ACCESS_TOKEN'
$ kubectl exec -ti reana-server-xxxx-yyy -- flask reana-admin users-create-default
$ kubectl create secret generic reana-admin-access-token --from-literal=ADMIN_ACCESS_TOKEN='$REANA_ADMIN_ACCESS_TOKEN'
Thanks for flying REANA 🚀

Note: this will would, of course, automatise for developers.

What do you think about this? Otherwise, we create a secret with a value that makes no sense.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, I think it makes sense to create the admin user manually instead of imposing it in /scripts/setup

https://github.com/reanahub/reana-server/blob/a22b50263764467ebe8a8ec28284431b62205c12/scripts/setup#L14

The only problem I see is how to retrieve this generated token later, which it's essentially the same problem we have for setup-environment.

Copy link
Member

Choose a reason for hiding this comment

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

👍 for allowing people to set their admin account details semi-manually instead of hard-coding values such as info@reana.io as in the old days.

We could have a helper script reana/scripts/create-admin-user.sh that would do all the steps in an unassisted way in order to simplify procedure.

  • One option would to read the wanted email address and token, as you described. However we should then make validation of the token values passed by the user, to make sure the values is compliant.

  • Another option would be to use uuidgen or equivalent in the script to generate token automatically, to avoid any problems. I'd like this option better. This would also simplify the interaction in that new admins would have to provide basically only their email, for example:

$ helm install reana reanahub/reana --wait
[INFO] ...
[INFO] ...
$ ./scripts/create-admin-user.sh john.doe@example.org
[INFO] Admin user john.doe@example.org created.
[INFO] Your REANA_ACCESS_TOKEN is aaaa-bbbb-1111-2222.

In the future, for systems with SSO login, this post-installation admin-creation part could be modified to e.g. require logging in via web, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only problem I see is how to retrieve this generated token later, which it's essentially the same problem we have for setup-environment.

I thought of this precisely to tackle the problem in this PR and the one you mention (#311 (comment)) as if this is done then we can instead of going to DB, instruct reana-dev to read it from the Kubernetes secrets store:

$ kubectl get secret -o json reana-dev-secrets | \
  jq -r .data.REANA_SECRET_KEY | base64 -D
secret_key

And that would solve it.

However, we shouldn't ignore that we would end up with the REANA admin access token secret in two places (DB and Kubernetes)... a duplication and all the problems it can potentially bring.

Copy link
Member

Choose a reason for hiding this comment

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

This is practically what reana-admin users-create-default does:

$ flask reana-admin users-create-default test@test.es
Created 1st user with access_token: fcFL7atgKmdD5LaP5wX1sln0GRJ97-MTfLHz2hz69ME

I would rename this command to something like create-admin-user and modify its output.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will do the fastest following what we discussed to get this going and perhaps we can ticketise #309 (comment) as some things I believe would take more time to implement and test.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, reana-server/script/setup does that, and takes care of creating DB as well... I'm not sure it should be done there though. Following "do-one-thing-and-do-it-well" principle, let's reserve reana-server for handing REST API only, and let's finish post-installation-related setup steps outside, right after Helm finishes? This would better split responsibilities, and would allow for easy customisation and scriptability of various local installations. For example, we shall have Alembic upgrade recipes soon; where would we like to run those? This should be an interactive CLI process to warn admins of failures, etc. The r-server component is not a place for that, I think such scripts should rather live in the reana package, where admins are already helming and such. I guess one day soon we shall probably take both the admin generation and the DB creation out of r-server.

@diegodelemos diegodelemos force-pushed the 308-cronjob-email-system-status-report branch 3 times, most recently from 82a4b74 to 430fda8 Compare May 20, 2020 12:33
reana/cli.py Outdated Show resolved Hide resolved
@diegodelemos diegodelemos force-pushed the 308-cronjob-email-system-status-report branch from 430fda8 to 72df4bb Compare May 20, 2020 13:10
@diegodelemos diegodelemos force-pushed the 308-cronjob-email-system-status-report branch from 72df4bb to aec056f Compare May 20, 2020 13:48
@diegodelemos diegodelemos merged commit aec056f into reanahub:master May 20, 2020
@diegodelemos diegodelemos deleted the 308-cronjob-email-system-status-report branch May 20, 2020 13:52
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.

cronjobs: regular cluster status summaries by email
3 participants