Automatic recording rules #416
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewazores
force-pushed
the
auto-rules
branch
from
0d008f4 to
d20d8af
Compare
This was referenced Jun 14, 2021
andrewazores
changed the title
|
@vic-ma finally, this first pass at the automatic rules/triggers API addition is ready. You have already reviewed all of the contents of this PR piece by piece, anyway. There are still some outstanding TODOs/FIXMEs in the code that are to be addressed, but I will do those as separate PRs later on and see if I can get @jan-law or @hareetd to start contributing patches or reviews toward that effort as well. |
Credentials persist optionally (default: false)
* Use URL-safe Base64 encoder/decoder (#419) * Use URL-safe Base64 encoder/decoder Use encoder which produces, and decoder which expects, the URL-safe Base64 alphabet variant. The decoder is also tolerant of missing padding characters at the end, which the client will not send due to their illegality in the WebSocket Subprotocol header value * Update web-client to include base64url client-side changes * Update web-client to fix build (#424) * Add Quarkus JVM sample app (#418) * Throw exception if added rule collides by name * Throw exception if integer attrs negative Add unit tests for rules post handler * Refactor to extract string constants to enum Add tests for non-integer params * Correct spotbugs findings * Add some Rule attribute assertions and tests * Do all attribute validation in Rule * Correct exception message formatting * Catch IllegalArgumentException and rethrow as ApiException 400 * Minor refactor * Remove no-op CHANGED event handling * Include exception reason in API response message
* Update rulestest.sh to match current smoketest.sh * Add rule deletion test step * Refactor to not swallow IOExceptions * Track archiver tasks by sref/rule tuple * fixup! Update rulestest.sh to match current smoketest.sh * Deleting Rule definition stops archival tasks using that rule * Use direct API request paths, not injected handler method calls * Respond 404 when deleting non-existent rule * Add unit tests
* Mention all missing form fields on credentials POST * Quote missing parameter names in error message * Handle any blank form value, not only null * Apply spotless formatting * fixup! Handle any blank form value, not only null
* PeriodicArchivers notify RuleProcessor on archive failure RuleProcessor uses this to cancel the archiver's future runs * Add test that PeriodicArchiver notifies on failure * Apply spotless formatting * Minor refactor * Add test that RuleProcessor cancels failed archivers * Remove unnecessary assertions
* Set USE_AUTH * Don't delete rule definition
* Respond 404 when deleting nonexistent credentials * Retrieve Credentials from Manager on each PeriodicArchiver run If Credentials are deleted afer a rule is triggered/a PeriodicArchiver starts, the PeriodicArchiver should fail to archive from that point forward since its Credentials access has been revoked * Test credentials revocation during archiver run
|
Minor rebase for #521. Didn't really need much. |
vic-ma
reviewed
Jun 18, 2021
src/main/java/io/cryostat/configuration/CredentialsManager.java
Outdated
Show resolved
Hide resolved
src/main/java/io/cryostat/net/web/http/api/v2/RulesPostHandler.java
Outdated
Show resolved
Hide resolved
|
For creating a new rule, when using JSON format, negative numbers don't get caught. Is that something that should be addressed or added as a todo? Otherwise everything looks good. |
Another good catch. I think addressing it as a new minor bugfix issue is sufficient. Thanks for all the review attention on this. |
vic-ma
approved these changes
Jun 22, 2021
andrewazores
added a commit
to andrewazores/cryostat
that referenced
this pull request
Jun 24, 2021
* Rebase fixes * Rough out (very rough) automatic rules * Add very rudimentary configuration file credentials management * Cleanup * Add FIXMEs * Fix mistake, retrieve credentials by alias (for now) * Store credentials under subdirectory of conf dir * Move subdirectory creation into module * Add FIXME * Persist rules to disk Credentials persist optionally (default: false) * Add file open options, some FIXMEs, fix imports * Expect and serialize single credential per file * Expect and serialize single rule per file * Split rule processing logic out of RuleRegistry * Ensure RuleProcessor is created, injected, and enabled by DI root * Extract helper method * Implement periodic archival in rules * Don't retry pruning already-pruned recordings * Normalize URLs (URIs) * Log at lower level * Implement variable number of preserved archives * Add maxAge/maxSize to Rule * Encapsulate Rule fields, add Builder * Rules sanitize own names * Add PeriodicArchiverFactory PeriodicArchiver provided through DI for testability/decoupling * Cleaning * Cleaning * Cleaning * Avoid hardcoded request paths * Cleanup * Add TODO * Add handlers for POSTing Credentials * Add handler for DELETEing Credentials * Implement handlers for POSTing rules * Fixup * Automate add/remove demo app * Implement GET handler for Rules * Change credentials POST path * Include Location header on rules creation response * Handle Rules POST as multipart form or JSON * Rename RulesGetHandler to RuleGetHandler * Implement GET for all rules * Remove unused method * Implement handler to DELETE rules by name * Update pom.xml for configuration directories * Fix Spotbugs bugs * Rename credentials POST handler to reflect targetId path param * Add FIXME * Fix content-type bug, apply formatting * Fix periodic archiver recording name * Make test script host URL configurable Allows for testing while using -web start:dev + CORS setup, for example * Update for multiple demo apps * Remove unnecessary(?) handling for extra form type * Remove 'persist' form attribute * Apply spotless formatting * Revert "Remove unnecessary(?) handling for extra form type" This reverts commit 8e20fad. * Remove TODO * Replace rulestest.sh with AutoRulesIT.java * Add RuleRegistryTest * Add RuleProcessorTest * Apply auto_ prefix to recordings in one place * Remove unnecessary client-side timeout * Minor refactor * Add PeriodicArchiverTest * Extract shared client credentials header logic * Restore rulestest.sh for manual testing * Ensure rule definitions do not partially collide * Implement deleteRules(ServiceRef serviceRef) * Cancel periodic archivers if associated target disappears * Fix logger message formatting * Enable JMX auth on test container * Update FIXME * Correct core version * [autorules] Rules POST handler enhancements (cryostatio#423) * Use URL-safe Base64 encoder/decoder (cryostatio#419) * Use URL-safe Base64 encoder/decoder Use encoder which produces, and decoder which expects, the URL-safe Base64 alphabet variant. The decoder is also tolerant of missing padding characters at the end, which the client will not send due to their illegality in the WebSocket Subprotocol header value * Update web-client to include base64url client-side changes * Update web-client to fix build (cryostatio#424) * Add Quarkus JVM sample app (cryostatio#418) * Throw exception if added rule collides by name * Throw exception if integer attrs negative Add unit tests for rules post handler * Refactor to extract string constants to enum Add tests for non-integer params * Correct spotbugs findings * Add some Rule attribute assertions and tests * Do all attribute validation in Rule * Correct exception message formatting * Catch IllegalArgumentException and rethrow as ApiException 400 * Minor refactor * Remove no-op CHANGED event handling * Include exception reason in API response message * Rebase fixes * Bugfix: double async target discovery notifications * [autorules] Rules deletion enhancements (cryostatio#506) * Update rulestest.sh to match current smoketest.sh * Add rule deletion test step * Refactor to not swallow IOExceptions * Track archiver tasks by sref/rule tuple * fixup! Update rulestest.sh to match current smoketest.sh * Deleting Rule definition stops archival tasks using that rule * Use direct API request paths, not injected handler method calls * Respond 404 when deleting non-existent rule * Add unit tests * [autorules] Mention all missing form fields on credentials POST (cryostatio#511) * Mention all missing form fields on credentials POST * Quote missing parameter names in error message * Handle any blank form value, not only null * Apply spotless formatting * fixup! Handle any blank form value, not only null * [autorules] Cancel periodic archivers on failure (cryostatio#513) * PeriodicArchivers notify RuleProcessor on archive failure RuleProcessor uses this to cancel the archiver's future runs * Add test that PeriodicArchiver notifies on failure * Apply spotless formatting * Minor refactor * Add test that RuleProcessor cancels failed archivers * Remove unnecessary assertions * [autorules] rulestest.sh touchup (cryostatio#519) * Set USE_AUTH * Don't delete rule definition * [autorules] Credentials deletion enhancements (cryostatio#516) * Respond 404 when deleting nonexistent credentials * Retrieve Credentials from Manager on each PeriodicArchiver run If Credentials are deleted afer a rule is triggered/a PeriodicArchiver starts, the PeriodicArchiver should fail to archive from that point forward since its Credentials access has been revoked * Test credentials revocation during archiver run * JMXServiceURL/ServiceRef decoupling rebase fixes * Correct log messages * Correct exception message * Remove rulestest.sh
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #355
#356 (comment)
POST /api/v2/rulesWhen a rule with the same name as an existing rule is created, it will overwrite that rule. Maybe this should be prevented and the user should have to delete the existing rule first. Or there could be a note in the response that says that a rule was overwritten.
The numerical parameters don't give an error if they are set to negative numbers. They get set to zero when this happens, but I think ideally negative numbers shouldn't be allowed. This would also more in line with TargetRecordingsPostHandler, which rejects negatives.
When a required parameter is missing, the response is a 500 with "data":{"reason":null}, resulting from an NPE. It would be helpful if the code explicitly checked the given parameters, and let the user know if any were missing.
Some stuff, like setting the numerical parameters to letters, or setting name to !- or ////////, will cause exceptions, and the response doesn't specifically say that X parameter was set to an invalid value. But I guess this is sort of intentional, right? Since the alternative would be to check every parameter and write the error messages into the response, which would take a lot of extra code.
DELETE /api/v2/rules/:ruleNameDeleting a non-existent rule, as well as an existing one, gives a 200. It would be helpful if non-existent-rule deletions gave a 404 instead, to indicate to the user that the given rule was not found and that no deletion has occurred. Or they could make a note of it in the result, if a 404 is inappropriate here.
If a rule is already running, and the user would like to stop that rule (delete the active recording and stop the PeriodicArchiver), is there any way for them to do this besides killing the target container? Should deleting a rule maybe be able to do this? Or maybe this could be the job of a new PATCH STOP handler?
POST /api/v2/targets/:targetId/credentialsDELETE /api/v2/targets/:targetId/credentialsMaybe the response could specify if credentials were found and deleted, or if there were no credentials to be deleted? Not super important, but might be helpful.
When a rule for a target is running, and I delete the credentials for that target, the archiver is still able to archive recordings, despite the fact that JMX auth is required for this, and the saved JMX auth credentials have been deleted. I see that this is because in the code, the credentials are given to the PeriodicArchiver in the constructor and saved to the object, not retreived from the CredentialsManager each time it is used. This is more efficient, but I'm wondering if it makes sense from the user's point of view. If they deleted some credentials, would they expect for CJFR to not have a copy of those credentials stored anywhere, anymore? Or at least not be able to use it?
General
Deleting the recording created by an automatic rule causes an exception whenever the archiver tries to archive the recording (expectedly). I imagine there's no good way to protect that recording from being deleting, but maybe there is a way to stop the archiver if the recording does get deleted? It does seem unlikely that a user would accidentally delete the automatic recording, though.
When a target that has a rule running is killed, I get a few exceptions from the archiver trying to archive the recording, before the target is discovered as LOST and the archiver is stopped. This is because I set a low archival period for testing, but obviously could also happen with a longer period, if the archiver just so happens to archive in the in-between time. Also, when this happens, an archived recording will be pruned, but a new one will not be made to replace it, so the end result is one less archived recording than the amount specified by preservedArchives. I don't know how big of a deal any of this is.