HDFS-16753. WebHDFSHandler should reject non-compliant requests #4834

snmvaughan · 2022-08-30T19:19:22Z

Description of PR

When the nnId is not provided to the WebHDFSClient, the request uses null to generate a URI using a host name of "null" to construct a DFSClient instance. In environments where the host name "null" doesn't resolve, the test passes due to the unresolvable name. If the host name "null" does resolve, then this results in repeated attempts through the retry mechanism, eventually causing a timeout and a failed test result.

This change make the parameter a precondition for constructing the DFSClient, which throws an exception, rejecting the request, and return the expected 400 status code.

Make a non-null nnId a precondition for creating a DFSClient.

How was this patch tested?

The patch was tested using both Maven Surefire and an IDE to demonstrate that the expected 400 status code was returned when the parameter was missing.

For code changes:

Does the title or this PR starts with the corresponding JIRA issue id (e.g. 'HADOOP-17799. Your PR title ...')?
Object storage: have the integration tests been executed and the endpoint declared according to the connector-specific documentation?
If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE, LICENSE-binary, NOTICE-binary files?

hadoop-yetus · 2022-08-31T01:08:17Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 42s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	38m 33s		trunk passed
+1 💚	compile	1m 32s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	compile	1m 29s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 17s		trunk passed
+1 💚	mvnsite	1m 44s		trunk passed
+1 💚	javadoc	1m 17s		trunk passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 43s		trunk passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 38s		trunk passed
+1 💚	shadedclient	23m 6s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 24s		the patch passed
+1 💚	compile	1m 23s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javac	1m 23s		the patch passed
+1 💚	compile	1m 19s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 19s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 58s		the patch passed
+1 💚	mvnsite	1m 25s		the patch passed
+1 💚	javadoc	0m 53s		the patch passed with JDK Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
+1 💚	javadoc	1m 28s		the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 22s		the patch passed
+1 💚	shadedclient	22m 46s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	238m 37s		hadoop-hdfs in the patch passed.
+1 💚	asflicense	1m 6s		The patch does not generate ASF License warnings.
		347m 37s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/1/artifact/out/Dockerfile
GITHUB PR	#4834
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux b3737185c4b3 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / cb6d793b9629f9b2efab6bae739c207259e841d6
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/1/testReport/
Max. process+thread count	3110 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/1/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

saintstack

Requiring non-null noid makes sense to me.

This looks hard to test for. Is there any coverage currently that you can cite?

snmvaughan · 2022-08-31T15:55:00Z

The original test is TestWebHdfsFileSystemContract#testResponseCode, which tests when NamenodeAddressParam is not set:

Initiates a request and gets a redirect in the response
Removes NamenodeAddressParam from the redirect
Connects to the DataNode using the modified URL and checking for a response status of SC_BAD_REQUEST

saintstack · 2022-08-31T22:37:13Z

So now the test passes TestWebHdfsFileSystemContract#testResponseCode when 'null' doesn't resolve -- as expected -- but also passes when 'null' resolves?

(What does resolve of 'null' even look like?).

snmvaughan · 2022-09-01T00:30:43Z

No. The update doesn't allow a null value (as opposed to a host name "null") to be misinterpreted as a string. You could still have a host named "null", but that doesn't apply in the case of a missing parameter.

hadoop-yetus · 2022-09-11T07:31:15Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 37s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	38m 45s		trunk passed
+1 💚	compile	1m 35s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	compile	1m 28s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 15s		trunk passed
+1 💚	mvnsite	1m 48s		trunk passed
+1 💚	javadoc	1m 15s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 46s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 35s		trunk passed
+1 💚	shadedclient	22m 44s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 23s		the patch passed
+1 💚	compile	1m 22s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javac	1m 22s		the patch passed
+1 💚	compile	1m 23s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 23s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 1s		the patch passed
+1 💚	mvnsite	1m 22s		the patch passed
+1 💚	javadoc	0m 58s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 26s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 27s		the patch passed
+1 💚	shadedclient	22m 52s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
-1 ❌	unit	242m 21s	/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt	hadoop-hdfs in the patch passed.
+1 💚	asflicense	1m 2s		The patch does not generate ASF License warnings.
		351m 27s

Reason	Tests
Failed junit tests	hadoop.hdfs.TestRollingUpgrade

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/2/artifact/out/Dockerfile
GITHUB PR	#4834
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux b70043807a16 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `9afc036`
Default Java	Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/2/testReport/
Max. process+thread count	3185 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/2/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

hadoop-yetus · 2022-10-13T20:12:53Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 40s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	39m 21s		trunk passed
+1 💚	compile	1m 35s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	compile	1m 40s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	1m 14s		trunk passed
+1 💚	mvnsite	1m 40s		trunk passed
+1 💚	javadoc	1m 23s		trunk passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 39s		trunk passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 43s		trunk passed
+1 💚	shadedclient	23m 27s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 23s		the patch passed
+1 💚	compile	1m 23s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javac	1m 23s		the patch passed
+1 💚	compile	1m 18s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 18s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	1m 0s		the patch passed
+1 💚	mvnsite	1m 25s		the patch passed
+1 💚	javadoc	0m 58s		the patch passed with JDK Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04
+1 💚	javadoc	1m 36s		the patch passed with JDK Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
+1 💚	spotbugs	3m 21s		the patch passed
+1 💚	shadedclient	22m 38s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
+1 💚	unit	243m 51s		hadoop-hdfs in the patch passed.
+1 💚	asflicense	0m 56s		The patch does not generate ASF License warnings.
		354m 22s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/3/artifact/out/Dockerfile
