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

feat: secrets.example.py #861

Merged
merged 5 commits into from
Sep 6, 2024
Merged

feat: secrets.example.py #861

merged 5 commits into from
Sep 6, 2024

Conversation

wesleyboar
Copy link
Member

@wesleyboar wesleyboar commented Aug 7, 2024

Overview

Create a secrets.example.py.

Related

Changes

  • added secrets.example.py
  • documented creation of secrets.py
  • documented creation of settings_custom.py

Testing & UI

N/A

Like in #577 for settings.
Copy link
Collaborator

@taoteg taoteg left a comment

Choose a reason for hiding this comment

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

A number of these settings are placed in the *.settings_local.py file rather than secrets.py.

Specifically:

  • ALLOWED_HOSTS
  • CEP_AUTH_VERIFICATION_ENDPOINT

Reason: These settings do not contain sensitive credentials.

Current Example: https://github.com/TACC/Core-Portal-Deployments/blob/main/ami/camino/prod.cms.settings_local.py

Can we split these into the appropriate files (using this same setup but including the settings_local.py file as well) so the usage matches the deployment environment?

@wesleyboar
Copy link
Member Author

Thanks, yes! Will do. I am glad to have your eyes on this.

Copy link
Member Author

@wesleyboar wesleyboar left a comment

Choose a reason for hiding this comment

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

@taoteg I moved secret values to settings_local.py. Also, please see my question about ES_ values.

Comment on lines +26 to +31
ES_AUTH = 'username:password'
ES_HOSTS = 'http://elasticsearch:9200'
ES_INDEX_PREFIX = 'cms-dev-{}'
ES_DOMAIN = 'http://localhost:8000'

HAYSTACK_CONNECTIONS = {
Copy link
Member Author

Choose a reason for hiding this comment

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

@taoteg, should all of these stay in secrets.py?

  • The ES_AUTH value seems secret.
  • The ES_… values are expected by HAYSTACK_CONNECTIONS.

Copy link
Collaborator

@taoteg taoteg Sep 6, 2024

Choose a reason for hiding this comment

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

Simplifying the response - it just depends, but here is what I would do if I was truly organizing them:

  • ES_AUTH in secrets.py (because sensitive)
  • ES_INDEX_PREFIX in ENV.cms.settings_local.py
  • ES_DOMAIN in ENV.cms.settings_local.py
  • ES_HOSTS in cms.settings_custom.py (because shared across all hosts)

That gets a bit messy though, so keeping them all together in secrets is just easier.
Also, moving them might break the HAYSTACK_ config block due to loading order IIRC?

Copy link
Member Author

@wesleyboar wesleyboar Sep 6, 2024

Choose a reason for hiding this comment

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

Moving ES_… values would break ES if HAYSTACK… is in any of those files.

I've experienced such a load order problem before. I solved it via new section in settings.py after settings/secrets import.

I think the complexity of ES_ in many files is "not worth it". I feel safe to do so, but I defer to you.1

Footnotes

  1. Would you feel compelled to update such files on servers or in Core-Portal-Deployments?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I 100% agree that it is "not worth it" due to the second-order effects.

Would you feel compelled to update such files on servers or in Core-Portal-Deployments?

I don't think any ES_ values live anywhere except in the secrets.py files, so you have to update the file on the individual host (and add any changes to the Stache entry, naturally).

Copy link
Member Author

Choose a reason for hiding this comment

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

K. So, good to go this PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

my bad, yes - approved!

Copy link
Collaborator

@taoteg taoteg left a comment

Choose a reason for hiding this comment

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

LGTM!

@wesleyboar wesleyboar merged commit 1f693ef into main Sep 6, 2024
@wesleyboar wesleyboar deleted the feat/secrets.example.py branch September 6, 2024 21:38
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.

2 participants