szilard-nemeth commented on a change in pull request #3209:
URL: https://github.com/apache/hadoop/pull/3209#discussion_r682491876
##########
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:
Maybe I'm missing something but this is not doing the same as the old
code block.
The old code checked if the configuration object has a key set for the
prefix + type.
With the new code, you are just checking if the Stream constructed from
authFilterConfigurationPrefixes does not match the prefix + type.
Is this intentional?
##########
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:
I don't get why this code block was deleted?
Also I don't see a call to assertServiceRespondsWithOK either.
Please elaborate.
##########
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:
Have you intentionally deleted this line?
```
props.setProperty(AuthenticationFilter.COOKIE_PATH, "/");
```
##########
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:
Same as above, please use List instead of ArrayList, interface types
over concrete types.
##########
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:
These removals seem unrelated to the patch, please revert these changes
if they are not crucial for the patch.
##########
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:
```suggestion
"previous value='{}'", key, value, previous);
```
##########
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:
Use List instead of ArrayList<>. It's a better practice to define fields
/ local variables with interface types, see:
https://stackoverflow.com/questions/9852831/polymorphism-why-use-list-list-new-arraylist-instead-of-arraylist-list-n
##########
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:
Was this only added for testing? If so, please add a VisibleForTesting
annotation over the method.
##########
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:
Please get rid of all System.out.println calls or alternatively, use the
Logger.info method.
##########
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:
Nit: testServiceWithSecretFileWithBothConfigOptionsOverwriteCheck
options: plural
overwrite: one word
##########
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:
Please add assertion messages.
##########
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:
Nit: testServiceWithSecretFileWithBothConfigOptions (plural)
--
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]
