tomicooler commented on a change in pull request #3209:
URL: https://github.com/apache/hadoop/pull/3209#discussion_r682508533
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
##########
@@ -69,27 +72,11 @@
@Override
protected Properties getConfiguration(String configPrefix,
FilterConfig filterConfig) throws ServletException{
- Properties props = new Properties();
+ System.out.println("getConfiguration1");
Review comment:
Somehow I managed to commit the debug logs.. :) I'll remove them.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -811,18 +822,26 @@ private static SignerSecretProvider
constructSecretProvider(final Builder b,
throws Exception {
final Configuration conf = b.conf;
Properties config = getFilterProperties(conf,
- b.authFilterConfigurationPrefix);
+ b.authFilterConfigurationPrefixes);
return AuthenticationFilter.constructSecretProvider(
ctx, config, b.disallowFallbackToRandomSignerSecretProvider);
}
- private static Properties getFilterProperties(Configuration conf, String
- prefix) {
- Properties prop = new Properties();
- Map<String, String> filterConfig = AuthenticationFilterInitializer
- .getFilterConfigMap(conf, prefix);
- prop.putAll(filterConfig);
- return prop;
+ public static Properties getFilterProperties(Configuration conf,
+ ArrayList<String> prefixes) {
+ Properties props = new Properties();
+ prefixes.forEach(prefix -> {
+ Map<String, String> filterConfigMap =
+ AuthenticationFilterInitializer.getFilterConfigMap(conf, prefix);
+ filterConfigMap.forEach((key, value) -> {
+ Object previous = props.setProperty(key, value);
+ if (previous != null && !previous.equals(value)) {
+ LOG.warn("Overwriting configuration for key='{}' with value='{}' " +
+ "previous_value='{}'", key, value, previous);
+ }
+ });
+ });
Review comment:
Ok, I'll rewrite it, thanks for the explanation. It seemed a little bit
shorter with the forEach, and more readable with the key value names for
example (instead of entry.getKey(), entry.getValue()).
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -243,7 +243,9 @@
private String hostName;
private boolean disallowFallbackToRandomSignerSecretProvider;
- private String authFilterConfigurationPrefix =
"hadoop.http.authentication.";
+ private final ArrayList<String> authFilterConfigurationPrefixes =
+ new ArrayList<>(Collections.singletonList(
+ "hadoop.http.authentication."));
Review comment:
The array is final, but the elements are inside it will be changed,
checkstyle gives me a warning if I rename it to upper case.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -243,7 +243,9 @@
private String hostName;
private boolean disallowFallbackToRandomSignerSecretProvider;
- private String authFilterConfigurationPrefix =
"hadoop.http.authentication.";
+ private final ArrayList<String> authFilterConfigurationPrefixes =
Review comment:
Thanks. Done.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -473,8 +482,10 @@ public HttpServer2 build() throws IOException {
HttpServer2 server = new HttpServer2(this);
if (this.securityEnabled &&
- !this.conf.get(authFilterConfigurationPrefix + "type").
- equals(PseudoAuthenticationHandler.TYPE)) {
+ authFilterConfigurationPrefixes.stream().noneMatch(
+ prefix -> (prefix + "type")
+ .equals(PseudoAuthenticationHandler.TYPE))
+ ) {
Review comment:
Good catch. it should have been this.conf.get(prefix + "type").equals.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
##########
@@ -71,53 +82,195 @@ public static void beforeClass() throws Exception {
System.setProperty("httpfs.home.dir", homeDir.getAbsolutePath());
System.setProperty("httpfs.log.dir", logsDir.getAbsolutePath());
System.setProperty("httpfs.config.dir", confDir.getAbsolutePath());
- FileUtils.writeStringToFile(new File(confDir, "httpfs-signature.secret"),
- "foo", StandardCharsets.UTF_8);
+ secretFile = new File(System.getProperty("httpfs.config.dir"),
+ "httpfs-signature-custom.secret");
}
- @Before
- public void setUp() throws Exception {
- Configuration conf = new Configuration();
- conf.set(HttpFSServerWebServer.HTTP_HOSTNAME_KEY, "localhost");
- conf.setInt(HttpFSServerWebServer.HTTP_PORT_KEY, 0);
- conf.set(AuthenticationFilter.SIGNATURE_SECRET_FILE,
- "httpfs-signature.secret");
- Configuration sslConf = new Configuration();
- webServer = new HttpFSServerWebServer(conf, sslConf);
+ @After
+ public void teardown() throws Exception {
+ if (webServer != null) {
+ webServer.stop();
+ }
}
@Test
public void testStartStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
- String user = HadoopUsersConfTestHelper.getHadoopUsers()[0];
- URL url = new URL(webServer.getUrl(), MessageFormat.format(
- "/webhdfs/v1/?user.name={0}&op=liststatus", user));
- HttpURLConnection conn = (HttpURLConnection) url.openConnection();
- Assert.assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode());
- BufferedReader reader = new BufferedReader(
- new InputStreamReader(conn.getInputStream()));
- reader.readLine();
- reader.close();
Review comment:
The following tests were just testing if the server could be
started/stopped without an exception (my guess):
- testStartStop
- testJustStop
- testJustStop
- testDoubleStart
I think this was the intention of the original author.
I introduced some other tests with a secret file, empty secret file, etc
with additional assert that actually checks if the secret provider is file
based and not random. That was actually not asserted before, the tests were
green, but the secret file was not even used. These tests uses the
assertServiceRespondsWithOK.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
##########
@@ -71,53 +82,195 @@ public static void beforeClass() throws Exception {
System.setProperty("httpfs.home.dir", homeDir.getAbsolutePath());
System.setProperty("httpfs.log.dir", logsDir.getAbsolutePath());
System.setProperty("httpfs.config.dir", confDir.getAbsolutePath());
- FileUtils.writeStringToFile(new File(confDir, "httpfs-signature.secret"),
- "foo", StandardCharsets.UTF_8);
+ secretFile = new File(System.getProperty("httpfs.config.dir"),
+ "httpfs-signature-custom.secret");
}
- @Before
- public void setUp() throws Exception {
- Configuration conf = new Configuration();
- conf.set(HttpFSServerWebServer.HTTP_HOSTNAME_KEY, "localhost");
- conf.setInt(HttpFSServerWebServer.HTTP_PORT_KEY, 0);
- conf.set(AuthenticationFilter.SIGNATURE_SECRET_FILE,
- "httpfs-signature.secret");
- Configuration sslConf = new Configuration();
- webServer = new HttpFSServerWebServer(conf, sslConf);
+ @After
+ public void teardown() throws Exception {
+ if (webServer != null) {
+ webServer.stop();
+ }
}
@Test
public void testStartStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
- String user = HadoopUsersConfTestHelper.getHadoopUsers()[0];
- URL url = new URL(webServer.getUrl(), MessageFormat.format(
- "/webhdfs/v1/?user.name={0}&op=liststatus", user));
- HttpURLConnection conn = (HttpURLConnection) url.openConnection();
- Assert.assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode());
- BufferedReader reader = new BufferedReader(
- new InputStreamReader(conn.getInputStream()));
- reader.readLine();
- reader.close();
webServer.stop();
}
@Test
public void testJustStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.stop();
}
@Test
public void testDoubleStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
webServer.stop();
webServer.stop();
}
@Test
public void testDoubleStart() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
+ webServer.start();
+ webServer.start();
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFile() throws Exception {
+ createSecretFile("foo");
+ webServer = createWebServer(createConfigurationWithSecretFile());
+ webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ FileSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithDeprecatedConfigOnly()
+ throws Exception {
+ createSecretFile("foo");
+ Configuration conf = createConfiguration();
+ setDeprecatedSecretFile(conf, secretFile.getAbsolutePath());
+ webServer = createWebServer(conf);
+
+ // HttpFSAuthenticationFilter gets the configuration from the webapp, the
defaults are loaded
+ // so the custom file location is overwritten with a non-existing default
one.
+ Exception exception =
+ Assert.assertThrows(IOException.class, new ThrowingRunnable() {
+ @Override
+ public void run() throws Throwable {
+ webServer.start();
+ }
+ });
+ Assert.assertTrue(exception.getMessage().startsWith(
+ "Unable to initialize"));
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithBothConfigOption() throws Exception
{
Review comment:
Done.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/client/TestHttpFSFileSystemLocalFileSystem.java
##########
@@ -32,8 +32,6 @@
import org.junit.runners.Parameterized;
import java.io.File;
-import java.net.URI;
Review comment:
Done.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
##########
@@ -69,27 +72,11 @@
@Override
protected Properties getConfiguration(String configPrefix,
FilterConfig filterConfig) throws ServletException{
- Properties props = new Properties();
+ System.out.println("getConfiguration1");
Review comment:
Done.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -811,18 +822,26 @@ private static SignerSecretProvider
constructSecretProvider(final Builder b,
throws Exception {
final Configuration conf = b.conf;
Properties config = getFilterProperties(conf,
- b.authFilterConfigurationPrefix);
+ b.authFilterConfigurationPrefixes);
return AuthenticationFilter.constructSecretProvider(
ctx, config, b.disallowFallbackToRandomSignerSecretProvider);
}
- private static Properties getFilterProperties(Configuration conf, String
- prefix) {
- Properties prop = new Properties();
- Map<String, String> filterConfig = AuthenticationFilterInitializer
- .getFilterConfigMap(conf, prefix);
- prop.putAll(filterConfig);
- return prop;
+ public static Properties getFilterProperties(Configuration conf,
+ ArrayList<String> prefixes) {
+ Properties props = new Properties();
+ prefixes.forEach(prefix -> {
+ Map<String, String> filterConfigMap =
+ AuthenticationFilterInitializer.getFilterConfigMap(conf, prefix);
+ filterConfigMap.forEach((key, value) -> {
+ Object previous = props.setProperty(key, value);
+ if (previous != null && !previous.equals(value)) {
+ LOG.warn("Overwriting configuration for key='{}' with value='{}' " +
+ "previous_value='{}'", key, value, previous);
Review comment:
Done.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -811,18 +822,26 @@ private static SignerSecretProvider
constructSecretProvider(final Builder b,
throws Exception {
final Configuration conf = b.conf;
Properties config = getFilterProperties(conf,
- b.authFilterConfigurationPrefix);
+ b.authFilterConfigurationPrefixes);
return AuthenticationFilter.constructSecretProvider(
ctx, config, b.disallowFallbackToRandomSignerSecretProvider);
}
- private static Properties getFilterProperties(Configuration conf, String
- prefix) {
- Properties prop = new Properties();
- Map<String, String> filterConfig = AuthenticationFilterInitializer
- .getFilterConfigMap(conf, prefix);
- prop.putAll(filterConfig);
- return prop;
+ public static Properties getFilterProperties(Configuration conf,
+ ArrayList<String> prefixes) {
Review comment:
Done.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -473,8 +482,10 @@ public HttpServer2 build() throws IOException {
HttpServer2 server = new HttpServer2(this);
if (this.securityEnabled &&
- !this.conf.get(authFilterConfigurationPrefix + "type").
- equals(PseudoAuthenticationHandler.TYPE)) {
+ authFilterConfigurationPrefixes.stream().noneMatch(
+ prefix -> (prefix + "type")
+ .equals(PseudoAuthenticationHandler.TYPE))
+ ) {
Review comment:
Good catch. it should have been this.conf.get(prefix + "type").equals.
Here I find the stream().noneMatch useful with the lambda expression. I won't
resolve the thread yet, so I can replace them with a helper function if needed.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
##########
@@ -71,53 +82,195 @@ public static void beforeClass() throws Exception {
System.setProperty("httpfs.home.dir", homeDir.getAbsolutePath());
System.setProperty("httpfs.log.dir", logsDir.getAbsolutePath());
System.setProperty("httpfs.config.dir", confDir.getAbsolutePath());
- FileUtils.writeStringToFile(new File(confDir, "httpfs-signature.secret"),
- "foo", StandardCharsets.UTF_8);
+ secretFile = new File(System.getProperty("httpfs.config.dir"),
+ "httpfs-signature-custom.secret");
}
- @Before
- public void setUp() throws Exception {
- Configuration conf = new Configuration();
- conf.set(HttpFSServerWebServer.HTTP_HOSTNAME_KEY, "localhost");
- conf.setInt(HttpFSServerWebServer.HTTP_PORT_KEY, 0);
- conf.set(AuthenticationFilter.SIGNATURE_SECRET_FILE,
- "httpfs-signature.secret");
- Configuration sslConf = new Configuration();
- webServer = new HttpFSServerWebServer(conf, sslConf);
+ @After
+ public void teardown() throws Exception {
+ if (webServer != null) {
+ webServer.stop();
+ }
}
@Test
public void testStartStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
- String user = HadoopUsersConfTestHelper.getHadoopUsers()[0];
- URL url = new URL(webServer.getUrl(), MessageFormat.format(
- "/webhdfs/v1/?user.name={0}&op=liststatus", user));
- HttpURLConnection conn = (HttpURLConnection) url.openConnection();
- Assert.assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode());
- BufferedReader reader = new BufferedReader(
- new InputStreamReader(conn.getInputStream()));
- reader.readLine();
- reader.close();
webServer.stop();
}
@Test
public void testJustStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.stop();
}
@Test
public void testDoubleStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
webServer.stop();
webServer.stop();
}
@Test
public void testDoubleStart() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
+ webServer.start();
+ webServer.start();
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFile() throws Exception {
+ createSecretFile("foo");
+ webServer = createWebServer(createConfigurationWithSecretFile());
+ webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ FileSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithDeprecatedConfigOnly()
+ throws Exception {
+ createSecretFile("foo");
+ Configuration conf = createConfiguration();
+ setDeprecatedSecretFile(conf, secretFile.getAbsolutePath());
+ webServer = createWebServer(conf);
+
+ // HttpFSAuthenticationFilter gets the configuration from the webapp, the
defaults are loaded
+ // so the custom file location is overwritten with a non-existing default
one.
+ Exception exception =
+ Assert.assertThrows(IOException.class, new ThrowingRunnable() {
+ @Override
+ public void run() throws Throwable {
+ webServer.start();
+ }
+ });
+ Assert.assertTrue(exception.getMessage().startsWith(
+ "Unable to initialize"));
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithBothConfigOption() throws Exception
{
+ createSecretFile("foo");
+ Configuration conf = createConfigurationWithSecretFile();
+ setDeprecatedSecretFile(conf, secretFile.getAbsolutePath());
+ webServer = createWebServer(conf);
+ webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ FileSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithBothConfigOptionOverWriteCheck()
Review comment:
Done.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
##########
@@ -98,6 +85,7 @@ protected Properties getConfiguration(String configPrefix,
}
if (!isRandomSecret(filterConfig)) {
+ System.out.println("FILE: " + signatureSecretFile);
Review comment:
Done
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
##########
@@ -69,27 +72,11 @@
@Override
protected Properties getConfiguration(String configPrefix,
FilterConfig filterConfig) throws ServletException{
- Properties props = new Properties();
+ System.out.println("getConfiguration1");
Configuration conf = HttpFSServerWebApp.get().getConfig();
-
- props.setProperty(AuthenticationFilter.COOKIE_PATH, "/");
- for (Map.Entry<String, String> entry : conf) {
- String name = entry.getKey();
- if (name.startsWith(HADOOP_HTTP_CONF_PREFIX)) {
- name = name.substring(HADOOP_HTTP_CONF_PREFIX.length());
- props.setProperty(name, entry.getValue());
- }
- }
-
- // Replace Hadoop Http Authentication Configs with HttpFS specific Configs
- for (Map.Entry<String, String> entry : conf) {
- String name = entry.getKey();
- if (name.startsWith(CONF_PREFIX)) {
- String value = conf.get(name);
- name = name.substring(CONF_PREFIX.length());
- props.setProperty(name, value);
- }
- }
+ Properties props = HttpServer2.getFilterProperties(conf,
+ new ArrayList<>(Arrays.asList(CONF_PREFIXES)));
+ System.out.println("getConfiguration2");
Review comment:
Done.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/test/java/org/apache/hadoop/fs/http/server/TestHttpFSServerWebServer.java
##########
@@ -71,53 +82,195 @@ public static void beforeClass() throws Exception {
System.setProperty("httpfs.home.dir", homeDir.getAbsolutePath());
System.setProperty("httpfs.log.dir", logsDir.getAbsolutePath());
System.setProperty("httpfs.config.dir", confDir.getAbsolutePath());
- FileUtils.writeStringToFile(new File(confDir, "httpfs-signature.secret"),
- "foo", StandardCharsets.UTF_8);
+ secretFile = new File(System.getProperty("httpfs.config.dir"),
+ "httpfs-signature-custom.secret");
}
- @Before
- public void setUp() throws Exception {
- Configuration conf = new Configuration();
- conf.set(HttpFSServerWebServer.HTTP_HOSTNAME_KEY, "localhost");
- conf.setInt(HttpFSServerWebServer.HTTP_PORT_KEY, 0);
- conf.set(AuthenticationFilter.SIGNATURE_SECRET_FILE,
- "httpfs-signature.secret");
- Configuration sslConf = new Configuration();
- webServer = new HttpFSServerWebServer(conf, sslConf);
+ @After
+ public void teardown() throws Exception {
+ if (webServer != null) {
+ webServer.stop();
+ }
}
@Test
public void testStartStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
- String user = HadoopUsersConfTestHelper.getHadoopUsers()[0];
- URL url = new URL(webServer.getUrl(), MessageFormat.format(
- "/webhdfs/v1/?user.name={0}&op=liststatus", user));
- HttpURLConnection conn = (HttpURLConnection) url.openConnection();
- Assert.assertEquals(HttpURLConnection.HTTP_OK, conn.getResponseCode());
- BufferedReader reader = new BufferedReader(
- new InputStreamReader(conn.getInputStream()));
- reader.readLine();
- reader.close();
webServer.stop();
}
@Test
public void testJustStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.stop();
}
@Test
public void testDoubleStop() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
webServer.start();
webServer.stop();
webServer.stop();
}
@Test
public void testDoubleStart() throws Exception {
+ webServer = createWebServer(createConfigurationWithRandomSecret());
+ webServer.start();
+ webServer.start();
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFile() throws Exception {
+ createSecretFile("foo");
+ webServer = createWebServer(createConfigurationWithSecretFile());
+ webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ FileSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithDeprecatedConfigOnly()
+ throws Exception {
+ createSecretFile("foo");
+ Configuration conf = createConfiguration();
+ setDeprecatedSecretFile(conf, secretFile.getAbsolutePath());
+ webServer = createWebServer(conf);
+
+ // HttpFSAuthenticationFilter gets the configuration from the webapp, the
defaults are loaded
+ // so the custom file location is overwritten with a non-existing default
one.
+ Exception exception =
+ Assert.assertThrows(IOException.class, new ThrowingRunnable() {
+ @Override
+ public void run() throws Throwable {
+ webServer.start();
+ }
+ });
+ Assert.assertTrue(exception.getMessage().startsWith(
+ "Unable to initialize"));
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithBothConfigOption() throws Exception
{
+ createSecretFile("foo");
+ Configuration conf = createConfigurationWithSecretFile();
+ setDeprecatedSecretFile(conf, secretFile.getAbsolutePath());
+ webServer = createWebServer(conf);
+ webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ FileSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithSecretFileWithBothConfigOptionOverWriteCheck()
+ throws Exception {
+ createSecretFile("foo");
+ Configuration conf = createConfigurationWithSecretFile();
+ setDeprecatedSecretFile(conf, "will-be-overwritten");
+ webServer = createWebServer(conf);
+ webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ FileSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithMissingSecretFile() throws Exception {
+ webServer = createWebServer(createConfigurationWithSecretFile());
webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ RandomSignerSecretProvider.class);
+ webServer.stop();
+ }
+
+ @Test
+ public void testServiceWithEmptySecretFile() throws Exception {
+ // The AuthenticationFilter.constructSecretProvider will do the fallback
+ // to the random secrets not the HttpFSAuthenticationFilter.
+ createSecretFile("");
+ webServer = createWebServer(createConfigurationWithSecretFile());
webServer.start();
+ assertServiceRespondsWithOK(webServer.getUrl());
+ assertSignerSecretProviderType(webServer.getHttpServer(),
+ RandomSignerSecretProvider.class);
webServer.stop();
}
+ private <T extends SignerSecretProvider> void assertSignerSecretProviderType(
+ HttpServer2 server, Class<T> expected) {
+ SignerSecretProvider secretProvider = (SignerSecretProvider)
+ server.getWebAppContext().getServletContext()
+ .getAttribute(SIGNER_SECRET_PROVIDER_ATTRIBUTE);
+ Assert.assertNotNull(secretProvider);
+ Assert.assertEquals(expected, secretProvider.getClass());
Review comment:
Done.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -473,8 +482,10 @@ public HttpServer2 build() throws IOException {
HttpServer2 server = new HttpServer2(this);
if (this.securityEnabled &&
- !this.conf.get(authFilterConfigurationPrefix + "type").
- equals(PseudoAuthenticationHandler.TYPE)) {
+ authFilterConfigurationPrefixes.stream().noneMatch(
+ prefix -> (prefix + "type")
+ .equals(PseudoAuthenticationHandler.TYPE))
+ ) {
Review comment:
Good catch. it should have been this.conf.get(prefix + "type").equals.
Here I find the stream().noneMatch useful with the lambda expression. I won't
resolve the thread yet, so I can replace it with a helper function if needed.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -473,8 +482,10 @@ public HttpServer2 build() throws IOException {
HttpServer2 server = new HttpServer2(this);
if (this.securityEnabled &&
- !this.conf.get(authFilterConfigurationPrefix + "type").
- equals(PseudoAuthenticationHandler.TYPE)) {
+ authFilterConfigurationPrefixes.stream().noneMatch(
+ prefix -> (prefix + "type")
+ .equals(PseudoAuthenticationHandler.TYPE))
+ ) {
Review comment:
Probably there is no test for this :/
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSAuthenticationFilter.java
##########
@@ -69,27 +72,11 @@
@Override
protected Properties getConfiguration(String configPrefix,
FilterConfig filterConfig) throws ServletException{
- Properties props = new Properties();
+ System.out.println("getConfiguration1");
Configuration conf = HttpFSServerWebApp.get().getConfig();
-
- props.setProperty(AuthenticationFilter.COOKIE_PATH, "/");
Review comment:
Yes, the HttpServer2.getFilterProperties() will end up doing this.
BTW the order of the overwriting has changed as I mentioned in the commit
message. Also here the line 80 contained an error, calling the getValue() did
not substituted the variable, but on the line 88 it did.
##########
File path:
hadoop-hdfs-project/hadoop-hdfs-httpfs/src/main/java/org/apache/hadoop/fs/http/server/HttpFSServerWebServer.java
##########
@@ -178,6 +179,10 @@ public URL getUrl() {
}
}
+ public HttpServer2 getHttpServer() {
Review comment:
Done.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -243,7 +243,9 @@
private String hostName;
private boolean disallowFallbackToRandomSignerSecretProvider;
- private String authFilterConfigurationPrefix =
"hadoop.http.authentication.";
+ private final ArrayList<String> authFilterConfigurationPrefixes =
+ new ArrayList<>(Collections.singletonList(
+ "hadoop.http.authentication."));
Review comment:
HttpServer2 is in the hadoop-common while HttpFSAuthenticationFilter is
in the hadoop-hdfs-project/hadoop-hdfs-httpfs. The later depends on the
hadoop-common :/.
##########
File path:
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java
##########
@@ -811,18 +822,25 @@ private static SignerSecretProvider
constructSecretProvider(final Builder b,
throws Exception {
final Configuration conf = b.conf;
Properties config = getFilterProperties(conf,
- b.authFilterConfigurationPrefix);
+ b.authFilterConfigurationPrefixes);
return AuthenticationFilter.constructSecretProvider(
ctx, config, b.disallowFallbackToRandomSignerSecretProvider);
}
- private static Properties getFilterProperties(Configuration conf, String
- prefix) {
- Properties prop = new Properties();
- Map<String, String> filterConfig = AuthenticationFilterInitializer
- .getFilterConfigMap(conf, prefix);
- prop.putAll(filterConfig);
- return prop;
+ public static Properties getFilterProperties(final Configuration conf, final
List<String> prefixes) {
Review comment:
Ok, I'll change it back and remove the finals from the inner lines too,
it is quite short anyway.
I read some debates on this topic, if final is used consistently then the
ones without final will catch your eyes more easily. (e.g.:
http://www.javapractices.com/topic/TopicAction.do?Id=23)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