GITHUB PR	#4834
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux 366deeb46b5e 4.15.0-191-generic #202-Ubuntu SMP Thu Aug 4 01:49:29 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `75b7e31`
Default Java	Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.16+8-post-Ubuntu-0ubuntu120.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_342-8u342-b07-0ubuntu1~20.04-b07
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/3/testReport/
Max. process+thread count	3311 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/3/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

Make a non-null nnId a precondition for creating a DFSClient.

hadoop-yetus · 2023-02-22T00:10:33Z

💔 -1 overall

Vote	Subsystem	Runtime	Logfile	Comment
+0 🆗	reexec	0m 38s		Docker mode activated.
			_ Prechecks _
+1 💚	dupname	0m 0s		No case conflicting files found.
+0 🆗	codespell	0m 0s		codespell was not available.
+0 🆗	detsecrets	0m 0s		detect-secrets was not available.
+1 💚	@author	0m 0s		The patch does not contain any @author tags.
-1 ❌	test4tests	0m 0s		The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
			_ trunk Compile Tests _
+1 💚	mvninstall	43m 28s		trunk passed
+1 💚	compile	1m 26s		trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04
+1 💚	compile	1m 24s		trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08
+1 💚	checkstyle	1m 7s		trunk passed
+1 💚	mvnsite	1m 30s		trunk passed
+1 💚	javadoc	1m 7s		trunk passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04
+1 💚	javadoc	1m 31s		trunk passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08
+1 💚	spotbugs	3m 23s		trunk passed
+1 💚	shadedclient	25m 44s		branch has no errors when building and testing our client artifacts.
			_ Patch Compile Tests _
+1 💚	mvninstall	1m 26s		the patch passed
+1 💚	compile	1m 18s		the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04
+1 💚	javac	1m 18s		the patch passed
+1 💚	compile	1m 14s		the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08
+1 💚	javac	1m 14s		the patch passed
+1 💚	blanks	0m 0s		The patch has no blanks issues.
+1 💚	checkstyle	0m 51s		the patch passed
+1 💚	mvnsite	1m 21s		the patch passed
+1 💚	javadoc	0m 50s		the patch passed with JDK Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04
+1 💚	javadoc	1m 25s		the patch passed with JDK Private Build-1.8.0_352-8u352-ga-1~20.04-b08
+1 💚	spotbugs	3m 11s		the patch passed
+1 💚	shadedclient	25m 45s		patch has no errors when building and testing our client artifacts.
			_ Other Tests _
-1 ❌	unit	206m 31s	/patch-unit-hadoop-hdfs-project_hadoop-hdfs.txt	hadoop-hdfs in the patch passed.
+1 💚	asflicense	0m 49s		The patch does not generate ASF License warnings.
		324m 17s

Reason	Tests
Failed junit tests	hadoop.hdfs.server.datanode.fsdataset.impl.TestFsDatasetImpl
	hadoop.hdfs.server.datanode.TestDirectoryScanner

Subsystem	Report/Notes
Docker	ClientAPI=1.42 ServerAPI=1.42 base: https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/4/artifact/out/Dockerfile
GITHUB PR	#4834
Optional Tests	dupname asflicense compile javac javadoc mvninstall mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets
uname	Linux e54a4d213c3a 4.15.0-200-generic #211-Ubuntu SMP Thu Nov 24 18:16:04 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	dev-support/bin/hadoop.sh
git revision	trunk / `38af11a`
Default Java	Private Build-1.8.0_352-8u352-ga-1~20.04-b08
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.17+8-post-Ubuntu-1ubuntu220.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_352-8u352-ga-1~20.04-b08
Test Results	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/4/testReport/
Max. process+thread count	3598 (vs. ulimit of 5500)
modules	C: hadoop-hdfs-project/hadoop-hdfs U: hadoop-hdfs-project/hadoop-hdfs
Console output	https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-4834/4/console
versions	git=2.25.1 maven=3.6.3 spotbugs=4.2.2
Powered by	Apache Yetus 0.14.0 https://yetus.apache.org

This message was automatically generated.

saintstack approved these changes Aug 31, 2022

View reviewed changes

snmvaughan force-pushed the feature/HDFS-16753 branch from cb6d793 to 9afc036 Compare September 11, 2022 01:38

snmvaughan force-pushed the feature/HDFS-16753 branch from 9afc036 to 75b7e31 Compare October 13, 2022 14:17

HDFS-16753. WebHDFSHandler should reject non-compliant requests

38af11a

Make a non-null nnId a precondition for creating a DFSClient.

snmvaughan force-pushed the feature/HDFS-16753 branch from 75b7e31 to 38af11a Compare February 21, 2023 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDFS-16753. WebHDFSHandler should reject non-compliant requests #4834

HDFS-16753. WebHDFSHandler should reject non-compliant requests #4834

snmvaughan commented Aug 30, 2022

hadoop-yetus commented Aug 31, 2022

saintstack left a comment •

edited

snmvaughan commented Aug 31, 2022

saintstack commented Aug 31, 2022

snmvaughan commented Sep 1, 2022

hadoop-yetus commented Sep 11, 2022

hadoop-yetus commented Oct 13, 2022

hadoop-yetus commented Feb 22, 2023

HDFS-16753. WebHDFSHandler should reject non-compliant requests #4834

Are you sure you want to change the base?

HDFS-16753. WebHDFSHandler should reject non-compliant requests #4834

Conversation

snmvaughan commented Aug 30, 2022

Description of PR

How was this patch tested?

For code changes:

hadoop-yetus commented Aug 31, 2022

saintstack left a comment • edited

Choose a reason for hiding this comment

snmvaughan commented Aug 31, 2022

saintstack commented Aug 31, 2022

snmvaughan commented Sep 1, 2022

hadoop-yetus commented Sep 11, 2022

hadoop-yetus commented Oct 13, 2022

hadoop-yetus commented Feb 22, 2023

saintstack left a comment •

edited