HDDS-14380. The user who starts Recon process will have administrator privilege by echonesis · Pull Request #9627 · apache/ozone

echonesis · 2026-01-12T15:44:26Z

What changes were proposed in this pull request?

This PR enhances the Recon administrator access control by automatically granting administrator privileges to the user who starts the Recon process, in addition to any configured administrators.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-14380

How was this patch tested?

GitHub Actions CI: https://github.com/echonesis/ozone/actions/runs/21214940123

… privilege

devmadhuu

Thanks @echonesis for the patch. Largely looks good, but just few minor comments.

devmadhuu · 2026-01-13T15:28:39Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconControllerModule.java

+
  @Override
  protected void configure() {
+    bind(ReconServer.class).toInstance(reconServer);


why we need reconServer instance to be passes explicitly to ReconControllerModule ? can we not use Singleton ?

This follows the same pattern as OM and SCM. In OM, the starter user and admin information are stored as instance fields in the OzoneManager object itself (https://github.com/apache/ozone/blob/master/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java#L680-L682). Similarly, we store this information in the ReconServer instance and make it accessible to other components via Guice.

Using bind(ReconServer.class).toInstance(reconServer) allows other components to inject and access the starter user information, just like how OM exposes this through its instance methods.

Okay, your implementation is correct for the current architecture. The toInstance() pattern is appropriate here because ReconServer is created and initialized before Guice, and we need to bind the specific initialized instance. While OM/SCM store admin info similarly, they don't use Guice, so in a way, the comparison isn't directly applicable.

Thanks for the clarification!

devmadhuu · 2026-01-13T15:31:11Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServer.java


+    String reconStarterUser = UserGroupInformation.getCurrentUser().getShortUserName();
+    Collection<String> adminUsers =
+        OzoneAdmins.getOzoneAdminsFromConfig(configuration, reconStarterUser);


Instead of calling Admins and AdminsGroups method separately here, can we use this method directly and then add recon admins and recon admin groups. this will minimise the code lines.

Thanks for the suggestion!
Since we need to add Recon-specific admins from OZONE_RECON_ADMINISTRATORS and OZONE_RECON_ADMINISTRATORS_GROUPS, using addAll() seemed like a straightforward approach.

Okay, can we refactor and extract to a helper method instead of inline code directly in call() method:

private OzoneAdmins createReconAdmins(OzoneConfiguration conf) { String starterUser = UserGroupInformation.getCurrentUser().getShortUserName(); Collection<String> adminUsers = OzoneAdmins.getOzoneAdminsFromConfig(conf, starterUser); adminUsers.addAll( conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS)); Collection<String> adminGroups = OzoneAdmins.getOzoneAdminsGroupsFromConfig(conf); adminGroups.addAll( conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS)); return new OzoneAdmins(adminUsers, adminGroups); }

Thanks!
I add it as expected in the latest commit.

devmadhuu · 2026-01-13T15:32:56Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServer.java

+   *
+   * @return Collection of admin groups
+   */
+  public Collection<String> getReconAdminGroups() {


Any specific usecase or need to define these admin and admin group getter methods ?

Thanks @devmadhuu !
These methods follow the same pattern as OM's getOmAdminUsernames() and getOmAdminGroups() for consistency. They're not currently used in Recon.
I will remove them in a follow-up commit if preferred.

devmadhuu

Thanks @echonesis for clarifying the comments. Just a nit: if you can handle. Overall LGTM +1

devmadhuu · 2026-01-19T09:33:32Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconControllerModule.java

+
  @Override
  protected void configure() {
+    bind(ReconServer.class).toInstance(reconServer);


Okay, your implementation is correct for the current architecture. The toInstance() pattern is appropriate here because ReconServer is created and initialized before Guice, and we need to bind the specific initialized instance. While OM/SCM store admin info similarly, they don't use Guice, so in a way, the comparison isn't directly applicable.

devmadhuu · 2026-01-19T09:40:20Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServer.java


+    String reconStarterUser = UserGroupInformation.getCurrentUser().getShortUserName();
+    Collection<String> adminUsers =
+        OzoneAdmins.getOzoneAdminsFromConfig(configuration, reconStarterUser);


Okay, can we refactor and extract to a helper method instead of inline code directly in call() method:

private OzoneAdmins createReconAdmins(OzoneConfiguration conf) { String starterUser = UserGroupInformation.getCurrentUser().getShortUserName(); Collection<String> adminUsers = OzoneAdmins.getOzoneAdminsFromConfig(conf, starterUser); adminUsers.addAll( conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS)); Collection<String> adminGroups = OzoneAdmins.getOzoneAdminsGroupsFromConfig(conf); adminGroups.addAll( conf.getStringCollection(ReconConfigKeys.OZONE_RECON_ADMINISTRATORS_GROUPS)); return new OzoneAdmins(adminUsers, adminGroups); }

priyeshkaratha

Thanks @echonesis for the patch. Overall changes LGTM. I also suggest to move to private method as Devesh suggested and have one minor comment.

priyeshkaratha · 2026-01-20T06:47:04Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/ReconServer.java

@@ -104,9 +108,23 @@ public Void call() throws Exception {
            ReconServer.class, originalArgs, LOG, configuration);
    ConfigurationProvider.setConfiguration(configuration);



The call to UserGroupInformation.getCurrentUser() can technically throw an IOException if there are issues with the Kerberos ticket or OS-level user retrieval.

So I am suggesting to ensure the ReconServer startup sequence gracefully handles or logs a clear error message if the current user cannot be identified

Thanks!
I update it as expected in the latest commit.

priyeshkaratha

Thanks @echonesis for improving the patch. Changes LGTM

devmadhuu

Thanks @echonesis for improving the patch. LGTM +1

HDDS-14380. The user who starts Recon process will have administrator…

dd073df

… privilege

jojochuang requested review from ArafatKhan2198, devabhishekpal and devmadhuu January 12, 2026 17:14

devmadhuu reviewed Jan 13, 2026

View reviewed changes

adoroszlai added the recon label Jan 15, 2026

fix: CR update

19cbd26

echonesis requested a review from devmadhuu January 18, 2026 06:35

devmadhuu reviewed Jan 19, 2026

View reviewed changes

priyeshkaratha reviewed Jan 20, 2026

View reviewed changes

fix: CR update

75dca98

echonesis requested review from devmadhuu and priyeshkaratha January 22, 2026 13:44

priyeshkaratha approved these changes Jan 23, 2026

View reviewed changes

devmadhuu approved these changes Jan 23, 2026

View reviewed changes

		@@ -104,9 +108,23 @@ public Void call() throws Exception {
		ReconServer.class, originalArgs, LOG, configuration);
		ConfigurationProvider.setConfiguration(configuration);

Conversation

echonesis commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

devmadhuu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devmadhuu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyeshkaratha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

priyeshkaratha left a comment

Choose a reason for hiding this comment

Uh oh!

devmadhuu left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

echonesis commented Jan 12, 2026 •

edited

Loading

priyeshkaratha left a comment •

edited

Loading