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

Graceful, Automatic Expired Key Handling #121

Merged
merged 15 commits into from
Mar 14, 2022
Merged

Graceful, Automatic Expired Key Handling #121

merged 15 commits into from
Mar 14, 2022

Conversation

shitwolfymakes
Copy link
Collaborator

@shitwolfymakes shitwolfymakes commented Mar 9, 2022

Overview

Manually updating the key yet again this morning really bothered me, so I figured I'd take a look at how the key is stored by MakeMKV, and look around on the forums to see what others came up with. Turns out it's stored plaintext in $HOME/.MakeMKV/settings.conf and looks like this:

app_Key = "<beta_key>"

So scraping and storing the key is now handled by scripts/update_key.sh.

Actually leveraging the new script was another matter entirely, and required unsnarling the monolithic mess that has been in there since the beginning of the upstream project.

  1. The logic to run the shell commands and handle exceptions that arose existed in 5 different places (three of those instances were exact duplicates of ~18 lines). Those 3 duplicates have now been moved into the function run_makemkv(cmd), which takes a shell command. No more chunks of duplicated code making makemkv() hard to read.

  2. I fixed the error handilng. subprocess.run() doesn't actually throw the subprocess.CalledProcessError that the code tries to catch unless it looks like subprocess.run(..., check=True). So I standardized that as well, and now the exceptions properly catch all non-zero error codes (more on this later).

  3. I have completely discarded handling the exception thrown when MakeMKV errors out on an expired key once arm enters the command crafting logic. Instead, the first thing makemkv() does is call the new prep_mkv(job) function. It takes the current job and runs makemkvcon info job.devpath. Normally this doesn't do anything, and provides no use.

    But, after the expiration of the key (regardless of what time of day MakeMKV's servers stop recognizing the previous month's key), it errors out with error code 253. The neat part is that doing this here allows us to handle key expiration in one place, well before entry into sensitive logic. So now we've gone from 5 locations needing handling for key expiration to 1.

    Once error code 253 is caught it calls the new update_key() function which runs scripts/update_key.sh and throws a RuntimeError that kills the job if it encounters an error during execution. Once update_key() has successfully returned, then the exception handling tries running makemkvcon info job.devpath again. For some weird reason, a successful run of that command has a return code of 10, so the handling throws an exception and kills the job on any non-zero return code that is not 10.

    Once the second run of makemkvcon info job.devpath is completed successfully, the exception handling returns without propagating the exception. Logging documents several steps of this process, and the rip continues as if nothing happened.

  4. Exceptions. By this point I had half a dozen instances or the same three lines of code that made up the entirety of the except body of the except block. So I moved that logic into a custom exception: MakeMkvRuntimeException. It takes the subprocess.CalledProcessError variable and uses that to craft a useful message. So everything looking like this:

    except subprocess.CalledProcessError as error_name:
        err = f'{MAKE_MKV_FAILED} {error_name.returncode} ({error_name.output})'
        logging.error(err)
        raise RuntimeError
    

    Becomes this:

    except subprocess.CalledProcessError as error_name:
        raise MakeMkvRuntimeError(error_name)
    

Overall, this makes the code cleaner, more concise, and significantly easier to read. In particular, makemkv() has gone from an enormous 161 SLoC to 101 SLoC. Additionally, all of the new functions, exception class, and docstrings for them all only increases the total size of ripper/makemkv.py by 2 SLoC!

Changelog

  • Added new Exception to ripper/makekmv.py: MakeMkvRuntimeError
  • Added new functions to ripper/makekmv.py: prep_makemkv(), update_key(), run_make()
  • Streamlined logic in makemkv()
  • Exception handling is now actually occurring
  • Added graceful handling of expired beta keys, reduced the number of locations where this case needs to be tested from 5 to 1
  • Added scripts/update_key.sh to scrape and store the latest beta key
  • Updated all install scripts to set permissions for update_key.sh during install
  • Added docstring documentation to all new functions and classes

Notes

  • I've tested this with DVDs, but I'll post the results of my Blu-ray test in the morning
  • To test the graceful key update, delete $HOME/.MakeMKV/settings.conf (for me it was /home/arm/.MakeMKV/settings.conf). All of the logging for this functionality is INFO level
  • This PR obviates the need for Add UI field for updating MakeMKV key #113, and solves the issue for [Docker] How do I update the MakeMKV key? #105. Merging this PR would allow for closing both
  • This PR obviates the need to create GitHub Actions to build new containers each month, even if no code has changed

Next Steps

  • Personally, I think when paired with Ubuntu Script Updates #116 this creates a great point to go ahead and release v2.6.0. I am planning to do so in my own fork once this is merged
  • Each of the manual installation guides in the wiki need to be updated to have sudo chmod +x /opt/arm/scripts/update_key.sh following the sudo chmod +x /opt/arm/scripts/arm_wrapper.sh

Logs

Attached is the log from my dvd test run:
POINT_BREAK_164679631972.log
KIKIS_DELIVERY_SERVICE_BD_164694558691.log

@shitwolfymakes
Copy link
Collaborator Author

Added log for successful Blu-ray test

@shitwolfymakes
Copy link
Collaborator Author

Any ETA on a merge or response? Multiple tests against Blu-ray and DVD disks have been successful

@1337-server
Copy link
Owner

This means that https://github.com/1337-server/automatic-ripping-machine/blob/v2_devel/arm/ripper/getkeys.py should probably be in line for removal.

Could you bump the VERSION number up ?

Thanks, and great work!

@codeclimate
Copy link

codeclimate bot commented Mar 14, 2022

Code Climate has analyzed commit bcd3630 and detected 0 issues on this pull request.

View more on Code Climate.

@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

@shitwolfymakes
Copy link
Collaborator Author

shitwolfymakes commented Mar 14, 2022

getkeys.py is actually for the UHD key hashes, not the MakeMKV Beta Key. I have reverted that removal, and bumped the version number as requested. It's good to merge.

@1337-server 1337-server merged commit 81cd58c into 1337-server:v2_devel Mar 14, 2022
@shitwolfymakes
Copy link
Collaborator Author

shitwolfymakes commented Mar 14, 2022

Lemme know when v2.6.0 has been tagged and released

Edit: merge #123 before tagging and releasing v2.6.0

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