-
Notifications
You must be signed in to change notification settings - Fork 9
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
Refactor: model secret fields #1874
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
b235a42
to
22db7a6
Compare
Azure KeyVault currently enforces the following rules: * The value must be between 1 and 127 characters long. * Secret names can only contain alphanumeric characters and dashes. Read more about Azure KeyVault naming rules: https://learn.microsoft.com/en-us/azure/azure-resource-manager/management/resource-name-rules#microsoftkeyvault Read more about Django validators: https://docs.djangoproject.com/en/5.0/ref/validators/#module-django.core.validators
* update definition to use new field * update migrations * remove env var from terraform definitions * move default value to .env.sample
* update definition to use new field * update migrations * remove env vars from terraform definitions * move default value to .env.sample
PemData favors the secret, but fallback to a remote URL this is to allow for simpler turn-key local development * update definition to use new field * update migrations * remove env vars from terraform definitions * move default keys to .env.sample
22db7a6
to
3721d8c
Compare
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.
This looks good to me.
Since we'll be connecting to the Key Vault every time we need a secret, I looked up the Key Vault service limits: https://learn.microsoft.com/en-us/azure/key-vault/general/service-limits#secrets-managed-storage-account-keys-and-vault-transactions
I think it should be fine for the scale at which we currently operate.
Good point! I hadn't thought of that.
Seems like WAY more than enough for now 😅 However we could look at adding more of the |
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.
- Ran locally sucessfully, going thru reading dev secrets
Defines a new field type
SecretNameField
that manages storage of the name of the secret for models that need it.Defines a custom
RegexValidator
to ensure secret names are valid according to Azure's rules.Refactors secret fields on models according to the field mapping spreadsheet to store the name of their corresponding secret using the new field type. Secret names are defined in new fields on each model named with the postfix
_secret_name
.Models then use the helper defined in #1859 to get the value on-demand via a
@property
using the original (sans-postfix) field name. This means calling code doesn't have to change.Closes #1847
Models to refactor
AuthProvider
EligiblityVerifier
PemData
Since the secret fields here are references toPaymentProcessor
PemData
, and that model has already been refactored, this model does not need to be further refactored.Reviewing this PR
It should be possible to set up your local environment to read secrets from
dev
:DJANGO_ALLOWED_HOSTS
to includedev-benefits.calitp.org,localhost
AUTH_PROVIDER_AUTHORITY
bin/init.sh
to recreate the databaseF5
to launch the app locally