Automatic Rules PeriodicArchiver should not use HTTP requests to self

[andrewazores]

        // FIXME using an HTTP request to localhost here works well enough, but is needlessly
        // complex. The API handler targeted here should be refactored to extract the logic that
        // creates the recording from the logic that simply figures out the recording parameters
        // from the POST form, path param, and headers. Then the handler should consume the API
        // exposed by this refactored chunk, and this refactored chunk can also be consumed here
        // rather than firing HTTP requests to ourselves

[andrewazores]

The RecordingCreationHelper from #538 could be useful.

[jan-law]

Will start on this after #538 is merged

[jan-law]

Where should I put the extracted logic to save or delete recordings?
I'd like to reuse private Optional<IRecordingDescriptor> getDescriptorByName from RecordingCreationHelper. Class names like RecordingDeletionHelper or RecordingSaveHelper would be more appropriate for helper functions that save or delete recordings though.

[andrewazores]

You could probably put it into RecordingCreationHelper and just rename that class to something a little more general.

[jan-law]

Why does cryostat need to supply credentials when making an http request to itself? Should the refactored PeriodicArchiver validate stored credentials before archiving/pruning recordings?

PeriodicArchiver.java, line 133:

this.webClient
   .patch(path)
   .putHeaders(headersFactory.apply(credentialsManager.getCredentials(serviceRef)))
...

[andrewazores]

Why does cryostat need to supply credentials when making an http request to itself?

In this case, Cryostat is acting as a web client to itself, so it has to go through all the usual communications protocols that any client does. If you chase through what is actually injected here it comes from the RulesModule, and this function attaches JMX Authorization credentials as a header if needed/if provided. So the PeriodicArchiver checks if the CredentialsManager has any stored credentials for the given target, and if it does, those credentials are included along with the request to archive the recording - which entails opening a connection to the target JVM and copying the JFR data from it. If the target requires credentials and none are provided with the API request to Cryostat, then Cryostat will try to connect to the target without credentials, and the target will reject the connection attempt.

We could modify the TargetConnectionManager to check with the CredentialsManager if there are any stored credentials as well, so that clients aren't required to supply them if they have already been stored, but currently that isn't done. Stored credentials are only used by the automatic rules system so that Cryostat can establish connections to targets at arbitrary times in the future when there is no user supplying credentials at request time.

Should the refactored PeriodicArchiver validate stored credentials before archiving/pruning recordings?

It won't need to "validate" them, but it will need to check if the CredentialsManager has any and ensure that they are passed to the TargetConnectionManager if so. This would be required when archiving since, as above, that means a connection must be opened to the target JVM, but for pruning archives there is no target connection and so the credentials are not actually needed.

[jan-law]

Looking at the unit tests, how would one trigger a failure without using a failed http response? I tried spying on PeriodicArchiver and the test results show no exceptions were thrown:

@Test
    void testNotifyOnFailure() throws Exception {

        PeriodicArchiver spy = Mockito.spy(archiver);
        Mockito.doThrow(new InterruptedException()).when(spy).performArchival();

        MatcherAssert.assertThat(failureCounter.intValue(), Matchers.equalTo(0));
        archiver.run();
        MatcherAssert.assertThat(failureCounter.intValue(), Matchers.equalTo(1));
    }

Output:

testNotifyOnFailure
java.lang.AssertionError: 
Expected: <1>
     but: was <0>

[andrewazores]

I think what's happening there is that you're created an object called spy, which wraps around the archiver instance, and has your customized behaviour (throw an exception when performArchival is called), but this doesn't actually change the behaviour of archiver - only spy.

If the archiver is modified so that run/performArchival no longer use webClient but instead use targetConnectionManager instead, then you would modify the targetConnectionManager spy/mock so that it throws an exception when some method that run/performArchival calls is called.