steveloughran commented on code in PR #7379:
URL: https://github.com/apache/hadoop/pull/7379#discussion_r2082410937
##########
hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/impl/AWSClientConfig.java:
##########
@@ -412,6 +419,36 @@ private static void initSigner(Configuration conf,
}
}
+ /**
+ * Initialize custom request headers for AWS clients.
+ * @param conf hadoop configuration
+ * @param clientConfig client configuration to update
+ * @param awsServiceIdentifier service name
+ */
+ private static void initRequestHeaders(Configuration conf,
+ ClientOverrideConfiguration.Builder clientConfig, String
awsServiceIdentifier) {
+ String configKey = null;
+ switch (awsServiceIdentifier) {
+ case AWS_SERVICE_IDENTIFIER_S3:
+ configKey = CUSTOM_HEADERS_S3;
+ break;
+ case AWS_SERVICE_IDENTIFIER_STS:
+ configKey = CUSTOM_HEADERS_STS;
+ break;
+ default:
+ // No known service.
+ }
+ if (configKey != null) {
+ Map<String, String> awsClientCustomHeadersMap =
+ S3AUtils.getTrimmedStringCollectionSplitByEquals(conf,
configKey);
+ awsClientCustomHeadersMap.forEach((header, valueString) -> {
+ List<String> headerValues = Arrays.asList(valueString.split(";"));
Review Comment:
trim the header values of whitespace? or is there some risk that this could
ever be needed?
##########
hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/impl/TestAwsClientConfig.java:
##########
@@ -201,4 +208,64 @@ public void testCreateApiConnectionSettingsDefault() {
private void setOptionsToValue(String value, Configuration conf, String...
keys) {
Arrays.stream(keys).forEach(key -> conf.set(key, value));
}
+
+ /**
+ * if {@link org.apache.hadoop.fs.s3a.Constants#CUSTOM_HEADERS_STS} is set,
+ * verify that returned client configuration has desired headers set.
+ */
+ @Test
+ public void testInitRequestHeadersForSTS() throws IOException {
+ final Configuration conf = new Configuration();
+ conf.set(CUSTOM_HEADERS_STS, "foo=bar;baz,qux=quux");
+ Assertions.assertThat(conf.get(CUSTOM_HEADERS_S3))
+ .describedAs("Custom client headers for s3 %s", CUSTOM_HEADERS_S3)
+ .isNull();
+
+ Assertions.assertThat(createClientConfigBuilder(conf,
AWS_SERVICE_IDENTIFIER_S3)
+ .headers().size())
+ .describedAs("Count of S3 client headers")
+ .isEqualTo(0);
+ Assertions.assertThat(createClientConfigBuilder(conf,
AWS_SERVICE_IDENTIFIER_STS)
+ .headers().size())
+ .describedAs("Count of STS client headers")
+ .isEqualTo(2);
+ Assertions.assertThat(createClientConfigBuilder(conf,
AWS_SERVICE_IDENTIFIER_STS)
+ .headers().get("foo"))
+ .describedAs("STS client 'foo' header value")
+ .isEqualTo(Lists.newArrayList("bar", "baz"));
+ Assertions.assertThat(createClientConfigBuilder(conf,
AWS_SERVICE_IDENTIFIER_STS)
+ .headers().get("qux"))
+ .describedAs("STS client 'qux' header value")
+ .isEqualTo(Lists.newArrayList("quux"));
+ }
+
+ /**
+ * if {@link org.apache.hadoop.fs.s3a.Constants#CUSTOM_HEADERS_S3} is set,
+ * verify that returned client configuration has desired headers set.
+ */
+ @Test
+ public void testInitRequestHeadersForS3() throws IOException {
+ final Configuration conf = new Configuration();
+ conf.set(CUSTOM_HEADERS_S3, "foo=bar;baz,qux=quux");
Review Comment:
repeat with a separate test adding spaces between all symbols, with
assertions on the values. if we trim. it should be similar to this
Add separate tests for
```
foo=,qux=quux
foo=;,qux=quux;
foo=;
foo=,
foo=;bar
=
foo=;;
foo=duplicate;duplicate // expect only one value
foo=1,foo=2 // which one is picked up
```
there's enough here to consider adding some common assert, e.g.
```
assertHeaderValues(string, List<string> header1_values, List<string>
header_2_values) {
// assert on each set of values
}
```
Also, let's cut foo/bar stuff.
How about:
"header1, header2", "value1, value2, value3"
This will be good when debugging assertion failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
