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

fix(archiveupload): improve performance of archive uploads and validation #742

Merged
merged 31 commits into from
Nov 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
6f8f5c1
write archive uploads into the existing archives directory
andrewazores Oct 27, 2021
854dcb5
perform faster and lighter validation of uploaded binaries
andrewazores Oct 27, 2021
3225edd
implement binary file uploading beta recordings handler
andrewazores Oct 28, 2021
43cf9df
document beta api handlers
andrewazores Oct 28, 2021
bba3276
pause/resume request to ensure all buffer data is correctly handled
andrewazores Oct 28, 2021
1532b29
Set content-type in example
andrewazores Oct 28, 2021
74f2289
Allow Content-Type headers in CORS
andrewazores Oct 28, 2021
ffd88ec
refactor to use more vertx async io
andrewazores Oct 28, 2021
fffab13
handle read timeouts
andrewazores Oct 28, 2021
a2c2d0e
use ctx.fail rather than throwing, to ensure cleanup endhandler is in…
andrewazores Oct 29, 2021
00623cb
fix(recordingupload): delete badly named uploaded temp files
andrewazores Oct 29, 2021
40c0f61
fix(templatesupload): delete badly named uploaded files
andrewazores Oct 29, 2021
321d422
fix(bodyhandlers): do not handle file uploads where not expected
andrewazores Oct 29, 2021
3cd1f37
create and open file in one step rather than two
andrewazores Oct 29, 2021
71e18e2
improve stream-to-file handling
andrewazores Oct 29, 2021
31a4914
ensure file-uploads dir is created
andrewazores Oct 30, 2021
2184af9
Revert "improve stream-to-file handling"
andrewazores Nov 1, 2021
63ef381
replace System.nanoTime with Clock.getMonotonicTime
andrewazores Nov 1, 2021
83a3dec
replace makeAsyncResult methods with FutureFactory
andrewazores Nov 1, 2021
5370ce9
fixup! ensure file-uploads dir is created
andrewazores Nov 1, 2021
bf0bd0a
reorder request header and param validations
andrewazores Nov 1, 2021
4282c96
Update doc with required headers
andrewazores Nov 1, 2021
2ffd56d
apply spotless formatting
andrewazores Nov 1, 2021
6c52c80
correct broken unit tests
andrewazores Nov 1, 2021
ace65d6
remove debug logging
andrewazores Nov 1, 2021
d5fa677
delete invalid uploaded files
andrewazores Nov 1, 2021
ddb6ffa
Upgrade vertx to 3.9.9
andrewazores Nov 1, 2021
2b23c17
Remove POST /api/beta/recordings handler
andrewazores Nov 2, 2021
d96dc47
apply spotless formatting
andrewazores Nov 2, 2021
fc034f8
remove unneeded CORS change after removal of Beta API handler
andrewazores Nov 2, 2021
543d51f
doc(httpapi): remove reference to Beta POST /api/beta/recordings/:rec…
andrewazores Nov 3, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ build/
target/
bin/

archive/
conf/
truststore/
clientlib/
Expand Down
27 changes: 27 additions & 0 deletions HTTP_API.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

