Skip to content

Commit 28dd8e1

Browse files
nicktindallnicktindall
authored
Make GCS HttpHandler more compliant (elastic#126007)
- Fixed bug where 416 was being erroneously returned for zero-length blobs even with no Range header - Fixed bug where partial upload wouldn't be completed if the last PUT included no data - Return 206 (partial content) status when a Range header is specified - Return an ETag on object get - BlobReadChannel uses this to ensure we fail when the blob is updated between successive chunks being fetched) - The 416 on zero-length blobs was one of(?) the causes of elastic#125668
1 parent bb76210 commit 28dd8e1

File tree

4 files changed

+134
-56
lines changed

4 files changed

+134
-56
lines changed

‎modules/repository-gcs/src/internalClusterTest/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobStoreRepositoryTests.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import org.elasticsearch.common.blobstore.BlobPath;
3131
import org.elasticsearch.common.blobstore.BlobStore;
3232
import org.elasticsearch.common.bytes.BytesArray;
33+
import org.elasticsearch.common.bytes.BytesReference;
3334
import org.elasticsearch.common.collect.Iterators;
3435
import org.elasticsearch.common.io.Streams;
3536
import org.elasticsearch.common.regex.Regex;
@@ -59,6 +60,7 @@
5960
import java.util.regex.Matcher;
6061
import java.util.regex.Pattern;
6162

63+
import static org.elasticsearch.common.io.Streams.readFully;
6264
import static org.elasticsearch.repositories.blobstore.BlobStoreTestUtil.randomPurpose;
6365
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.CREDENTIALS_FILE_SETTING;
6466
import static org.elasticsearch.repositories.gcs.GoogleCloudStorageClientSettings.ENDPOINT_SETTING;
@@ -206,6 +208,21 @@ public void testWriteReadLarge() throws IOException {
206208
}
207209
}
208210

