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

Adding ability to override directory permissions for db backup #8888

Conversation

d00medman
Copy link
Contributor

What type of PR is this?
Feature

What does this PR do? Why is it needed?
Creates a command line argument which allows the db backup directory to override existing file permissions on the directory

Which issues(s) does this PR fix?

Fixes #8504

Other notes for review
First PR, tested locally and it worked; added test cases for the file handler; happy to add more if needed.

It does not feel great to make such a broad change for such a seemingly small gain. Main issue is that doing this requires either altering mkdirall (which I opted for) or spinning up a new mkdir method for backups with this ability.

@d00medman d00medman requested a review from a team as a code owner May 12, 2021 21:22
@CLAassistant
Copy link

CLAassistant commented May 12, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #8888 (86691fc) into develop (fbed11b) will decrease coverage by 0.54%.
The diff coverage is 39.13%.

@@             Coverage Diff             @@
##           develop    #8888      +/-   ##
===========================================
- Coverage    60.30%   59.75%   -0.55%     
===========================================
  Files          541      541              
  Lines        38603    38621      +18     
===========================================
- Hits         23280    23079     -201     
- Misses       11982    12216     +234     
+ Partials      3341     3326      -15     

@nisdas
Copy link
Member

nisdas commented May 14, 2021

This change would be pretty invasive to add in, as it adds in a new flag specifically just to add in a different set of permissions. In the original issue, there might be a better way of instantiating this rather than adding in an override everywhere.

@d00medman
Copy link
Contributor Author

@nisdas I was thinking that myself. I think the less invasive way of doing this would be to swap out mkdirall for a method used exclusively by backup (HandleBackupDir, which would look almost exactly like mkdirall, but with the additional chmod).

If a command line flag is not the way to go, how would we activate this? One option would be a url param for the backup endpoint; the other would be to always perform the permission override for the backup directory; my guess is the former would be preferable.

@d00medman
Copy link
Contributor Author

Ok, I've reduced the footprint by having the backup use a distinct method for file handling. Only outstanding question is "should this override to always be on for backup creation?" If this should be user-enabled, but not via a command line flag, the only other way I see this being viable is with a url parameter (something like /db/backup?override). Having the backup always override to the correct permissions would be cleaner, but the issue's OP requested a user enabled override. I'm fine either way, please advise which makes more sense.

@nisdas
Copy link
Member

nisdas commented May 26, 2021

@d00medman

Hey sorry for taking a long time to reply on this, I am bit skeptical on introducing a new flag for this. Given that this a minor user issue, having a flag specifically for overriding permissions seems excessive(since this is a backup). Maybe specifying this via a url parameter would be a better approach here as you have mentioned.

@stale
Copy link

stale bot commented Jun 3, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale There hasn't been any activity here in some time... label Jun 3, 2021
@rauljordan rauljordan removed the Stale There hasn't been any activity here in some time... label Jun 4, 2021
@rauljordan rauljordan added OK to merge Ready For Review A pull request ready for code review labels Jun 18, 2021
@prylabs-bulldozer prylabs-bulldozer bot merged commit ae7e276 into prysmaticlabs:develop Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Offer an option to ignore the file permissions check when calling the DB backup webhook
4 participants