* [V1](#V1-API)
* [V2](#V2-API)
* [Beta](#Beta-API)

## V1 API

Expand Down Expand Up @@ -1696,3 +1697,29 @@ The handler-specific descriptions below describe how each handler populates the
$ curl -F cert=@vertx-fib-demo.cer localhost:8181/api/v2/certificates
{"meta":{"type":"text/plain","status":"OK"},"data":{"result":"/truststore/vertx-fib-demo.cer"}}
```

## Beta API

### Quick Reference

| What you want to do | Which handler you should use |
| ------------------------------------------------------------------------- | --------------------------------------------------------------------------------|
| **Miscellaneous** | |
| View targets in overall deployment environment | [`DiscoveryGetHandler`](#DiscoveryGetHandler) |

### Miscellaneous

* #### `DiscoveryGetHandler`

###### synopsis
Queries the platform client(s) for the discoverable targets and constructs a
hierarchical tree view of the full deployment environment with targets
belonging to ex. Pods, belonging to Deployments, etc.

###### request
`GET /api/beta/discovery`

###### response
`200` - The result is the path of the saved file in the server's storage.

`401` - The user does not have sufficient permissions.
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
<org.apache.commons.io.version>2.8.0</org.apache.commons.io.version>
<org.apache.httpcomponents.version>4.5.13</org.apache.httpcomponents.version>
<io.fabric8.client.version>5.4.1</io.fabric8.client.version>
<io.vertx.web.version>3.9.7</io.vertx.web.version>
<io.vertx.web.version>3.9.9</io.vertx.web.version>
<org.slf4j.version>1.7.30</org.slf4j.version>
<com.google.code.gson.version>2.8.6</com.google.code.gson.version>
<com.github.ben-manes.caffeine.version>3.0.1</com.github.ben-manes.caffeine.version>
Expand Down
6 changes: 5 additions & 1 deletion run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ if [ -z "$KEYSTORE_PATH" ] && [ -f "$(dirname $0)/certs/cryostat-keystore.p12" ]
KEYSTORE_PASS="$(cat $(dirname $0)/certs/keystore.pass)"
fi

if [ ! -d "$(dirname $0)/archive" ]; then
mkdir "$(dirname $0)/archive"
fi

if [ ! -d "$(dirname $0)/conf" ]; then
mkdir "$(dirname $0)/conf"
fi
Expand Down Expand Up @@ -79,8 +83,8 @@ fi
podman run \
--pod cryostat \
--memory 512M \
--mount type=tmpfs,target=/opt/cryostat.d/recordings.d \
--mount type=tmpfs,target=/opt/cryostat.d/templates.d \
--mount type=bind,source="$(dirname $0)/archive",destination=/opt/cryostat.d/recordings.d,relabel=shared,bind-propagation=shared \
--mount type=bind,source="$(dirname $0)/conf",destination=/opt/cryostat.d/conf.d,relabel=shared,bind-propagation=shared \
--mount type=bind,source="$(dirname $0)/truststore",destination=/truststore,relabel=shared,bind-propagation=shared \
--mount type=bind,source="$(dirname $0)/certs",destination=/certs,relabel=shared,bind-propagation=shared \
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,16 @@
*/
package io.cryostat.net.web.http.api.v1;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.Set;

import javax.inject.Inject;
import javax.inject.Named;

import io.cryostat.MainModule;
import io.cryostat.core.sys.FileSystem;
import io.cryostat.net.AuthManager;
import io.cryostat.net.security.ResourceAction;
import io.cryostat.net.web.http.AbstractAuthenticatedRequestHandler;
Expand All @@ -55,9 +61,21 @@ class RecordingsPostBodyHandler extends AbstractAuthenticatedRequestHandler {
private final BodyHandler bodyHandler;

@Inject
RecordingsPostBodyHandler(AuthManager auth) {
RecordingsPostBodyHandler(
AuthManager auth,
@Named(MainModule.RECORDINGS_PATH) Path recordingsPath,
FileSystem fs) {
super(auth);
this.bodyHandler = BodyHandler.create(true);
Path fileUploads = recordingsPath.resolve("file-uploads");
this.bodyHandler = BodyHandler.create(fileUploads.toAbsolutePath().toString());
try {
// FIXME put this somewhere more appropriate
if (!fs.isDirectory(fileUploads)) {
Files.createDirectories(fileUploads);
}
} catch (IOException ioe) {
throw new RuntimeException(ioe);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@
*/
package io.cryostat.net.web.http.api.v1;

import java.io.File;
import java.io.BufferedInputStream;
import java.io.FileInputStream;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
Expand All @@ -51,7 +52,8 @@
import javax.inject.Named;

import org.openjdk.jmc.flightrecorder.CouldNotLoadRecordingException;
import org.openjdk.jmc.flightrecorder.JfrLoaderToolkit;
import org.openjdk.jmc.flightrecorder.internal.FlightRecordingLoader;
import org.openjdk.jmc.flightrecorder.internal.InvalidJfrFileException;

import io.cryostat.MainModule;
import io.cryostat.core.log.Logger;
Expand Down Expand Up @@ -154,7 +156,9 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception {
// ignore unrecognized form fields
if ("recording".equals(fu.name())) {
upload = fu;
break;
} else {
Path p = savedRecordingsPath.resolve("file-uploads").resolve(fu.uploadedFileName());
vertx.fileSystem().deleteBlocking(p.toString());
}
}

Expand Down Expand Up @@ -207,8 +211,6 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception {
HttpMimeType.JSON.mime())
.end(gson.toJson(Map.of("name", res2.result())));

logger.info("Recording saved as {}", res2.result());

notificationFactory
.createBuilder()
.metaCategory(NOTIFICATION_CATEGORY)
Expand All @@ -223,9 +225,14 @@ private void validateRecording(String recordingFile, Handler<AsyncResult<Void>>
vertx.executeBlocking(
event -> {
try {
JfrLoaderToolkit.loadEvents(
new File(recordingFile)); // try loading events to see if
// it's a valid file
// try loading chunk info to see if it's a valid file
try (var is = new BufferedInputStream(new FileInputStream(recordingFile))) {
var supplier = FlightRecordingLoader.createChunkSupplier(is);
var chunks = FlightRecordingLoader.readChunkInfo(supplier);
if (chunks.size() < 1) {
throw new InvalidJfrFileException();
}
}
event.complete();
} catch (CouldNotLoadRecordingException | IOException e) {
event.fail(e);
Expand All @@ -241,6 +248,7 @@ private void validateRecording(String recordingFile, Handler<AsyncResult<Void>>
} else {
t = res.cause();
}
vertx.fileSystem().deleteBlocking(recordingFile);

handler.handle(makeFailedAsyncResult(t));
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

class TargetRecordingOptionsPatchBodyHandler extends AbstractAuthenticatedRequestHandler {

static final BodyHandler BODY_HANDLER = BodyHandler.create(true);
static final BodyHandler BODY_HANDLER = BodyHandler.create(true).setHandleFileUploads(false);

@Inject
TargetRecordingOptionsPatchBodyHandler(AuthManager auth) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

class TargetRecordingPatchBodyHandler extends AbstractAuthenticatedRequestHandler {

static final BodyHandler BODY_HANDLER = BodyHandler.create(true);
static final BodyHandler BODY_HANDLER = BodyHandler.create(true).setHandleFileUploads(false);

@Inject
TargetRecordingPatchBodyHandler(AuthManager auth) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class TargetRecordingsPostBodyHandler extends AbstractAuthenticatedRequestHandle
@Inject
TargetRecordingsPostBodyHandler(AuthManager auth) {
super(auth);
this.bodyHandler = BodyHandler.create(true);
this.bodyHandler = BodyHandler.create(true).setHandleFileUploads(false);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,16 @@ public boolean isAsync() {

@Override
public void handleAuthenticated(RoutingContext ctx) throws Exception {
boolean handledUpload = false;
try {
for (FileUpload u : ctx.fileUploads()) {
Path path = fs.pathOf(u.uploadedFileName());
if (!"template".equals(u.name())) {
fs.deleteIfExists(path);
continue;
}
handledUpload = true;
try (InputStream is = fs.newInputStream(path)) {
if (!"template".equals(u.name())) {
throw new HttpStatusException(
400,
String.format(
"Received unexpected file upload named {}", u.name()));
}
notificationFactory
.createBuilder()
.metaCategory(NOTIFICATION_CATEGORY)
Expand All @@ -138,6 +138,9 @@ public void handleAuthenticated(RoutingContext ctx) throws Exception {
} catch (InvalidXmlException | InvalidEventTemplateException e) {
throw new HttpStatusException(400, e.getMessage(), e);
}
if (!handledUpload) {
throw new HttpStatusException(400, "No template submission");
}
ctx.response().end();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

class RulesPostBodyHandler extends AbstractAuthenticatedRequestHandler {

static final BodyHandler BODY_HANDLER = BodyHandler.create(true);
static final BodyHandler BODY_HANDLER = BodyHandler.create(true).setHandleFileUploads(false);

@Inject
RulesPostBodyHandler(AuthManager auth) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

class TargetCredentialsPostBodyHandler extends AbstractAuthenticatedRequestHandler {

static final BodyHandler BODY_HANDLER = BodyHandler.create(true);
static final BodyHandler BODY_HANDLER = BodyHandler.create(true).setHandleFileUploads(false);

@Inject
TargetCredentialsPostBodyHandler(AuthManager auth) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@

class TargetsPostBodyHandler extends AbstractAuthenticatedRequestHandler {

static final BodyHandler BODY_HANDLER = BodyHandler.create(true);
static final BodyHandler BODY_HANDLER = BodyHandler.create(true).setHandleFileUploads(false);

@Inject
TargetsPostBodyHandler(AuthManager auth) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ void shouldHaveExpectedRequiredPermissions() {
void shouldThrowIfWriteFails() throws Exception {
RoutingContext ctx = Mockito.mock(RoutingContext.class);
FileUpload upload = Mockito.mock(FileUpload.class);
Mockito.when(upload.name()).thenReturn("template");
Mockito.when(ctx.fileUploads()).thenReturn(Set.of(upload));
Mockito.when(upload.uploadedFileName()).thenReturn("/file-uploads/abcd-1234");

Expand Down Expand Up @@ -170,9 +171,6 @@ void shouldThrowIfTemplateUploadNameInvalid() throws Exception {
Path uploadPath = Mockito.mock(Path.class);
Mockito.when(fs.pathOf("/file-uploads/abcd-1234")).thenReturn(uploadPath);

InputStream stream = Mockito.mock(InputStream.class);
Mockito.when(fs.newInputStream(Mockito.any())).thenReturn(stream);

HttpStatusException ex =
Assertions.assertThrows(
HttpStatusException.class, () -> handler.handleAuthenticated(ctx));
Expand Down