211+
public void testWriteFileMultipleOfChunkSize() throws IOException {
212+
final int uploadSize = randomIntBetween(2, 4) * GoogleCloudStorageBlobStore.SDK_DEFAULT_CHUNK_SIZE;
213+
try (BlobStore store = newBlobStore()) {
214+
final BlobContainer container = store.blobContainer(BlobPath.EMPTY);
215+
final String key = randomIdentifier();
216+
byte[] initialValue = randomByteArrayOfLength(uploadSize);
217+
container.writeBlob(randomPurpose(), key, new BytesArray(initialValue), true);
218+
219+
BytesReference reference = readFully(container.readBlob(randomPurpose(), key));
220+
assertEquals(new BytesArray(initialValue), reference);
221+
222+
container.deleteBlobsIgnoringIfNotExists(randomPurpose(), Iterators.single(key));
223+
}
224+
}
225+
209226
public static class TestGoogleCloudStoragePlugin extends GoogleCloudStoragePlugin {
210227

211228
public TestGoogleCloudStoragePlugin(Settings settings) {

‎test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java‎

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -129,36 +129,33 @@ public void handle(final HttpExchange exchange) throws IOException {
129129
final MockGcsBlobStore.BlobVersion blob = mockGcsBlobStore.getBlob(path, ifGenerationMatch);
130130
if (blob != null) {
131131
final String rangeHeader = exchange.getRequestHeaders().getFirst("Range");
132-
final long offset;
133-
final long end;
132+
final BytesReference response;
133+
final int statusCode;
134134
if (rangeHeader == null) {
135-
offset = 0L;
136-
end = blob.contents().length() - 1;
135+
response = blob.contents();
136+
statusCode = RestStatus.OK.getStatus();
137137
} else {
138138
final HttpHeaderParser.Range range = HttpHeaderParser.parseRangeHeader(rangeHeader);
139139
if (range == null) {
140140
throw new AssertionError("Range bytes header does not match expected format: " + rangeHeader);
141141
}
142-
offset = range.start();
143-
end = range.end();
144-
}
145142

146-
if (offset >= blob.contents().length()) {
147-
exchange.getResponseHeaders().add("Content-Type", "application/octet-stream");
148-
exchange.sendResponseHeaders(RestStatus.REQUESTED_RANGE_NOT_SATISFIED.getStatus(), -1);
149-
return;
150-
}
143+
if (range.start() >= blob.contents().length()) {
144+
exchange.getResponseHeaders().add("Content-Type", "application/octet-stream");
145+
exchange.sendResponseHeaders(RestStatus.REQUESTED_RANGE_NOT_SATISFIED.getStatus(), -1);
146+
return;
147+
}
151148

152-
BytesReference response = blob.contents();
153-
exchange.getResponseHeaders().add("Content-Type", "application/octet-stream");
154-
final int bufferedLength = response.length();
155-
if (offset > 0 || bufferedLength > end) {
156-
response = response.slice(
157-
Math.toIntExact(offset),
158-
Math.toIntExact(Math.min(end + 1 - offset, bufferedLength - offset))
159-
);
149+
final long lastIndex = Math.min(range.end(), blob.contents().length() - 1);
150+
response = blob.contents().slice(Math.toIntExact(range.start()), Math.toIntExact(lastIndex - range.start() + 1));
151+
statusCode = RestStatus.PARTIAL_CONTENT.getStatus();
160152
}
161-
exchange.sendResponseHeaders(RestStatus.OK.getStatus(), response.length());
153+
// I think it's enough to use the generation here, at least until
154+
// we implement "metageneration", at that point we must incorporate both
155+
// See: https://cloud.google.com/storage/docs/metadata#etags
156+
exchange.getResponseHeaders().add("ETag", String.valueOf(blob.generation()));
157+
exchange.getResponseHeaders().add("Content-Type", "application/octet-stream");
158+
exchange.sendResponseHeaders(statusCode, response.length());
162159
response.writeTo(exchange.getResponseBody());
163160
} else {
164161
exchange.sendResponseHeaders(RestStatus.NOT_FOUND.getStatus(), -1);

‎test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MockGcsBlobStore.java‎

Lines changed: 66 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,53 @@ public class MockGcsBlobStore {
3838

3939
record BlobVersion(String path, long generation, BytesReference contents) {}
4040

41-
record ResumableUpload(String uploadId, String path, Long ifGenerationMatch, BytesReference contents, boolean completed) {
41+
record ResumableUpload(
42+
String uploadId,
43+
String path,
44+
Long ifGenerationMatch,
45+
BytesReference contents,
46+
Integer finalLength,
47+
boolean completed
48+
) {
49+
50+
ResumableUpload(String uploadId, String path, Long ifGenerationMatch) {
51+
this(uploadId, path, ifGenerationMatch, BytesArray.EMPTY, null, false);
52+
}
53+
54+
public ResumableUpload update(BytesReference contents) {
55+
if (completed) {
56+
throw new IllegalStateException("Blob already completed");
57+
}
58+
return new ResumableUpload(uploadId, path, ifGenerationMatch, contents, null, false);
59+
}
60+
61+
/**
62+
* When we complete, we nullify our reference to the contents to allow it to be collected if it gets overwritten
63+
*/
64+
public ResumableUpload complete() {
65+
if (completed) {
66+
throw new IllegalStateException("Blob already completed");
67+
}
68+
return new ResumableUpload(uploadId, path, ifGenerationMatch, null, contents.length(), true);
69+
}
4270

43-
public ResumableUpload update(BytesReference contents, boolean completed) {
44-
return new ResumableUpload(uploadId, path, ifGenerationMatch, contents, completed);
71+
public HttpHeaderParser.Range getRange() {
72+
int length = length();
73+
if (length > 0) {
74+
return new HttpHeaderParser.Range(0, length - 1);
75+
} else {
76+
return null;
77+
}
78+
}
79+
80+
public int length() {
81+
if (finalLength != null) {
82+
return finalLength;
83+
}
84+
if (contents != null) {
85+
return contents.length();
86+
}
87+
return 0;
4588
}
4689
}
4790

@@ -93,7 +136,7 @@ BlobVersion updateBlob(String path, Long ifGenerationMatch, BytesReference conte
93136

94137
ResumableUpload createResumableUpload(String path, Long ifGenerationMatch) {
95138
final String uploadId = UUIDs.randomBase64UUID();
96-
final ResumableUpload value = new ResumableUpload(uploadId, path, ifGenerationMatch, BytesArray.EMPTY, false);
139+
final ResumableUpload value = new ResumableUpload(uploadId, path, ifGenerationMatch);
97140
resumableUploads.put(uploadId, value);
98141
return value;
99142
}
@@ -114,16 +157,14 @@ UpdateResponse updateResumableUpload(String uploadId, HttpHeaderParser.ContentRa
114157
throw failAndThrow("Attempted to update a non-existent resumable: " + uid);
115158
}
116159

117-
if (contentRange.hasRange() == false) {
118-
// Content-Range: */... is a status check https://cloud.google.com/storage/docs/performing-resumable-uploads#status-check
160+
ResumableUpload valueToReturn = existing;
161+
162+
// Handle the request, a range indicates a chunk of data was submitted
163+
if (contentRange.hasRange()) {
119164
if (existing.completed) {
120-
updateResponse.set(new UpdateResponse(RestStatus.OK.getStatus(), calculateRangeHeader(blobs.get(existing.path))));
121-
} else {
122-
final HttpHeaderParser.Range range = calculateRangeHeader(existing);
123-
updateResponse.set(new UpdateResponse(RESUME_INCOMPLETE, range));
165+
throw failAndThrow("Attempted to write more to a completed resumable upload");
124166
}
125-
return existing;
126-
} else {
167+
127168
if (contentRange.start() > contentRange.end()) {
128169
throw failAndThrow("Invalid content range " + contentRange);
129170
}
@@ -143,29 +184,25 @@ UpdateResponse updateResumableUpload(String uploadId, HttpHeaderParser.ContentRa
143184
existing.contents,
144185
requestBody.slice(offset, requestBody.length())
145186
);
146-
// We just received the last chunk, update the blob and remove the resumable upload from the map
147-
if (contentRange.hasSize() && updatedContent.length() == contentRange.size()) {
148-
updateBlob(existing.path(), existing.ifGenerationMatch, updatedContent);
149-
updateResponse.set(new UpdateResponse(RestStatus.OK.getStatus(), null));
150-
return existing.update(BytesArray.EMPTY, true);
151-
}
152-
final ResumableUpload updated = existing.update(updatedContent, false);
153-
updateResponse.set(new UpdateResponse(RESUME_INCOMPLETE, calculateRangeHeader(updated)));
154-
return updated;
187+
valueToReturn = existing.update(updatedContent);
188+
}
189+
190+
// Next we determine the response
191+
if (valueToReturn.completed) {
192+
updateResponse.set(new UpdateResponse(RestStatus.OK.getStatus(), valueToReturn.getRange()));
193+
} else if (contentRange.hasSize() && contentRange.size() == valueToReturn.contents.length()) {
194+
updateBlob(valueToReturn.path(), valueToReturn.ifGenerationMatch(), valueToReturn.contents);
195+
valueToReturn = valueToReturn.complete();
196+
updateResponse.set(new UpdateResponse(RestStatus.OK.getStatus(), valueToReturn.getRange()));
197+
} else {
198+
updateResponse.set(new UpdateResponse(RESUME_INCOMPLETE, valueToReturn.getRange()));
155199
}
200+
return valueToReturn;
156201
});
157202
assert updateResponse.get() != null : "Should always produce an update response";
158203
return updateResponse.get();
159204
}
160205

161-
private static HttpHeaderParser.Range calculateRangeHeader(ResumableUpload resumableUpload) {
162-
return resumableUpload.contents.length() > 0 ? new HttpHeaderParser.Range(0, resumableUpload.contents.length() - 1) : null;
163-
}
164-
165-
private static HttpHeaderParser.Range calculateRangeHeader(BlobVersion blob) {
166-
return blob.contents.length() > 0 ? new HttpHeaderParser.Range(0, blob.contents.length() - 1) : null;
167-
}
168-
169206
record UpdateResponse(int statusCode, HttpHeaderParser.Range rangeHeader) {}
170207

171208
void deleteBlob(String path) {

‎test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -157,14 +157,14 @@ public void testGetWithBytesRange() {
157157
var end = blobBytes.length() - 1;
158158
assertEquals(
159159
"Exact Range: bytes=0-" + end,
160-
new TestHttpResponse(RestStatus.OK, blobBytes, TestHttpExchange.EMPTY_HEADERS),
160+
new TestHttpResponse(RestStatus.PARTIAL_CONTENT, blobBytes, TestHttpExchange.EMPTY_HEADERS),
161161
getBlobContents(handler, bucket, blobName, null, new HttpHeaderParser.Range(0, end))
162162
);
163163

164164
end = randomIntBetween(blobBytes.length() - 1, Integer.MAX_VALUE);
165165
assertEquals(
166166
"Larger Range: bytes=0-" + end,
167-
new TestHttpResponse(RestStatus.OK, blobBytes, TestHttpExchange.EMPTY_HEADERS),
167+
new TestHttpResponse(RestStatus.PARTIAL_CONTENT, blobBytes, TestHttpExchange.EMPTY_HEADERS),
168168
getBlobContents(handler, bucket, blobName, null, new HttpHeaderParser.Range(0, end))
169169
);
170170

@@ -181,11 +181,38 @@ public void testGetWithBytesRange() {
181181
end = start + length - 1;
182182
assertEquals(
183183
"Range: bytes=" + start + '-' + end,
184-
new TestHttpResponse(RestStatus.OK, blobBytes.slice(start, length), TestHttpExchange.EMPTY_HEADERS),
184+
new TestHttpResponse(RestStatus.PARTIAL_CONTENT, blobBytes.slice(start, length), TestHttpExchange.EMPTY_HEADERS),
185185
getBlobContents(handler, bucket, blobName, null, new HttpHeaderParser.Range(start, end))
186186
);
187187
}
188188

189+
public void testZeroLengthObjectGets() {
190+
final var bucket = randomIdentifier();
191+
final var handler = new GoogleCloudStorageHttpHandler(bucket);
192+
final var blobName = "blob_name_" + randomIdentifier();
193+
final var blobBytes = BytesArray.EMPTY;
194+
195+
assertEquals(RestStatus.OK, executeMultipartUpload(handler, bucket, blobName, blobBytes, 0L).restStatus());
196+
197+
assertEquals(
198+
"No Range",
199+
new TestHttpResponse(RestStatus.OK, blobBytes, TestHttpExchange.EMPTY_HEADERS),
200+
getBlobContents(handler, bucket, blobName, null, null)
201+
);
202+
203+
assertEquals(
204+
"Range 0-0",
205+
new TestHttpResponse(RestStatus.REQUESTED_RANGE_NOT_SATISFIED, BytesArray.EMPTY, TestHttpExchange.EMPTY_HEADERS),
206+
getBlobContents(handler, bucket, blobName, null, new HttpHeaderParser.Range(0, 0))
207+
);
208+
209+
assertEquals(
210+
"Random range x-y",
211+
new TestHttpResponse(RestStatus.REQUESTED_RANGE_NOT_SATISFIED, BytesArray.EMPTY, TestHttpExchange.EMPTY_HEADERS),
212+
getBlobContents(handler, bucket, blobName, null, new HttpHeaderParser.Range(randomIntBetween(0, 30), randomIntBetween(31, 100)))
213+
);
214+
}
215+
189216
public void testResumableUpload() {
190217
final var bucket = randomIdentifier();
191218
final var handler = new GoogleCloudStorageHttpHandler(bucket);
@@ -225,7 +252,7 @@ public void testResumableUpload() {
225252

226253
final var part3 = randomAlphaOfLength(30);
227254
final var uploadPart3Response = handleRequest(handler, "PUT", sessionURI, part3, contentRangeHeader(100, 129, 130));
228-
assertEquals(new TestHttpResponse(RestStatus.OK, TestHttpExchange.EMPTY_HEADERS), uploadPart3Response);
255+
assertEquals(new TestHttpResponse(RestStatus.OK, rangeHeader(0, 129)), uploadPart3Response);
229256

230257
// status check
231258
assertEquals(
@@ -545,7 +572,6 @@ private static TestHttpResponse executeUpload(
545572
BytesReference bytes,
546573
Long ifGenerationMatch
547574
) {
548-
assert bytes.length() > 20;
549575
if (randomBoolean()) {
550576
return executeResumableUpload(handler, bucket, blobName, bytes, ifGenerationMatch);
551577
} else {
@@ -560,6 +586,7 @@ private static TestHttpResponse executeResumableUpload(
560586
BytesReference bytes,
561587
Long ifGenerationMatch
562588
) {
589+
assert bytes.length() >= 2 : "We can't split anything smaller than two";
563590
final var createUploadResponse = handleRequest(
564591
handler,
565592
"POST",
@@ -572,7 +599,7 @@ private static TestHttpResponse executeResumableUpload(
572599
final var sessionURI = locationHeader.substring(locationHeader.indexOf(HOST) + HOST.length());
573600
assertEquals(RestStatus.OK, createUploadResponse.restStatus());
574601

575-
final int partBoundary = randomIntBetween(10, bytes.length() - 1);
602+
final int partBoundary = randomIntBetween(1, bytes.length() - 1);
576603
final var part1 = bytes.slice(0, partBoundary);
577604
final var uploadPart1Response = handleRequest(handler, "PUT", sessionURI, part1, contentRangeHeader(0, partBoundary - 1, null));
578605
assertEquals(RESUME_INCOMPLETE, uploadPart1Response.status());

0 commit comments

Comments
 (0)