Return ETag from S3 fixture GetObject API
#138514
Conversation
AWS S3 uses the `ETag` header to identify the object contents in various API responses. `S3HttpHandler` returns this header on some paths, but not very many, and the returned header does not conform to the spec (particularly, it is not surrounded by `"` characters). This commit adds the missing response header to the `GetObject` API, fixes its format, and uses SHA256 rather than MD5 to compute the result.
DaveCTurner
added
>test
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
joshua-adams-1
left a comment
joshua-adams-1
left a comment
There was a problem hiding this comment.
Looks good, just had a few small points
| return; | ||
| } | ||
|
|
||
| exchange.getResponseHeaders().add("ETag", getEtagFromContents(blob)); |
There was a problem hiding this comment.
Could we have a brief comment here explaining what an ETag is?
There was a problem hiding this comment.
I added a comment in da4e70e. Not sure where to put it really, ETag is a pretty standard piece of HTTP.
There was a problem hiding this comment.
AFAIK S3 returns ETag on creation(PutObject, MPU) too, but our fixture does not return ETag unless there is precondition failure. Should we add ETag to all operations that suppose to have one?
Also can compute ETag once on creation and store together with blob
record Blob (BytesReference bytes, String etag) {}
private final ConcurrentMap<String, Blob> blobs = new ConcurrentHashMap<>();
There was a problem hiding this comment.
We have no plans to use the ETag returned by the PutObject and CompleteMultipartUpload APIs so I'm hesitant to add them now. It'd be easy enough to add later if needed.
I did consider tracking etags alongside the object contents but it's a bigger change and not really necessary. This is purely test fixture code, and indeed a bit of latency here and there is helpful for test coverage.
| } | ||
| } | ||
|
|
||
| public static String getEtagFromContents(BytesReference blobContents) { |
There was a problem hiding this comment.
Is there any value adding a specific unit test for this method?
There was a problem hiding this comment.
Eh possibly, I meant to at least. Added in 6e98292.
| */ | ||
| private static final Set<String> METHODS_HAVING_NO_REQUEST_BODY = Set.of("GET", "HEAD", "DELETE"); | ||
|
|
||
| private static final String SHA_256_ETAG_PREFIX = "es-test-sha-256-"; |
There was a problem hiding this comment.
Is this string going to be visible?
There was a problem hiding this comment.
No, this is only in the test fixture, I'm using something descriptive here just to aid future troubleshooting. It's treated as an opaque string everywhere else.
Where else should this be added?
Is there an obvious reason for this? |
Really just necessity - we haven't needed it so it hasn't been included. |
DaveCTurner
changed the title
ETag header from S3 fixtureETag from S3 fixture GetObject API
AWS S3 uses the `ETag` header to identify the object contents in various API responses. `S3HttpHandler` doesn't today return this header from its `GetObject` API, and its APIs which do return an `ETag` do not properly conform to its spec (particularly, they are not surrounded by `"` characters). This commit adds the missing response header to the `GetObject` API, fixes its format, and uses SHA256 rather than MD5 to compute the result.
AWS S3 uses the `ETag` header to identify the object contents in various API responses. `S3HttpHandler` doesn't today return this header from its `GetObject` API, and its APIs which do return an `ETag` do not properly conform to its spec (particularly, they are not surrounded by `"` characters). This commit adds the missing response header to the `GetObject` API, fixes its format, and uses SHA256 rather than MD5 to compute the result.
4 participants
AWS S3 uses the
ETagheader to identify the object contents in variousAPI responses.
S3HttpHandlerdoesn't today return this header from itsGetObjectAPI, and its APIs which do return anETagdo not properlyconform to its spec (particularly, they are not surrounded by
"characters). This commit adds the missing response header to the
GetObjectAPI, fixes its format, and uses SHA256 rather than MD5 tocompute the result.