Skip to content

TEZ-4741: ProfileOutputServlet: require instrumentation ACL to read profiler output - #526

Merged
abstractdog merged 1 commit into
apache:masterfrom
Aggarwal-Raghav:TEZ-4741
Jul 22, 2026
Merged

TEZ-4741: ProfileOutputServlet: require instrumentation ACL to read profiler output#526
abstractdog merged 1 commit into
apache:masterfrom
Aggarwal-Raghav:TEZ-4741

Conversation

@Aggarwal-Raghav

Copy link
Copy Markdown
Contributor

No description provided.

@Aggarwal-Raghav
Aggarwal-Raghavforce-pushed the TEZ-4741 branch 2 times, most recently from 3f71350 to f2ccf17CompareJuly 20, 2026 16:46
@tez-yetus

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 8sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
-1 ❌test4tests0m 0sThe 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.
_ master Compile Tests _
+1 💚mvninstall3m 10smaster passed
+1 💚compile2m 33smaster passed
+1 💚checkstyle0m 22smaster passed
+1 💚javadoc0m 17smaster passed
+0 🆗spotbugs0m 33stez-common in master has 13 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚mvninstall2m 9sthe patch passed
+1 💚codespell0m 49sNo new issues.
+1 💚compile2m 31sthe patch passed
+1 💚javac2m 31sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
+1 💚checkstyle0m 15sthe patch passed
+1 💚javadoc0m 18sthe patch passed
+1 💚spotbugs0m 41sthe patch passed
_ Other Tests _
+1 💚unit60m 16sroot in the patch passed.
+1 💚asflicense0m 21sThe patch does not generate ASF License warnings.
75m 27s
SubsystemReport/Notes
DockerClientAPI=1.55 ServerAPI=1.55 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-526/3/artifact/out/Dockerfile
Optional Testsdupname compile unit asflicense javac javadoc spotbugs checkstyle codespell detsecrets
unameLinux a74df8733a9d 5.15.0-179-generic #189-Ubuntu SMP Tue May 5 18:20:56 UTC 2026 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitytez-personality.sh
git revisionmaster / 0f389d2
Default JavaEclipse Adoptium-21.0.11+10-LTS
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-526/3/testReport/
Max. process+thread count1422 (vs. ulimit of 5500)
modulesC: tez-common U: tez-common
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-526/3/console
versionsgit=2.43.0 maven=3.9.15 spotbugs=4.9.3 codespell=2.4.1
Powered byApache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

@Aggarwal-Raghav

Copy link
Copy Markdown
ContributorAuthor

CC @abstractdog , ready for review

@abstractdog
abstractdog self-requested a review July 21, 2026 08:53

@abstractdogabstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

https://github.com/apache/hadoop/blob/767c9da6f34f9ed57311ba9f43497563c05b9f46/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/http/HttpServer2.java#L1730

apparently, unless the "requires admin" config is set explicitly to true, it's going to keep on working, but this way, cluster administrators have the chance to protect this endpoint

@abstractdog

abstractdog commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

oh, 1 more thing, sorry @Aggarwal-Raghav , I can see the introduced code is sligtly different from the one in ProfileServlet, can you please unify those? (e.g. exact same message, maybe even a public constant)

response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
setResponseHeader(response);
out.println("Unauthorized: Instrumentation access is not allowed!");

@abstractdog
abstractdog self-requested a review July 21, 2026 09:02

@abstractdogabstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made comment

@Aggarwal-Raghav

Copy link
Copy Markdown
ContributorAuthor

oh, 1 more thing, sorry @Aggarwal-Raghav , I can see the introduced code is sligtly different from the one in ProfileServlet, can you please unify those? (e.g. exact same message, maybe even a public constant)

response.setStatus(HttpServletResponse.SC_UNAUTHORIZED);
setResponseHeader(response);
out.println("Unauthorized: Instrumentation access is not allowed!");

@abstractdog , I was planning to do major refactor as part of #527 but the comments are valid for this PR. Will address

@Aggarwal-Raghav

Copy link
Copy Markdown
ContributorAuthor

For some reason in this PR, CI is failing, re-triggered it

@Aggarwal-Raghav

Copy link
Copy Markdown
ContributorAuthor

Looks some jenkins agent / node issue

Screenshot 2026-07-21 at 9 48 09 PM

@Aggarwal-Raghav

Copy link
Copy Markdown
ContributorAuthor

hadoop16 node/agent seems to be problematic. Run 1,4, 5 all failed on that.

@tez-yetus

Copy link
Copy Markdown

💔 -1 overall

VoteSubsystemRuntimeLogfileComment
+0 🆗reexec0m 8sDocker mode activated.
_ Prechecks _
+1 💚dupname0m 0sNo case conflicting files found.
+0 🆗detsecrets0m 0sdetect-secrets was not available.
+1 💚@author0m 0sThe patch does not contain any @author tags.
-1 ❌test4tests0m 0sThe 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.
_ master Compile Tests _
+1 💚mvninstall2m 58smaster passed
+1 💚compile2m 34smaster passed
+1 💚checkstyle0m 23smaster passed
+1 💚javadoc0m 19smaster passed
+0 🆗spotbugs0m 33stez-common in master has 13 extant spotbugs warnings.
_ Patch Compile Tests _
+1 💚mvninstall2m 12sthe patch passed
+1 💚codespell0m 51sNo new issues.
+1 💚compile2m 30sthe patch passed
+1 💚javac2m 30sthe patch passed
+1 💚blanks0m 0sThe patch has no blanks issues.
+1 💚checkstyle0m 14sthe patch passed
+1 💚javadoc0m 17sthe patch passed
+1 💚spotbugs0m 39sthe patch passed
_ Other Tests _
+1 💚unit61m 25sroot in the patch passed.
+1 💚asflicense0m 22sThe patch does not generate ASF License warnings.
76m 28s
SubsystemReport/Notes
DockerClientAPI=1.55 ServerAPI=1.55 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-526/6/artifact/out/Dockerfile
Optional Testsdupname compile unit asflicense javac javadoc spotbugs checkstyle codespell detsecrets
unameLinux 231e9657ba09 5.15.0-179-generic #189-Ubuntu SMP Tue May 5 18:20:56 UTC 2026 x86_64 x86_64 x86_64 GNU/Linux
Build toolmaven
Personalitytez-personality.sh
git revisionmaster / 74bb821
Default JavaEclipse Adoptium-21.0.11+10-LTS
Test Resultshttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-526/6/testReport/
Max. process+thread count1380 (vs. ulimit of 5500)
modulesC: tez-common U: tez-common
Console outputhttps://ci-hadoop.apache.org/job/tez-multibranch/job/PR-526/6/console
versionsgit=2.43.0 maven=3.9.15 spotbugs=4.9.3 codespell=2.4.1
Powered byApache Yetus 0.15.1 https://yetus.apache.org

This message was automatically generated.

@abstractdogabstractdog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
sorry for the infra problems

@Aggarwal-Raghav

Copy link
Copy Markdown
ContributorAuthor

LGTM sorry for the infra problems

No worries, its not in our control

@abstractdog
abstractdog merged commit 41145de into apache:masterJul 22, 2026
6 checks passed
asf-gitbox-commits pushed a commit that referenced this pull request Aug 10, 2026
…rofiler output (#526) (Raghav Aggarwal reviewed by Laszlo Bodor)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Aggarwal-Raghav@tez-yetus@abstractdog