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

Add symlink to /home/indy/.indy_client for backwards compatibility #2443

Merged

Conversation

esune
Copy link
Member

@esune esune commented Aug 24, 2023

This should resolve an issue happening when migrating an indy-based agent to an askar-only image: the home directories will change, however the database will contain references to previously created artifacts (specifically tails files in the case that triggered this fix) that will not be resolvable anymore.

Creating a symlink for /home/indy/.indy-client/ pointing to /home/aries/.indy_client should resolve issues with path resolution without needing to update path references stored in the database.

We could potentially create a symlink between /home/indy and /home/aries, but I don't like the idea of symlinking home directories.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Aug 24, 2023

I've built ghcr.io/hyperledger/aries-cloudagent-python:py3.9-0.10.0-dev0 off this commit for testing.

@WadeBarnes
Copy link
Contributor

@swcurran, @dbluhm, We'll need this to be included in 0.10.0, but we need to ensure the permissions on the folders work before merging this PR.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 24, 2023

This doesn't feel like the right solution to me; this will mean that data will continue to be stored in the /home/indy directory, right? At what point do we actually make the switch to not having references back to indy-isms? What would be different about the solution at that point and now?

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Aug 24, 2023

Data will (can/should) actually be stored in /home/aries, but the symlink will allow it to be accessed via /home/indy for backward compatibility for agents that have been migrated from indy to askar storage and to the askar only images.

@WadeBarnes
Copy link
Contributor

WadeBarnes commented Aug 24, 2023

The scenario we ran into was I migrated an agent from an indy-sdk based image to the askar only image. In doing so the PVC containing the tails files was mounted to /home/aries/.indy_client/tails rather than /home/indy/.indy_client/tails. The agent's secure storage still contained references to /home/indy/.indy_client/tails and therefore the agent could not find the files it was looking for. The addition of the symlink allows the tails files to be stored in /home/aries/.indy_client/tails moving forward, while still allowing them to be found using the existing references to /home/indy/.indy_client/tails.

@WadeBarnes
Copy link
Contributor

These sorts of issues have been brought about due to the fact we've been using /home directories for application data rather than using application folders for data and config. Something we should consider changing in the future.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 24, 2023

Ah I see. I had the source and target of the symlink swapped around. Thanks for explaining. This seems like a "necessary evil" unless we wanted to do an upgrade script that rewrites locations.

@dbluhm
Copy link
Contributor

dbluhm commented Aug 24, 2023

As an aside about those tails locations, with the AnonCreds Rust changes, I believe this will no longer be an issue because the only time an actual tails file will need to exist will be when uploading it to the tails server. So a path won't be stored in the DB anymore.

@WadeBarnes
Copy link
Contributor

Please hold merging this PR until we've completed testing.

Preliminary results:

I've deployed an instance of the dev image and had a look at the symlink and folder permissions. On the surface everything looks good. I've asked the team to rerun the tests to confirm the issue they encountered has been fixed.

@swcurran
Copy link
Contributor

Please let me know if after we test/merge this we need to do an RC2, or if it is safe enough to go to the final 0.10.0.

@esune
Copy link
Member Author

esune commented Aug 24, 2023

Please let me know if after we test/merge this we need to do an RC2, or if it is safe enough to go to the final 0.10.0.

I feel like this is innocuous enough to go straight into an official release, plus we're testing it on a custom image @WadeBarnes built before we merge it.

@swcurran
Copy link
Contributor

With the #2409 fallout, I think we will need an RC2 anyway, unfortunately.

@WadeBarnes
Copy link
Contributor

With the #2409 fallout, I think we will need an RC2 anyway, unfortunately.

Agreed

@swcurran swcurran enabled auto-merge August 25, 2023 16:37
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@swcurran swcurran merged commit 4e8fc1d into openwallet-foundation:main Aug 25, 2023
@esune esune deleted the fix/resolve-old-tails-files branch August 25, 2023 23:11
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.

4 participants