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

🔥 (nordigen) removing env var/config file support #235

Merged
merged 3 commits into from
Jul 29, 2023

Conversation

MatissJanis
Copy link
Member

Removing env var/config file support for Nordigen secrets. These have been migrated to the new secrets db table a while ago. We can now clean up the legacy implementation for these secrets.

@trafico-bot trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review labels Jul 29, 2023
@MatissJanis MatissJanis merged commit ce8d53a into master Jul 29, 2023
8 checks passed
@MatissJanis MatissJanis deleted the matiss/nordigen-drop-envvar-support branch July 29, 2023 18:59
@trafico-bot trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved Pull Request has been approved and can be merged labels Jul 29, 2023
@samip5
Copy link

samip5 commented Aug 3, 2023

This is a huge inconvenience for us using Kubernetes as previously those could be fetched from Kubernetes secret and imported to the environment. :(

@MatissJanis
Copy link
Member Author

This is a huge inconvenience for us using Kubernetes as previously those could be fetched from Kubernetes secret and imported to the environment. :(

Is there a reason why storing the secrets in the database would not be possible in k8s? It should actually be much simpler to manage this secret that way for you as you can set it via the UI.

@samip5
Copy link

samip5 commented Aug 4, 2023

This is a huge inconvenience for us using Kubernetes as previously those could be fetched from Kubernetes secret and imported to the environment. :(

Is there a reason why storing the secrets in the database would not be possible in k8s? It should actually be much simpler to manage this secret that way for you as you can set it via the UI.

It's possible to store them in the DB yes, but that's manual and not very GitOps friendly practice (I might have them already in a secret somewhere in my cluster). We would have to rely on an external way to insert them into DB on bootstrap of the app as one needs to wait for the app to have fully initialized (so not possible to use an init-container) before one can script the DB insert from secret if it doesn't exist. I hope it would be reconsidered.

@MatissJanis
Copy link
Member Author

It's possible to store them in the DB yes, but that's manual and not very GitOps friendly practice (I might have them already in a secret somewhere in my cluster). We would have to rely on an external way to insert them into DB on bootstrap of the app as one needs to wait for the app to have fully initialized (so not possible to use an init-container) before one can script the DB insert from secret if it doesn't exist. I hope it would be reconsidered.

Are you auto-scaling your instances? Are you spinning up a new instance every time you visit the site? I'm trying to understand under what circumstances you would need to automate the set-up. Maybe you could elaborate a bit more on your set-up?

The first time you use the app we ask you to create a new budget (or import). Fundamentally the Nordigen secrets act in the same way - the first time you use this feature - you need to set it up (i.e. submit the credentials via UI). But every subsequent usage does not require providing the credentials anymore. They are persisted in the database alongside your budget.

@samip5
Copy link

samip5 commented Aug 5, 2023

Are you auto-scaling your instances? Are you spinning up a new instance every time you visit the site? I'm trying to understand under what circumstances you would need to automate the set-up. Maybe you could elaborate a bit more on your set-up?

I'm using Renovate to keep things up-to-date which means that could happen anytime an update is pushed so it would probably need to be checked every time that does.

Other possibility is that it just needs to be done once when deploying the app which the DB insert/UI click populate is not very GitOps friendly as in you cannot define it in Git anymore so that portion is "hidden" away of the deployment if you're looking for anyone else's deployment on Kubernetes of the same via our k8s-at-home search.

The first time you use the app we ask you to create a new budget (or import). Fundamentally the Nordigen secrets act in the same way - the first time you use this feature - you need to set it up (i.e. submit the credentials via UI). But every subsequent usage does not require providing the credentials anymore. They are persisted in the database alongside your budget.

It doesn't really act in the same way before this PR though, as even though one needs to go through the PSD2/Open Banking authorisation, it's not the same due to how the credentials are already set up for Nordigen prior to going to UI which is not possible anymore.

What was the original reason for wanting to move it to the DB instead of supporting it via the environment?

@MatissJanis
Copy link
Member Author

What was the original reason for wanting to move it to the DB instead of supporting it via the environment?

We want Actual to be usable by everyone. This includes non-technical folk. As such - we cannot ask them to configure env variables/configuration files. All the configuration has to be exposed via the UI. The env/config file configuration was only a temporary measure while the feature was experimental.

Honestly, I'm still confused why configuring the credentials via UI is such a hot-topic here. You only need to do it one time. And then they are persisted forever (or until you manually delete them).

@samip5
Copy link

samip5 commented Aug 6, 2023

Honestly, I'm still confused why configuring the credentials via UI is such a hot-topic here. You only need to do it one time. And then they are persisted forever (or until you manually delete them).

The reason for it being a hot-topic is that it's very ClickOps aka having to click though UI's to accomplish things are more and more not looked at like good UX at least that's my take on it.
I understand that having to do all that is probably to simplify it for non tech-savy people but at some UX cost for tech-savy people.

(Similar things can be said about Home Assistant being very ClickOps and more and more things cannot be done via yaml files..)

@kashalls
Copy link

What was the original reason for wanting to move it to the DB instead of supporting it via the environment?

We want Actual to be usable by everyone. This includes non-technical folk. As such - we cannot ask them to configure env variables/configuration files. All the configuration has to be exposed via the UI. The env/config file configuration was only a temporary measure while the feature was experimental.

Honestly, I'm still confused why configuring the credentials via UI is such a hot-topic here. You only need to do it one time. And then they are persisted forever (or until you manually delete them).

Its more of a repeatable thing? If something happens to the database during a migration or whatnot, you'd have to configure everything again.

Why can't you keep support for secrets/env? Its easy enough to choose to use the database entry over env. With env, I am able to securely store my secrets and deploy them dynamically to anywhere I need. If I need to change one, its a click away and its done and updated. I don't have to login through the app and jump through menus to find the setting I need to change.

MMichotte pushed a commit to MMichotte/actual-server that referenced this pull request Sep 9, 2024
Removing env var/config file support for Nordigen secrets. These have
been migrated to the new `secrets` db table a while ago. We can now
clean up the legacy implementation for these secrets.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Merged Pull Request has been merged successfully
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants